[PATCH] pwm: sysfs: Set class on pwm devices

[PATCH] pwm: sysfs: Set class on pwm devices

[poeschel]

From: Lars Poeschel <poeschel@lemonage.de>

This adds a class to exported pwm devices.
Exporting a pwm through sysfs did not yield udev events. The
dev_uevent_filter function does filter-out devices without a bus or
class.
This was already addressed in commit
commit 7e5d1fd75c3d ("pwm: Set class for exported channels in sysfs")
but this did cause problems and the commit got reverted with
commit c289d6625237 ("Revert "pwm: Set class for exported channels in
sysfs"")

pwm's have to be local to its pwmchip, so we create an individual class
for each pwmchip and assign this class to the pwm belonging to the
pwmchip. This does better map to structure that is also visible in
sysfs.

Signed-off-by: Lars Poeschel <poeschel@lemonage.de>
---
 drivers/pwm/sysfs.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c
index 449dbc0f49ed..e2dfbc335366 100644
--- a/drivers/pwm/sysfs.c
+++ b/drivers/pwm/sysfs.c
@@ -259,7 +259,7 @@ static int pwm_export_child(struct device *parent, struct pwm_device *pwm)
 	export->child.release = pwm_export_release;
 	export->child.parent = parent;
 	export->child.devt = MKDEV(0, 0);
-	export->child.groups = pwm_groups;
+	export->child.class = parent->class;
 	dev_set_name(&export->child, "pwm%u", pwm->hwpwm);
 
 	ret = device_register(&export->child);
@@ -499,6 +499,9 @@ void pwmchip_sysfs_export(struct pwm_chip *chip)
 		dev_warn(chip->dev,
 			 "device_create failed for pwm_chip sysfs export\n");
 	}
+
+	parent->class = class_create(THIS_MODULE, parent->kobj.name);
+	parent->class->dev_groups = pwm_groups;
 }
 
 void pwmchip_sysfs_unexport(struct pwm_chip *chip)
@@ -518,6 +521,7 @@ void pwmchip_sysfs_unexport(struct pwm_chip *chip)
 			pwm_unexport_child(parent, pwm);
 	}
 
+	class_destroy(parent->class);
 	put_device(parent);
 	device_unregister(parent);
 }
-- 
2.28.0

Re: [PATCH] pwm: sysfs: Set class on pwm devices

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2533 bytes --]

Hello Lars,

On Tue, Sep 29, 2020 at 02:19:53PM +0200, poeschel@lemonage.de wrote:
> From: Lars Poeschel <poeschel@lemonage.de>
> 
> This adds a class to exported pwm devices.
> Exporting a pwm through sysfs did not yield udev events. The

I wonder what is your use-case here. This for sure also has a place to
be mentioned in the commit log. I suspect there is a better way to
accomplish you way.

> dev_uevent_filter function does filter-out devices without a bus or
> class.
> This was already addressed in commit
> commit 7e5d1fd75c3d ("pwm: Set class for exported channels in sysfs")
> but this did cause problems and the commit got reverted with
> commit c289d6625237 ("Revert "pwm: Set class for exported channels in
> sysfs"")
> 
> pwm's have to be local to its pwmchip, so we create an individual class
> for each pwmchip

This sounds conceptually wrong. 

> and assign this class to the pwm belonging to the
> pwmchip. This does better map to structure that is also visible in
> sysfs.
> 
> Signed-off-by: Lars Poeschel <poeschel@lemonage.de>
> ---
>  drivers/pwm/sysfs.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c
> index 449dbc0f49ed..e2dfbc335366 100644
> --- a/drivers/pwm/sysfs.c
> +++ b/drivers/pwm/sysfs.c
> @@ -259,7 +259,7 @@ static int pwm_export_child(struct device *parent, struct pwm_device *pwm)
>  	export->child.release = pwm_export_release;
>  	export->child.parent = parent;
>  	export->child.devt = MKDEV(0, 0);
> -	export->child.groups = pwm_groups;
> +	export->child.class = parent->class;
>  	dev_set_name(&export->child, "pwm%u", pwm->hwpwm);
>  
>  	ret = device_register(&export->child);
> @@ -499,6 +499,9 @@ void pwmchip_sysfs_export(struct pwm_chip *chip)
>  		dev_warn(chip->dev,
>  			 "device_create failed for pwm_chip sysfs export\n");
>  	}
> +
> +	parent->class = class_create(THIS_MODULE, parent->kobj.name);

This needs error handling.

> +	parent->class->dev_groups = pwm_groups;
>  }
>  
>  void pwmchip_sysfs_unexport(struct pwm_chip *chip)
> @@ -518,6 +521,7 @@ void pwmchip_sysfs_unexport(struct pwm_chip *chip)
>  			pwm_unexport_child(parent, pwm);
>  	}
>  
> +	class_destroy(parent->class);
>  	put_device(parent);
>  	device_unregister(parent);
>  }

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] pwm: sysfs: Set class on pwm devices

[Lars Poeschel]

Hi Uwe,

thank you for your review!

On Wed, Sep 30, 2020 at 08:57:26AM +0200, Uwe Kleine-König wrote:
> Hello Lars,
> 
> On Tue, Sep 29, 2020 at 02:19:53PM +0200, poeschel@lemonage.de wrote:
> > From: Lars Poeschel <poeschel@lemonage.de>
> > 
> > This adds a class to exported pwm devices.
> > Exporting a pwm through sysfs did not yield udev events. The
> 
> I wonder what is your use-case here. This for sure also has a place to
> be mentioned in the commit log. I suspect there is a better way to
> accomplish you way.

Use-case is to be able to use a pwm from a non-root userspace process.
I use udev rules to adjust permissions.

> > dev_uevent_filter function does filter-out devices without a bus or
> > class.
> > This was already addressed in commit
> > commit 7e5d1fd75c3d ("pwm: Set class for exported channels in sysfs")
> > but this did cause problems and the commit got reverted with
> > commit c289d6625237 ("Revert "pwm: Set class for exported channels in
> > sysfs"")
> > 
> > pwm's have to be local to its pwmchip, so we create an individual class
> > for each pwmchip
> 
> This sounds conceptually wrong. 

I suspect you mean the second part is wrong and we agree on the first
part, that pwm's have to be local to its pwmchip.

There seems to be a need for this as 7e5d1fd75c3d tried this already.
Problem with this approach was, that the pwms where not local to their
pwmchip and if you then have the same pwm number exported from different
pwmchips this did collide.

Ok, now the uevent_ops filter function blocks the uevent I want to see
based on if device has a bus or a class set.

Can you recommend a better solution ?
Write a different filter function for this case ?

> > and assign this class to the pwm belonging to the
> > pwmchip. This does better map to structure that is also visible in
> > sysfs.
> > 
> > Signed-off-by: Lars Poeschel <poeschel@lemonage.de>
> > ---
> >  drivers/pwm/sysfs.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c
> > index 449dbc0f49ed..e2dfbc335366 100644
> > --- a/drivers/pwm/sysfs.c
> > +++ b/drivers/pwm/sysfs.c
> > @@ -259,7 +259,7 @@ static int pwm_export_child(struct device *parent, struct pwm_device *pwm)
> >  	export->child.release = pwm_export_release;
> >  	export->child.parent = parent;
> >  	export->child.devt = MKDEV(0, 0);
> > -	export->child.groups = pwm_groups;
> > +	export->child.class = parent->class;
> >  	dev_set_name(&export->child, "pwm%u", pwm->hwpwm);
> >  
> >  	ret = device_register(&export->child);
> > @@ -499,6 +499,9 @@ void pwmchip_sysfs_export(struct pwm_chip *chip)
> >  		dev_warn(chip->dev,
> >  			 "device_create failed for pwm_chip sysfs export\n");
> >  	}
> > +
> > +	parent->class = class_create(THIS_MODULE, parent->kobj.name);
> 
> This needs error handling.

If this concept has a chance after discussion, I will change this.

Thanks again,
Lars

Re: [PATCH] pwm: sysfs: Set class on pwm devices

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1502 bytes --]

Hello,

I added Greg Kroah-Hartman who I discussed this with via irc a bit to
Cc:.

On Wed, Sep 30, 2020 at 11:20:56AM +0200, Lars Poeschel wrote:
> thank you for your review!
> 
> On Wed, Sep 30, 2020 at 08:57:26AM +0200, Uwe Kleine-König wrote:
> > On Tue, Sep 29, 2020 at 02:19:53PM +0200, poeschel@lemonage.de wrote:
> > > From: Lars Poeschel <poeschel@lemonage.de>
> > > 
> > > This adds a class to exported pwm devices.
> > > Exporting a pwm through sysfs did not yield udev events. The
> > 
> > I wonder what is your use-case here. This for sure also has a place to
> > be mentioned in the commit log. I suspect there is a better way to
> > accomplish you way.
> 
> Use-case is to be able to use a pwm from a non-root userspace process.
> I use udev rules to adjust permissions.

Hmm, how do you trigger the export? Without being aware of all the
details in the sysfs code I would expect that the exported stuff is
available instantly once the write used to export the PWM is completed.
So changing the permissions can be done directly after triggering the
export in the same process.

Out of interest: What do you use the pwm for? Isn't there a suitable
kernel driver that can do the required stuff? Compared to the kernel-API
the sysfs interface isn't atomic. Is this an annoyance?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] pwm: sysfs: Set class on pwm devices

[Greg Kroah-Hartman]

On Wed, Sep 30, 2020 at 11:41:46AM +0200, Uwe Kleine-König wrote:
> Hello,
> 
> I added Greg Kroah-Hartman who I discussed this with via irc a bit to
> Cc:.
> 
> On Wed, Sep 30, 2020 at 11:20:56AM +0200, Lars Poeschel wrote:
> > thank you for your review!
> > 
> > On Wed, Sep 30, 2020 at 08:57:26AM +0200, Uwe Kleine-König wrote:
> > > On Tue, Sep 29, 2020 at 02:19:53PM +0200, poeschel@lemonage.de wrote:
> > > > From: Lars Poeschel <poeschel@lemonage.de>
> > > > 
> > > > This adds a class to exported pwm devices.
> > > > Exporting a pwm through sysfs did not yield udev events. The
> > > 
> > > I wonder what is your use-case here. This for sure also has a place to
> > > be mentioned in the commit log. I suspect there is a better way to
> > > accomplish you way.
> > 
> > Use-case is to be able to use a pwm from a non-root userspace process.
> > I use udev rules to adjust permissions.
> 
> Hmm, how do you trigger the export? Without being aware of all the
> details in the sysfs code I would expect that the exported stuff is
> available instantly once the write used to export the PWM is completed.
> So changing the permissions can be done directly after triggering the
> export in the same process.

It looks like userspace wants to see when a pwmX device shows up, right?

And it's not because those devices do not belong to any class or bus, so
they are just "floating" out there (they might show up under
/sys/bus/virtual, if you set things up right, which I don't think is
happening here...)

So yes, you need to create a class, or assign this to a bus, which is
fine, but it looks like no one is doing that.  Don't create new classes
dynamically, but rather, just assign this to the existing pwm class.
What's wrong with that?  I saw an older patch that did that, what did
that break?

thanks,

greg k-h

Re: [PATCH] pwm: sysfs: Set class on pwm devices

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2604 bytes --]

On Wed, Sep 30, 2020 at 11:52:04AM +0200, Greg Kroah-Hartman wrote:
> On Wed, Sep 30, 2020 at 11:41:46AM +0200, Uwe Kleine-König wrote:
> > Hello,
> > 
> > I added Greg Kroah-Hartman who I discussed this with via irc a bit to
> > Cc:.
> > 
> > On Wed, Sep 30, 2020 at 11:20:56AM +0200, Lars Poeschel wrote:
> > > thank you for your review!
> > > 
> > > On Wed, Sep 30, 2020 at 08:57:26AM +0200, Uwe Kleine-König wrote:
> > > > On Tue, Sep 29, 2020 at 02:19:53PM +0200, poeschel@lemonage.de wrote:
> > > > > From: Lars Poeschel <poeschel@lemonage.de>
> > > > > 
> > > > > This adds a class to exported pwm devices.
> > > > > Exporting a pwm through sysfs did not yield udev events. The
> > > > 
> > > > I wonder what is your use-case here. This for sure also has a place to
> > > > be mentioned in the commit log. I suspect there is a better way to
> > > > accomplish you way.
> > > 
> > > Use-case is to be able to use a pwm from a non-root userspace process.
> > > I use udev rules to adjust permissions.
> > 
> > Hmm, how do you trigger the export? Without being aware of all the
> > details in the sysfs code I would expect that the exported stuff is
> > available instantly once the write used to export the PWM is completed.
> > So changing the permissions can be done directly after triggering the
> > export in the same process.
> 
> It looks like userspace wants to see when a pwmX device shows up, right?
> 
> And it's not because those devices do not belong to any class or bus, so
> they are just "floating" out there (they might show up under
> /sys/bus/virtual, if you set things up right, which I don't think is
> happening here...)
> 
> So yes, you need to create a class, or assign this to a bus, which is
> fine, but it looks like no one is doing that.  Don't create new classes
> dynamically, but rather, just assign this to the existing pwm class.
> What's wrong with that?  I saw an older patch that did that, what did
> that break?

Are you refering to 7e5d1fd75c3dde9fc10c4472b9368089d1b81d00? Did you
read the reverting commit's log message? (i.e.
c289d6625237aa785b484b4e94c23b3b91ea7e60)

I guess the breakage is that the resulting name then is:

	"pwm%d", pwm->id

where pwm->id is a number unique to the pwmchip. So doing

	echo 0 > pwmchip1/export
	echo 0 > pwmchip2/export

breaks because both want to create pwm0 in the class directory.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] pwm: sysfs: Set class on pwm devices

[Greg Kroah-Hartman]

