* [PATCH v3 0/1] power: Emit change uevent when updating sysfs @ 2020-07-07 15:49 Abhishek Pandit-Subedi 2020-07-07 15:49 ` [PATCH v3 1/1] power: Emit changed uevent on wakeup_sysfs_add/remove Abhishek Pandit-Subedi 0 siblings, 1 reply; 5+ messages in thread From: Abhishek Pandit-Subedi @ 2020-07-07 15:49 UTC (permalink / raw) To: Greg Kroah-Hartman, linux-pm Cc: linux-bluetooth, chromeos-bluetooth-upstreaming, rafael.j.wysocki, swboyd, Abhishek Pandit-Subedi, Rafael J. Wysocki, linux-kernel, Len Brown, Pavel Machek 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) Thanks Abhishek 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v3 1/1] power: Emit changed uevent on wakeup_sysfs_add/remove 2020-07-07 15:49 [PATCH v3 0/1] power: Emit change uevent when updating sysfs Abhishek Pandit-Subedi @ 2020-07-07 15:49 ` Abhishek Pandit-Subedi 2020-07-07 15:53 ` Rafael J. Wysocki 2020-07-07 16:00 ` Greg Kroah-Hartman 0 siblings, 2 replies; 5+ messages in thread From: Abhishek Pandit-Subedi @ 2020-07-07 15:49 UTC (permalink / raw) To: Greg Kroah-Hartman, linux-pm Cc: linux-bluetooth, chromeos-bluetooth-upstreaming, rafael.j.wysocki, swboyd, Abhishek Pandit-Subedi, Rafael J. Wysocki, linux-kernel, Len Brown, Pavel Machek 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 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..04c82373c8f240 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); + return kobject_uevent(&dev->kobj, KOBJ_CHANGE); } int pm_qos_sysfs_add_resume_latency(struct device *dev) -- 2.27.0.212.ge8ba1cc988-goog ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v3 1/1] power: Emit changed uevent on wakeup_sysfs_add/remove 2020-07-07 15:49 ` [PATCH v3 1/1] power: Emit changed uevent on wakeup_sysfs_add/remove Abhishek Pandit-Subedi @ 2020-07-07 15:53 ` Rafael J. Wysocki 2020-07-07 16:00 ` Greg Kroah-Hartman 1 sibling, 0 replies; 5+ messages in thread From: Rafael J. Wysocki @ 2020-07-07 15:53 UTC (permalink / raw) To: Abhishek Pandit-Subedi Cc: Greg Kroah-Hartman, Linux PM, open list:BLUETOOTH DRIVERS, chromeos-bluetooth-upstreaming, Rafael Wysocki, Stephen Boyd, Rafael J. Wysocki, Linux Kernel Mailing List, Len Brown, Pavel Machek On Tue, Jul 7, 2020 at 5:49 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 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..04c82373c8f240 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); Returning an error code here may be misleading, because the basic operation (i.e. merging the attribute group) has succeeded already. What about printing a message if kobject_uevent() returns an error and returning 0 anyway? > } > > void wakeup_sysfs_remove(struct device *dev) > { > sysfs_unmerge_group(&dev->kobj, &pm_wakeup_attr_group); > + return kobject_uevent(&dev->kobj, KOBJ_CHANGE); This is a void function, so it cannot return anything. > } > > int pm_qos_sysfs_add_resume_latency(struct device *dev) > -- ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v3 1/1] power: Emit changed uevent on wakeup_sysfs_add/remove 2020-07-07 15:49 ` [PATCH v3 1/1] power: Emit changed uevent on wakeup_sysfs_add/remove Abhishek Pandit-Subedi 2020-07-07 15:53 ` Rafael J. Wysocki @ 2020-07-07 16:00 ` Greg Kroah-Hartman 2020-07-07 16:11 ` Abhishek Pandit-Subedi 1 sibling, 1 reply; 5+ messages in thread From: Greg Kroah-Hartman @ 2020-07-07 16:00 UTC (permalink / raw) To: Abhishek Pandit-Subedi Cc: linux-pm, linux-bluetooth, chromeos-bluetooth-upstreaming, rafael.j.wysocki, swboyd, Rafael J. Wysocki, linux-kernel, Len Brown, Pavel Machek On Tue, Jul 07, 2020 at 08:49:05AM -0700, Abhishek Pandit-Subedi 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 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..04c82373c8f240 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); > + return kobject_uevent(&dev->kobj, KOBJ_CHANGE); Always test build and hopefully actually test, your patches before sending them out :( ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v3 1/1] power: Emit changed uevent on wakeup_sysfs_add/remove 2020-07-07 16:00 ` Greg Kroah-Hartman @ 2020-07-07 16:11 ` Abhishek Pandit-Subedi 0 siblings, 0 replies; 5+ messages in thread From: Abhishek Pandit-Subedi @ 2020-07-07 16:11 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Linux PM, Bluez mailing list, ChromeOS Bluetooth Upstreaming, rafael.j.wysocki, Stephen Boyd, Rafael J. Wysocki, LKML, Len Brown, Pavel Machek I admit I didn't test this but I built it with allmodconfig. Sadly, this only raised a warning instead of an error so I didn't go look through the build logs (ignore the 5.4, the remote here is the linux-pm git repo): /usr/local/google/home/abhishekpandit/chromiumos/src/third_party/kernel/v5.4/drivers/base/power/sysfs.c: In function ‘wakeup_sysfs_remove’: /usr/local/google/home/abhishekpandit/chromiumos/src/third_party/kernel/v5.4/drivers/base/power/sysfs.c:754:9: warning: ‘return’ with a value, in function returning void [-Wreturn-type] 754 | return kobject_uevent(&dev->kobj, KOBJ_CHANGE); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /usr/local/google/home/abhishekpandit/chromiumos/src/third_party/kernel/v5.4/drivers/base/power/sysfs.c:751:6: note: declared here 751 | void wakeup_sysfs_remove(struct device *dev) Will send up another fix. Thanks Abhishek On Tue, Jul 7, 2020 at 9:00 AM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Tue, Jul 07, 2020 at 08:49:05AM -0700, Abhishek Pandit-Subedi 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 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..04c82373c8f240 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); > > + return kobject_uevent(&dev->kobj, KOBJ_CHANGE); > > Always test build and hopefully actually test, your patches before > sending them out :( > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-07-07 16:11 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-07-07 15:49 [PATCH v3 0/1] power: Emit change uevent when updating sysfs Abhishek Pandit-Subedi 2020-07-07 15:49 ` [PATCH v3 1/1] power: Emit changed uevent on wakeup_sysfs_add/remove Abhishek Pandit-Subedi 2020-07-07 15:53 ` Rafael J. Wysocki 2020-07-07 16:00 ` Greg Kroah-Hartman 2020-07-07 16:11 ` Abhishek Pandit-Subedi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).