[PATCH v4 0/1] power: Emit change uevent when updating sysfs

[PATCH v4 0/1] power: Emit change uevent when updating sysfs

[Abhishek Pandit-Subedi]

Hi linux-pm,

ChromeOS has a udev rule to chown the `power/wakeup` attribute so that
the power manager can modify it during runtime.

(https://source.chromium.org/chromiumos/chromiumos/codesearch/+/master:src/platform2/power_manager/udev/99-powerd-permissions.rules)

In our automated tests, we found that the `power/wakeup` attributes
weren't being chown-ed for some boards. On investigating, I found that
when the drivers probe and call device_set_wakeup_capable, no uevent was
being emitted for the newly added power/wakeup attribute. This was
manifesting at boot on some boards (Marvell SDIO bluetooth and Broadcom
Serial bluetooth drivers) or during usb disconnects during resume
(Realtek btusb driver with reset resume quirk).

It seems reasonable to me that changes to the attributes of a device
should cause a changed uevent so I have added that here.

Here's an example of the kernel events after toggling the authorized
bit of /sys/bus/usb/devices/1-3/

$ echo 0 > /sys/bus/usb/devices/1-3/authorized
KERNEL[27.357994] remove   /devices/pci0000:00/0000:00:15.0/usb1/1-3/1-3:1.0/bluetooth/hci0/rfkill1 (rfkill)
KERNEL[27.358049] remove   /devices/pci0000:00/0000:00:15.0/usb1/1-3/1-3:1.0/bluetooth/hci0 (bluetooth)
KERNEL[27.358458] remove   /devices/pci0000:00/0000:00:15.0/usb1/1-3/1-3:1.0 (usb)
KERNEL[27.358486] remove   /devices/pci0000:00/0000:00:15.0/usb1/1-3/1-3:1.1 (usb)
KERNEL[27.358529] change   /devices/pci0000:00/0000:00:15.0/usb1/1-3 (usb)

$ echo 1 > /sys/bus/usb/devices/1-3/authorized
KERNEL[36.415749] change   /devices/pci0000:00/0000:00:15.0/usb1/1-3 (usb)
KERNEL[36.415798] add      /devices/pci0000:00/0000:00:15.0/usb1/1-3/1-3:1.0 (usb)
KERNEL[36.417414] add      /devices/pci0000:00/0000:00:15.0/usb1/1-3/1-3:1.0/bluetooth/hci0 (bluetooth)
KERNEL[36.417447] add      /devices/pci0000:00/0000:00:15.0/usb1/1-3/1-3:1.0/bluetooth/hci0/rfkill2 (rfkill)
KERNEL[36.417481] add      /devices/pci0000:00/0000:00:15.0/usb1/1-3/1-3:1.1 (usb)

Series-version 4 update:
Tested again on device and verified it's working as expected:
KERNEL[52.678174] remove   /devices/pci0000:00/0000:00:15.0/usb1/1-3/1-3:1.0/bluetooth/hci0/rfkill2 (rfkill)
KERNEL[52.678211] remove   /devices/pci0000:00/0000:00:15.0/usb1/1-3/1-3:1.0/bluetooth/hci0 (bluetooth)
KERNEL[52.678251] remove   /devices/pci0000:00/0000:00:15.0/usb1/1-3/1-3:1.0 (usb)
KERNEL[52.679721] remove   /devices/pci0000:00/0000:00:15.0/usb1/1-3/1-3:1.1 (usb)
KERNEL[52.679772] change   /devices/pci0000:00/0000:00:15.0/usb1/1-3 (usb)
KERNEL[56.022259] change   /devices/pci0000:00/0000:00:15.0/usb1/1-3 (usb)
KERNEL[56.022306] add      /devices/pci0000:00/0000:00:15.0/usb1/1-3/1-3:1.0 (usb)
KERNEL[56.022488] add      /devices/pci0000:00/0000:00:15.0/usb1/1-3/1-3:1.0/bluetooth/hci0 (bluetooth)
KERNEL[56.022531] add      /devices/pci0000:00/0000:00:15.0/usb1/1-3/1-3:1.0/bluetooth/hci0/rfkill3 (rfkill)
KERNEL[56.023392] add      /devices/pci0000:00/0000:00:15.0/usb1/1-3/1-3:1.1 (usb)

Thanks
Abhishek

Changes in v4:
- Fix warning where returning from void and tested on device

Changes in v3:
- Simplified error handling

Changes in v2:
- Add newline at end of bt_dev_err

Abhishek Pandit-Subedi (1):
  power: Emit changed uevent on wakeup_sysfs_add/remove

 drivers/base/power/sysfs.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

-- 
2.27.0.212.ge8ba1cc988-goog

[PATCH v4 1/1] power: Emit changed uevent on wakeup_sysfs_add/remove

[Abhishek Pandit-Subedi]

Udev rules that depend on the power/wakeup attribute don't get triggered
correctly if device_set_wakeup_capable is called after the device is
created. This can happen for several reasons (driver sets wakeup after
device is created, wakeup is changed on parent device, etc) and it seems
reasonable to emit a changed event when adding or removing attributes on
the device.

Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
---

Changes in v4:
- Fix warning where returning from void and tested on device

Changes in v3:
- Simplified error handling

Changes in v2:
- Add newline at end of bt_dev_err

 drivers/base/power/sysfs.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
index 24d25cf8ab1487..aeb58d40aac8de 100644
--- a/drivers/base/power/sysfs.c
+++ b/drivers/base/power/sysfs.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
 /* sysfs entries for device PM */
 #include <linux/device.h>
+#include <linux/kobject.h>
 #include <linux/string.h>
 #include <linux/export.h>
 #include <linux/pm_qos.h>
@@ -739,12 +740,18 @@ int dpm_sysfs_change_owner(struct device *dev, kuid_t kuid, kgid_t kgid)
 
 int wakeup_sysfs_add(struct device *dev)
 {
-	return sysfs_merge_group(&dev->kobj, &pm_wakeup_attr_group);
+	int ret = sysfs_merge_group(&dev->kobj, &pm_wakeup_attr_group);
+
+	if (ret)
+		return ret;
+
+	return kobject_uevent(&dev->kobj, KOBJ_CHANGE);
 }
 
 void wakeup_sysfs_remove(struct device *dev)
 {
 	sysfs_unmerge_group(&dev->kobj, &pm_wakeup_attr_group);
+	kobject_uevent(&dev->kobj, KOBJ_CHANGE);
 }
 
 int pm_qos_sysfs_add_resume_latency(struct device *dev)
-- 
2.27.0.212.ge8ba1cc988-goog

Re: [PATCH v4 1/1] power: Emit changed uevent on wakeup_sysfs_add/remove

[Abhishek Pandit-Subedi]

Built and tested on Chromebook w/ 5.4 kernel.

Sorry about the churn -- will start building with warnings = errors
before I send patches upstream.

Thanks
Abhishek

On Tue, Jul 7, 2020 at 9:24 AM Abhishek Pandit-Subedi
<abhishekpandit@chromium.org> wrote:
>
> Udev rules that depend on the power/wakeup attribute don't get triggered
> correctly if device_set_wakeup_capable is called after the device is
> created. This can happen for several reasons (driver sets wakeup after
> device is created, wakeup is changed on parent device, etc) and it seems
> reasonable to emit a changed event when adding or removing attributes on
> the device.
>
> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> ---
>
> Changes in v4:
> - Fix warning where returning from void and tested on device
>
> Changes in v3:
> - Simplified error handling
>
> Changes in v2:
> - Add newline at end of bt_dev_err
>
>  drivers/base/power/sysfs.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
> index 24d25cf8ab1487..aeb58d40aac8de 100644
> --- a/drivers/base/power/sysfs.c
> +++ b/drivers/base/power/sysfs.c
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0
>  /* sysfs entries for device PM */
>  #include <linux/device.h>
> +#include <linux/kobject.h>
>  #include <linux/string.h>
>  #include <linux/export.h>
>  #include <linux/pm_qos.h>
> @@ -739,12 +740,18 @@ int dpm_sysfs_change_owner(struct device *dev, kuid_t kuid, kgid_t kgid)
>
>  int wakeup_sysfs_add(struct device *dev)
>  {
> -       return sysfs_merge_group(&dev->kobj, &pm_wakeup_attr_group);
> +       int ret = sysfs_merge_group(&dev->kobj, &pm_wakeup_attr_group);
> +
> +       if (ret)
> +               return ret;
> +
> +       return kobject_uevent(&dev->kobj, KOBJ_CHANGE);
>  }
>
>  void wakeup_sysfs_remove(struct device *dev)
>  {
>         sysfs_unmerge_group(&dev->kobj, &pm_wakeup_attr_group);
> +       kobject_uevent(&dev->kobj, KOBJ_CHANGE);
>  }
>
>  int pm_qos_sysfs_add_resume_latency(struct device *dev)
> --
> 2.27.0.212.ge8ba1cc988-goog
>

Re: [PATCH v4 1/1] power: Emit changed uevent on wakeup_sysfs_add/remove

[Rafael J. Wysocki]

On Tue, Jul 7, 2020 at 6:24 PM Abhishek Pandit-Subedi
<abhishekpandit@chromium.org> wrote:
>
> Udev rules that depend on the power/wakeup attribute don't get triggered
> correctly if device_set_wakeup_capable is called after the device is
> created. This can happen for several reasons (driver sets wakeup after
> device is created, wakeup is changed on parent device, etc) and it seems
> reasonable to emit a changed event when adding or removing attributes on
> the device.
>
> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> ---
>
> Changes in v4:
> - Fix warning where returning from void and tested on device
>
> Changes in v3:
> - Simplified error handling
>
> Changes in v2:
> - Add newline at end of bt_dev_err
>
>  drivers/base/power/sysfs.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
> index 24d25cf8ab1487..aeb58d40aac8de 100644
> --- a/drivers/base/power/sysfs.c
> +++ b/drivers/base/power/sysfs.c
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0
>  /* sysfs entries for device PM */
>  #include <linux/device.h>
> +#include <linux/kobject.h>
>  #include <linux/string.h>
>  #include <linux/export.h>
>  #include <linux/pm_qos.h>
> @@ -739,12 +740,18 @@ int dpm_sysfs_change_owner(struct device *dev, kuid_t kuid, kgid_t kgid)
>
>  int wakeup_sysfs_add(struct device *dev)
>  {
> -       return sysfs_merge_group(&dev->kobj, &pm_wakeup_attr_group);
> +       int ret = sysfs_merge_group(&dev->kobj, &pm_wakeup_attr_group);
> +
> +       if (ret)
> +               return ret;
> +
> +       return kobject_uevent(&dev->kobj, KOBJ_CHANGE);

So let me repeat the previous comment:

If you return an error here, it may confuse the caller to think that
the operation has failed completely, whereas the merging of the
attribute group has been successful already.

I don't think that an error can be returned at this point.

>  }
>
>  void wakeup_sysfs_remove(struct device *dev)
>  {
>         sysfs_unmerge_group(&dev->kobj, &pm_wakeup_attr_group);
> +       kobject_uevent(&dev->kobj, KOBJ_CHANGE);
>  }
>
>  int pm_qos_sysfs_add_resume_latency(struct device *dev)
> --
> 2.27.0.212.ge8ba1cc988-goog
>

Re: [PATCH v4 1/1] power: Emit changed uevent on wakeup_sysfs_add/remove

[Abhishek Pandit-Subedi]

Hi Rafael,

(resent in plain text)

On Tue, Jul 7, 2020 at 9:28 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Tue, Jul 7, 2020 at 6:24 PM Abhishek Pandit-Subedi
> <abhishekpandit@chromium.org> wrote:
> >
> > Udev rules that depend on the power/wakeup attribute don't get triggered
> > correctly if device_set_wakeup_capable is called after the device is
> > created. This can happen for several reasons (driver sets wakeup after
> > device is created, wakeup is changed on parent device, etc) and it seems
> > reasonable to emit a changed event when adding or removing attributes on
> > the device.
> >
> > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> > ---
> >
> > Changes in v4:
> > - Fix warning where returning from void and tested on device
> >
> > Changes in v3:
> > - Simplified error handling
> >
> > Changes in v2:
> > - Add newline at end of bt_dev_err
> >
> >  drivers/base/power/sysfs.c | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
> > index 24d25cf8ab1487..aeb58d40aac8de 100644
> > --- a/drivers/base/power/sysfs.c
> > +++ b/drivers/base/power/sysfs.c
> > @@ -1,6 +1,7 @@
> >  // SPDX-License-Identifier: GPL-2.0
> >  /* sysfs entries for device PM */
> >  #include <linux/device.h>
> > +#include <linux/kobject.h>
> >  #include <linux/string.h>
> >  #include <linux/export.h>
> >  #include <linux/pm_qos.h>
> > @@ -739,12 +740,18 @@ int dpm_sysfs_change_owner(struct device *dev, kuid_t kuid, kgid_t kgid)
> >
> >  int wakeup_sysfs_add(struct device *dev)
> >  {
> > -       return sysfs_merge_group(&dev->kobj, &pm_wakeup_attr_group);
> > +       int ret = sysfs_merge_group(&dev->kobj, &pm_wakeup_attr_group);
> > +
> > +       if (ret)
> > +               return ret;
> > +
> > +       return kobject_uevent(&dev->kobj, KOBJ_CHANGE);
>
> So let me repeat the previous comment:
>
> If you return an error here, it may confuse the caller to think that
> the operation has failed completely, whereas the merging of the
> attribute group has been successful already.
>
> I don't think that an error can be returned at this point.
>

The caller looks at the return code and just logs that an error
occurred (no other action). It's also unlikely for kobject_uevent to
fail (I saw mostly -ENOMEM and an -ENOENT when the kobj wasn't in the
correct set).

Call site:
    int ret = wakeup_sysfs_add(dev);

    if (ret)
        dev_info(dev, "Wakeup sysfs attributes not added\n");

So I'm ok with either keeping this as-is (caller isn't getting
confused, just logging) or swallowing the return of kobject_uevent.

> >  }
> >
> >  void wakeup_sysfs_remove(struct device *dev)
> >  {
> >         sysfs_unmerge_group(&dev->kobj, &pm_wakeup_attr_group);
> > +       kobject_uevent(&dev->kobj, KOBJ_CHANGE);
> >  }
> >
> >  int pm_qos_sysfs_add_resume_latency(struct device *dev)
> > --
> > 2.27.0.212.ge8ba1cc988-goog
> >