On Wed, Sep 30, 2020 at 12:01:26PM +0200, Uwe Kleine-König wrote:
> On Wed, Sep 30, 2020 at 11:52:04AM +0200, Greg Kroah-Hartman wrote:
> > On Wed, Sep 30, 2020 at 11:41:46AM +0200, Uwe Kleine-König wrote:
> > > Hello,
> > > 
> > > I added Greg Kroah-Hartman who I discussed this with via irc a bit to
> > > Cc:.
> > > 
> > > On Wed, Sep 30, 2020 at 11:20:56AM +0200, Lars Poeschel wrote:
> > > > thank you for your review!
> > > > 
> > > > On Wed, Sep 30, 2020 at 08:57:26AM +0200, Uwe Kleine-König wrote:
> > > > > On Tue, Sep 29, 2020 at 02:19:53PM +0200, poeschel@lemonage.de wrote:
> > > > > > From: Lars Poeschel <poeschel@lemonage.de>
> > > > > > 
> > > > > > This adds a class to exported pwm devices.
> > > > > > Exporting a pwm through sysfs did not yield udev events. The
> > > > > 
> > > > > I wonder what is your use-case here. This for sure also has a place to
> > > > > be mentioned in the commit log. I suspect there is a better way to
> > > > > accomplish you way.
> > > > 
> > > > Use-case is to be able to use a pwm from a non-root userspace process.
> > > > I use udev rules to adjust permissions.
> > > 
> > > Hmm, how do you trigger the export? Without being aware of all the
> > > details in the sysfs code I would expect that the exported stuff is
> > > available instantly once the write used to export the PWM is completed.
> > > So changing the permissions can be done directly after triggering the
> > > export in the same process.
> > 
> > It looks like userspace wants to see when a pwmX device shows up, right?
> > 
> > And it's not because those devices do not belong to any class or bus, so
> > they are just "floating" out there (they might show up under
> > /sys/bus/virtual, if you set things up right, which I don't think is
> > happening here...)
> > 
> > So yes, you need to create a class, or assign this to a bus, which is
> > fine, but it looks like no one is doing that.  Don't create new classes
> > dynamically, but rather, just assign this to the existing pwm class.
> > What's wrong with that?  I saw an older patch that did that, what did
> > that break?
> 
> Are you refering to 7e5d1fd75c3dde9fc10c4472b9368089d1b81d00? Did you
> read the reverting commit's log message? (i.e.
> c289d6625237aa785b484b4e94c23b3b91ea7e60)
> 
> I guess the breakage is that the resulting name then is:
> 
> 	"pwm%d", pwm->id
> 
> where pwm->id is a number unique to the pwmchip. So doing
> 
> 	echo 0 > pwmchip1/export
> 	echo 0 > pwmchip2/export
> 
> breaks because both want to create pwm0 in the class directory.

Ah, that makes more sense why that didn't work.

Ok, can the "name" of the new export chip be changed?  Is that
hard-coded somewhere in userspace tools already?  Depending on that, the
solution for this will change...

thanks,

greg k-h

Re: [PATCH] pwm: sysfs: Set class on pwm devices

[Lars Poeschel]

On Wed, Sep 30, 2020 at 12:52:38PM +0200, Greg Kroah-Hartman wrote:
> On Wed, Sep 30, 2020 at 12:01:26PM +0200, Uwe Kleine-König wrote:
> > On Wed, Sep 30, 2020 at 11:52:04AM +0200, Greg Kroah-Hartman wrote:
> > > On Wed, Sep 30, 2020 at 11:41:46AM +0200, Uwe Kleine-König wrote:
> > > > Hello,
> > > > 
> > > > I added Greg Kroah-Hartman who I discussed this with via irc a bit to
> > > > Cc:.
> > > > 
> > > > On Wed, Sep 30, 2020 at 11:20:56AM +0200, Lars Poeschel wrote:
> > > > > thank you for your review!
> > > > > 
> > > > > On Wed, Sep 30, 2020 at 08:57:26AM +0200, Uwe Kleine-König wrote:
> > > > > > On Tue, Sep 29, 2020 at 02:19:53PM +0200, poeschel@lemonage.de wrote:
> > > > > > > From: Lars Poeschel <poeschel@lemonage.de>
> > > > > > > 
> > > > > > > This adds a class to exported pwm devices.
> > > > > > > Exporting a pwm through sysfs did not yield udev events. The
> > > > > > 
> > > > > > I wonder what is your use-case here. This for sure also has a place to
> > > > > > be mentioned in the commit log. I suspect there is a better way to
> > > > > > accomplish you way.
> > > > > 
> > > > > Use-case is to be able to use a pwm from a non-root userspace process.
> > > > > I use udev rules to adjust permissions.
> > > > 
> > > > Hmm, how do you trigger the export? Without being aware of all the
> > > > details in the sysfs code I would expect that the exported stuff is
> > > > available instantly once the write used to export the PWM is completed.
> > > > So changing the permissions can be done directly after triggering the
> > > > export in the same process.
> > > 
> > > It looks like userspace wants to see when a pwmX device shows up, right?
> > > 
> > > And it's not because those devices do not belong to any class or bus, so
> > > they are just "floating" out there (they might show up under
> > > /sys/bus/virtual, if you set things up right, which I don't think is
> > > happening here...)
> > > 
> > > So yes, you need to create a class, or assign this to a bus, which is
> > > fine, but it looks like no one is doing that.  Don't create new classes
> > > dynamically, but rather, just assign this to the existing pwm class.
> > > What's wrong with that?  I saw an older patch that did that, what did
> > > that break?
> > 
> > Are you refering to 7e5d1fd75c3dde9fc10c4472b9368089d1b81d00? Did you
> > read the reverting commit's log message? (i.e.
> > c289d6625237aa785b484b4e94c23b3b91ea7e60)
> > 
> > I guess the breakage is that the resulting name then is:
> > 
> > 	"pwm%d", pwm->id
> > 
> > where pwm->id is a number unique to the pwmchip. So doing
> > 
> > 	echo 0 > pwmchip1/export
> > 	echo 0 > pwmchip2/export
> > 
> > breaks because both want to create pwm0 in the class directory.
> 
> Ah, that makes more sense why that didn't work.
> 
> Ok, can the "name" of the new export chip be changed?  Is that
> hard-coded somewhere in userspace tools already?  Depending on that, the
> solution for this will change...

I know that back then, when sysfs for pwm was created, Thierry didn't
want to have one global namespace like gpio sysfs has. What you ask for
is something like:
	pwm-{chipnumber}-{pwmnumber}
Right ? Can that be considered non-global ?

Thierry's mail from back then is here:
https://lore.kernel.org/lkml/20130408081745.GA21392@avionic-0098.mockup.avionic-design.de/

A short search on github I found this:
https://github.com/vsergeev/c-periphery/blob/d34077d7ee45fa7d1947cc0174919452fac31597/src/pwm.c#L74

Seems to match your hardcoded criteria ?

Regards,
Lars

Re: [PATCH] pwm: sysfs: Set class on pwm devices

[Greg Kroah-Hartman]

On Wed, Sep 30, 2020 at 01:27:20PM +0200, Lars Poeschel wrote:
> On Wed, Sep 30, 2020 at 12:52:38PM +0200, Greg Kroah-Hartman wrote:
> > On Wed, Sep 30, 2020 at 12:01:26PM +0200, Uwe Kleine-König wrote:
> > > On Wed, Sep 30, 2020 at 11:52:04AM +0200, Greg Kroah-Hartman wrote:
> > > > On Wed, Sep 30, 2020 at 11:41:46AM +0200, Uwe Kleine-König wrote:
> > > > > Hello,
> > > > > 
> > > > > I added Greg Kroah-Hartman who I discussed this with via irc a bit to
> > > > > Cc:.
> > > > > 
> > > > > On Wed, Sep 30, 2020 at 11:20:56AM +0200, Lars Poeschel wrote:
> > > > > > thank you for your review!
> > > > > > 
> > > > > > On Wed, Sep 30, 2020 at 08:57:26AM +0200, Uwe Kleine-König wrote:
> > > > > > > On Tue, Sep 29, 2020 at 02:19:53PM +0200, poeschel@lemonage.de wrote:
> > > > > > > > From: Lars Poeschel <poeschel@lemonage.de>
> > > > > > > > 
> > > > > > > > This adds a class to exported pwm devices.
> > > > > > > > Exporting a pwm through sysfs did not yield udev events. The
> > > > > > > 
> > > > > > > I wonder what is your use-case here. This for sure also has a place to
> > > > > > > be mentioned in the commit log. I suspect there is a better way to
> > > > > > > accomplish you way.
> > > > > > 
> > > > > > Use-case is to be able to use a pwm from a non-root userspace process.
> > > > > > I use udev rules to adjust permissions.
> > > > > 
> > > > > Hmm, how do you trigger the export? Without being aware of all the
> > > > > details in the sysfs code I would expect that the exported stuff is
> > > > > available instantly once the write used to export the PWM is completed.
> > > > > So changing the permissions can be done directly after triggering the
> > > > > export in the same process.
> > > > 
> > > > It looks like userspace wants to see when a pwmX device shows up, right?
> > > > 
> > > > And it's not because those devices do not belong to any class or bus, so
> > > > they are just "floating" out there (they might show up under
> > > > /sys/bus/virtual, if you set things up right, which I don't think is
> > > > happening here...)
> > > > 
> > > > So yes, you need to create a class, or assign this to a bus, which is
> > > > fine, but it looks like no one is doing that.  Don't create new classes
> > > > dynamically, but rather, just assign this to the existing pwm class.
> > > > What's wrong with that?  I saw an older patch that did that, what did
> > > > that break?
> > > 
> > > Are you refering to 7e5d1fd75c3dde9fc10c4472b9368089d1b81d00? Did you
> > > read the reverting commit's log message? (i.e.
> > > c289d6625237aa785b484b4e94c23b3b91ea7e60)
> > > 
> > > I guess the breakage is that the resulting name then is:
> > > 
> > > 	"pwm%d", pwm->id
> > > 
> > > where pwm->id is a number unique to the pwmchip. So doing
> > > 
> > > 	echo 0 > pwmchip1/export
> > > 	echo 0 > pwmchip2/export
> > > 
> > > breaks because both want to create pwm0 in the class directory.
> > 
> > Ah, that makes more sense why that didn't work.
> > 
> > Ok, can the "name" of the new export chip be changed?  Is that
> > hard-coded somewhere in userspace tools already?  Depending on that, the
> > solution for this will change...
> 
> I know that back then, when sysfs for pwm was created, Thierry didn't
> want to have one global namespace like gpio sysfs has. What you ask for
> is something like:
> 	pwm-{chipnumber}-{pwmnumber}
> Right ? Can that be considered non-global ?

Yes, and that's just "global" for the pwm class namespace.

> Thierry's mail from back then is here:
> https://lore.kernel.org/lkml/20130408081745.GA21392@avionic-0098.mockup.avionic-design.de/
> 
> A short search on github I found this:
> https://github.com/vsergeev/c-periphery/blob/d34077d7ee45fa7d1947cc0174919452fac31597/src/pwm.c#L74
> 
> Seems to match your hardcoded criteria ?

