diff mbox series

[v2,1/1] power: Emit changed uevent on wakeup_sysfs_add/remove

Message ID 20200706140715.v2.1.I51f5a0be89595b73c4dc17e6cf4cc6f26dc7f2fc@changeid (mailing list archive)
State Superseded
Headers show
Series power: Emit change uevent when updating sysfs | expand

Commit Message

Abhishek Pandit-Subedi July 6, 2020, 9:07 p.m. UTC 
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(-)

Comments

Greg Kroah-Hartman July 7, 2020, 2:28 p.m. UTC | #1 
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
Abhishek Pandit-Subedi July 7, 2020, 3:31 p.m. UTC | #2 
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
Abhishek Pandit-Subedi July 14, 2020, 4:41 p.m. UTC | #3 
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
>
diff mbox series

Patch

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)