* [PATCH v2 0/1] power: Emit change uevent when updating sysfs
@ 2020-07-06 21:07 Abhishek Pandit-Subedi
2020-07-06 21:07 ` [PATCH v2 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-06 21:07 UTC (permalink / raw)
To: linux-pm
Cc: linux-bluetooth, chromeos-bluetooth-upstreaming,
rafael.j.wysocki, swboyd, Abhishek Pandit-Subedi,
Rafael J. Wysocki, linux-kernel, Len Brown, Greg Kroah-Hartman,
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 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 | 21 ++++++++++++++++++++-
1 file changed, 20 insertions(+), 1 deletion(-)
--
2.27.0.212.ge8ba1cc988-goog
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2 1/1] power: Emit changed uevent on wakeup_sysfs_add/remove
2020-07-06 21:07 [PATCH v2 0/1] power: Emit change uevent when updating sysfs Abhishek Pandit-Subedi
@ 2020-07-06 21:07 ` Abhishek Pandit-Subedi
2020-07-07 14:28 ` Greg Kroah-Hartman
[not found] ` <20200714052941.GB3874@shao2-debian>
0 siblings, 2 replies; 5+ messages in thread
From: Abhishek Pandit-Subedi @ 2020-07-06 21:07 UTC (permalink / raw)
To: linux-pm
Cc: linux-bluetooth, chromeos-bluetooth-upstreaming,
rafael.j.wysocki, swboyd, Abhishek Pandit-Subedi,
Rafael J. Wysocki, linux-kernel, Len Brown, Greg Kroah-Hartman,
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 v2:
- Add newline at end of bt_dev_err
drivers/base/power/sysfs.c | 21 ++++++++++++++++++++-
1 file changed, 20 insertions(+), 1 deletion(-)
diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
index 24d25cf8ab1487..d57e8e7f175ebf 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,30 @@ 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) {
+ int tmp = kobject_uevent(&dev->kobj, KOBJ_CHANGE);
+
+ if (tmp)
+ dev_err(dev,
+ "Error in uevent for wakeup_sysfs_add: %d\n",
+ tmp);
+ }
+
+ return ret;
}
void wakeup_sysfs_remove(struct device *dev)
{
+ int tmp;
+
sysfs_unmerge_group(&dev->kobj, &pm_wakeup_attr_group);
+
+ tmp = kobject_uevent(&dev->kobj, KOBJ_CHANGE);
+ if (tmp)
+ dev_err(dev, "Error in uevent for wakeup_sysfs_remove: %d\n",
+ tmp);
}
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 v2 1/1] power: Emit changed uevent on wakeup_sysfs_add/remove
2020-07-06 21:07 ` [PATCH v2 1/1] power: Emit changed uevent on wakeup_sysfs_add/remove Abhishek Pandit-Subedi
@ 2020-07-07 14:28 ` Greg Kroah-Hartman
2020-07-07 15:31 ` Abhishek Pandit-Subedi
[not found] ` <20200714052941.GB3874@shao2-debian>
1 sibling, 1 reply; 5+ messages in thread
From: Greg Kroah-Hartman @ 2020-07-07 14:28 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 Mon, Jul 06, 2020 at 02:07:17PM -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 v2:
> - Add newline at end of bt_dev_err
>
> drivers/base/power/sysfs.c | 21 ++++++++++++++++++++-
> 1 file changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
> index 24d25cf8ab1487..d57e8e7f175ebf 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,30 @@ 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) {
> + int tmp = kobject_uevent(&dev->kobj, KOBJ_CHANGE);
> +
> + if (tmp)
> + dev_err(dev,
> + "Error in uevent for wakeup_sysfs_add: %d\n",
> + tmp);
> + }
> +
> + return ret;
> }
Shouldn't the above function look like this instead to be simpler:
int wakeup_sysfs_add(struct device *dev)
{
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)
> {
> + int tmp;
Use 'ret' like the above function had, to be consistent.
> +
> sysfs_unmerge_group(&dev->kobj, &pm_wakeup_attr_group);
> +
> + tmp = kobject_uevent(&dev->kobj, KOBJ_CHANGE);
> + if (tmp)
> + dev_err(dev, "Error in uevent for wakeup_sysfs_remove: %d\n",
nit, use __func__ to describe a function name, if you really want it.
Why do you need to send a message for this error, will that really ever
happen?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2 1/1] power: Emit changed uevent on wakeup_sysfs_add/remove
2020-07-07 14:28 ` Greg Kroah-Hartman
@ 2020-07-07 15:31 ` Abhishek Pandit-Subedi
0 siblings, 0 replies; 5+ messages in thread
From: Abhishek Pandit-Subedi @ 2020-07-07 15:31 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
Hi Greg,
On Tue, Jul 7, 2020 at 7:29 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Mon, Jul 06, 2020 at 02:07:17PM -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 v2:
> > - Add newline at end of bt_dev_err
> >
> > drivers/base/power/sysfs.c | 21 ++++++++++++++++++++-
> > 1 file changed, 20 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
> > index 24d25cf8ab1487..d57e8e7f175ebf 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,30 @@ 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) {
> > + int tmp = kobject_uevent(&dev->kobj, KOBJ_CHANGE);
> > +
> > + if (tmp)
> > + dev_err(dev,
> > + "Error in uevent for wakeup_sysfs_add: %d\n",
> > + tmp);
> > + }
> > +
> > + return ret;
> > }
>
> Shouldn't the above function look like this instead to be simpler:
>
> int wakeup_sysfs_add(struct device *dev)
> {
> 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)
> > {
> > + int tmp;
>
> Use 'ret' like the above function had, to be consistent.
>
> > +
> > sysfs_unmerge_group(&dev->kobj, &pm_wakeup_attr_group);
> > +
> > + tmp = kobject_uevent(&dev->kobj, KOBJ_CHANGE);
> > + if (tmp)
> > + dev_err(dev, "Error in uevent for wakeup_sysfs_remove: %d\n",
>
> nit, use __func__ to describe a function name, if you really want it.
> Why do you need to send a message for this error, will that really ever
> happen?
>
Looking through kobject_uevent, it does look like the errors are
unlikely to be seen (-ENOMEM, -ENOENT) and probably don't need to be
logged.
Will apply the suggestions above and send a v3.
Thanks,
Abhishek
> thanks,
>
> greg k-h
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [power] 47b918cf9a: kmsg.power_supply_ADP1:Error_in_uevent_for_wakeup_sysfs_add
[not found] ` <20200714052941.GB3874@shao2-debian>
@ 2020-07-14 16:41 ` Abhishek Pandit-Subedi
0 siblings, 0 replies; 5+ messages in thread
From: Abhishek Pandit-Subedi @ 2020-07-14 16:41 UTC (permalink / raw)
To: kernel test robot
Cc: Linux PM, Bluez mailing list, ChromeOS Bluetooth Upstreaming,
Rafael Wysocki, Stephen Boyd, Rafael J. Wysocki, LKML, Len Brown,
Greg Kroah-Hartman, Pavel Machek, lkp, yu.c.chen, Zhang, Rui
This version of the patch was not merged and the message above doesn't
exist in the merged patch:
https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git/commit/?h=bleeding-edge&id=9a3e9e6ff6d7f6b8ce7903893962d50adcbe82d2
The err log was emitted during boot as well and is innocuous since the
power_supply initializes fully in the next line:
kern :err : [ 5.918034] power_supply ADP1: Error in uevent for
wakeup_sysfs_add: -11
kern :info : [ 5.918300] ACPI: AC Adapter [ADP1] (on-line)
Abhishek
On Mon, Jul 13, 2020 at 10:30 PM kernel test robot
<rong.a.chen@intel.com> wrote:
>
> Greeting,
>
> FYI, we noticed the following commit (built with gcc-9):
>
> commit: 47b918cf9a1d2b6e36706fd2be2b91e65f490146 ("[PATCH v2 1/1] power: Emit changed uevent on wakeup_sysfs_add/remove")
> url: https://github.com/0day-ci/linux/commits/Abhishek-Pandit-Subedi/power-Emit-changed-uevent-on-wakeup_sysfs_add-remove/20200707-050912
> base: https://git.kernel.org/cgit/linux/kernel/git/rafael/linux-pm.git linux-next
>
> in testcase: suspend-stress
> with following parameters:
>
> mode: freeze
> iterations: 10
>
>
>
> on test machine: 4 threads Ivy Bridge with 4G memory
>
> caused below changes (please refer to attached dmesg/kmsg for entire log/backtrace):
>
>
>
>
> If you fix the issue, kindly add following tag
> Reported-by: kernel test robot <rong.a.chen@intel.com>
>
>
>
> kern :debug : [ 5.917685] calling acpi_ac_init+0x0/0xa3 @ 1
> kern :err : [ 5.918034] power_supply ADP1: Error in uevent for wakeup_sysfs_add: -11
> kern :info : [ 5.918300] ACPI: AC Adapter [ADP1] (on-line)
> kern :debug : [ 5.918500] initcall acpi_ac_init+0x0/0xa3 returned 0 after 609 usecs
> kern :debug : [ 5.918725] calling acpi_button_driver_init+0x0/0x53 @ 1
> kern :info : [ 5.919006] input: Power Button as /devices/LNXSYSTM:00/LNXSYBUS:00/PNP0C0C:00/input/input0
> kern :info : [ 5.919367] ACPI: Power Button [PWRB]
> kern :info : [ 5.919580] input: Lid Switch as /devices/LNXSYSTM:00/LNXSYBUS:00/PNP0C0D:00/input/input1
> kern :info : [ 5.919927] ACPI: Lid Switch [LID]
> kern :info : [ 5.920131] input: Power Button as /devices/LNXSYSTM:00/LNXPWRBN:00/input/input2
> kern :info : [ 5.920455] ACPI: Power Button [PWRF]
> kern :debug : [ 5.920644] initcall acpi_button_driver_init+0x0/0x53 returned 0 after 1669 usecs
> kern :debug : [ 5.920944] calling acpi_fan_driver_init+0x0/0x13 @ 1
> kern :debug : [ 5.921155] initcall acpi_fan_driver_init+0x0/0x13 returned 0 after 11 usecs
> kern :debug : [ 5.921388] calling acpi_processor_driver_init+0x0/0xb7 @ 1
> kern :debug : [ 5.921905] initcall acpi_processor_driver_init+0x0/0xb7 returned 0 after 299 usecs
> kern :debug : [ 5.922203] calling acpi_thermal_init+0x0/0x82 @ 1
> kern :info : [ 5.922755] thermal LNXTHERM:00: registered as thermal_zone0
> kern :info : [ 5.922977] ACPI: Thermal Zone [TZ01] (16 C)
> kern :debug : [ 5.923177] initcall acpi_thermal_init+0x0/0x82 returned 0 after 759 usecs
> kern :debug : [ 5.923409] calling acpi_battery_init+0x0/0x39 @ 1
> kern :debug : [ 5.923606] initcall acpi_battery_init+0x0/0x39 returned 0 after 4 usecs
> kern :debug : [ 5.923841] calling acpi_hed_driver_init+0x0/0x11 @ 1
> kern :debug : [ 5.924075] initcall acpi_hed_driver_init+0x0/0x11 returned 0 after 32 usecs
> kern :info : [ 5.924178] battery: ACPI: Battery Slot [BAT1] (battery present)
> kern :debug : [ 5.924309] calling bgrt_init+0x0/0xbe @ 1
> kern :debug : [ 5.924312] initcall bgrt_init+0x0/0xbe returned -19 after 0 usecs
> kern :debug : [ 5.924928] calling erst_init+0x0/0x309 @ 1
> kern :debug : [ 5.925110] initcall erst_init+0x0/0x309 returned 0 after 0 usecs
> kern :debug : [ 5.925325] calling ghes_init+0x0/0xe5 @ 1
> kern :debug : [ 5.925504] initcall ghes_init+0x0/0xe5 returned -19 after 0 usecs
> kern :debug : [ 5.925721] calling erst_dbg_init+0x0/0x2c @ 1
> kern :info : [ 5.925912] ERST DBG: ERST support is disabled.
>
>
>
> To reproduce:
>
> git clone https://github.com/intel/lkp-tests.git
> cd lkp-tests
> bin/lkp install job.yaml # job file is attached in this email
> bin/lkp run job.yaml
>
>
>
> Thanks,
> Rong Chen
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-07-14 16:41 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-06 21:07 [PATCH v2 0/1] power: Emit change uevent when updating sysfs Abhishek Pandit-Subedi
2020-07-06 21:07 ` [PATCH v2 1/1] power: Emit changed uevent on wakeup_sysfs_add/remove Abhishek Pandit-Subedi
2020-07-07 14:28 ` Greg Kroah-Hartman
2020-07-07 15:31 ` Abhishek Pandit-Subedi
[not found] ` <20200714052941.GB3874@shao2-debian>
2020-07-14 16:41 ` [power] 47b918cf9a: kmsg.power_supply_ADP1:Error_in_uevent_for_wakeup_sysfs_add 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).