Yes, ugh :(

Ok, now I see why the "lots of pwm classes!" patch was proposed.

And maybe that's really the only way forward here, as the chip namespace
is the only unique thing.

But wow, it feels wrong...

greg k-h

Re: [PATCH] pwm: sysfs: Set class on pwm devices

[Lars Poeschel]

On Wed, Sep 30, 2020 at 01:51:06PM +0200, Greg Kroah-Hartman wrote:
> On Wed, Sep 30, 2020 at 01:27:20PM +0200, Lars Poeschel wrote:
> > On Wed, Sep 30, 2020 at 12:52:38PM +0200, Greg Kroah-Hartman wrote:
> > > On Wed, Sep 30, 2020 at 12:01:26PM +0200, Uwe Kleine-König wrote:
> > > > On Wed, Sep 30, 2020 at 11:52:04AM +0200, Greg Kroah-Hartman wrote:
> > > > > On Wed, Sep 30, 2020 at 11:41:46AM +0200, Uwe Kleine-König wrote:
> > > > > > Hello,
> > > > > > 
> > > > > > I added Greg Kroah-Hartman who I discussed this with via irc a bit to
> > > > > > Cc:.
> > > > > > 
> > > > > > On Wed, Sep 30, 2020 at 11:20:56AM +0200, Lars Poeschel wrote:
> > > > > > > thank you for your review!
> > > > > > > 
> > > > > > > On Wed, Sep 30, 2020 at 08:57:26AM +0200, Uwe Kleine-König wrote:
> > > > > > > > On Tue, Sep 29, 2020 at 02:19:53PM +0200, poeschel@lemonage.de wrote:
> > > > > > > > > From: Lars Poeschel <poeschel@lemonage.de>
> > > > > > > > > 
> > > > > > > > > This adds a class to exported pwm devices.
> > > > > > > > > Exporting a pwm through sysfs did not yield udev events. The
> > > > > > > > 
> > > > > > > > I wonder what is your use-case here. This for sure also has a place to
> > > > > > > > be mentioned in the commit log. I suspect there is a better way to
> > > > > > > > accomplish you way.
> > > > > > > 
> > > > > > > Use-case is to be able to use a pwm from a non-root userspace process.
> > > > > > > I use udev rules to adjust permissions.
> > > > > > 
> > > > > > Hmm, how do you trigger the export? Without being aware of all the
> > > > > > details in the sysfs code I would expect that the exported stuff is
> > > > > > available instantly once the write used to export the PWM is completed.
> > > > > > So changing the permissions can be done directly after triggering the
> > > > > > export in the same process.
> > > > > 
> > > > > It looks like userspace wants to see when a pwmX device shows up, right?
> > > > > 
> > > > > And it's not because those devices do not belong to any class or bus, so
> > > > > they are just "floating" out there (they might show up under
> > > > > /sys/bus/virtual, if you set things up right, which I don't think is
> > > > > happening here...)
> > > > > 
> > > > > So yes, you need to create a class, or assign this to a bus, which is
> > > > > fine, but it looks like no one is doing that.  Don't create new classes
> > > > > dynamically, but rather, just assign this to the existing pwm class.
> > > > > What's wrong with that?  I saw an older patch that did that, what did
> > > > > that break?
> > > > 
> > > > Are you refering to 7e5d1fd75c3dde9fc10c4472b9368089d1b81d00? Did you
> > > > read the reverting commit's log message? (i.e.
> > > > c289d6625237aa785b484b4e94c23b3b91ea7e60)
> > > > 
> > > > I guess the breakage is that the resulting name then is:
> > > > 
> > > > 	"pwm%d", pwm->id
> > > > 
> > > > where pwm->id is a number unique to the pwmchip. So doing
> > > > 
> > > > 	echo 0 > pwmchip1/export
> > > > 	echo 0 > pwmchip2/export
> > > > 
> > > > breaks because both want to create pwm0 in the class directory.
> > > 
> > > Ah, that makes more sense why that didn't work.
> > > 
> > > Ok, can the "name" of the new export chip be changed?  Is that
> > > hard-coded somewhere in userspace tools already?  Depending on that, the
> > > solution for this will change...
> > 
> > I know that back then, when sysfs for pwm was created, Thierry didn't
> > want to have one global namespace like gpio sysfs has. What you ask for
> > is something like:
> > 	pwm-{chipnumber}-{pwmnumber}
> > Right ? Can that be considered non-global ?
> 
> Yes, and that's just "global" for the pwm class namespace.
> 
> > Thierry's mail from back then is here:
> > https://lore.kernel.org/lkml/20130408081745.GA21392@avionic-0098.mockup.avionic-design.de/
> > 
> > A short search on github I found this:
> > https://github.com/vsergeev/c-periphery/blob/d34077d7ee45fa7d1947cc0174919452fac31597/src/pwm.c#L74
> > 
> > Seems to match your hardcoded criteria ?
> 
> Yes, ugh :(
> 
> Ok, now I see why the "lots of pwm classes!" patch was proposed.
> 
> And maybe that's really the only way forward here, as the chip namespace
> is the only unique thing.
> 
> But wow, it feels wrong...

Would the following feel better:
* use the new naming scheme you proposed for pwm's :
  pwm-{chipnumber}-{pwmnumber}
* assign the normal pwm class to the exported pwm devices. That lets
  them appear in the global /sys/class/pwm directory as e.g. pwm-0-0
* maintain backward compatibility through symlinks e.g.:
  pwmchip0/pwm0 -> ../pwm-0-0

Or does this feel even more horrible ?
I may miss some subtleties.

Regards,
Lars

Re: [PATCH] pwm: sysfs: Set class on pwm devices

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 5440 bytes --]

On Wed, Sep 30, 2020 at 04:13:52PM +0200, Lars Poeschel wrote:
> On Wed, Sep 30, 2020 at 01:51:06PM +0200, Greg Kroah-Hartman wrote:
> > On Wed, Sep 30, 2020 at 01:27:20PM +0200, Lars Poeschel wrote:
> > > On Wed, Sep 30, 2020 at 12:52:38PM +0200, Greg Kroah-Hartman wrote:
> > > > On Wed, Sep 30, 2020 at 12:01:26PM +0200, Uwe Kleine-König wrote:
> > > > > On Wed, Sep 30, 2020 at 11:52:04AM +0200, Greg Kroah-Hartman wrote:
> > > > > > On Wed, Sep 30, 2020 at 11:41:46AM +0200, Uwe Kleine-König wrote:
> > > > > > > Hello,
> > > > > > > 
> > > > > > > I added Greg Kroah-Hartman who I discussed this with via irc a bit to
> > > > > > > Cc:.
> > > > > > > 
> > > > > > > On Wed, Sep 30, 2020 at 11:20:56AM +0200, Lars Poeschel wrote:
> > > > > > > > thank you for your review!
> > > > > > > > 
> > > > > > > > On Wed, Sep 30, 2020 at 08:57:26AM +0200, Uwe Kleine-König wrote:
> > > > > > > > > On Tue, Sep 29, 2020 at 02:19:53PM +0200, poeschel@lemonage.de wrote:
> > > > > > > > > > From: Lars Poeschel <poeschel@lemonage.de>
> > > > > > > > > > 
> > > > > > > > > > This adds a class to exported pwm devices.
> > > > > > > > > > Exporting a pwm through sysfs did not yield udev events. The
> > > > > > > > > 
> > > > > > > > > I wonder what is your use-case here. This for sure also has a place to
> > > > > > > > > be mentioned in the commit log. I suspect there is a better way to
> > > > > > > > > accomplish you way.
> > > > > > > > 
> > > > > > > > Use-case is to be able to use a pwm from a non-root userspace process.
> > > > > > > > I use udev rules to adjust permissions.
> > > > > > > 
> > > > > > > Hmm, how do you trigger the export? Without being aware of all the
> > > > > > > details in the sysfs code I would expect that the exported stuff is
> > > > > > > available instantly once the write used to export the PWM is completed.
> > > > > > > So changing the permissions can be done directly after triggering the
> > > > > > > export in the same process.
> > > > > > 
> > > > > > It looks like userspace wants to see when a pwmX device shows up, right?
> > > > > > 
> > > > > > And it's not because those devices do not belong to any class or bus, so
> > > > > > they are just "floating" out there (they might show up under
> > > > > > /sys/bus/virtual, if you set things up right, which I don't think is
> > > > > > happening here...)
> > > > > > 
> > > > > > So yes, you need to create a class, or assign this to a bus, which is
> > > > > > fine, but it looks like no one is doing that.  Don't create new classes
> > > > > > dynamically, but rather, just assign this to the existing pwm class.
> > > > > > What's wrong with that?  I saw an older patch that did that, what did
> > > > > > that break?
> > > > > 
> > > > > Are you refering to 7e5d1fd75c3dde9fc10c4472b9368089d1b81d00? Did you
> > > > > read the reverting commit's log message? (i.e.
> > > > > c289d6625237aa785b484b4e94c23b3b91ea7e60)
> > > > > 
> > > > > I guess the breakage is that the resulting name then is:
> > > > > 
> > > > > 	"pwm%d", pwm->id
> > > > > 
> > > > > where pwm->id is a number unique to the pwmchip. So doing
> > > > > 
> > > > > 	echo 0 > pwmchip1/export
> > > > > 	echo 0 > pwmchip2/export
> > > > > 
> > > > > breaks because both want to create pwm0 in the class directory.
> > > > 
> > > > Ah, that makes more sense why that didn't work.
> > > > 
> > > > Ok, can the "name" of the new export chip be changed?  Is that
> > > > hard-coded somewhere in userspace tools already?  Depending on that, the
> > > > solution for this will change...
> > > 
> > > I know that back then, when sysfs for pwm was created, Thierry didn't
> > > want to have one global namespace like gpio sysfs has. What you ask for
> > > is something like:
> > > 	pwm-{chipnumber}-{pwmnumber}
> > > Right ? Can that be considered non-global ?
> > 
> > Yes, and that's just "global" for the pwm class namespace.
> > 
> > > Thierry's mail from back then is here:
> > > https://lore.kernel.org/lkml/20130408081745.GA21392@avionic-0098.mockup.avionic-design.de/
> > > 
> > > A short search on github I found this:
> > > https://github.com/vsergeev/c-periphery/blob/d34077d7ee45fa7d1947cc0174919452fac31597/src/pwm.c#L74
> > > 
> > > Seems to match your hardcoded criteria ?
> > 
> > Yes, ugh :(
> > 
> > Ok, now I see why the "lots of pwm classes!" patch was proposed.
> > 
> > And maybe that's really the only way forward here, as the chip namespace
> > is the only unique thing.
> > 
> > But wow, it feels wrong...
> 
> Would the following feel better:
> * use the new naming scheme you proposed for pwm's :
>   pwm-{chipnumber}-{pwmnumber}
> * assign the normal pwm class to the exported pwm devices. That lets
>   them appear in the global /sys/class/pwm directory as e.g. pwm-0-0
> * maintain backward compatibility through symlinks e.g.:
>   pwmchip0/pwm0 -> ../pwm-0-0

My preferred way forward is: Create a proper device (i.e. something like
/dev/pwmchipX) and make that usable with atomic operations. Then we
don't need to go through sysfs and can modify more than one property at
a time.

But other than that your suggestion sounds better than one class per
chip.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] pwm: sysfs: Set class on pwm devices

[Greg Kroah-Hartman]

On Wed, Sep 30, 2020 at 05:03:02PM +0200, Uwe Kleine-König wrote:
> On Wed, Sep 30, 2020 at 04:13:52PM +0200, Lars Poeschel wrote:
> > On Wed, Sep 30, 2020 at 01:51:06PM +0200, Greg Kroah-Hartman wrote:
> > > On Wed, Sep 30, 2020 at 01:27:20PM +0200, Lars Poeschel wrote:
> > > > On Wed, Sep 30, 2020 at 12:52:38PM +0200, Greg Kroah-Hartman wrote:
> > > > > On Wed, Sep 30, 2020 at 12:01:26PM +0200, Uwe Kleine-König wrote:
> > > > > > On Wed, Sep 30, 2020 at 11:52:04AM +0200, Greg Kroah-Hartman wrote:
> > > > > > > On Wed, Sep 30, 2020 at 11:41:46AM +0200, Uwe Kleine-König wrote:
> > > > > > > > Hello,
> > > > > > > > 
> > > > > > > > I added Greg Kroah-Hartman who I discussed this with via irc a bit to
> > > > > > > > Cc:.
> > > > > > > > 
> > > > > > > > On Wed, Sep 30, 2020 at 11:20:56AM +0200, Lars Poeschel wrote:
> > > > > > > > > thank you for your review!
> > > > > > > > > 
> > > > > > > > > On Wed, Sep 30, 2020 at 08:57:26AM +0200, Uwe Kleine-König wrote:
> > > > > > > > > > On Tue, Sep 29, 2020 at 02:19:53PM +0200, poeschel@lemonage.de wrote:
> > > > > > > > > > > From: Lars Poeschel <poeschel@lemonage.de>
> > > > > > > > > > > 
> > > > > > > > > > > This adds a class to exported pwm devices.
> > > > > > > > > > > Exporting a pwm through sysfs did not yield udev events. The
> > > > > > > > > > 
> > > > > > > > > > I wonder what is your use-case here. This for sure also has a place to
> > > > > > > > > > be mentioned in the commit log. I suspect there is a better way to
> > > > > > > > > > accomplish you way.
> > > > > > > > > 
> > > > > > > > > Use-case is to be able to use a pwm from a non-root userspace process.
> > > > > > > > > I use udev rules to adjust permissions.
> > > > > > > > 
> > > > > > > > Hmm, how do you trigger the export? Without being aware of all the
> > > > > > > > details in the sysfs code I would expect that the exported stuff is
> > > > > > > > available instantly once the write used to export the PWM is completed.
> > > > > > > > So changing the permissions can be done directly after triggering the
> > > > > > > > export in the same process.
> > > > > > > 
> > > > > > > It looks like userspace wants to see when a pwmX device shows up, right?
> > > > > > > 
> > > > > > > And it's not because those devices do not belong to any class or bus, so
> > > > > > > they are just "floating" out there (they might show up under
> > > > > > > /sys/bus/virtual, if you set things up right, which I don't think is
> > > > > > > happening here...)
> > > > > > > 
> > > > > > > So yes, you need to create a class, or assign this to a bus, which is
> > > > > > > fine, but it looks like no one is doing that.  Don't create new classes
> > > > > > > dynamically, but rather, just assign this to the existing pwm class.
> > > > > > > What's wrong with that?  I saw an older patch that did that, what did
> > > > > > > that break?
> > > > > > 
> > > > > > Are you refering to 7e5d1fd75c3dde9fc10c4472b9368089d1b81d00? Did you
> > > > > > read the reverting commit's log message? (i.e.
> > > > > > c289d6625237aa785b484b4e94c23b3b91ea7e60)
> > > > > > 
> > > > > > I guess the breakage is that the resulting name then is:
> > > > > > 
> > > > > > 	"pwm%d", pwm->id
> > > > > > 
> > > > > > where pwm->id is a number unique to the pwmchip. So doing
> > > > > > 
> > > > > > 	echo 0 > pwmchip1/export
> > > > > > 	echo 0 > pwmchip2/export
> > > > > > 
> > > > > > breaks because both want to create pwm0 in the class directory.
> > > > > 
> > > > > Ah, that makes more sense why that didn't work.
> > > > > 
> > > > > Ok, can the "name" of the new export chip be changed?  Is that
> > > > > hard-coded somewhere in userspace tools already?  Depending on that, the
> > > > > solution for this will change...
> > > > 
> > > > I know that back then, when sysfs for pwm was created, Thierry didn't
> > > > want to have one global namespace like gpio sysfs has. What you ask for
> > > > is something like:
> > > > 	pwm-{chipnumber}-{pwmnumber}
> > > > Right ? Can that be considered non-global ?
> > > 
> > > Yes, and that's just "global" for the pwm class namespace.
> > > 
> > > > Thierry's mail from back then is here:
> > > > https://lore.kernel.org/lkml/20130408081745.GA21392@avionic-0098.mockup.avionic-design.de/
> > > > 
> > > > A short search on github I found this:
> > > > https://github.com/vsergeev/c-periphery/blob/d34077d7ee45fa7d1947cc0174919452fac31597/src/pwm.c#L74
> > > > 
> > > > Seems to match your hardcoded criteria ?
> > > 
> > > Yes, ugh :(
> > > 
> > > Ok, now I see why the "lots of pwm classes!" patch was proposed.
> > > 
> > > And maybe that's really the only way forward here, as the chip namespace
> > > is the only unique thing.
> > > 
> > > But wow, it feels wrong...
> > 
> > Would the following feel better:
> > * use the new naming scheme you proposed for pwm's :
> >   pwm-{chipnumber}-{pwmnumber}
> > * assign the normal pwm class to the exported pwm devices. That lets
> >   them appear in the global /sys/class/pwm directory as e.g. pwm-0-0
> > * maintain backward compatibility through symlinks e.g.:
> >   pwmchip0/pwm0 -> ../pwm-0-0
> 
> My preferred way forward is: Create a proper device (i.e. something like
> /dev/pwmchipX) and make that usable with atomic operations. Then we
> don't need to go through sysfs and can modify more than one property at
> a time.

Kind of like what gpio did with the move to a char device node instead
of sysfs files?  That's fine with me if you all want to do that.

> But other than that your suggestion sounds better than one class per
> chip.

I agree, that seems sane.  Well, semi-sane, symlinks are "fun" :)

