[RESEND PATCH] Revert "pwm: Set class for exported channels in sysfs"

[RESEND PATCH] Revert "pwm: Set class for exported channels in sysfs"

[Fabrice Gasnier]

This reverts commit 7e5d1fd75c3dde9fc10c4472b9368089d1b81d00 as it causes
regression with multiple pwm chip. It creates a new entry in
'/sys/class/pwm' every time a 'pwmX' is exported with 'echo X > export':
- 1st time export will create an entry in /sys/class/pwm/pwmX
- when another export happens on another pwmchip, it can't be created
(e.g. -EEXIST)

This also changes existing ABI (Documentation/ABI/testing/sysfs-class-pwm):
- pmwX should be there: /sys/class/pwm/pwmchipN/pwmX

Example on stm32 (stm32429i-eval) platform:
$ ls /sys/class/pwm
pwmchip0 pwmchip4

$ cd /sys/class/pwm/pwmchip0/
$ echo 0 > export
$ ls /sys/class/pwm
pwm0 pwmchip0 pwmchip4

$ cd /sys/class/pwm/pwmchip4/
$ echo 0 > export
sysfs: cannot create duplicate filename '/class/pwm/pwm0'
...Exception stack follows...

Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
---
 drivers/pwm/sysfs.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c
index 7c71cdb..4726d43 100644
--- a/drivers/pwm/sysfs.c
+++ b/drivers/pwm/sysfs.c
@@ -263,7 +263,6 @@ static int pwm_export_child(struct device *parent, struct pwm_device *pwm)
 	export->pwm = pwm;
 	mutex_init(&export->lock);
 
-	export->child.class = parent->class;
 	export->child.release = pwm_export_release;
 	export->child.parent = parent;
 	export->child.devt = MKDEV(0, 0);
-- 
1.9.1

Re: [RESEND PATCH] Revert "pwm: Set class for exported channels in sysfs"

[Thierry Reding]

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

On Fri, Sep 21, 2018 at 04:02:47PM +0200, Fabrice Gasnier wrote:
> This reverts commit 7e5d1fd75c3dde9fc10c4472b9368089d1b81d00 as it causes
> regression with multiple pwm chip. It creates a new entry in
> '/sys/class/pwm' every time a 'pwmX' is exported with 'echo X > export':
> - 1st time export will create an entry in /sys/class/pwm/pwmX
> - when another export happens on another pwmchip, it can't be created
> (e.g. -EEXIST)
> 
> This also changes existing ABI (Documentation/ABI/testing/sysfs-class-pwm):
> - pmwX should be there: /sys/class/pwm/pwmchipN/pwmX
> 
> Example on stm32 (stm32429i-eval) platform:
> $ ls /sys/class/pwm
> pwmchip0 pwmchip4
> 
> $ cd /sys/class/pwm/pwmchip0/
> $ echo 0 > export
> $ ls /sys/class/pwm
> pwm0 pwmchip0 pwmchip4
> 
> $ cd /sys/class/pwm/pwmchip4/
> $ echo 0 > export
> sysfs: cannot create duplicate filename '/class/pwm/pwm0'
> ...Exception stack follows...
> 
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
> ---
>  drivers/pwm/sysfs.c | 1 -
>  1 file changed, 1 deletion(-)

Can we come up with an alternative that allows us to have both? We want
uevent and proper sysfs creation, or is that not possible?

Thierry

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

Re: [RESEND PATCH] Revert "pwm: Set class for exported channels in sysfs"

[Fabrice Gasnier]

