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 <[email protected]>
---
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
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 <[email protected]>
> ---
>
> 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
Hi Greg,
On Tue, Jul 7, 2020 at 7:29 AM Greg Kroah-Hartman
<[email protected]> 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 <[email protected]>
> > ---
> >
> > 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
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 <[email protected]>
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
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
<[email protected]> 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 <[email protected]>
>
>
>
> 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
>