thanks,

greg k-h

Re: [PATCH] pwm: sysfs: Set class on pwm devices

[Lars Poeschel]

On Wed, Sep 30, 2020 at 11:41:46AM +0200, Uwe Kleine-König wrote:
> Hello,
> 
> I added Greg Kroah-Hartman who I discussed this with via irc a bit to
> Cc:.
> 
> On Wed, Sep 30, 2020 at 11:20:56AM +0200, Lars Poeschel wrote:
> > thank you for your review!
> > 
> > On Wed, Sep 30, 2020 at 08:57:26AM +0200, Uwe Kleine-König wrote:
> > > On Tue, Sep 29, 2020 at 02:19:53PM +0200, poeschel@lemonage.de wrote:
> > > > From: Lars Poeschel <poeschel@lemonage.de>
> > > > 
> > > > This adds a class to exported pwm devices.
> > > > Exporting a pwm through sysfs did not yield udev events. The
> > > 
> > > I wonder what is your use-case here. This for sure also has a place to
> > > be mentioned in the commit log. I suspect there is a better way to
> > > accomplish you way.
> > 
> > Use-case is to be able to use a pwm from a non-root userspace process.
> > I use udev rules to adjust permissions.
> 
> Hmm, how do you trigger the export? Without being aware of all the
> details in the sysfs code I would expect that the exported stuff is
> available instantly once the write used to export the PWM is completed.
> So changing the permissions can be done directly after triggering the
> export in the same process.

The export is triggered through the userspace process itself. Why can it
do this ? Because there is another udev rule, that changes permissions
when a pwmchip appears.
Then I'd like to have the second udev rule, that changes permissions on
the freshly exported pwm. The userspace process can't do this.
You are right I could propably do everything from within udev: If a
pwmchip appears, export certain pwms and right away change their
permissions. It does not also not feel right. It'd require knowledge
from the userspace application to be mapped to udev.

> Out of interest: What do you use the pwm for? Isn't there a suitable
> kernel driver that can do the required stuff? Compared to the kernel-API
> the sysfs interface isn't atomic. Is this an annoyance?

Use-case is generating a voltage from the pwm. This voltage is used to
signal different states and does not change very often. This is
absolutely not annoying that this is not atomic. We just change the duty
cycle on the fly. Everything else is configured one time at startup.
I'd call what I need pwm-dac. I could not find a ready to use driver.
Maybe I could misuse some kernel driver for this. Maybe I could use
pwm-led or pwm-brightness or pwm-fan. Propably pwm-regulator could work,
there is even a userspace facing part for this, but this is not
devicetree ready.
...and the worst, please don't blame me: The application is java, so
ioctl is a problem.
So, sysfs was quite a good choice for me.

Regards,
Lars

Re: [PATCH] pwm: sysfs: Set class on pwm devices

[Greg Kroah-Hartman]

On Thu, Oct 01, 2020 at 11:05:31AM +0200, Lars Poeschel wrote:
> On Wed, Sep 30, 2020 at 11:41:46AM +0200, Uwe Kleine-König wrote:
> > Hello,
> > 
> > I added Greg Kroah-Hartman who I discussed this with via irc a bit to
> > Cc:.
> > 
> > On Wed, Sep 30, 2020 at 11:20:56AM +0200, Lars Poeschel wrote:
> > > thank you for your review!
> > > 
> > > On Wed, Sep 30, 2020 at 08:57:26AM +0200, Uwe Kleine-König wrote:
> > > > On Tue, Sep 29, 2020 at 02:19:53PM +0200, poeschel@lemonage.de wrote:
> > > > > From: Lars Poeschel <poeschel@lemonage.de>
> > > > > 
> > > > > This adds a class to exported pwm devices.
> > > > > Exporting a pwm through sysfs did not yield udev events. The
> > > > 
> > > > I wonder what is your use-case here. This for sure also has a place to
> > > > be mentioned in the commit log. I suspect there is a better way to
> > > > accomplish you way.
> > > 
> > > Use-case is to be able to use a pwm from a non-root userspace process.
> > > I use udev rules to adjust permissions.
> > 
> > Hmm, how do you trigger the export? Without being aware of all the
> > details in the sysfs code I would expect that the exported stuff is
> > available instantly once the write used to export the PWM is completed.
> > So changing the permissions can be done directly after triggering the
> > export in the same process.
> 
> The export is triggered through the userspace process itself. Why can it
> do this ? Because there is another udev rule, that changes permissions
> when a pwmchip appears.
> Then I'd like to have the second udev rule, that changes permissions on
> the freshly exported pwm. The userspace process can't do this.
> You are right I could propably do everything from within udev: If a
> pwmchip appears, export certain pwms and right away change their
> permissions. It does not also not feel right. It'd require knowledge
> from the userspace application to be mapped to udev.

The way the kernel code is now, yes, you will not have any way to
trigger it by userspace as the kernel is creating a "raw" struct device
that isn't assigned to anything.  That is what needs to be fixed here.

> > Out of interest: What do you use the pwm for? Isn't there a suitable
> > kernel driver that can do the required stuff? Compared to the kernel-API
> > the sysfs interface isn't atomic. Is this an annoyance?
> 
> Use-case is generating a voltage from the pwm. This voltage is used to
> signal different states and does not change very often. This is
> absolutely not annoying that this is not atomic. We just change the duty
> cycle on the fly. Everything else is configured one time at startup.
> I'd call what I need pwm-dac. I could not find a ready to use driver.
> Maybe I could misuse some kernel driver for this. Maybe I could use
> pwm-led or pwm-brightness or pwm-fan. Propably pwm-regulator could work,
> there is even a userspace facing part for this, but this is not
> devicetree ready.
> ...and the worst, please don't blame me: The application is java, so
> ioctl is a problem.

I thought java could do ioctls, otherwise how would it ever be able to
talk to serial ports?

Anyway, this needs to be fixed in the kernel...

thanks,

greg k-h

Re: [PATCH] pwm: sysfs: Set class on pwm devices

[Lars Poeschel]

On Thu, Oct 01, 2020 at 01:24:49PM +0200, Greg Kroah-Hartman wrote:
> On Thu, Oct 01, 2020 at 11:05:31AM +0200, Lars Poeschel wrote:
> > On Wed, Sep 30, 2020 at 11:41:46AM +0200, Uwe Kleine-König wrote:
> > > Hello,
> > > 
> > > I added Greg Kroah-Hartman who I discussed this with via irc a bit to
> > > Cc:.
> > > 
> > > On Wed, Sep 30, 2020 at 11:20:56AM +0200, Lars Poeschel wrote:
> > > > thank you for your review!
> > > > 
> > > > On Wed, Sep 30, 2020 at 08:57:26AM +0200, Uwe Kleine-König wrote:
> > > > > On Tue, Sep 29, 2020 at 02:19:53PM +0200, poeschel@lemonage.de wrote:
> > > > > > From: Lars Poeschel <poeschel@lemonage.de>
> > > > > > 
> > > > > > This adds a class to exported pwm devices.
> > > > > > Exporting a pwm through sysfs did not yield udev events. The
> > > > > 
> > > > > I wonder what is your use-case here. This for sure also has a place to
> > > > > be mentioned in the commit log. I suspect there is a better way to
> > > > > accomplish you way.
> > > > 
> > > > Use-case is to be able to use a pwm from a non-root userspace process.
> > > > I use udev rules to adjust permissions.
> > > 
> > > Hmm, how do you trigger the export? Without being aware of all the
> > > details in the sysfs code I would expect that the exported stuff is
> > > available instantly once the write used to export the PWM is completed.
> > > So changing the permissions can be done directly after triggering the
> > > export in the same process.
> > 
> > The export is triggered through the userspace process itself. Why can it
> > do this ? Because there is another udev rule, that changes permissions
> > when a pwmchip appears.
> > Then I'd like to have the second udev rule, that changes permissions on
> > the freshly exported pwm. The userspace process can't do this.
> > You are right I could propably do everything from within udev: If a
> > pwmchip appears, export certain pwms and right away change their
> > permissions. It does not also not feel right. It'd require knowledge
> > from the userspace application to be mapped to udev.
> 
> The way the kernel code is now, yes, you will not have any way to
> trigger it by userspace as the kernel is creating a "raw" struct device
> that isn't assigned to anything.  That is what needs to be fixed here.

I did a first try with our approach.
I set the class of the child to its parent class. This does work and
create the directories right under /sys/pwm but because the child now
also inherits the dev_groups from the parent its directory also contain
export, unexport and npwm files, that don't apply for pwm's as soon a I
register the device to driver core.

Did we miss something or is there a way to avoid that ? I had a look at
device_add and saw that as soon as a class it set it's dev_groups get
exported through device_add_attrs.

> > > Out of interest: What do you use the pwm for? Isn't there a suitable
> > > kernel driver that can do the required stuff? Compared to the kernel-API
> > > the sysfs interface isn't atomic. Is this an annoyance?
> > 
> > Use-case is generating a voltage from the pwm. This voltage is used to
> > signal different states and does not change very often. This is
> > absolutely not annoying that this is not atomic. We just change the duty
> > cycle on the fly. Everything else is configured one time at startup.
> > I'd call what I need pwm-dac. I could not find a ready to use driver.
> > Maybe I could misuse some kernel driver for this. Maybe I could use
> > pwm-led or pwm-brightness or pwm-fan. Propably pwm-regulator could work,
> > there is even a userspace facing part for this, but this is not
> > devicetree ready.
> > ...and the worst, please don't blame me: The application is java, so
> > ioctl is a problem.
> 
> I thought java could do ioctls, otherwise how would it ever be able to
> talk to serial ports?

It is not impossible but it is horrible. java itself can access the
ports through normal file I/O, but it can not set control lines or
baudrate or anything. You need some C-code for this, that is not part of
the java vm itself and has to be called through something called JNI -
java native interface.

Regards,
Lars

Re: [PATCH] pwm: sysfs: Set class on pwm devices

[Greg Kroah-Hartman]

On Thu, Oct 01, 2020 at 03:50:09PM +0200, Lars Poeschel wrote:
> On Thu, Oct 01, 2020 at 01:24:49PM +0200, Greg Kroah-Hartman wrote:
> > On Thu, Oct 01, 2020 at 11:05:31AM +0200, Lars Poeschel wrote:
> > > On Wed, Sep 30, 2020 at 11:41:46AM +0200, Uwe Kleine-König wrote:
> > > > Hello,
> > > > 
> > > > I added Greg Kroah-Hartman who I discussed this with via irc a bit to
> > > > Cc:.
> > > > 
> > > > On Wed, Sep 30, 2020 at 11:20:56AM +0200, Lars Poeschel wrote:
> > > > > thank you for your review!
> > > > > 
> > > > > On Wed, Sep 30, 2020 at 08:57:26AM +0200, Uwe Kleine-König wrote:
> > > > > > On Tue, Sep 29, 2020 at 02:19:53PM +0200, poeschel@lemonage.de wrote:
> > > > > > > From: Lars Poeschel <poeschel@lemonage.de>
> > > > > > > 
> > > > > > > This adds a class to exported pwm devices.
> > > > > > > Exporting a pwm through sysfs did not yield udev events. The
> > > > > > 
> > > > > > I wonder what is your use-case here. This for sure also has a place to
> > > > > > be mentioned in the commit log. I suspect there is a better way to
> > > > > > accomplish you way.
> > > > > 
> > > > > Use-case is to be able to use a pwm from a non-root userspace process.
> > > > > I use udev rules to adjust permissions.
> > > > 
> > > > Hmm, how do you trigger the export? Without being aware of all the
> > > > details in the sysfs code I would expect that the exported stuff is
> > > > available instantly once the write used to export the PWM is completed.
> > > > So changing the permissions can be done directly after triggering the
> > > > export in the same process.
> > > 
> > > The export is triggered through the userspace process itself. Why can it
> > > do this ? Because there is another udev rule, that changes permissions
> > > when a pwmchip appears.
> > > Then I'd like to have the second udev rule, that changes permissions on
> > > the freshly exported pwm. The userspace process can't do this.
> > > You are right I could propably do everything from within udev: If a
> > > pwmchip appears, export certain pwms and right away change their
> > > permissions. It does not also not feel right. It'd require knowledge
> > > from the userspace application to be mapped to udev.
> > 
> > The way the kernel code is now, yes, you will not have any way to
> > trigger it by userspace as the kernel is creating a "raw" struct device
> > that isn't assigned to anything.  That is what needs to be fixed here.
> 
> I did a first try with our approach.
> I set the class of the child to its parent class. This does work and
> create the directories right under /sys/pwm but because the child now
> also inherits the dev_groups from the parent its directory also contain
> export, unexport and npwm files, that don't apply for pwm's as soon a I
> register the device to driver core.
> 
> Did we miss something or is there a way to avoid that ? I had a look at
> device_add and saw that as soon as a class it set it's dev_groups get
> exported through device_add_attrs.