On 09/24/2018 01:53 PM, Thierry Reding wrote:
> On Fri, Sep 21, 2018 at 04:02:47PM +0200, Fabrice Gasnier wrote:
>> This reverts commit 7e5d1fd75c3dde9fc10c4472b9368089d1b81d00 as it causes
>> regression with multiple pwm chip. It creates a new entry in
>> '/sys/class/pwm' every time a 'pwmX' is exported with 'echo X > export':
>> - 1st time export will create an entry in /sys/class/pwm/pwmX
>> - when another export happens on another pwmchip, it can't be created
>> (e.g. -EEXIST)
>>
>> This also changes existing ABI (Documentation/ABI/testing/sysfs-class-pwm):
>> - pmwX should be there: /sys/class/pwm/pwmchipN/pwmX
>>
>> Example on stm32 (stm32429i-eval) platform:
>> $ ls /sys/class/pwm
>> pwmchip0 pwmchip4
>>
>> $ cd /sys/class/pwm/pwmchip0/
>> $ echo 0 > export
>> $ ls /sys/class/pwm
>> pwm0 pwmchip0 pwmchip4
>>
>> $ cd /sys/class/pwm/pwmchip4/
>> $ echo 0 > export
>> sysfs: cannot create duplicate filename '/class/pwm/pwm0'
>> ...Exception stack follows...
>>
>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
>> ---
>>  drivers/pwm/sysfs.c | 1 -
>>  1 file changed, 1 deletion(-)
> 
> Can we come up with an alternative that allows us to have both? We want
> uevent and proper sysfs creation, or is that not possible?

Hi Thierry, all,

With current approach:
- "export->child.class = parent->class"
- ABI (e.g. "pwm%d") device name isn't unique with multiple pwm chip.
I think this is not possible.

Trying to think of an alternative... I just did a quick test, by
changing device name, to take pwmchip into account:
+       export->child.class = parent->class;
        export->child.release = pwm_export_release;
        export->child.parent = parent;
        export->child.devt = MKDEV(0, 0);
        export->child.groups = pwm_groups;
-       dev_set_name(&export->child, "pwm%u", pwm->hwpwm);
+       dev_set_name(&export->child, "pwmchip%d-pwm%u", chip->base,
pwm->hwpwm);