Re: [PATCH v4 1/1] power: Emit changed uevent on wakeup_sysfs_add/remove

[Rafael J. Wysocki]

On Tue, Jul 7, 2020 at 6:48 PM Abhishek Pandit-Subedi
<abhishekpandit@chromium.org> wrote:
>
> Hi Rafael,
>
> (resent in plain text)
>
> On Tue, Jul 7, 2020 at 9:28 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Tue, Jul 7, 2020 at 6:24 PM Abhishek Pandit-Subedi
> > <abhishekpandit@chromium.org> wrote:
> > >
> > > Udev rules that depend on the power/wakeup attribute don't get triggered
> > > correctly if device_set_wakeup_capable is called after the device is
> > > created. This can happen for several reasons (driver sets wakeup after
> > > device is created, wakeup is changed on parent device, etc) and it seems
> > > reasonable to emit a changed event when adding or removing attributes on
> > > the device.
> > >
> > > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> > > ---
> > >
> > > Changes in v4:
> > > - Fix warning where returning from void and tested on device
> > >
> > > Changes in v3:
> > > - Simplified error handling
> > >
> > > Changes in v2:
> > > - Add newline at end of bt_dev_err
> > >
> > >  drivers/base/power/sysfs.c | 9 ++++++++-
> > >  1 file changed, 8 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
> > > index 24d25cf8ab1487..aeb58d40aac8de 100644
> > > --- a/drivers/base/power/sysfs.c
> > > +++ b/drivers/base/power/sysfs.c
> > > @@ -1,6 +1,7 @@
> > >  // SPDX-License-Identifier: GPL-2.0
> > >  /* sysfs entries for device PM */
> > >  #include <linux/device.h>
> > > +#include <linux/kobject.h>
> > >  #include <linux/string.h>
> > >  #include <linux/export.h>
> > >  #include <linux/pm_qos.h>
> > > @@ -739,12 +740,18 @@ int dpm_sysfs_change_owner(struct device *dev, kuid_t kuid, kgid_t kgid)
> > >
> > >  int wakeup_sysfs_add(struct device *dev)
> > >  {
> > > -       return sysfs_merge_group(&dev->kobj, &pm_wakeup_attr_group);
> > > +       int ret = sysfs_merge_group(&dev->kobj, &pm_wakeup_attr_group);
> > > +
> > > +       if (ret)
> > > +               return ret;
> > > +
> > > +       return kobject_uevent(&dev->kobj, KOBJ_CHANGE);
> >
> > So let me repeat the previous comment:
> >
> > If you return an error here, it may confuse the caller to think that
> > the operation has failed completely, whereas the merging of the
> > attribute group has been successful already.
> >
> > I don't think that an error can be returned at this point.
> >
>
> The caller looks at the return code and just logs that an error
> occurred (no other action). It's also unlikely for kobject_uevent to
> fail (I saw mostly -ENOMEM and an -ENOENT when the kobj wasn't in the
> correct set).
>
> Call site:
>     int ret = wakeup_sysfs_add(dev);
>
>     if (ret)
>         dev_info(dev, "Wakeup sysfs attributes not added\n");

Yes, which is confusing, because the wakeup attributes may in fact
have been added.  Which is my point.

>
> So I'm ok with either keeping this as-is (caller isn't getting
> confused, just logging) or swallowing the return of kobject_uevent.