Ah, you need to tweak that group to only show up for a specific "type"
of device.  There is a is_visable callback for a group that should be
used for this, and you can check the type of device being affected here,
try messing with that.  And make sure you set a new type for the new
devices you are creating.

I know that's vague, if you need more help I can work on it next week.

> > > > Out of interest: What do you use the pwm for? Isn't there a suitable
> > > > kernel driver that can do the required stuff? Compared to the kernel-API
> > > > the sysfs interface isn't atomic. Is this an annoyance?
> > > 
> > > Use-case is generating a voltage from the pwm. This voltage is used to
> > > signal different states and does not change very often. This is
> > > absolutely not annoying that this is not atomic. We just change the duty
> > > cycle on the fly. Everything else is configured one time at startup.
> > > I'd call what I need pwm-dac. I could not find a ready to use driver.
> > > Maybe I could misuse some kernel driver for this. Maybe I could use
> > > pwm-led or pwm-brightness or pwm-fan. Propably pwm-regulator could work,
> > > there is even a userspace facing part for this, but this is not
> > > devicetree ready.
> > > ...and the worst, please don't blame me: The application is java, so
> > > ioctl is a problem.
> > 
> > I thought java could do ioctls, otherwise how would it ever be able to
> > talk to serial ports?
> 
> It is not impossible but it is horrible. java itself can access the
> ports through normal file I/O, but it can not set control lines or
> baudrate or anything. You need some C-code for this, that is not part of
> the java vm itself and has to be called through something called JNI -
> java native interface.

Ah, ok, yeah, that's not ok, sorry to hear you are stuck with Java :(

greg k-h

Re: [PATCH] pwm: sysfs: Set class on pwm devices

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 4359 bytes --]

On Thu, Oct 01, 2020 at 01:24:49PM +0200, Greg Kroah-Hartman wrote:
> On Thu, Oct 01, 2020 at 11:05:31AM +0200, Lars Poeschel wrote:
> > On Wed, Sep 30, 2020 at 11:41:46AM +0200, Uwe Kleine-König wrote:
> > > Hello,
> > > 
> > > I added Greg Kroah-Hartman who I discussed this with via irc a bit to
> > > Cc:.
> > > 
> > > On Wed, Sep 30, 2020 at 11:20:56AM +0200, Lars Poeschel wrote:
> > > > thank you for your review!
> > > > 
> > > > On Wed, Sep 30, 2020 at 08:57:26AM +0200, Uwe Kleine-König wrote:
> > > > > On Tue, Sep 29, 2020 at 02:19:53PM +0200, poeschel@lemonage.de wrote:
> > > > > > From: Lars Poeschel <poeschel@lemonage.de>
> > > > > > 
> > > > > > This adds a class to exported pwm devices.
> > > > > > Exporting a pwm through sysfs did not yield udev events. The
> > > > > 
> > > > > I wonder what is your use-case here. This for sure also has a place to
> > > > > be mentioned in the commit log. I suspect there is a better way to
> > > > > accomplish you way.
> > > > 
> > > > Use-case is to be able to use a pwm from a non-root userspace process.
> > > > I use udev rules to adjust permissions.
> > > 
> > > Hmm, how do you trigger the export? Without being aware of all the
> > > details in the sysfs code I would expect that the exported stuff is
> > > available instantly once the write used to export the PWM is completed.
> > > So changing the permissions can be done directly after triggering the
> > > export in the same process.
> > 
> > The export is triggered through the userspace process itself. Why can it
> > do this ? Because there is another udev rule, that changes permissions
> > when a pwmchip appears.
> > Then I'd like to have the second udev rule, that changes permissions on
> > the freshly exported pwm. The userspace process can't do this.
> > You are right I could propably do everything from within udev: If a
> > pwmchip appears, export certain pwms and right away change their
> > permissions. It does not also not feel right. It'd require knowledge
> > from the userspace application to be mapped to udev.
> 
> The way the kernel code is now, yes, you will not have any way to
> trigger it by userspace as the kernel is creating a "raw" struct device
> that isn't assigned to anything.  That is what needs to be fixed here.
> 
> > > Out of interest: What do you use the pwm for? Isn't there a suitable
> > > kernel driver that can do the required stuff? Compared to the kernel-API
> > > the sysfs interface isn't atomic. Is this an annoyance?
> > 
> > Use-case is generating a voltage from the pwm. This voltage is used to
> > signal different states and does not change very often. This is
> > absolutely not annoying that this is not atomic. We just change the duty
> > cycle on the fly. Everything else is configured one time at startup.
> > I'd call what I need pwm-dac. I could not find a ready to use driver.
> > Maybe I could misuse some kernel driver for this. Maybe I could use
> > pwm-led or pwm-brightness or pwm-fan. Propably pwm-regulator could work,
> > there is even a userspace facing part for this, but this is not
> > devicetree ready.
> > ...and the worst, please don't blame me: The application is java, so
> > ioctl is a problem.
> 
> I thought java could do ioctls, otherwise how would it ever be able to
> talk to serial ports?
> 
> Anyway, this needs to be fixed in the kernel...

If atomicity was a problem, we could potentially add a mechanism to the
sysfs interface to enable that. I don't see a good way of doing that in
a single file, since that works against how sysfs is designed. But one
thing I could imagine is adding a file ("lock", or whatever you want to
call it) that you can use for atomic fencing:

	$ echo 1 > lock # locks the hardware state
	$ echo 100 > period
	$ echo 50 > duty_cycle
	$ echo 0 > lock # flushes state to hardware

But it sounds like that's not even a big issue.

However, given the use-case description, it sounds to me like
pwm-regulator would be a better candidate to solve this, because it's
purpose is literally to generate a voltage using a PWM. There is a
device tree binding for pwm-regulator, so this should work there as
well.

Lars, what exactly are the problems that you're running into when trying
to use pwm-regulator with device tree?

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] pwm: sysfs: Set class on pwm devices

[Greg Kroah-Hartman]

On Mon, Oct 05, 2020 at 11:30:16AM +0200, Thierry Reding wrote:
> On Thu, Oct 01, 2020 at 01:24:49PM +0200, Greg Kroah-Hartman wrote:
> > On Thu, Oct 01, 2020 at 11:05:31AM +0200, Lars Poeschel wrote:
> > > On Wed, Sep 30, 2020 at 11:41:46AM +0200, Uwe Kleine-König wrote:
> > > > Hello,
> > > > 
> > > > I added Greg Kroah-Hartman who I discussed this with via irc a bit to
> > > > Cc:.
> > > > 
> > > > On Wed, Sep 30, 2020 at 11:20:56AM +0200, Lars Poeschel wrote:
> > > > > thank you for your review!
> > > > > 
> > > > > On Wed, Sep 30, 2020 at 08:57:26AM +0200, Uwe Kleine-König wrote:
> > > > > > On Tue, Sep 29, 2020 at 02:19:53PM +0200, poeschel@lemonage.de wrote:
> > > > > > > From: Lars Poeschel <poeschel@lemonage.de>
> > > > > > > 
> > > > > > > This adds a class to exported pwm devices.
> > > > > > > Exporting a pwm through sysfs did not yield udev events. The
> > > > > > 
> > > > > > I wonder what is your use-case here. This for sure also has a place to
> > > > > > be mentioned in the commit log. I suspect there is a better way to
> > > > > > accomplish you way.
> > > > > 
> > > > > Use-case is to be able to use a pwm from a non-root userspace process.
> > > > > I use udev rules to adjust permissions.
> > > > 
> > > > Hmm, how do you trigger the export? Without being aware of all the
> > > > details in the sysfs code I would expect that the exported stuff is
> > > > available instantly once the write used to export the PWM is completed.
> > > > So changing the permissions can be done directly after triggering the
> > > > export in the same process.
> > > 
> > > The export is triggered through the userspace process itself. Why can it
> > > do this ? Because there is another udev rule, that changes permissions
> > > when a pwmchip appears.
> > > Then I'd like to have the second udev rule, that changes permissions on
> > > the freshly exported pwm. The userspace process can't do this.
> > > You are right I could propably do everything from within udev: If a
> > > pwmchip appears, export certain pwms and right away change their
> > > permissions. It does not also not feel right. It'd require knowledge
> > > from the userspace application to be mapped to udev.
> > 
> > The way the kernel code is now, yes, you will not have any way to
> > trigger it by userspace as the kernel is creating a "raw" struct device
> > that isn't assigned to anything.  That is what needs to be fixed here.
> > 
> > > > Out of interest: What do you use the pwm for? Isn't there a suitable
> > > > kernel driver that can do the required stuff? Compared to the kernel-API
> > > > the sysfs interface isn't atomic. Is this an annoyance?
> > > 
> > > Use-case is generating a voltage from the pwm. This voltage is used to
> > > signal different states and does not change very often. This is
> > > absolutely not annoying that this is not atomic. We just change the duty
> > > cycle on the fly. Everything else is configured one time at startup.
> > > I'd call what I need pwm-dac. I could not find a ready to use driver.
> > > Maybe I could misuse some kernel driver for this. Maybe I could use
> > > pwm-led or pwm-brightness or pwm-fan. Propably pwm-regulator could work,
> > > there is even a userspace facing part for this, but this is not
> > > devicetree ready.
> > > ...and the worst, please don't blame me: The application is java, so
> > > ioctl is a problem.
> > 
> > I thought java could do ioctls, otherwise how would it ever be able to
> > talk to serial ports?
> > 
> > Anyway, this needs to be fixed in the kernel...
> 
> If atomicity was a problem, we could potentially add a mechanism to the
> sysfs interface to enable that. I don't see a good way of doing that in
> a single file, since that works against how sysfs is designed. But one
> thing I could imagine is adding a file ("lock", or whatever you want to
> call it) that you can use for atomic fencing:
> 
> 	$ echo 1 > lock # locks the hardware state
> 	$ echo 100 > period
> 	$ echo 50 > duty_cycle
> 	$ echo 0 > lock # flushes state to hardware
> 
> But it sounds like that's not even a big issue.

That is exactly what configfs was designed for :)

thanks,

greg k-h

Re: [PATCH] pwm: sysfs: Set class on pwm devices

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 5382 bytes --]

On Mon, Oct 05, 2020 at 11:45:30AM +0200, Greg Kroah-Hartman wrote:
> On Mon, Oct 05, 2020 at 11:30:16AM +0200, Thierry Reding wrote:
> > On Thu, Oct 01, 2020 at 01:24:49PM +0200, Greg Kroah-Hartman wrote:
> > > On Thu, Oct 01, 2020 at 11:05:31AM +0200, Lars Poeschel wrote:
> > > > On Wed, Sep 30, 2020 at 11:41:46AM +0200, Uwe Kleine-König wrote:
> > > > > Hello,
> > > > > 
> > > > > I added Greg Kroah-Hartman who I discussed this with via irc a bit to
> > > > > Cc:.
> > > > > 
> > > > > On Wed, Sep 30, 2020 at 11:20:56AM +0200, Lars Poeschel wrote:
> > > > > > thank you for your review!
> > > > > > 
> > > > > > On Wed, Sep 30, 2020 at 08:57:26AM +0200, Uwe Kleine-König wrote:
> > > > > > > On Tue, Sep 29, 2020 at 02:19:53PM +0200, poeschel@lemonage.de wrote:
> > > > > > > > From: Lars Poeschel <poeschel@lemonage.de>
> > > > > > > > 
> > > > > > > > This adds a class to exported pwm devices.
> > > > > > > > Exporting a pwm through sysfs did not yield udev events. The
> > > > > > > 
> > > > > > > I wonder what is your use-case here. This for sure also has a place to
> > > > > > > be mentioned in the commit log. I suspect there is a better way to
> > > > > > > accomplish you way.
> > > > > > 
> > > > > > Use-case is to be able to use a pwm from a non-root userspace process.
> > > > > > I use udev rules to adjust permissions.
> > > > > 
> > > > > Hmm, how do you trigger the export? Without being aware of all the
> > > > > details in the sysfs code I would expect that the exported stuff is
> > > > > available instantly once the write used to export the PWM is completed.
> > > > > So changing the permissions can be done directly after triggering the
> > > > > export in the same process.
> > > > 
> > > > The export is triggered through the userspace process itself. Why can it
> > > > do this ? Because there is another udev rule, that changes permissions
> > > > when a pwmchip appears.
> > > > Then I'd like to have the second udev rule, that changes permissions on
> > > > the freshly exported pwm. The userspace process can't do this.
> > > > You are right I could propably do everything from within udev: If a
> > > > pwmchip appears, export certain pwms and right away change their
> > > > permissions. It does not also not feel right. It'd require knowledge
> > > > from the userspace application to be mapped to udev.
> > > 
> > > The way the kernel code is now, yes, you will not have any way to
> > > trigger it by userspace as the kernel is creating a "raw" struct device
> > > that isn't assigned to anything.  That is what needs to be fixed here.
> > > 
> > > > > Out of interest: What do you use the pwm for? Isn't there a suitable
> > > > > kernel driver that can do the required stuff? Compared to the kernel-API
> > > > > the sysfs interface isn't atomic. Is this an annoyance?
> > > > 
> > > > Use-case is generating a voltage from the pwm. This voltage is used to
> > > > signal different states and does not change very often. This is
> > > > absolutely not annoying that this is not atomic. We just change the duty
> > > > cycle on the fly. Everything else is configured one time at startup.
> > > > I'd call what I need pwm-dac. I could not find a ready to use driver.
> > > > Maybe I could misuse some kernel driver for this. Maybe I could use
> > > > pwm-led or pwm-brightness or pwm-fan. Propably pwm-regulator could work,
> > > > there is even a userspace facing part for this, but this is not
> > > > devicetree ready.
> > > > ...and the worst, please don't blame me: The application is java, so
> > > > ioctl is a problem.
> > > 
> > > I thought java could do ioctls, otherwise how would it ever be able to
> > > talk to serial ports?
> > > 
> > > Anyway, this needs to be fixed in the kernel...
> > 
> > If atomicity was a problem, we could potentially add a mechanism to the
> > sysfs interface to enable that. I don't see a good way of doing that in
> > a single file, since that works against how sysfs is designed. But one
> > thing I could imagine is adding a file ("lock", or whatever you want to
> > call it) that you can use for atomic fencing:
> > 
> > 	$ echo 1 > lock # locks the hardware state
> > 	$ echo 100 > period
> > 	$ echo 50 > duty_cycle
> > 	$ echo 0 > lock # flushes state to hardware
> > 
> > But it sounds like that's not even a big issue.
> 
> That is exactly what configfs was designed for :)

