From: Lars Poeschel <[email protected]>
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"")
Problem with the previous approach was, that there is a clash if we have
multiple pwmchips:
echo 0 > pwmchip0/export
echo 0 > pwmchip1/export
would both export /sys/class/pwm/pwm0 .
Now this patch changes the sysfs interface. We do include the pwmchip
number into the pwm directory that gets exported.
With the example above we get:
/sys/class/pwm/pwm-0-0
/sys/class/pwm/pwm-1-0
We maintain ABI backward compatibility through symlinks.
/sys/class/pwm/pwmchip0/pwm0
/sys/class/pwm/pwmchip1/pwm0
are now symbolic links to the new names.
Cc: Greg Kroah-Hartman <[email protected]>
Signed-off-by: Lars Poeschel <[email protected]>
---
drivers/pwm/sysfs.c | 57 +++++++++++++++++++++++++++++++++++++--------
1 file changed, 47 insertions(+), 10 deletions(-)
diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c
index 449dbc0f49ed..c708da17a857 100644
--- a/drivers/pwm/sysfs.c
+++ b/drivers/pwm/sysfs.c
@@ -240,8 +240,10 @@ static void pwm_export_release(struct device *child)
static int pwm_export_child(struct device *parent, struct pwm_device *pwm)
{
+ struct pwm_chip *chip = dev_get_drvdata(parent);
struct pwm_export *export;
char *pwm_prop[2];
+ char *link_name;
int ret;
if (test_and_set_bit(PWMF_EXPORTED, &pwm->flags))
@@ -256,25 +258,39 @@ 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);
export->child.groups = pwm_groups;
- dev_set_name(&export->child, "pwm%u", pwm->hwpwm);
+ dev_set_name(&export->child, "pwm-%u-%u", chip->base, pwm->hwpwm);
ret = device_register(&export->child);
- if (ret) {
- clear_bit(PWMF_EXPORTED, &pwm->flags);
- put_device(&export->child);
- export = NULL;
- return ret;
+ if (ret)
+ goto error;
+
+ link_name = kasprintf(GFP_KERNEL, "pwm%u", pwm->hwpwm);
+ if (link_name == NULL) {
+ ret = -ENOMEM;
+ goto dev_unregister;
}
- pwm_prop[0] = kasprintf(GFP_KERNEL, "EXPORT=pwm%u", pwm->hwpwm);
+
+ pwm_prop[0] = kasprintf(GFP_KERNEL, "EXPORT=%s",
+ export->child.kobj.name);
pwm_prop[1] = NULL;
kobject_uevent_env(&parent->kobj, KOBJ_CHANGE, pwm_prop);
kfree(pwm_prop[0]);
- return 0;
+ ret = sysfs_create_link(&parent->kobj, &export->child.kobj, link_name);
+ return ret;
+
+dev_unregister:
+ device_unregister(&export->child);
+error:
+ clear_bit(PWMF_EXPORTED, &pwm->flags);
+ put_device(&export->child);
+ export = NULL;
+ return ret;
}
static int pwm_unexport_match(struct device *child, void *data)
@@ -286,6 +302,7 @@ static int pwm_unexport_child(struct device *parent, struct pwm_device *pwm)
{
struct device *child;
char *pwm_prop[2];
+ char *link_name;
if (!test_and_clear_bit(PWMF_EXPORTED, &pwm->flags))
return -ENODEV;
@@ -294,7 +311,11 @@ static int pwm_unexport_child(struct device *parent, struct pwm_device *pwm)
if (!child)
return -ENODEV;
- pwm_prop[0] = kasprintf(GFP_KERNEL, "UNEXPORT=pwm%u", pwm->hwpwm);
+ link_name = kasprintf(GFP_KERNEL, "pwm%u", pwm->hwpwm);
+ if (link_name)
+ sysfs_delete_link(&parent->kobj, &child->kobj, link_name);
+
+ pwm_prop[0] = kasprintf(GFP_KERNEL, "UNEXPORT=%s", child->kobj.name);
pwm_prop[1] = NULL;
kobject_uevent_env(&parent->kobj, KOBJ_CHANGE, pwm_prop);
kfree(pwm_prop[0]);
@@ -365,13 +386,29 @@ static ssize_t npwm_show(struct device *parent, struct device_attribute *attr,
}
static DEVICE_ATTR_RO(npwm);
+static umode_t pwm_is_visible(struct kobject *kobj, struct attribute *attr,
+ int n)
+{
+ struct device *dev = container_of(kobj, struct device, kobj);
+
+ if (dev->groups == pwm_groups)
+ return 0;
+
+ return attr->mode;
+}
+
static struct attribute *pwm_chip_attrs[] = {
&dev_attr_export.attr,
&dev_attr_unexport.attr,
&dev_attr_npwm.attr,
NULL,
};
-ATTRIBUTE_GROUPS(pwm_chip);
+
+static const struct attribute_group pwm_chip_group = {
+ .attrs = pwm_chip_attrs,
+ .is_visible = pwm_is_visible,
+};
+__ATTRIBUTE_GROUPS(pwm_chip);
/* takes export->lock on success */
static struct pwm_export *pwm_class_get_state(struct device *parent,
--
2.28.0
From: Lars Poeschel <[email protected]>
This is an update to the documentation to reflect the change to pwm
sysfs.
/sys/class/pwm/pwmchipN/pwmX style exports are marked as deprecated.
They are still available as symlinks to the new interface.
New exports are available as /sys/class/pwm/pwm-N-X
Cc: Greg Kroah-Hartman <[email protected]>
Signed-off-by: Lars Poeschel <[email protected]>
---
Documentation/ABI/obsolete/sysfs-class-pwm | 52 +++++++++++++++++++
Documentation/ABI/testing/sysfs-class-pwm | 60 ++++++++++++----------
Documentation/driver-api/pwm.rst | 7 +--
3 files changed, 90 insertions(+), 29 deletions(-)
create mode 100644 Documentation/ABI/obsolete/sysfs-class-pwm
diff --git a/Documentation/ABI/obsolete/sysfs-class-pwm b/Documentation/ABI/obsolete/sysfs-class-pwm
new file mode 100644
index 000000000000..92ba4f3586e9
--- /dev/null
+++ b/Documentation/ABI/obsolete/sysfs-class-pwm
@@ -0,0 +1,52 @@
+What: /sys/class/pwm/pwmchipN/pwmX
+Date: May 2013
+KernelVersion: 3.11
+Contact: H Hartley Sweeten <[email protected]>
+Description:
+ A /sys/class/pwm/pwmchipN/pwmX directory is created for
+ each exported PWM channel where X is the exported PWM
+ channel number.
+
+What: /sys/class/pwm/pwmchipN/pwmX/period
+Date: May 2013
+KernelVersion: 3.11
+Contact: H Hartley Sweeten <[email protected]>
+Description:
+ Sets the PWM signal period in nanoseconds.
+
+What: /sys/class/pwm/pwmchipN/pwmX/duty_cycle
+Date: May 2013
+KernelVersion: 3.11
+Contact: H Hartley Sweeten <[email protected]>
+Description:
+ Sets the PWM signal duty cycle in nanoseconds.
+
+What: /sys/class/pwm/pwmchipN/pwmX/polarity
+Date: May 2013
+KernelVersion: 3.11
+Contact: H Hartley Sweeten <[email protected]>
+Description:
+ Sets the output polarity of the PWM signal to "normal" or
+ "inversed".
+
+What: /sys/class/pwm/pwmchipN/pwmX/enable
+Date: May 2013
+KernelVersion: 3.11
+Contact: H Hartley Sweeten <[email protected]>
+Description:
+ Enable/disable the PWM signal.
+ 0 is disabled
+ 1 is enabled
+
+What: /sys/class/pwm/pwmchipN/pwmX/capture
+Date: June 2016
+KernelVersion: 4.8
+Contact: Lee Jones <[email protected]>
+Description:
+ Capture information about a PWM signal. The output format is a
+ pair unsigned integers (period and duty cycle), separated by a
+ single space.
+
+ This ABI is deprecated and will be removed after 2025. It is replaced by
+ another sysfs ABI documented in Documentation/ABI/testing/sysfs-class-pwm
+
diff --git a/Documentation/ABI/testing/sysfs-class-pwm b/Documentation/ABI/testing/sysfs-class-pwm
index c20e61354561..87582dea1027 100644
--- a/Documentation/ABI/testing/sysfs-class-pwm
+++ b/Documentation/ABI/testing/sysfs-class-pwm
@@ -38,50 +38,58 @@ Contact: H Hartley Sweeten <[email protected]>
Description:
Unexports a PWM channel.
-What: /sys/class/pwm/pwmchipN/pwmX
-Date: May 2013
-KernelVersion: 3.11
-Contact: H Hartley Sweeten <[email protected]>
+What: /sys/class/pwm/pwm-N-X
+ /sys/class/pwm/pwmchipN/pwm-N-X
+Date: October 2020
+KernelVersion: 5.9
+Contact: Lars Poeschel <[email protected]>
Description:
- A /sys/class/pwm/pwmchipN/pwmX directory is created for
+ A /sys/class/pwm/pwm-N-X directory is created for
each exported PWM channel where X is the exported PWM
- channel number.
+ channel number and N is the number of the pwmchip
+ that this pwm belongs to.
+ /sys/class/pwm/pwmchipN/pwm-N-X is the same
-What: /sys/class/pwm/pwmchipN/pwmX/period
-Date: May 2013
-KernelVersion: 3.11
-Contact: H Hartley Sweeten <[email protected]>
+What: /sys/class/pwm/pwm-N-X/period
+ /sys/class/pwm/pwmchipN/pwm-N-X/period
+Date: October 2020
+KernelVersion: 5.9
+Contact: Lars Poeschel <[email protected]>
Description:
Sets the PWM signal period in nanoseconds.
-What: /sys/class/pwm/pwmchipN/pwmX/duty_cycle
-Date: May 2013
-KernelVersion: 3.11
-Contact: H Hartley Sweeten <[email protected]>
+What: /sys/class/pwm/pwm-N-X/duty_cycle
+ /sys/class/pwm/pwmchipN/pwm-N-X/duty_cycle
+Date: October 2020
+KernelVersion: 5.9
+Contact: Lars Poeschel <[email protected]>
Description:
Sets the PWM signal duty cycle in nanoseconds.
-What: /sys/class/pwm/pwmchipN/pwmX/polarity
-Date: May 2013
-KernelVersion: 3.11
-Contact: H Hartley Sweeten <[email protected]>
+What: /sys/class/pwm/pwm-N-X/polarity
+ /sys/class/pwm/pwmchipN/pwm-N-X/polarity
+Date: October 2020
+KernelVersion: 5.9
+Contact: Lars Poeschel <[email protected]>
Description:
Sets the output polarity of the PWM signal to "normal" or
"inversed".
-What: /sys/class/pwm/pwmchipN/pwmX/enable
-Date: May 2013
-KernelVersion: 3.11
-Contact: H Hartley Sweeten <[email protected]>
+What: /sys/class/pwm/pwm-N-X/enable
+ /sys/class/pwm/pwmchipN/pwm-N-X/enable
+Date: October 2020
+KernelVersion: 5.9
+Contact: Lars Poeschel <[email protected]>
Description:
Enable/disable the PWM signal.
0 is disabled
1 is enabled
-What: /sys/class/pwm/pwmchipN/pwmX/capture
-Date: June 2016
-KernelVersion: 4.8
-Contact: Lee Jones <[email protected]>
+What: /sys/class/pwm/pwm-N-X/capture
+ /sys/class/pwm/pwmchipN/pwm-N-X/capture
+Date: October 2020
+KernelVersion: 5.9
+Contact: Lars Poeschel <[email protected]>
Description:
Capture information about a PWM signal. The output format is a
pair unsigned integers (period and duty cycle), separated by a
diff --git a/Documentation/driver-api/pwm.rst b/Documentation/driver-api/pwm.rst
index ab62f1bb0366..9361cd9b136c 100644
--- a/Documentation/driver-api/pwm.rst
+++ b/Documentation/driver-api/pwm.rst
@@ -89,9 +89,10 @@ will find:
The PWM channels are numbered using a per-chip index from 0 to npwm-1.
-When a PWM channel is exported a pwmX directory will be created in the
-pwmchipN directory it is associated with, where X is the number of the
-channel that was exported. The following properties will then be available:
+When a PWM channel is exported a pwm-N-X directory will be created in the
+/sys/class/pwm/ directory. N is number of the PWM chip this pwm is associated
+with and X is the number of the channel that was exported. The following
+properties will then be available:
period
The total period of the PWM signal (read/write).
--
2.28.0
On Fri, Oct 02, 2020 at 02:30:47PM +0200, [email protected] wrote:
> + pwm_prop[0] = kasprintf(GFP_KERNEL, "EXPORT=%s",
> + export->child.kobj.name);
Nit, you should use dev_name() here instead of trying to get the raw
kobject name.
thanks,
greg k-h
On Fri, Oct 02, 2020 at 02:30:47PM +0200, [email protected] wrote:
> From: Lars Poeschel <[email protected]>
>
> 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"")
> Problem with the previous approach was, that there is a clash if we have
> multiple pwmchips:
> echo 0 > pwmchip0/export
> echo 0 > pwmchip1/export
> would both export /sys/class/pwm/pwm0 .
>
> Now this patch changes the sysfs interface. We do include the pwmchip
> number into the pwm directory that gets exported.
> With the example above we get:
> /sys/class/pwm/pwm-0-0
> /sys/class/pwm/pwm-1-0
> We maintain ABI backward compatibility through symlinks.
> /sys/class/pwm/pwmchip0/pwm0
> /sys/class/pwm/pwmchip1/pwm0
> are now symbolic links to the new names.
>
> Cc: Greg Kroah-Hartman <[email protected]>
> Signed-off-by: Lars Poeschel <[email protected]>
> ---
> drivers/pwm/sysfs.c | 57 +++++++++++++++++++++++++++++++++++++--------
> 1 file changed, 47 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c
> index 449dbc0f49ed..c708da17a857 100644
> --- a/drivers/pwm/sysfs.c
> +++ b/drivers/pwm/sysfs.c
> @@ -240,8 +240,10 @@ static void pwm_export_release(struct device *child)
>
> static int pwm_export_child(struct device *parent, struct pwm_device *pwm)
> {
> + struct pwm_chip *chip = dev_get_drvdata(parent);
> struct pwm_export *export;
> char *pwm_prop[2];
> + char *link_name;
> int ret;
>
> if (test_and_set_bit(PWMF_EXPORTED, &pwm->flags))
> @@ -256,25 +258,39 @@ 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);
> export->child.groups = pwm_groups;
> - dev_set_name(&export->child, "pwm%u", pwm->hwpwm);
> + dev_set_name(&export->child, "pwm-%u-%u", chip->base, pwm->hwpwm);
>
> ret = device_register(&export->child);
> - if (ret) {
> - clear_bit(PWMF_EXPORTED, &pwm->flags);
> - put_device(&export->child);
> - export = NULL;
> - return ret;
> + if (ret)
> + goto error;
> +
> + link_name = kasprintf(GFP_KERNEL, "pwm%u", pwm->hwpwm);
> + if (link_name == NULL) {
> + ret = -ENOMEM;
> + goto dev_unregister;
> }
> - pwm_prop[0] = kasprintf(GFP_KERNEL, "EXPORT=pwm%u", pwm->hwpwm);
> +
> + pwm_prop[0] = kasprintf(GFP_KERNEL, "EXPORT=%s",
> + export->child.kobj.name);
> pwm_prop[1] = NULL;
> kobject_uevent_env(&parent->kobj, KOBJ_CHANGE, pwm_prop);
Do you still need to do this by hand? Why can't this uevent field
belong to the class and have it create this for you automatically when
the device is added?
> kfree(pwm_prop[0]);
>
> - return 0;
> + ret = sysfs_create_link(&parent->kobj, &export->child.kobj, link_name);
You create the link _after_ you told userspace it was there, you raced
and lost :(
> + return ret;
> +
> +dev_unregister:
> + device_unregister(&export->child);
> +error:
> + clear_bit(PWMF_EXPORTED, &pwm->flags);
> + put_device(&export->child);
> + export = NULL;
> + return ret;
> }
>
> static int pwm_unexport_match(struct device *child, void *data)
> @@ -286,6 +302,7 @@ static int pwm_unexport_child(struct device *parent, struct pwm_device *pwm)
> {
> struct device *child;
> char *pwm_prop[2];
> + char *link_name;
>
> if (!test_and_clear_bit(PWMF_EXPORTED, &pwm->flags))
> return -ENODEV;
> @@ -294,7 +311,11 @@ static int pwm_unexport_child(struct device *parent, struct pwm_device *pwm)
> if (!child)
> return -ENODEV;
>
> - pwm_prop[0] = kasprintf(GFP_KERNEL, "UNEXPORT=pwm%u", pwm->hwpwm);
> + link_name = kasprintf(GFP_KERNEL, "pwm%u", pwm->hwpwm);
> + if (link_name)
> + sysfs_delete_link(&parent->kobj, &child->kobj, link_name);
> +
> + pwm_prop[0] = kasprintf(GFP_KERNEL, "UNEXPORT=%s", child->kobj.name);
> pwm_prop[1] = NULL;
> kobject_uevent_env(&parent->kobj, KOBJ_CHANGE, pwm_prop);
Same uevent question here.
Otherwise, this looks good, nice work in figuring out the is_visable
stuff and everything.
thanks,
greg k-h
On Fri, Oct 02, 2020 at 02:46:16PM +0200, Greg Kroah-Hartman wrote:
> On Fri, Oct 02, 2020 at 02:30:47PM +0200, [email protected] wrote:
> > From: Lars Poeschel <[email protected]>
> >
> > 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"")
> > Problem with the previous approach was, that there is a clash if we have
> > multiple pwmchips:
> > echo 0 > pwmchip0/export
> > echo 0 > pwmchip1/export
> > would both export /sys/class/pwm/pwm0 .
> >
> > Now this patch changes the sysfs interface. We do include the pwmchip
> > number into the pwm directory that gets exported.
> > With the example above we get:
> > /sys/class/pwm/pwm-0-0
> > /sys/class/pwm/pwm-1-0
> > We maintain ABI backward compatibility through symlinks.
> > /sys/class/pwm/pwmchip0/pwm0
> > /sys/class/pwm/pwmchip1/pwm0
> > are now symbolic links to the new names.
> >
> > Cc: Greg Kroah-Hartman <[email protected]>
> > Signed-off-by: Lars Poeschel <[email protected]>
> > ---
> > drivers/pwm/sysfs.c | 57 +++++++++++++++++++++++++++++++++++++--------
> > 1 file changed, 47 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c
> > index 449dbc0f49ed..c708da17a857 100644
> > --- a/drivers/pwm/sysfs.c
> > +++ b/drivers/pwm/sysfs.c
> > @@ -240,8 +240,10 @@ static void pwm_export_release(struct device *child)
> >
> > static int pwm_export_child(struct device *parent, struct pwm_device *pwm)
> > {
> > + struct pwm_chip *chip = dev_get_drvdata(parent);
> > struct pwm_export *export;
> > char *pwm_prop[2];
> > + char *link_name;
> > int ret;
> >
> > if (test_and_set_bit(PWMF_EXPORTED, &pwm->flags))
> > @@ -256,25 +258,39 @@ 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);
> > export->child.groups = pwm_groups;
> > - dev_set_name(&export->child, "pwm%u", pwm->hwpwm);
> > + dev_set_name(&export->child, "pwm-%u-%u", chip->base, pwm->hwpwm);
> >
> > ret = device_register(&export->child);
> > - if (ret) {
> > - clear_bit(PWMF_EXPORTED, &pwm->flags);
> > - put_device(&export->child);
> > - export = NULL;
> > - return ret;
> > + if (ret)
> > + goto error;
> > +
> > + link_name = kasprintf(GFP_KERNEL, "pwm%u", pwm->hwpwm);
> > + if (link_name == NULL) {
> > + ret = -ENOMEM;
> > + goto dev_unregister;
> > }
> > - pwm_prop[0] = kasprintf(GFP_KERNEL, "EXPORT=pwm%u", pwm->hwpwm);
> > +
> > + pwm_prop[0] = kasprintf(GFP_KERNEL, "EXPORT=%s",
> > + export->child.kobj.name);
> > pwm_prop[1] = NULL;
> > kobject_uevent_env(&parent->kobj, KOBJ_CHANGE, pwm_prop);
>
> Do you still need to do this by hand? Why can't this uevent field
> belong to the class and have it create this for you automatically when
> the device is added?
I did not add this with my patch, it was there before and I wonder, what
purpose it served, since the uevent was filtered because there was no
class there.
Now we have a class and now it works and this is what happens:
/sys/class/pwm# echo 0 > pwmchip1/export
KERNEL[2111.952725] add /devices/platform/ocp/48302000.epwmss/48302200.pwm/pwm/pwmchip1/pwm-1-0 (pwm)
ACTION=add
DEVPATH=/devices/platform/ocp/48302000.epwmss/48302200.pwm/pwm/pwmchip1/pwm-1-0
SEQNUM=1546
SUBSYSTEM=pwm
KERNEL[2111.955155] change /devices/platform/ocp/48302000.epwmss/48302200.pwm/pwm/pwmchip1 (pwm)
ACTION=change
DEVPATH=/devices/platform/ocp/48302000.epwmss/48302200.pwm/pwm/pwmchip1
EXPORT=pwm-1-0
SEQNUM=1547
SUBSYSTEM=pwm
The first event is the event from device_register. It informs us that we
now have a new pwm-1-0. Nice.
The second is the event done here "by hand". It informs us, that
pwmchip1 changed. It has a new export now. For me personally this is not
needed, but also I don't think it is wrong.
You decide!
> > kfree(pwm_prop[0]);
> >
> > - return 0;
> > + ret = sysfs_create_link(&parent->kobj, &export->child.kobj, link_name);
>
> You create the link _after_ you told userspace it was there, you raced
> and lost :(
Are you sure ?
We inform userspace that there is a "pwm-1-0" now available. We do not say
anything about "pwm0". "pwm0" is the name of our symlink. This should
not be a problem.
> > + return ret;
> > +
> > +dev_unregister:
> > + device_unregister(&export->child);
> > +error:
> > + clear_bit(PWMF_EXPORTED, &pwm->flags);
> > + put_device(&export->child);
> > + export = NULL;
> > + return ret;
> > }
> >
> > static int pwm_unexport_match(struct device *child, void *data)
> > @@ -286,6 +302,7 @@ static int pwm_unexport_child(struct device *parent, struct pwm_device *pwm)
> > {
> > struct device *child;
> > char *pwm_prop[2];
> > + char *link_name;
> >
> > if (!test_and_clear_bit(PWMF_EXPORTED, &pwm->flags))
> > return -ENODEV;
> > @@ -294,7 +311,11 @@ static int pwm_unexport_child(struct device *parent, struct pwm_device *pwm)
> > if (!child)
> > return -ENODEV;
> >
> > - pwm_prop[0] = kasprintf(GFP_KERNEL, "UNEXPORT=pwm%u", pwm->hwpwm);
> > + link_name = kasprintf(GFP_KERNEL, "pwm%u", pwm->hwpwm);
> > + if (link_name)
> > + sysfs_delete_link(&parent->kobj, &child->kobj, link_name);
> > +
> > + pwm_prop[0] = kasprintf(GFP_KERNEL, "UNEXPORT=%s", child->kobj.name);
> > pwm_prop[1] = NULL;
> > kobject_uevent_env(&parent->kobj, KOBJ_CHANGE, pwm_prop);
>
> Same uevent question here.
>
> Otherwise, this looks good, nice work in figuring out the is_visable
> stuff and everything.
Thanks! :-)
And thank you for your help so far.
Regards,
Lars
On Fri, Oct 02, 2020 at 03:08:44PM +0200, Lars Poeschel wrote:
> On Fri, Oct 02, 2020 at 02:46:16PM +0200, Greg Kroah-Hartman wrote:
> > On Fri, Oct 02, 2020 at 02:30:47PM +0200, [email protected] wrote:
> > > From: Lars Poeschel <[email protected]>
> > >
> > > 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"")
> > > Problem with the previous approach was, that there is a clash if we have
> > > multiple pwmchips:
> > > echo 0 > pwmchip0/export
> > > echo 0 > pwmchip1/export
> > > would both export /sys/class/pwm/pwm0 .
> > >
> > > Now this patch changes the sysfs interface. We do include the pwmchip
> > > number into the pwm directory that gets exported.
> > > With the example above we get:
> > > /sys/class/pwm/pwm-0-0
> > > /sys/class/pwm/pwm-1-0
> > > We maintain ABI backward compatibility through symlinks.
> > > /sys/class/pwm/pwmchip0/pwm0
> > > /sys/class/pwm/pwmchip1/pwm0
> > > are now symbolic links to the new names.
> > >
> > > Cc: Greg Kroah-Hartman <[email protected]>
> > > Signed-off-by: Lars Poeschel <[email protected]>
> > > ---
> > > drivers/pwm/sysfs.c | 57 +++++++++++++++++++++++++++++++++++++--------
> > > 1 file changed, 47 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c
> > > index 449dbc0f49ed..c708da17a857 100644
> > > --- a/drivers/pwm/sysfs.c
> > > +++ b/drivers/pwm/sysfs.c
> > > @@ -240,8 +240,10 @@ static void pwm_export_release(struct device *child)
> > >
> > > static int pwm_export_child(struct device *parent, struct pwm_device *pwm)
> > > {
> > > + struct pwm_chip *chip = dev_get_drvdata(parent);
> > > struct pwm_export *export;
> > > char *pwm_prop[2];
> > > + char *link_name;
> > > int ret;
> > >
> > > if (test_and_set_bit(PWMF_EXPORTED, &pwm->flags))
> > > @@ -256,25 +258,39 @@ 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);
> > > export->child.groups = pwm_groups;
> > > - dev_set_name(&export->child, "pwm%u", pwm->hwpwm);
> > > + dev_set_name(&export->child, "pwm-%u-%u", chip->base, pwm->hwpwm);
> > >
> > > ret = device_register(&export->child);
> > > - if (ret) {
> > > - clear_bit(PWMF_EXPORTED, &pwm->flags);
> > > - put_device(&export->child);
> > > - export = NULL;
> > > - return ret;
> > > + if (ret)
> > > + goto error;
> > > +
> > > + link_name = kasprintf(GFP_KERNEL, "pwm%u", pwm->hwpwm);
> > > + if (link_name == NULL) {
> > > + ret = -ENOMEM;
> > > + goto dev_unregister;
> > > }
> > > - pwm_prop[0] = kasprintf(GFP_KERNEL, "EXPORT=pwm%u", pwm->hwpwm);
> > > +
> > > + pwm_prop[0] = kasprintf(GFP_KERNEL, "EXPORT=%s",
> > > + export->child.kobj.name);
> > > pwm_prop[1] = NULL;
> > > kobject_uevent_env(&parent->kobj, KOBJ_CHANGE, pwm_prop);
> >
> > Do you still need to do this by hand? Why can't this uevent field
> > belong to the class and have it create this for you automatically when
> > the device is added?
>
> I did not add this with my patch, it was there before and I wonder, what
> purpose it served, since the uevent was filtered because there was no
> class there.
> Now we have a class and now it works and this is what happens:
>
> /sys/class/pwm# echo 0 > pwmchip1/export
> KERNEL[2111.952725] add /devices/platform/ocp/48302000.epwmss/48302200.pwm/pwm/pwmchip1/pwm-1-0 (pwm)
> ACTION=add
> DEVPATH=/devices/platform/ocp/48302000.epwmss/48302200.pwm/pwm/pwmchip1/pwm-1-0
> SEQNUM=1546
> SUBSYSTEM=pwm
>
> KERNEL[2111.955155] change /devices/platform/ocp/48302000.epwmss/48302200.pwm/pwm/pwmchip1 (pwm)
> ACTION=change
> DEVPATH=/devices/platform/ocp/48302000.epwmss/48302200.pwm/pwm/pwmchip1
> EXPORT=pwm-1-0
> SEQNUM=1547
> SUBSYSTEM=pwm
>
> The first event is the event from device_register. It informs us that we
> now have a new pwm-1-0. Nice.
> The second is the event done here "by hand". It informs us, that
> pwmchip1 changed. It has a new export now. For me personally this is not
> needed, but also I don't think it is wrong.
> You decide!
If the uevent was being filtered out anyway, and never sent, then let's
just drop the thing as there is nothing to keep backwards compatible.
thanks,
greg k-h
On Fri, Oct 02, 2020 at 03:35:12PM +0200, Greg Kroah-Hartman wrote:
> On Fri, Oct 02, 2020 at 03:08:44PM +0200, Lars Poeschel wrote:
> > On Fri, Oct 02, 2020 at 02:46:16PM +0200, Greg Kroah-Hartman wrote:
> > > On Fri, Oct 02, 2020 at 02:30:47PM +0200, [email protected] wrote:
> > > > From: Lars Poeschel <[email protected]>
> > > >
> > > > 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"")
> > > > Problem with the previous approach was, that there is a clash if we have
> > > > multiple pwmchips:
> > > > echo 0 > pwmchip0/export
> > > > echo 0 > pwmchip1/export
> > > > would both export /sys/class/pwm/pwm0 .
> > > >
> > > > Now this patch changes the sysfs interface. We do include the pwmchip
> > > > number into the pwm directory that gets exported.
> > > > With the example above we get:
> > > > /sys/class/pwm/pwm-0-0
> > > > /sys/class/pwm/pwm-1-0
> > > > We maintain ABI backward compatibility through symlinks.
> > > > /sys/class/pwm/pwmchip0/pwm0
> > > > /sys/class/pwm/pwmchip1/pwm0
> > > > are now symbolic links to the new names.
> > > >
> > > > Cc: Greg Kroah-Hartman <[email protected]>
> > > > Signed-off-by: Lars Poeschel <[email protected]>
> > > > ---
> > > > drivers/pwm/sysfs.c | 57 +++++++++++++++++++++++++++++++++++++--------
> > > > 1 file changed, 47 insertions(+), 10 deletions(-)
> > > >
> > > > diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c
> > > > index 449dbc0f49ed..c708da17a857 100644
> > > > --- a/drivers/pwm/sysfs.c
> > > > +++ b/drivers/pwm/sysfs.c
> > > > @@ -240,8 +240,10 @@ static void pwm_export_release(struct device *child)
> > > >
> > > > static int pwm_export_child(struct device *parent, struct pwm_device *pwm)
> > > > {
> > > > + struct pwm_chip *chip = dev_get_drvdata(parent);
> > > > struct pwm_export *export;
> > > > char *pwm_prop[2];
> > > > + char *link_name;
> > > > int ret;
> > > >
> > > > if (test_and_set_bit(PWMF_EXPORTED, &pwm->flags))
> > > > @@ -256,25 +258,39 @@ 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);
> > > > export->child.groups = pwm_groups;
> > > > - dev_set_name(&export->child, "pwm%u", pwm->hwpwm);
> > > > + dev_set_name(&export->child, "pwm-%u-%u", chip->base, pwm->hwpwm);
> > > >
> > > > ret = device_register(&export->child);
> > > > - if (ret) {
> > > > - clear_bit(PWMF_EXPORTED, &pwm->flags);
> > > > - put_device(&export->child);
> > > > - export = NULL;
> > > > - return ret;
> > > > + if (ret)
> > > > + goto error;
> > > > +
> > > > + link_name = kasprintf(GFP_KERNEL, "pwm%u", pwm->hwpwm);
> > > > + if (link_name == NULL) {
> > > > + ret = -ENOMEM;
> > > > + goto dev_unregister;
> > > > }
> > > > - pwm_prop[0] = kasprintf(GFP_KERNEL, "EXPORT=pwm%u", pwm->hwpwm);
> > > > +
> > > > + pwm_prop[0] = kasprintf(GFP_KERNEL, "EXPORT=%s",
> > > > + export->child.kobj.name);
> > > > pwm_prop[1] = NULL;
> > > > kobject_uevent_env(&parent->kobj, KOBJ_CHANGE, pwm_prop);
> > >
> > > Do you still need to do this by hand? Why can't this uevent field
> > > belong to the class and have it create this for you automatically when
> > > the device is added?
> >
> > I did not add this with my patch, it was there before and I wonder, what
> > purpose it served, since the uevent was filtered because there was no
> > class there.
> > Now we have a class and now it works and this is what happens:
> >
> > /sys/class/pwm# echo 0 > pwmchip1/export
> > KERNEL[2111.952725] add /devices/platform/ocp/48302000.epwmss/48302200.pwm/pwm/pwmchip1/pwm-1-0 (pwm)
> > ACTION=add
> > DEVPATH=/devices/platform/ocp/48302000.epwmss/48302200.pwm/pwm/pwmchip1/pwm-1-0
> > SEQNUM=1546
> > SUBSYSTEM=pwm
> >
> > KERNEL[2111.955155] change /devices/platform/ocp/48302000.epwmss/48302200.pwm/pwm/pwmchip1 (pwm)
> > ACTION=change
> > DEVPATH=/devices/platform/ocp/48302000.epwmss/48302200.pwm/pwm/pwmchip1
> > EXPORT=pwm-1-0
> > SEQNUM=1547
> > SUBSYSTEM=pwm
> >
> > The first event is the event from device_register. It informs us that we
> > now have a new pwm-1-0. Nice.
> > The second is the event done here "by hand". It informs us, that
> > pwmchip1 changed. It has a new export now. For me personally this is not
> > needed, but also I don't think it is wrong.
> > You decide!
>
> If the uevent was being filtered out anyway, and never sent, then let's
> just drop the thing as there is nothing to keep backwards compatible.
I had a sleepless night about this. I felt something is wrong with this.
I investigated a bit:
- for the kobject_uevent_env line git blame found this commit 552c02e3e7cfe
("pwm: Send a uevent on the pwmchip device upon channel sysfs
(un)export")
- the commit message explicitly mentions my use case! (udev event to
change permissions of exported pwm)
- git log says the commit right before is this one: commit c289d6625237
("Revert "pwm: Set class for exported channels in sysfs""). This is
the commit, that reverts the previous addition of the pwmchip class
right as is to the pwm. The one that had the sysfs name clash.
I must have done something horribly wrong. I tried again and now I can
get this udev change event. I don't know what I did wrong. The event is
not filtered, because this is sent on behalf of the parent that had the
class attached right from the start. This is my mistake. I am very sorry
for the noise.
I think best is now to keep everything as is, right ?
Regards,
Lars