But this also impacts existing ABI :-(
Would you have suggestions to send an uevent, without modifying ABI ?

Please advise,
Best regards,
Fabrice

> 
> Thierry
> 

Re: [RESEND PATCH] Revert "pwm: Set class for exported channels in sysfs"

[Thierry Reding]

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

On Mon, Sep 24, 2018 at 03:59:03PM +0200, Fabrice Gasnier wrote:
> On 09/24/2018 01:53 PM, Thierry Reding wrote:
> > On Fri, Sep 21, 2018 at 04:02:47PM +0200, Fabrice Gasnier wrote:
> >> This reverts commit 7e5d1fd75c3dde9fc10c4472b9368089d1b81d00 as it causes
> >> regression with multiple pwm chip. It creates a new entry in
> >> '/sys/class/pwm' every time a 'pwmX' is exported with 'echo X > export':
> >> - 1st time export will create an entry in /sys/class/pwm/pwmX
> >> - when another export happens on another pwmchip, it can't be created
> >> (e.g. -EEXIST)
> >>
> >> This also changes existing ABI (Documentation/ABI/testing/sysfs-class-pwm):
> >> - pmwX should be there: /sys/class/pwm/pwmchipN/pwmX
> >>
> >> Example on stm32 (stm32429i-eval) platform:
> >> $ ls /sys/class/pwm
> >> pwmchip0 pwmchip4
> >>
> >> $ cd /sys/class/pwm/pwmchip0/
> >> $ echo 0 > export
> >> $ ls /sys/class/pwm
> >> pwm0 pwmchip0 pwmchip4
> >>
> >> $ cd /sys/class/pwm/pwmchip4/
> >> $ echo 0 > export
> >> sysfs: cannot create duplicate filename '/class/pwm/pwm0'
> >> ...Exception stack follows...
> >>
> >> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
> >> ---
> >>  drivers/pwm/sysfs.c | 1 -
> >>  1 file changed, 1 deletion(-)
> > 
> > Can we come up with an alternative that allows us to have both? We want
> > uevent and proper sysfs creation, or is that not possible?
> 
> Hi Thierry, all,
> 
> With current approach:
> - "export->child.class = parent->class"
> - ABI (e.g. "pwm%d") device name isn't unique with multiple pwm chip.
> I think this is not possible.
> 
> Trying to think of an alternative... I just did a quick test, by
> changing device name, to take pwmchip into account:
> +       export->child.class = parent->class;
>         export->child.release = pwm_export_release;
>         export->child.parent = parent;
>         export->child.devt = MKDEV(0, 0);
>         export->child.groups = pwm_groups;
> -       dev_set_name(&export->child, "pwm%u", pwm->hwpwm);
> +       dev_set_name(&export->child, "pwmchip%d-pwm%u", chip->base,
> pwm->hwpwm);
> 
> But this also impacts existing ABI :-(
> Would you have suggestions to send an uevent, without modifying ABI ?

I don't quite understand why, in the example you show in the commit
message, the pwmX nodes appear in the top-level /sys/class/pwm
directory. According to Documentation/ABI/testing/sysfs-class-pwm they
should appear as /sys/class/pwm/pwmchipN/pwmX. I can only imagine that
setting the class may have changed that. If so, perhaps we can
workaround that by creating a new class that is not parent->class?

Thierry

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

Re: [RESEND PATCH] Revert "pwm: Set class for exported channels in sysfs"

[Fabrice Gasnier]

On 09/24/2018 04:23 PM, Thierry Reding wrote:
> On Mon, Sep 24, 2018 at 03:59:03PM +0200, Fabrice Gasnier wrote:
>> On 09/24/2018 01:53 PM, Thierry Reding wrote:
>>> On Fri, Sep 21, 2018 at 04:02:47PM +0200, Fabrice Gasnier wrote:
>>>> This reverts commit 7e5d1fd75c3dde9fc10c4472b9368089d1b81d00 as it causes
>>>> regression with multiple pwm chip. It creates a new entry in
>>>> '/sys/class/pwm' every time a 'pwmX' is exported with 'echo X > export':
>>>> - 1st time export will create an entry in /sys/class/pwm/pwmX
>>>> - when another export happens on another pwmchip, it can't be created
>>>> (e.g. -EEXIST)
>>>>
>>>> This also changes existing ABI (Documentation/ABI/testing/sysfs-class-pwm):
>>>> - pmwX should be there: /sys/class/pwm/pwmchipN/pwmX
>>>>
>>>> Example on stm32 (stm32429i-eval) platform:
>>>> $ ls /sys/class/pwm
>>>> pwmchip0 pwmchip4
>>>>
>>>> $ cd /sys/class/pwm/pwmchip0/
>>>> $ echo 0 > export
>>>> $ ls /sys/class/pwm
>>>> pwm0 pwmchip0 pwmchip4
>>>>
>>>> $ cd /sys/class/pwm/pwmchip4/
>>>> $ echo 0 > export
>>>> sysfs: cannot create duplicate filename '/class/pwm/pwm0'
>>>> ...Exception stack follows...
>>>>
>>>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
>>>> ---
>>>>  drivers/pwm/sysfs.c | 1 -
>>>>  1 file changed, 1 deletion(-)
>>>
>>> Can we come up with an alternative that allows us to have both? We want
>>> uevent and proper sysfs creation, or is that not possible?
>>
>> Hi Thierry, all,
>>
>> With current approach:
>> - "export->child.class = parent->class"
>> - ABI (e.g. "pwm%d") device name isn't unique with multiple pwm chip.
>> I think this is not possible.
>>
>> Trying to think of an alternative... I just did a quick test, by
>> changing device name, to take pwmchip into account:
>> +       export->child.class = parent->class;
>>         export->child.release = pwm_export_release;
>>         export->child.parent = parent;
>>         export->child.devt = MKDEV(0, 0);
>>         export->child.groups = pwm_groups;
>> -       dev_set_name(&export->child, "pwm%u", pwm->hwpwm);
>> +       dev_set_name(&export->child, "pwmchip%d-pwm%u", chip->base,
>> pwm->hwpwm);
>>
>> But this also impacts existing ABI :-(
>> Would you have suggestions to send an uevent, without modifying ABI ?
> 
> I don't quite understand why, in the example you show in the commit
> message, the pwmX nodes appear in the top-level /sys/class/pwm
> directory. According to Documentation/ABI/testing/sysfs-class-pwm they
> should appear as /sys/class/pwm/pwmchipN/pwmX. I can only imagine that
> setting the class may have changed that.

Yes, adding the class makes the link to be created under /sys/class/pwm:
device_register() -> device_add() -> device_add_class_symlinks()
In device_add_class_symlinks():
...
	if (!dev->class)
		return 0;
...
	/* link in the class directory pointing to the device */
	error = sysfs_create_link(&dev->class->p->subsys.kobj,
				  &dev->kobj, dev_name(dev));
...

> If so, perhaps we can
> workaround that by creating a new class that is not parent->class?

And this link is added using dev_name(). So I doubt adding a new class
will change the current behavior.

> 
> Thierry
> 

Re: [RESEND PATCH] Revert "pwm: Set class for exported channels in sysfs"

[Fabrice Gasnier]

On 09/24/2018 05:50 PM, Fabrice Gasnier wrote:
> On 09/24/2018 04:23 PM, Thierry Reding wrote:
>> On Mon, Sep 24, 2018 at 03:59:03PM +0200, Fabrice Gasnier wrote:
>>> On 09/24/2018 01:53 PM, Thierry Reding wrote:
>>>> On Fri, Sep 21, 2018 at 04:02:47PM +0200, Fabrice Gasnier wrote:
>>>>> This reverts commit 7e5d1fd75c3dde9fc10c4472b9368089d1b81d00 as it causes
>>>>> regression with multiple pwm chip. It creates a new entry in
>>>>> '/sys/class/pwm' every time a 'pwmX' is exported with 'echo X > export':
>>>>> - 1st time export will create an entry in /sys/class/pwm/pwmX
>>>>> - when another export happens on another pwmchip, it can't be created
>>>>> (e.g. -EEXIST)
>>>>>
>>>>> This also changes existing ABI (Documentation/ABI/testing/sysfs-class-pwm):
>>>>> - pmwX should be there: /sys/class/pwm/pwmchipN/pwmX
>>>>>
>>>>> Example on stm32 (stm32429i-eval) platform:
>>>>> $ ls /sys/class/pwm
>>>>> pwmchip0 pwmchip4
>>>>>
>>>>> $ cd /sys/class/pwm/pwmchip0/
>>>>> $ echo 0 > export
>>>>> $ ls /sys/class/pwm
>>>>> pwm0 pwmchip0 pwmchip4
>>>>>
>>>>> $ cd /sys/class/pwm/pwmchip4/
>>>>> $ echo 0 > export
>>>>> sysfs: cannot create duplicate filename '/class/pwm/pwm0'
>>>>> ...Exception stack follows...
>>>>>
>>>>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
>>>>> ---
>>>>>  drivers/pwm/sysfs.c | 1 -
>>>>>  1 file changed, 1 deletion(-)
>>>>
>>>> Can we come up with an alternative that allows us to have both? We want
>>>> uevent and proper sysfs creation, or is that not possible?
>>>
>>> Hi Thierry, all,
>>>
>>> With current approach:
>>> - "export->child.class = parent->class"
>>> - ABI (e.g. "pwm%d") device name isn't unique with multiple pwm chip.
>>> I think this is not possible.
>>>
>>> Trying to think of an alternative... I just did a quick test, by
>>> changing device name, to take pwmchip into account:
>>> +       export->child.class = parent->class;
>>>         export->child.release = pwm_export_release;
>>>         export->child.parent = parent;
>>>         export->child.devt = MKDEV(0, 0);
>>>         export->child.groups = pwm_groups;
>>> -       dev_set_name(&export->child, "pwm%u", pwm->hwpwm);
>>> +       dev_set_name(&export->child, "pwmchip%d-pwm%u", chip->base,
>>> pwm->hwpwm);
>>>
>>> But this also impacts existing ABI :-(
>>> Would you have suggestions to send an uevent, without modifying ABI ?
>>
>> I don't quite understand why, in the example you show in the commit
>> message, the pwmX nodes appear in the top-level /sys/class/pwm
>> directory. According to Documentation/ABI/testing/sysfs-class-pwm they
>> should appear as /sys/class/pwm/pwmchipN/pwmX. I can only imagine that
>> setting the class may have changed that.
> 
> Yes, adding the class makes the link to be created under /sys/class/pwm:
> device_register() -> device_add() -> device_add_class_symlinks()
> In device_add_class_symlinks():
> ...
> 	if (!dev->class)
> 		return 0;
> ...
> 	/* link in the class directory pointing to the device */
> 	error = sysfs_create_link(&dev->class->p->subsys.kobj,
> 				  &dev->kobj, dev_name(dev));
> ...
> 
>> If so, perhaps we can
>> workaround that by creating a new class that is not parent->class?

Hi Thierry,

Maybe there's a way around, keeping the revert patch, without impacting
the ABI:
- pwmX cannot be added to pwm/another class without issue as discussed
(numbering isn't unique).
- pwmchipN already belongs to pwm class.

I did some testing, trying to send uevent on the pwmX directly, without
success: uevents are filtered as pwmX doesn't belong to a class.

Still, it is possible to send uevent (KOBJ_CHANGE) on pwmchipN device,
to notify of a change, e.g. pwmX channel being exported/unexported.

I run following prototype (with revert patch).

static int pwm_export_child(struct device *parent, struct pwm_device *pwm)
 {
+       char *pwm_prop[2];
        struct pwm_export *export;
        int ret;
 ...
                kfree(export);
                return ret;
        }
+       pwm_prop[0] = kasprintf(GFP_KERNEL, "EXPORT=pwm%u", pwm->hwpwm);
+       pwm_prop[1] = NULL;
+       kobject_uevent_env(&parent->kobj, KOBJ_CHANGE, pwm_prop);
+       kfree(pwm_prop[0]);

        return 0;
 }

 static int pwm_unexport_child(struct device *parent, struct pwm_device
*pwm)
 {
        struct device *child;
+       char *pwm_prop[2];

        if (!test_and_clear_bit(PWMF_EXPORTED, &pwm->flags))
                return -ENODEV;
...
        if (!child)
                return -ENODEV;

+       pwm_prop[0] = kasprintf(GFP_KERNEL, "UNEXPORT=pwm%u", pwm->hwpwm);
+       pwm_prop[1] = NULL;
+       kobject_uevent_env(&parent->kobj, KOBJ_CHANGE, pwm_prop);
+       kfree(pwm_prop[0]);
+
        /* for device_find_child() */

Then, I run a quick test:

# udevadm monitor --environment &
# echo 0 > /sys/class/pwm/pwmchip0/export
KERNEL[197.321736] change   /devices/.../pwm/pwmchip0 (pwm)
ACTION=change
DEVPATH=/devices/.../pwm/pwmchip0
EXPORT=pwm0
SEQNUM=2045
SUBSYSTEM=pwm

# echo 0 > /sys/class/pwm/pwmchip4/export
KERNEL[202.089676] change   /devices/.../pwm/pwmchip4 (pwm)
ACTION=change
DEVPATH=/devices/.../pwm/pwmchip4
EXPORT=pwm0
SEQNUM=2046
SUBSYSTEM=pwm


Also unexport will report change events to userland:

# echo 0 > /sys/class/pwm/pwmchip0/unexport
KERNEL[1691.112765] change   /devices/.../pwm/pwmchip0 (pwm)
ACTION=change
DEVPATH=/devices/.../pwm/pwmchip0
SEQNUM=2047
SUBSYSTEM=pwm
UNEXPORT=pwm0

Do you think this may be a way around?
Please let me know if this may satisfy need for uevents.

Best regards,
Fabrice
> 
> And this link is added using dev_name(). So I doubt adding a new class
> will change the current behavior.
> 
>>
>> Thierry
>>

Re: [RESEND PATCH] Revert "pwm: Set class for exported channels in sysfs"

[Thierry Reding]

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

On Tue, Sep 25, 2018 at 03:59:26PM +0200, Fabrice Gasnier wrote:
> On 09/24/2018 05:50 PM, Fabrice Gasnier wrote:
> > On 09/24/2018 04:23 PM, Thierry Reding wrote:
> >> On Mon, Sep 24, 2018 at 03:59:03PM +0200, Fabrice Gasnier wrote:
> >>> On 09/24/2018 01:53 PM, Thierry Reding wrote:
> >>>> On Fri, Sep 21, 2018 at 04:02:47PM +0200, Fabrice Gasnier wrote:
> >>>>> This reverts commit 7e5d1fd75c3dde9fc10c4472b9368089d1b81d00 as it causes
> >>>>> regression with multiple pwm chip. It creates a new entry in
> >>>>> '/sys/class/pwm' every time a 'pwmX' is exported with 'echo X > export':
> >>>>> - 1st time export will create an entry in /sys/class/pwm/pwmX
> >>>>> - when another export happens on another pwmchip, it can't be created
> >>>>> (e.g. -EEXIST)
> >>>>>
> >>>>> This also changes existing ABI (Documentation/ABI/testing/sysfs-class-pwm):
> >>>>> - pmwX should be there: /sys/class/pwm/pwmchipN/pwmX
> >>>>>
> >>>>> Example on stm32 (stm32429i-eval) platform:
> >>>>> $ ls /sys/class/pwm
> >>>>> pwmchip0 pwmchip4
> >>>>>
> >>>>> $ cd /sys/class/pwm/pwmchip0/
> >>>>> $ echo 0 > export
> >>>>> $ ls /sys/class/pwm
> >>>>> pwm0 pwmchip0 pwmchip4
> >>>>>
> >>>>> $ cd /sys/class/pwm/pwmchip4/
> >>>>> $ echo 0 > export
> >>>>> sysfs: cannot create duplicate filename '/class/pwm/pwm0'
> >>>>> ...Exception stack follows...
> >>>>>
> >>>>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
> >>>>> ---
> >>>>>  drivers/pwm/sysfs.c | 1 -
> >>>>>  1 file changed, 1 deletion(-)
> >>>>
> >>>> Can we come up with an alternative that allows us to have both? We want
> >>>> uevent and proper sysfs creation, or is that not possible?
> >>>
> >>> Hi Thierry, all,
> >>>
> >>> With current approach:
> >>> - "export->child.class = parent->class"
> >>> - ABI (e.g. "pwm%d") device name isn't unique with multiple pwm chip.
> >>> I think this is not possible.
> >>>
> >>> Trying to think of an alternative... I just did a quick test, by
> >>> changing device name, to take pwmchip into account:
> >>> +       export->child.class = parent->class;
> >>>         export->child.release = pwm_export_release;
> >>>         export->child.parent = parent;
> >>>         export->child.devt = MKDEV(0, 0);
> >>>         export->child.groups = pwm_groups;
> >>> -       dev_set_name(&export->child, "pwm%u", pwm->hwpwm);
> >>> +       dev_set_name(&export->child, "pwmchip%d-pwm%u", chip->base,
> >>> pwm->hwpwm);
> >>>
> >>> But this also impacts existing ABI :-(
> >>> Would you have suggestions to send an uevent, without modifying ABI ?
> >>
> >> I don't quite understand why, in the example you show in the commit
> >> message, the pwmX nodes appear in the top-level /sys/class/pwm
> >> directory. According to Documentation/ABI/testing/sysfs-class-pwm they
> >> should appear as /sys/class/pwm/pwmchipN/pwmX. I can only imagine that
> >> setting the class may have changed that.
> > 
> > Yes, adding the class makes the link to be created under /sys/class/pwm:
> > device_register() -> device_add() -> device_add_class_symlinks()
> > In device_add_class_symlinks():
> > ...
> > 	if (!dev->class)
> > 		return 0;
> > ...
> > 	/* link in the class directory pointing to the device */
> > 	error = sysfs_create_link(&dev->class->p->subsys.kobj,
> > 				  &dev->kobj, dev_name(dev));
> > ...
> > 
> >> If so, perhaps we can
> >> workaround that by creating a new class that is not parent->class?
> 
> Hi Thierry,
> 
> Maybe there's a way around, keeping the revert patch, without impacting
> the ABI:
> - pwmX cannot be added to pwm/another class without issue as discussed
> (numbering isn't unique).
> - pwmchipN already belongs to pwm class.

Yeah, I think setting the exported PWM's class to that of the parent is
completely wrong. I just gave that a quick test and we get some strange
behaviour with that. For example we actually get two directories
created, one in /sys/class/pwm and another in the parent chip's
directory (e.g. /sys/class/pwmchip0).

One issue is of course that, as you reported, we get a duplicate file
because the numbering starts at 0 for each PWM chip. But what we also
get is a situation where each PWM channel is interpreted as a PWM chip
because it has the same class as the PWM chip.

So the /sys/class/pwm/pwm0 device gets the same attributes as its parent
chip, which means you get, among others, also the export attribute. What
happens then is really bad, because you can write to that file and the
kernel will oops. In my case it didn't actually panic, but it ended up
killing the shell and giving me a login prompt. The reason is of course
that the sysfs implementation assumes that anything that has the PWM
class is actually a PWM chip.

So I think the best course of action is to revert the offending commit
for now and get it reverted on all stable releases since it was applied
given how easy it now is to crash the kernel. Luckily sysfs requires
root privileges, so it's not a major security issue by default. However
since the purpose of the offending commit was to allow userspace to get
access to a PWM for non-root users, it means that random users can
potentially crash the kernel if userspace is configured to hand out
access randomly.

> I did some testing, trying to send uevent on the pwmX directly, without
> success: uevents are filtered as pwmX doesn't belong to a class.
> 
> Still, it is possible to send uevent (KOBJ_CHANGE) on pwmchipN device,
> to notify of a change, e.g. pwmX channel being exported/unexported.
> 
> I run following prototype (with revert patch).
> 
> static int pwm_export_child(struct device *parent, struct pwm_device *pwm)
>  {
> +       char *pwm_prop[2];
>         struct pwm_export *export;
>         int ret;
>  ...
>                 kfree(export);
>                 return ret;
>         }
> +       pwm_prop[0] = kasprintf(GFP_KERNEL, "EXPORT=pwm%u", pwm->hwpwm);
> +       pwm_prop[1] = NULL;
> +       kobject_uevent_env(&parent->kobj, KOBJ_CHANGE, pwm_prop);
> +       kfree(pwm_prop[0]);
> 
>         return 0;
>  }
> 
>  static int pwm_unexport_child(struct device *parent, struct pwm_device
> *pwm)
>  {
>         struct device *child;
> +       char *pwm_prop[2];
> 
>         if (!test_and_clear_bit(PWMF_EXPORTED, &pwm->flags))
>                 return -ENODEV;
> ...
>         if (!child)
>                 return -ENODEV;
> 
> +       pwm_prop[0] = kasprintf(GFP_KERNEL, "UNEXPORT=pwm%u", pwm->hwpwm);
> +       pwm_prop[1] = NULL;
> +       kobject_uevent_env(&parent->kobj, KOBJ_CHANGE, pwm_prop);
> +       kfree(pwm_prop[0]);
> +
>         /* for device_find_child() */
> 
> Then, I run a quick test:
> 
> # udevadm monitor --environment &
> # echo 0 > /sys/class/pwm/pwmchip0/export
> KERNEL[197.321736] change   /devices/.../pwm/pwmchip0 (pwm)
> ACTION=change
> DEVPATH=/devices/.../pwm/pwmchip0
> EXPORT=pwm0
> SEQNUM=2045
> SUBSYSTEM=pwm
> 
> # echo 0 > /sys/class/pwm/pwmchip4/export
> KERNEL[202.089676] change   /devices/.../pwm/pwmchip4 (pwm)
> ACTION=change
> DEVPATH=/devices/.../pwm/pwmchip4
> EXPORT=pwm0
> SEQNUM=2046
> SUBSYSTEM=pwm
> 
> 
> Also unexport will report change events to userland:
> 
> # echo 0 > /sys/class/pwm/pwmchip0/unexport
> KERNEL[1691.112765] change   /devices/.../pwm/pwmchip0 (pwm)
> ACTION=change
> DEVPATH=/devices/.../pwm/pwmchip0
> SEQNUM=2047
> SUBSYSTEM=pwm
> UNEXPORT=pwm0
> 
> Do you think this may be a way around?
> Please let me know if this may satisfy need for uevents.

This is certainly interesting. However, if I interpret the commit
message of the offending commit correctly, the reason why the class was
set is because they wanted uevents to be sent for the individual PWM
devices. And I think they want KOBJ_ADD events to be sent so that they
can set permissions using udev, for example.

I'm not sure if udev could be configured to do this based on the EXPORT
and UNEXPORT uevent variables.

Let's see if Gottfried can clarify that.

Thanks for investigating this,
Thierry

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

Re: [RESEND PATCH] Revert "pwm: Set class for exported channels in sysfs"

[Gottfried Haider]

Hello Thierry & Fabrice,

>> Still, it is possible to send uevent (KOBJ_CHANGE) on pwmchipN device,
>> to notify of a change, e.g. pwmX channel being exported/unexported.

I tested this patch, and I am happy to report that it works with the
udev rule that Raspbian (Raspberry Pi's Debian derivative) has in
place:

SUBSYSTEM=="pwm*", PROGRAM="/bin/sh -c '\
        chown -R root:gpio /sys/class/pwm && chmod -R 770 /sys/class/pwm;\
        chown -R root:gpio
/sys/devices/platform/soc/*.pwm/pwm/pwmchip* && chmod -R 770
/sys/devices/platform/soc/*.pwm/pwm/pwmchip*\
'"


So while I can't comment on the uevent semantics, it would solve what
the original patch attempted to enable (non-root use of pwm).

Tested-by: Gottfried Haider <gottfried.haider@gmail.com>

Best,
Gottfried

Re: [RESEND PATCH] Revert "pwm: Set class for exported channels in sysfs"

[Fabrice Gasnier]

On 09/29/2018 02:19 AM, Gottfried Haider wrote:
> Hello Thierry & Fabrice,
> 
>>> Still, it is possible to send uevent (KOBJ_CHANGE) on pwmchipN device,
>>> to notify of a change, e.g. pwmX channel being exported/unexported.
> 
> I tested this patch, and I am happy to report that it works with the
> udev rule that Raspbian (Raspberry Pi's Debian derivative) has in
> place:
> 
> SUBSYSTEM=="pwm*", PROGRAM="/bin/sh -c '\
>         chown -R root:gpio /sys/class/pwm && chmod -R 770 /sys/class/pwm;\
>         chown -R root:gpio
> /sys/devices/platform/soc/*.pwm/pwm/pwmchip* && chmod -R 770
> /sys/devices/platform/soc/*.pwm/pwm/pwmchip*\
> '"
> 
> 
> So while I can't comment on the uevent semantics, it would solve what
> the original patch attempted to enable (non-root use of pwm).
> 
> Tested-by: Gottfried Haider <gottfried.haider@gmail.com>

Hi Gottfried, Thierry,

Thanks for testing. I just sent a new series with:
- revert patch
- additional patch with proposed uevent notification (change) on pwmchip.

Best Regards,
Fabrice

> 
> Best,
> Gottfried
> 

[RESEND PATCH] Revert "pwm: Set class for exported channels in sysfs"

[Fabrice Gasnier]

This reverts commit 7e5d1fd75c3dde9fc10c4472b9368089d1b81d00 as it causes
regression with multiple pwm chip. It creates a new entry in
'/sys/class/pwm' every time a 'pwmX' is exported with 'echo X > export':
- 1st time export will create an entry in /sys/class/pwm/pwmX
- when another export happens on another pwmchip, it can't be created
(e.g. -EEXIST)

This also changes existing ABI (Documentation/ABI/testing/sysfs-class-pwm):
- pmwX should be there: /sys/class/pwm/pwmchipN/pwmX

Example on stm32 (stm32429i-eval) platform:
$ ls /sys/class/pwm
pwmchip0 pwmchip4

$ cd /sys/class/pwm/pwmchip0/
$ echo 0 > export
$ ls /sys/class/pwm
pwm0 pwmchip0 pwmchip4

$ cd /sys/class/pwm/pwmchip4/
$ echo 0 > export
sysfs: cannot create duplicate filename '/class/pwm/pwm0'
...Exception stack follows...

Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
---
 drivers/pwm/sysfs.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c
index 7c71cdb..4726d43 100644
--- a/drivers/pwm/sysfs.c
+++ b/drivers/pwm/sysfs.c
@@ -263,7 +263,6 @@ static int pwm_export_child(struct device *parent, struct pwm_device *pwm)
 	export->pwm = pwm;
 	mutex_init(&export->lock);
 
-	export->child.class = parent->class;
 	export->child.release = pwm_export_release;
 	export->child.parent = parent;
 	export->child.devt = MKDEV(0, 0);
-- 
1.9.1