Interesting... for some reason I had never thought about configfs in
this context. But it does indeed sound like it could solve this problem
in a better way.

My memory is a bit sketchy, but I think for USB device controllers this
works by exposing each controller in configfs and then configuring
things like endpoints within the controller's directory. So I wonder if
instead of requesting PWMs via sysfs, we should rather expose them via
configfs items.

Something like:

	# mkdir /configfs/7000a000.pwm/0

could then be the equivalent of exporting PWM channel 0 of the given PWM
chip in sysfs, except that now you get a configfs directory with
attributes that you can use to inspect and reconfigure the PWM channel
and ultimately apply the changes atomically.

How does that work from a permissions point of view? How do we ensure
that people don't need root privileges to access these?

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] pwm: sysfs: Set class on pwm devices

[Greg Kroah-Hartman]

On Mon, Oct 05, 2020 at 12:17:21PM +0200, Thierry Reding wrote:
> On Mon, Oct 05, 2020 at 11:45:30AM +0200, Greg Kroah-Hartman wrote:
> > On Mon, Oct 05, 2020 at 11:30:16AM +0200, Thierry Reding wrote:
> > > On Thu, Oct 01, 2020 at 01:24:49PM +0200, Greg Kroah-Hartman wrote:
> > > > On Thu, Oct 01, 2020 at 11:05:31AM +0200, Lars Poeschel wrote:
> > > > > On Wed, Sep 30, 2020 at 11:41:46AM +0200, Uwe Kleine-König wrote:
> > > > > > Hello,
> > > > > > 
> > > > > > I added Greg Kroah-Hartman who I discussed this with via irc a bit to
> > > > > > Cc:.
> > > > > > 
> > > > > > On Wed, Sep 30, 2020 at 11:20:56AM +0200, Lars Poeschel wrote:
> > > > > > > thank you for your review!
> > > > > > > 
> > > > > > > On Wed, Sep 30, 2020 at 08:57:26AM +0200, Uwe Kleine-König wrote:
> > > > > > > > On Tue, Sep 29, 2020 at 02:19:53PM +0200, poeschel@lemonage.de wrote:
> > > > > > > > > From: Lars Poeschel <poeschel@lemonage.de>
> > > > > > > > > 
> > > > > > > > > This adds a class to exported pwm devices.
> > > > > > > > > Exporting a pwm through sysfs did not yield udev events. The
> > > > > > > > 
> > > > > > > > I wonder what is your use-case here. This for sure also has a place to
> > > > > > > > be mentioned in the commit log. I suspect there is a better way to
> > > > > > > > accomplish you way.
> > > > > > > 
> > > > > > > Use-case is to be able to use a pwm from a non-root userspace process.
> > > > > > > I use udev rules to adjust permissions.
> > > > > > 
> > > > > > Hmm, how do you trigger the export? Without being aware of all the
> > > > > > details in the sysfs code I would expect that the exported stuff is
> > > > > > available instantly once the write used to export the PWM is completed.
> > > > > > So changing the permissions can be done directly after triggering the
> > > > > > export in the same process.
> > > > > 
> > > > > The export is triggered through the userspace process itself. Why can it
> > > > > do this ? Because there is another udev rule, that changes permissions
> > > > > when a pwmchip appears.
> > > > > Then I'd like to have the second udev rule, that changes permissions on
> > > > > the freshly exported pwm. The userspace process can't do this.
> > > > > You are right I could propably do everything from within udev: If a
> > > > > pwmchip appears, export certain pwms and right away change their
> > > > > permissions. It does not also not feel right. It'd require knowledge
> > > > > from the userspace application to be mapped to udev.
> > > > 
> > > > The way the kernel code is now, yes, you will not have any way to
> > > > trigger it by userspace as the kernel is creating a "raw" struct device
> > > > that isn't assigned to anything.  That is what needs to be fixed here.
> > > > 
> > > > > > Out of interest: What do you use the pwm for? Isn't there a suitable
> > > > > > kernel driver that can do the required stuff? Compared to the kernel-API
> > > > > > the sysfs interface isn't atomic. Is this an annoyance?
> > > > > 
> > > > > Use-case is generating a voltage from the pwm. This voltage is used to
> > > > > signal different states and does not change very often. This is
> > > > > absolutely not annoying that this is not atomic. We just change the duty
> > > > > cycle on the fly. Everything else is configured one time at startup.
> > > > > I'd call what I need pwm-dac. I could not find a ready to use driver.
> > > > > Maybe I could misuse some kernel driver for this. Maybe I could use
> > > > > pwm-led or pwm-brightness or pwm-fan. Propably pwm-regulator could work,
> > > > > there is even a userspace facing part for this, but this is not
> > > > > devicetree ready.
> > > > > ...and the worst, please don't blame me: The application is java, so
> > > > > ioctl is a problem.
> > > > 
> > > > I thought java could do ioctls, otherwise how would it ever be able to
> > > > talk to serial ports?
> > > > 
> > > > Anyway, this needs to be fixed in the kernel...
> > > 
> > > If atomicity was a problem, we could potentially add a mechanism to the
> > > sysfs interface to enable that. I don't see a good way of doing that in
> > > a single file, since that works against how sysfs is designed. But one
> > > thing I could imagine is adding a file ("lock", or whatever you want to
> > > call it) that you can use for atomic fencing:
> > > 
> > > 	$ echo 1 > lock # locks the hardware state
> > > 	$ echo 100 > period
> > > 	$ echo 50 > duty_cycle
> > > 	$ echo 0 > lock # flushes state to hardware
> > > 
> > > But it sounds like that's not even a big issue.
> > 
> > That is exactly what configfs was designed for :)
> 
> Interesting... for some reason I had never thought about configfs in
> this context. But it does indeed sound like it could solve this problem
> in a better way.
> 
> My memory is a bit sketchy, but I think for USB device controllers this
> works by exposing each controller in configfs and then configuring
> things like endpoints within the controller's directory. So I wonder if
> instead of requesting PWMs via sysfs, we should rather expose them via
> configfs items.
> 
> Something like:
> 
> 	# mkdir /configfs/7000a000.pwm/0
> 
> could then be the equivalent of exporting PWM channel 0 of the given PWM
> chip in sysfs, except that now you get a configfs directory with
> attributes that you can use to inspect and reconfigure the PWM channel
> and ultimately apply the changes atomically.
> 
> How does that work from a permissions point of view? How do we ensure
> that people don't need root privileges to access these?

To change things in configfs, yes, I'm pretty sure you need root access.
But to read things, sysfs is fine.  I don't really know what you are
wanting to do here, both create a device and change the options over
time?

thanks,

greg k-h

Re: [PATCH] pwm: sysfs: Set class on pwm devices

[Lars Poeschel]

On Mon, Oct 05, 2020 at 11:30:16AM +0200, Thierry Reding wrote:
> On Thu, Oct 01, 2020 at 01:24:49PM +0200, Greg Kroah-Hartman wrote:
> > On Thu, Oct 01, 2020 at 11:05:31AM +0200, Lars Poeschel wrote:
> > > On Wed, Sep 30, 2020 at 11:41:46AM +0200, Uwe Kleine-König wrote:
> > > > Hello,
> > > > 
> > > > I added Greg Kroah-Hartman who I discussed this with via irc a bit to
> > > > Cc:.
> > > > 
> > > > On Wed, Sep 30, 2020 at 11:20:56AM +0200, Lars Poeschel wrote:
> > > > > thank you for your review!
> > > > > 
> > > > > On Wed, Sep 30, 2020 at 08:57:26AM +0200, Uwe Kleine-König wrote:
> > > > > > On Tue, Sep 29, 2020 at 02:19:53PM +0200, poeschel@lemonage.de wrote:
> > > > > > > From: Lars Poeschel <poeschel@lemonage.de>
> > > > > > > 
> > > > > > > This adds a class to exported pwm devices.
> > > > > > > Exporting a pwm through sysfs did not yield udev events. The
> > > > > > 
> > > > > > I wonder what is your use-case here. This for sure also has a place to
> > > > > > be mentioned in the commit log. I suspect there is a better way to
> > > > > > accomplish you way.
> > > > > 
> > > > > Use-case is to be able to use a pwm from a non-root userspace process.
> > > > > I use udev rules to adjust permissions.
> > > > 
> > > > Hmm, how do you trigger the export? Without being aware of all the
> > > > details in the sysfs code I would expect that the exported stuff is
> > > > available instantly once the write used to export the PWM is completed.
> > > > So changing the permissions can be done directly after triggering the
> > > > export in the same process.
> > > 
> > > The export is triggered through the userspace process itself. Why can it
> > > do this ? Because there is another udev rule, that changes permissions
> > > when a pwmchip appears.
> > > Then I'd like to have the second udev rule, that changes permissions on
> > > the freshly exported pwm. The userspace process can't do this.
> > > You are right I could propably do everything from within udev: If a
> > > pwmchip appears, export certain pwms and right away change their
> > > permissions. It does not also not feel right. It'd require knowledge
> > > from the userspace application to be mapped to udev.
> > 
> > The way the kernel code is now, yes, you will not have any way to
> > trigger it by userspace as the kernel is creating a "raw" struct device
> > that isn't assigned to anything.  That is what needs to be fixed here.
> > 
> > > > Out of interest: What do you use the pwm for? Isn't there a suitable
> > > > kernel driver that can do the required stuff? Compared to the kernel-API
> > > > the sysfs interface isn't atomic. Is this an annoyance?
> > > 
> > > Use-case is generating a voltage from the pwm. This voltage is used to
> > > signal different states and does not change very often. This is
> > > absolutely not annoying that this is not atomic. We just change the duty
> > > cycle on the fly. Everything else is configured one time at startup.
> > > I'd call what I need pwm-dac. I could not find a ready to use driver.
> > > Maybe I could misuse some kernel driver for this. Maybe I could use
> > > pwm-led or pwm-brightness or pwm-fan. Propably pwm-regulator could work,
> > > there is even a userspace facing part for this, but this is not
> > > devicetree ready.
> > > ...and the worst, please don't blame me: The application is java, so
> > > ioctl is a problem.
> > 
> > I thought java could do ioctls, otherwise how would it ever be able to
> > talk to serial ports?
> > 
> > Anyway, this needs to be fixed in the kernel...
> 
> If atomicity was a problem, we could potentially add a mechanism to the
> sysfs interface to enable that. I don't see a good way of doing that in
> a single file, since that works against how sysfs is designed. But one
> thing I could imagine is adding a file ("lock", or whatever you want to
> call it) that you can use for atomic fencing:
> 
> 	$ echo 1 > lock # locks the hardware state
> 	$ echo 100 > period
> 	$ echo 50 > duty_cycle
> 	$ echo 0 > lock # flushes state to hardware
> 
> But it sounds like that's not even a big issue.

For my use case atomicity is absolutely not a problem, but of course
things should be solved in a generic way.

> However, given the use-case description, it sounds to me like
> pwm-regulator would be a better candidate to solve this, because it's
> purpose is literally to generate a voltage using a PWM. There is a
> device tree binding for pwm-regulator, so this should work there as
> well.
> 
> Lars, what exactly are the problems that you're running into when trying
> to use pwm-regulator with device tree?

The problem is not to use pwm-regulator from device tree. But I have no
userspace facing part then. pwm-regulator is for kernel drivers only (as
far as I can see). I can not do anything with it from userspace.
I found that "userspace-consumer" solves this, but this thing does not
have a device tree binding. I could add this of course, but pwm sysfs
was there ready to be used.

Regards,
Lars

Re: [PATCH] pwm: sysfs: Set class on pwm devices

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 6915 bytes --]