I would just ignore the return value of kobject_uevent() along the
lines of wakeup_sysfs_remove() below.

Thanks!

> > >  }
> > >
> > >  void wakeup_sysfs_remove(struct device *dev)
> > >  {
> > >         sysfs_unmerge_group(&dev->kobj, &pm_wakeup_attr_group);
> > > +       kobject_uevent(&dev->kobj, KOBJ_CHANGE);
> > >  }
> > >
> > >  int pm_qos_sysfs_add_resume_latency(struct device *dev)
> > > --
> > > 2.27.0.212.ge8ba1cc988-goog
> > >

Re: [PATCH v4 1/1] power: Emit changed uevent on wakeup_sysfs_add/remove

[Abhishek Pandit-Subedi]

Sounds good to me. Patch v5 incoming after compile-test.

Thanks
Abhishek

On Tue, Jul 7, 2020 at 10:16 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Tue, Jul 7, 2020 at 6:48 PM Abhishek Pandit-Subedi
> <abhishekpandit@chromium.org> wrote:
> >
> > Hi Rafael,
> >
> > (resent in plain text)
> >
> > On Tue, Jul 7, 2020 at 9:28 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > >
> > > On Tue, Jul 7, 2020 at 6:24 PM Abhishek Pandit-Subedi
> > > <abhishekpandit@chromium.org> wrote:
> > > >
> > > > Udev rules that depend on the power/wakeup attribute don't get triggered
> > > > correctly if device_set_wakeup_capable is called after the device is
> > > > created. This can happen for several reasons (driver sets wakeup after
> > > > device is created, wakeup is changed on parent device, etc) and it seems
> > > > reasonable to emit a changed event when adding or removing attributes on
> > > > the device.
> > > >
> > > > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> > > > ---
> > > >
> > > > Changes in v4:
> > > > - Fix warning where returning from void and tested on device
> > > >
> > > > Changes in v3:
> > > > - Simplified error handling
> > > >
> > > > Changes in v2:
> > > > - Add newline at end of bt_dev_err
> > > >
> > > >  drivers/base/power/sysfs.c | 9 ++++++++-
> > > >  1 file changed, 8 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
> > > > index 24d25cf8ab1487..aeb58d40aac8de 100644
> > > > --- a/drivers/base/power/sysfs.c
> > > > +++ b/drivers/base/power/sysfs.c
> > > > @@ -1,6 +1,7 @@
> > > >  // SPDX-License-Identifier: GPL-2.0
> > > >  /* sysfs entries for device PM */
> > > >  #include <linux/device.h>
> > > > +#include <linux/kobject.h>
> > > >  #include <linux/string.h>
> > > >  #include <linux/export.h>
> > > >  #include <linux/pm_qos.h>
> > > > @@ -739,12 +740,18 @@ int dpm_sysfs_change_owner(struct device *dev, kuid_t kuid, kgid_t kgid)
> > > >
> > > >  int wakeup_sysfs_add(struct device *dev)
> > > >  {
> > > > -       return sysfs_merge_group(&dev->kobj, &pm_wakeup_attr_group);
> > > > +       int ret = sysfs_merge_group(&dev->kobj, &pm_wakeup_attr_group);
> > > > +
> > > > +       if (ret)
> > > > +               return ret;
> > > > +
> > > > +       return kobject_uevent(&dev->kobj, KOBJ_CHANGE);
> > >
> > > So let me repeat the previous comment:
> > >
> > > If you return an error here, it may confuse the caller to think that
> > > the operation has failed completely, whereas the merging of the
> > > attribute group has been successful already.
> > >
> > > I don't think that an error can be returned at this point.
> > >
> >
> > The caller looks at the return code and just logs that an error
> > occurred (no other action). It's also unlikely for kobject_uevent to
> > fail (I saw mostly -ENOMEM and an -ENOENT when the kobj wasn't in the
> > correct set).
> >
> > Call site:
> >     int ret = wakeup_sysfs_add(dev);
> >
> >     if (ret)
> >         dev_info(dev, "Wakeup sysfs attributes not added\n");
>
> Yes, which is confusing, because the wakeup attributes may in fact
> have been added.  Which is my point.
>
> >
> > So I'm ok with either keeping this as-is (caller isn't getting
> > confused, just logging) or swallowing the return of kobject_uevent.
>
> I would just ignore the return value of kobject_uevent() along the
> lines of wakeup_sysfs_remove() below.
>
> Thanks!
>
> > > >  }
> > > >
> > > >  void wakeup_sysfs_remove(struct device *dev)
> > > >  {
> > > >         sysfs_unmerge_group(&dev->kobj, &pm_wakeup_attr_group);
> > > > +       kobject_uevent(&dev->kobj, KOBJ_CHANGE);
> > > >  }
> > > >
> > > >  int pm_qos_sysfs_add_resume_latency(struct device *dev)
> > > > --
> > > > 2.27.0.212.ge8ba1cc988-goog
> > > >