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

linux-pm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed

* [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

[parent not found: <20200714052941.GB3874@shao2-debian>]

* 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).