On Mon, Oct 05, 2020 at 12:40:23PM +0200, Greg Kroah-Hartman wrote:
> On Mon, Oct 05, 2020 at 12:17:21PM +0200, Thierry Reding wrote:
> > On Mon, Oct 05, 2020 at 11:45:30AM +0200, Greg Kroah-Hartman wrote:
> > > On Mon, Oct 05, 2020 at 11:30:16AM +0200, Thierry Reding wrote:
> > > > On Thu, Oct 01, 2020 at 01:24:49PM +0200, Greg Kroah-Hartman wrote:
> > > > > On Thu, Oct 01, 2020 at 11:05:31AM +0200, Lars Poeschel wrote:
> > > > > > On Wed, Sep 30, 2020 at 11:41:46AM +0200, Uwe Kleine-König wrote:
> > > > > > > Hello,
> > > > > > > 
> > > > > > > I added Greg Kroah-Hartman who I discussed this with via irc a bit to
> > > > > > > Cc:.
> > > > > > > 
> > > > > > > On Wed, Sep 30, 2020 at 11:20:56AM +0200, Lars Poeschel wrote:
> > > > > > > > thank you for your review!
> > > > > > > > 
> > > > > > > > On Wed, Sep 30, 2020 at 08:57:26AM +0200, Uwe Kleine-König wrote:
> > > > > > > > > On Tue, Sep 29, 2020 at 02:19:53PM +0200, poeschel@lemonage.de wrote:
> > > > > > > > > > From: Lars Poeschel <poeschel@lemonage.de>
> > > > > > > > > > 
> > > > > > > > > > This adds a class to exported pwm devices.
> > > > > > > > > > Exporting a pwm through sysfs did not yield udev events. The
> > > > > > > > > 
> > > > > > > > > I wonder what is your use-case here. This for sure also has a place to
> > > > > > > > > be mentioned in the commit log. I suspect there is a better way to
> > > > > > > > > accomplish you way.
> > > > > > > > 
> > > > > > > > Use-case is to be able to use a pwm from a non-root userspace process.
> > > > > > > > I use udev rules to adjust permissions.
> > > > > > > 
> > > > > > > Hmm, how do you trigger the export? Without being aware of all the
> > > > > > > details in the sysfs code I would expect that the exported stuff is
> > > > > > > available instantly once the write used to export the PWM is completed.
> > > > > > > So changing the permissions can be done directly after triggering the
> > > > > > > export in the same process.
> > > > > > 
> > > > > > The export is triggered through the userspace process itself. Why can it
> > > > > > do this ? Because there is another udev rule, that changes permissions
> > > > > > when a pwmchip appears.
> > > > > > Then I'd like to have the second udev rule, that changes permissions on
> > > > > > the freshly exported pwm. The userspace process can't do this.
> > > > > > You are right I could propably do everything from within udev: If a
> > > > > > pwmchip appears, export certain pwms and right away change their
> > > > > > permissions. It does not also not feel right. It'd require knowledge
> > > > > > from the userspace application to be mapped to udev.
> > > > > 
> > > > > The way the kernel code is now, yes, you will not have any way to
> > > > > trigger it by userspace as the kernel is creating a "raw" struct device
> > > > > that isn't assigned to anything.  That is what needs to be fixed here.
> > > > > 
> > > > > > > Out of interest: What do you use the pwm for? Isn't there a suitable
> > > > > > > kernel driver that can do the required stuff? Compared to the kernel-API
> > > > > > > the sysfs interface isn't atomic. Is this an annoyance?
> > > > > > 
> > > > > > Use-case is generating a voltage from the pwm. This voltage is used to
> > > > > > signal different states and does not change very often. This is
> > > > > > absolutely not annoying that this is not atomic. We just change the duty
> > > > > > cycle on the fly. Everything else is configured one time at startup.
> > > > > > I'd call what I need pwm-dac. I could not find a ready to use driver.
> > > > > > Maybe I could misuse some kernel driver for this. Maybe I could use
> > > > > > pwm-led or pwm-brightness or pwm-fan. Propably pwm-regulator could work,
> > > > > > there is even a userspace facing part for this, but this is not
> > > > > > devicetree ready.
> > > > > > ...and the worst, please don't blame me: The application is java, so
> > > > > > ioctl is a problem.
> > > > > 
> > > > > I thought java could do ioctls, otherwise how would it ever be able to
> > > > > talk to serial ports?
> > > > > 
> > > > > Anyway, this needs to be fixed in the kernel...
> > > > 
> > > > If atomicity was a problem, we could potentially add a mechanism to the
> > > > sysfs interface to enable that. I don't see a good way of doing that in
> > > > a single file, since that works against how sysfs is designed. But one
> > > > thing I could imagine is adding a file ("lock", or whatever you want to
> > > > call it) that you can use for atomic fencing:
> > > > 
> > > > 	$ echo 1 > lock # locks the hardware state
> > > > 	$ echo 100 > period
> > > > 	$ echo 50 > duty_cycle
> > > > 	$ echo 0 > lock # flushes state to hardware
> > > > 
> > > > But it sounds like that's not even a big issue.
> > > 
> > > That is exactly what configfs was designed for :)
> > 
> > Interesting... for some reason I had never thought about configfs in
> > this context. But it does indeed sound like it could solve this problem
> > in a better way.
> > 
> > My memory is a bit sketchy, but I think for USB device controllers this
> > works by exposing each controller in configfs and then configuring
> > things like endpoints within the controller's directory. So I wonder if
> > instead of requesting PWMs via sysfs, we should rather expose them via
> > configfs items.
> > 
> > Something like:
> > 
> > 	# mkdir /configfs/7000a000.pwm/0
> > 
> > could then be the equivalent of exporting PWM channel 0 of the given PWM
> > chip in sysfs, except that now you get a configfs directory with
> > attributes that you can use to inspect and reconfigure the PWM channel
> > and ultimately apply the changes atomically.
> > 
> > How does that work from a permissions point of view? How do we ensure
> > that people don't need root privileges to access these?
> 
> To change things in configfs, yes, I'm pretty sure you need root access.
> But to read things, sysfs is fine.  I don't really know what you are
> wanting to do here, both create a device and change the options over
> time?

Yes, I'm wondering if we should replace the write usages in sysfs with a
better configfs implementation. We obviously can't remove the existing
sysfs ABI, but for anything that's meant to be atomic we could point
people at the configfs interface.

If configfs would also solve the permissions problem that Lars and
others have been trying to solve that would be two birds with one stone.

On the other hand, I'm wondering how much use there is in pursuing this
because as I mentioned earlier there are better alternatives (such as
pwm-regulator) that cater better to these specific use-cases and already
deal with atomicity. pwm-regulator should also show up as a separate
device in sysfs and allow permissions to be set using udev.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] pwm: sysfs: Set class on pwm devices

[Greg Kroah-Hartman]

On Mon, Oct 05, 2020 at 01:08:19PM +0200, Thierry Reding wrote:
> On Mon, Oct 05, 2020 at 12:40:23PM +0200, Greg Kroah-Hartman wrote:
> > On Mon, Oct 05, 2020 at 12:17:21PM +0200, Thierry Reding wrote:
> > > On Mon, Oct 05, 2020 at 11:45:30AM +0200, Greg Kroah-Hartman wrote:
> > > > On Mon, Oct 05, 2020 at 11:30:16AM +0200, Thierry Reding wrote:
> > > > > On Thu, Oct 01, 2020 at 01:24:49PM +0200, Greg Kroah-Hartman wrote:
> > > > > > On Thu, Oct 01, 2020 at 11:05:31AM +0200, Lars Poeschel wrote:
> > > > > > > On Wed, Sep 30, 2020 at 11:41:46AM +0200, Uwe Kleine-König wrote:
> > > > > > > > Hello,
> > > > > > > > 
> > > > > > > > I added Greg Kroah-Hartman who I discussed this with via irc a bit to
> > > > > > > > Cc:.
> > > > > > > > 
> > > > > > > > On Wed, Sep 30, 2020 at 11:20:56AM +0200, Lars Poeschel wrote:
> > > > > > > > > thank you for your review!
> > > > > > > > > 
> > > > > > > > > On Wed, Sep 30, 2020 at 08:57:26AM +0200, Uwe Kleine-König wrote:
> > > > > > > > > > On Tue, Sep 29, 2020 at 02:19:53PM +0200, poeschel@lemonage.de wrote:
> > > > > > > > > > > From: Lars Poeschel <poeschel@lemonage.de>
> > > > > > > > > > > 
> > > > > > > > > > > This adds a class to exported pwm devices.
> > > > > > > > > > > Exporting a pwm through sysfs did not yield udev events. The
> > > > > > > > > > 
> > > > > > > > > > I wonder what is your use-case here. This for sure also has a place to
> > > > > > > > > > be mentioned in the commit log. I suspect there is a better way to
> > > > > > > > > > accomplish you way.
> > > > > > > > > 
> > > > > > > > > Use-case is to be able to use a pwm from a non-root userspace process.
> > > > > > > > > I use udev rules to adjust permissions.
> > > > > > > > 
> > > > > > > > Hmm, how do you trigger the export? Without being aware of all the
> > > > > > > > details in the sysfs code I would expect that the exported stuff is
> > > > > > > > available instantly once the write used to export the PWM is completed.
> > > > > > > > So changing the permissions can be done directly after triggering the
> > > > > > > > export in the same process.
> > > > > > > 
> > > > > > > The export is triggered through the userspace process itself. Why can it
> > > > > > > do this ? Because there is another udev rule, that changes permissions
> > > > > > > when a pwmchip appears.
> > > > > > > Then I'd like to have the second udev rule, that changes permissions on
> > > > > > > the freshly exported pwm. The userspace process can't do this.
> > > > > > > You are right I could propably do everything from within udev: If a
> > > > > > > pwmchip appears, export certain pwms and right away change their
> > > > > > > permissions. It does not also not feel right. It'd require knowledge
> > > > > > > from the userspace application to be mapped to udev.
> > > > > > 
> > > > > > The way the kernel code is now, yes, you will not have any way to
> > > > > > trigger it by userspace as the kernel is creating a "raw" struct device
> > > > > > that isn't assigned to anything.  That is what needs to be fixed here.
> > > > > > 
> > > > > > > > Out of interest: What do you use the pwm for? Isn't there a suitable
> > > > > > > > kernel driver that can do the required stuff? Compared to the kernel-API
> > > > > > > > the sysfs interface isn't atomic. Is this an annoyance?
> > > > > > > 
> > > > > > > Use-case is generating a voltage from the pwm. This voltage is used to
> > > > > > > signal different states and does not change very often. This is
> > > > > > > absolutely not annoying that this is not atomic. We just change the duty
> > > > > > > cycle on the fly. Everything else is configured one time at startup.
> > > > > > > I'd call what I need pwm-dac. I could not find a ready to use driver.
> > > > > > > Maybe I could misuse some kernel driver for this. Maybe I could use
> > > > > > > pwm-led or pwm-brightness or pwm-fan. Propably pwm-regulator could work,
> > > > > > > there is even a userspace facing part for this, but this is not
> > > > > > > devicetree ready.
> > > > > > > ...and the worst, please don't blame me: The application is java, so
> > > > > > > ioctl is a problem.
> > > > > > 
> > > > > > I thought java could do ioctls, otherwise how would it ever be able to
> > > > > > talk to serial ports?
> > > > > > 
> > > > > > Anyway, this needs to be fixed in the kernel...
> > > > > 
> > > > > If atomicity was a problem, we could potentially add a mechanism to the
> > > > > sysfs interface to enable that. I don't see a good way of doing that in
> > > > > a single file, since that works against how sysfs is designed. But one
> > > > > thing I could imagine is adding a file ("lock", or whatever you want to
> > > > > call it) that you can use for atomic fencing:
> > > > > 
> > > > > 	$ echo 1 > lock # locks the hardware state
> > > > > 	$ echo 100 > period
> > > > > 	$ echo 50 > duty_cycle
> > > > > 	$ echo 0 > lock # flushes state to hardware
> > > > > 
> > > > > But it sounds like that's not even a big issue.
> > > > 
> > > > That is exactly what configfs was designed for :)
> > > 
> > > Interesting... for some reason I had never thought about configfs in
> > > this context. But it does indeed sound like it could solve this problem
> > > in a better way.
> > > 
> > > My memory is a bit sketchy, but I think for USB device controllers this
> > > works by exposing each controller in configfs and then configuring
> > > things like endpoints within the controller's directory. So I wonder if
> > > instead of requesting PWMs via sysfs, we should rather expose them via
> > > configfs items.
> > > 
> > > Something like:
> > > 
> > > 	# mkdir /configfs/7000a000.pwm/0
> > > 
> > > could then be the equivalent of exporting PWM channel 0 of the given PWM
> > > chip in sysfs, except that now you get a configfs directory with
> > > attributes that you can use to inspect and reconfigure the PWM channel
> > > and ultimately apply the changes atomically.
> > > 
> > > How does that work from a permissions point of view? How do we ensure
> > > that people don't need root privileges to access these?
> > 
> > To change things in configfs, yes, I'm pretty sure you need root access.
> > But to read things, sysfs is fine.  I don't really know what you are
> > wanting to do here, both create a device and change the options over
> > time?
> 
> Yes, I'm wondering if we should replace the write usages in sysfs with a
> better configfs implementation. We obviously can't remove the existing
> sysfs ABI, but for anything that's meant to be atomic we could point
> people at the configfs interface.

How about fixing the sysfs interface so that it's usable, like the
proposed patch does?  What you all have now is not working.

When the revised version is sent, not this version...

thanks,

greg k-h

Re: [PATCH] pwm: sysfs: Set class on pwm devices

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 7353 bytes --]

On Mon, Oct 05, 2020 at 01:17:38PM +0200, Greg Kroah-Hartman wrote:
> On Mon, Oct 05, 2020 at 01:08:19PM +0200, Thierry Reding wrote:
> > On Mon, Oct 05, 2020 at 12:40:23PM +0200, Greg Kroah-Hartman wrote:
> > > On Mon, Oct 05, 2020 at 12:17:21PM +0200, Thierry Reding wrote:
> > > > On Mon, Oct 05, 2020 at 11:45:30AM +0200, Greg Kroah-Hartman wrote:
> > > > > On Mon, Oct 05, 2020 at 11:30:16AM +0200, Thierry Reding wrote:
> > > > > > On Thu, Oct 01, 2020 at 01:24:49PM +0200, Greg Kroah-Hartman wrote:
> > > > > > > On Thu, Oct 01, 2020 at 11:05:31AM +0200, Lars Poeschel wrote:
> > > > > > > > On Wed, Sep 30, 2020 at 11:41:46AM +0200, Uwe Kleine-König wrote:
> > > > > > > > > Hello,
> > > > > > > > > 
> > > > > > > > > I added Greg Kroah-Hartman who I discussed this with via irc a bit to
> > > > > > > > > Cc:.
> > > > > > > > > 
> > > > > > > > > On Wed, Sep 30, 2020 at 11:20:56AM +0200, Lars Poeschel wrote:
> > > > > > > > > > thank you for your review!
> > > > > > > > > > 
> > > > > > > > > > On Wed, Sep 30, 2020 at 08:57:26AM +0200, Uwe Kleine-König wrote:
> > > > > > > > > > > On Tue, Sep 29, 2020 at 02:19:53PM +0200, poeschel@lemonage.de wrote:
> > > > > > > > > > > > From: Lars Poeschel <poeschel@lemonage.de>
> > > > > > > > > > > > 
> > > > > > > > > > > > This adds a class to exported pwm devices.
> > > > > > > > > > > > Exporting a pwm through sysfs did not yield udev events. The
> > > > > > > > > > > 
> > > > > > > > > > > I wonder what is your use-case here. This for sure also has a place to
> > > > > > > > > > > be mentioned in the commit log. I suspect there is a better way to
> > > > > > > > > > > accomplish you way.
> > > > > > > > > > 
> > > > > > > > > > Use-case is to be able to use a pwm from a non-root userspace process.
> > > > > > > > > > I use udev rules to adjust permissions.
> > > > > > > > > 
> > > > > > > > > Hmm, how do you trigger the export? Without being aware of all the
> > > > > > > > > details in the sysfs code I would expect that the exported stuff is
> > > > > > > > > available instantly once the write used to export the PWM is completed.
> > > > > > > > > So changing the permissions can be done directly after triggering the
> > > > > > > > > export in the same process.
> > > > > > > > 
> > > > > > > > The export is triggered through the userspace process itself. Why can it
> > > > > > > > do this ? Because there is another udev rule, that changes permissions
> > > > > > > > when a pwmchip appears.
> > > > > > > > Then I'd like to have the second udev rule, that changes permissions on
> > > > > > > > the freshly exported pwm. The userspace process can't do this.
> > > > > > > > You are right I could propably do everything from within udev: If a
> > > > > > > > pwmchip appears, export certain pwms and right away change their
> > > > > > > > permissions. It does not also not feel right. It'd require knowledge
> > > > > > > > from the userspace application to be mapped to udev.
> > > > > > > 
> > > > > > > The way the kernel code is now, yes, you will not have any way to
> > > > > > > trigger it by userspace as the kernel is creating a "raw" struct device
> > > > > > > that isn't assigned to anything.  That is what needs to be fixed here.
> > > > > > > 
> > > > > > > > > Out of interest: What do you use the pwm for? Isn't there a suitable
> > > > > > > > > kernel driver that can do the required stuff? Compared to the kernel-API
> > > > > > > > > the sysfs interface isn't atomic. Is this an annoyance?
> > > > > > > > 
> > > > > > > > Use-case is generating a voltage from the pwm. This voltage is used to
> > > > > > > > signal different states and does not change very often. This is
> > > > > > > > absolutely not annoying that this is not atomic. We just change the duty
> > > > > > > > cycle on the fly. Everything else is configured one time at startup.
> > > > > > > > I'd call what I need pwm-dac. I could not find a ready to use driver.
> > > > > > > > Maybe I could misuse some kernel driver for this. Maybe I could use
> > > > > > > > pwm-led or pwm-brightness or pwm-fan. Propably pwm-regulator could work,
> > > > > > > > there is even a userspace facing part for this, but this is not
> > > > > > > > devicetree ready.
> > > > > > > > ...and the worst, please don't blame me: The application is java, so
> > > > > > > > ioctl is a problem.
> > > > > > > 
> > > > > > > I thought java could do ioctls, otherwise how would it ever be able to
> > > > > > > talk to serial ports?
> > > > > > > 
> > > > > > > Anyway, this needs to be fixed in the kernel...
> > > > > > 
> > > > > > If atomicity was a problem, we could potentially add a mechanism to the
> > > > > > sysfs interface to enable that. I don't see a good way of doing that in
> > > > > > a single file, since that works against how sysfs is designed. But one
> > > > > > thing I could imagine is adding a file ("lock", or whatever you want to
> > > > > > call it) that you can use for atomic fencing:
> > > > > > 
> > > > > > 	$ echo 1 > lock # locks the hardware state
> > > > > > 	$ echo 100 > period
> > > > > > 	$ echo 50 > duty_cycle
> > > > > > 	$ echo 0 > lock # flushes state to hardware
> > > > > > 
> > > > > > But it sounds like that's not even a big issue.
> > > > > 
> > > > > That is exactly what configfs was designed for :)
> > > > 
> > > > Interesting... for some reason I had never thought about configfs in
> > > > this context. But it does indeed sound like it could solve this problem
> > > > in a better way.
> > > > 
> > > > My memory is a bit sketchy, but I think for USB device controllers this
> > > > works by exposing each controller in configfs and then configuring
> > > > things like endpoints within the controller's directory. So I wonder if
> > > > instead of requesting PWMs via sysfs, we should rather expose them via
> > > > configfs items.
> > > > 
> > > > Something like:
> > > > 
> > > > 	# mkdir /configfs/7000a000.pwm/0
> > > > 
> > > > could then be the equivalent of exporting PWM channel 0 of the given PWM
> > > > chip in sysfs, except that now you get a configfs directory with
> > > > attributes that you can use to inspect and reconfigure the PWM channel
> > > > and ultimately apply the changes atomically.
> > > > 
> > > > How does that work from a permissions point of view? How do we ensure
> > > > that people don't need root privileges to access these?
> > > 
> > > To change things in configfs, yes, I'm pretty sure you need root access.
> > > But to read things, sysfs is fine.  I don't really know what you are
> > > wanting to do here, both create a device and change the options over
> > > time?
> > 
> > Yes, I'm wondering if we should replace the write usages in sysfs with a
> > better configfs implementation. We obviously can't remove the existing
> > sysfs ABI, but for anything that's meant to be atomic we could point
> > people at the configfs interface.
> 
> How about fixing the sysfs interface so that it's usable, like the
> proposed patch does?  What you all have now is not working.
> 
> When the revised version is sent, not this version...

I'm not sure which patch you're referring to. I don't see anything in my
inbox. I'll go check the spam, perhaps it's landed there.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] pwm: sysfs: Set class on pwm devices

[Greg Kroah-Hartman]

On Mon, Oct 05, 2020 at 01:58:19PM +0200, Thierry Reding wrote:
> On Mon, Oct 05, 2020 at 01:17:38PM +0200, Greg Kroah-Hartman wrote:
> > On Mon, Oct 05, 2020 at 01:08:19PM +0200, Thierry Reding wrote:
> > > On Mon, Oct 05, 2020 at 12:40:23PM +0200, Greg Kroah-Hartman wrote:
> > > > On Mon, Oct 05, 2020 at 12:17:21PM +0200, Thierry Reding wrote:
> > > > > On Mon, Oct 05, 2020 at 11:45:30AM +0200, Greg Kroah-Hartman wrote:
> > > > > > On Mon, Oct 05, 2020 at 11:30:16AM +0200, Thierry Reding wrote:
> > > > > > > On Thu, Oct 01, 2020 at 01:24:49PM +0200, Greg Kroah-Hartman wrote:
> > > > > > > > On Thu, Oct 01, 2020 at 11:05:31AM +0200, Lars Poeschel wrote:
> > > > > > > > > On Wed, Sep 30, 2020 at 11:41:46AM +0200, Uwe Kleine-König wrote:
> > > > > > > > > > Hello,
> > > > > > > > > > 
> > > > > > > > > > I added Greg Kroah-Hartman who I discussed this with via irc a bit to
> > > > > > > > > > Cc:.
> > > > > > > > > > 
> > > > > > > > > > On Wed, Sep 30, 2020 at 11:20:56AM +0200, Lars Poeschel wrote:
> > > > > > > > > > > thank you for your review!
> > > > > > > > > > > 
> > > > > > > > > > > On Wed, Sep 30, 2020 at 08:57:26AM +0200, Uwe Kleine-König wrote:
> > > > > > > > > > > > On Tue, Sep 29, 2020 at 02:19:53PM +0200, poeschel@lemonage.de wrote:
> > > > > > > > > > > > > From: Lars Poeschel <poeschel@lemonage.de>
> > > > > > > > > > > > > 
> > > > > > > > > > > > > This adds a class to exported pwm devices.
> > > > > > > > > > > > > Exporting a pwm through sysfs did not yield udev events. The
> > > > > > > > > > > > 
> > > > > > > > > > > > I wonder what is your use-case here. This for sure also has a place to
> > > > > > > > > > > > be mentioned in the commit log. I suspect there is a better way to
> > > > > > > > > > > > accomplish you way.
> > > > > > > > > > > 
> > > > > > > > > > > Use-case is to be able to use a pwm from a non-root userspace process.
> > > > > > > > > > > I use udev rules to adjust permissions.
> > > > > > > > > > 
> > > > > > > > > > Hmm, how do you trigger the export? Without being aware of all the
> > > > > > > > > > details in the sysfs code I would expect that the exported stuff is
> > > > > > > > > > available instantly once the write used to export the PWM is completed.
> > > > > > > > > > So changing the permissions can be done directly after triggering the
> > > > > > > > > > export in the same process.
> > > > > > > > > 
> > > > > > > > > The export is triggered through the userspace process itself. Why can it
> > > > > > > > > do this ? Because there is another udev rule, that changes permissions
> > > > > > > > > when a pwmchip appears.
> > > > > > > > > Then I'd like to have the second udev rule, that changes permissions on
> > > > > > > > > the freshly exported pwm. The userspace process can't do this.
> > > > > > > > > You are right I could propably do everything from within udev: If a
> > > > > > > > > pwmchip appears, export certain pwms and right away change their
> > > > > > > > > permissions. It does not also not feel right. It'd require knowledge
> > > > > > > > > from the userspace application to be mapped to udev.
> > > > > > > > 
> > > > > > > > The way the kernel code is now, yes, you will not have any way to
> > > > > > > > trigger it by userspace as the kernel is creating a "raw" struct device
> > > > > > > > that isn't assigned to anything.  That is what needs to be fixed here.
> > > > > > > > 
> > > > > > > > > > Out of interest: What do you use the pwm for? Isn't there a suitable
> > > > > > > > > > kernel driver that can do the required stuff? Compared to the kernel-API
> > > > > > > > > > the sysfs interface isn't atomic. Is this an annoyance?
> > > > > > > > > 
> > > > > > > > > Use-case is generating a voltage from the pwm. This voltage is used to
> > > > > > > > > signal different states and does not change very often. This is
> > > > > > > > > absolutely not annoying that this is not atomic. We just change the duty
> > > > > > > > > cycle on the fly. Everything else is configured one time at startup.
> > > > > > > > > I'd call what I need pwm-dac. I could not find a ready to use driver.
> > > > > > > > > Maybe I could misuse some kernel driver for this. Maybe I could use
> > > > > > > > > pwm-led or pwm-brightness or pwm-fan. Propably pwm-regulator could work,
> > > > > > > > > there is even a userspace facing part for this, but this is not
> > > > > > > > > devicetree ready.
> > > > > > > > > ...and the worst, please don't blame me: The application is java, so
> > > > > > > > > ioctl is a problem.
> > > > > > > > 
> > > > > > > > I thought java could do ioctls, otherwise how would it ever be able to
> > > > > > > > talk to serial ports?
> > > > > > > > 
> > > > > > > > Anyway, this needs to be fixed in the kernel...
> > > > > > > 
> > > > > > > If atomicity was a problem, we could potentially add a mechanism to the
> > > > > > > sysfs interface to enable that. I don't see a good way of doing that in
> > > > > > > a single file, since that works against how sysfs is designed. But one
> > > > > > > thing I could imagine is adding a file ("lock", or whatever you want to
> > > > > > > call it) that you can use for atomic fencing:
> > > > > > > 
> > > > > > > 	$ echo 1 > lock # locks the hardware state
> > > > > > > 	$ echo 100 > period
> > > > > > > 	$ echo 50 > duty_cycle
> > > > > > > 	$ echo 0 > lock # flushes state to hardware
> > > > > > > 
> > > > > > > But it sounds like that's not even a big issue.
> > > > > > 
> > > > > > That is exactly what configfs was designed for :)
> > > > > 
> > > > > Interesting... for some reason I had never thought about configfs in
> > > > > this context. But it does indeed sound like it could solve this problem
> > > > > in a better way.
> > > > > 
> > > > > My memory is a bit sketchy, but I think for USB device controllers this
> > > > > works by exposing each controller in configfs and then configuring
> > > > > things like endpoints within the controller's directory. So I wonder if
> > > > > instead of requesting PWMs via sysfs, we should rather expose them via
> > > > > configfs items.
> > > > > 
> > > > > Something like:
> > > > > 
> > > > > 	# mkdir /configfs/7000a000.pwm/0
> > > > > 
> > > > > could then be the equivalent of exporting PWM channel 0 of the given PWM
> > > > > chip in sysfs, except that now you get a configfs directory with
> > > > > attributes that you can use to inspect and reconfigure the PWM channel
> > > > > and ultimately apply the changes atomically.
> > > > > 
> > > > > How does that work from a permissions point of view? How do we ensure
> > > > > that people don't need root privileges to access these?
> > > > 
> > > > To change things in configfs, yes, I'm pretty sure you need root access.
> > > > But to read things, sysfs is fine.  I don't really know what you are
> > > > wanting to do here, both create a device and change the options over
> > > > time?
> > > 
> > > Yes, I'm wondering if we should replace the write usages in sysfs with a
> > > better configfs implementation. We obviously can't remove the existing
> > > sysfs ABI, but for anything that's meant to be atomic we could point
> > > people at the configfs interface.
> > 
> > How about fixing the sysfs interface so that it's usable, like the
> > proposed patch does?  What you all have now is not working.
> > 
> > When the revised version is sent, not this version...
> 
> I'm not sure which patch you're referring to. I don't see anything in my
> inbox. I'll go check the spam, perhaps it's landed there.

See:
	https://lore.kernel.org/r/20201002123048.3073128-1-poeschel@lemonage.de