PM: runtime: Add support to disable wakeup sources

PATCH Superseded Vimal Kumar 3 participants 7 messages Aug 21, 2022 – Sep 1, 2022

Subsystems (4)

Driver core Suspend to RAM Hibernation Power management core

lore

Share & tools

lore

Vimal Kumar

author

2022-08-21 13:46 UTC #0 lore raw

PATCH 1/1 PM: runtime: Add support to disable wakeup sources

User could find many wakeup sources available in the bsp, which they won't be using. Currently users can only get the status and list of enabled wakeup sources, but users can't disable it runtime. It's very difficult to find the driver for each wakeup sources from where it's getting enabled and make the changes for disabling it.

This will help users to disable any wakeup sources at runtime, avoiding any code change and re-compilation. A new class attribute "disable_ws" will be added in the wakeup calss. If user want to disable any wakeup sources, user need to find the wakeup dev node associated with the particular wakeup source and write the devnode name to the class attribute "disable_ws".

Example:
Need to disable the wakeup source '1c08000.qcom,pcie'. The dev node associated with this wakeup source is:
cat /sys/class/wakeup3/name ==> "1c08000.qcom,pcie", then for disabling this wakeup source :
echo wakeup3 > /sys/class/wakeup/disable_ws

Co-developed-by Mintu Patel <[email protected]>

Signed-off-by Mintu Patel <[email protected]>

Co-developed-by Vishal Badole <[email protected]>

Signed-off-by Vishal Badole <[email protected]>

Signed-off-by Vimal Kumar <[email protected]>

---
 drivers/base/power/wakeup_stats.c | 63 ++++++++++++++++++++++++++++++-
 1 file changed, 62 insertions(+), 1 deletion(-)

drivers/base/power/wakeup_stats.c +62 -1

index 924fac493c4f..ad30e97f168b 100644
@@ -15,6 +15,7 @@
 #include <linux/kobject.h>
 #include <linux/slab.h>
 #include <linux/timekeeping.h>
+#include <linux/uaccess.h>
 
 #include "power.h"

@@ -208,9 +209,69 @@ void wakeup_source_sysfs_remove(struct wakeup_source *ws)
 	device_unregister(ws->dev);
 }
 
+static ssize_t disable_ws_store(struct class *class,
+				struct class_attribute *attr,
+				const char *buf, size_t len)
+{
+	struct device		*dev;
+	struct wakeup_source	*ws;
+	char                    *ws_name;
+	int                     status;
+
+	ws_name = kzalloc(sizeof(*(buf)), GFP_KERNEL);
+	if (!ws_name)
+		return -ENOMEM;
+
+	if (copy_from_user(ws_name, buf, sizeof(*(buf))))
+		return -EFAULT;
+
+	dev = class_find_device_by_name(wakeup_class, ws_name);
+	if (!dev)
+		pr_err("%s : %s dev not found\n", __func__, ws_name);
+
+	ws = dev_get_drvdata(dev);
+	if (ws->dev->parent != NULL) {
+
+		status = device_wakeup_disable(ws->dev->parent);
+		if (status < 0) {
+			/* In case of virtual device, return code will be -EINVAL
+			 * then unregister the wakeup source associated with it
+			 */
+			wakeup_source_unregister(ws);
+		}
+	} else
+		/* If the parent device is NULL, just unregister the wakeup source */
+		wakeup_source_unregister(ws);
+
+	return len;
+}
+
+static CLASS_ATTR_WO(disable_ws);
+
+static struct attribute *wakeup_class_attrs[] = {
+	&class_attr_disable_ws.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(wakeup_class);
+
 static int __init wakeup_sources_sysfs_init(void)
 {
-	wakeup_class = class_create(THIS_MODULE, "wakeup");
+	int status;
+
+	wakeup_class = kzalloc(sizeof(*wakeup_class), GFP_KERNEL);
+	if (!wakeup_class)
+		return -ENOMEM;
+
+	wakeup_class->name = "wakeup";
+	wakeup_class->owner = THIS_MODULE;
+	wakeup_class->class_groups = wakeup_class_groups;
+
+	status = class_register(wakeup_class);
+
+	if (status < 0) {
+		pr_err("%s: class register failed %d\n", __func__, status);
+		return status;
+	}
 
 	return PTR_ERR_OR_ZERO(wakeup_class);
 }

Signature · 1 line

2.25.1

Greg KH

maintainer

2022-08-21 14:02 UTC #1 lore raw

Quoted 20 lines · Vimal Kumar

User could find many wakeup sources available in the bsp, which they won't be using. Currently users can only get the status and list of enabled wakeup sources, but users can't disable it runtime. It's very difficult to find the driver for each wakeup sources from where it's getting enabled and make the changes for disabling it.
This will help users to disable any wakeup sources at runtime, avoiding any code change and re-compilation. A new class attribute "disable_ws" will be added in the wakeup calss. If user want to disable any wakeup sources, user need to find the wakeup dev node associated with the particular wakeup source and write the devnode name to the class attribute "disable_ws".
Example:
Need to disable the wakeup source '1c08000.qcom,pcie'. The dev node associated with this wakeup source is:
cat /sys/class/wakeup3/name ==> "1c08000.qcom,pcie", then for disabling this wakeup source :
echo wakeup3 > /sys/class/wakeup/disable_ws

Quoted 5 lines

Co-developed-by Mintu Patel <[email protected]>
Signed-off-by Mintu Patel <[email protected]>
Co-developed-by Vishal Badole <[email protected]>
Signed-off-by Vishal Badole <[email protected]>
Signed-off-by Vimal Kumar <[email protected]>

That's a lot of gmail accounts, no "company" accounts? :)

Quoted 2 lines

---
 drivers/base/power/wakeup_stats.c | 63 ++++++++++++++++++++++++++++++-

You need a Documentation/ABI/ entry for any new sysfs file so we can properly review this and support it over time.

thanks,

greg k-h

Greg KH

maintainer

2022-08-21 14:04 UTC #2 lore raw

In reply to Vimal Kumar

Quoted 43 lines · Vimal Kumar

User could find many wakeup sources available in the bsp, which they won't be using. Currently users can only get the status and list of enabled wakeup sources, but users can't disable it runtime. It's very difficult to find the driver for each wakeup sources from where it's getting enabled and make the changes for disabling it.
This will help users to disable any wakeup sources at runtime, avoiding any code change and re-compilation. A new class attribute "disable_ws" will be added in the wakeup calss. If user want to disable any wakeup sources, user need to find the wakeup dev node associated with the particular wakeup source and write the devnode name to the class attribute "disable_ws".
Example:
Need to disable the wakeup source '1c08000.qcom,pcie'. The dev node associated with this wakeup source is:
cat /sys/class/wakeup3/name ==> "1c08000.qcom,pcie", then for disabling this wakeup source :
echo wakeup3 > /sys/class/wakeup/disable_ws
Co-developed-by Mintu Patel <[email protected]>
Signed-off-by Mintu Patel <[email protected]>
Co-developed-by Vishal Badole <[email protected]>
Signed-off-by Vishal Badole <[email protected]>
Signed-off-by Vimal Kumar <[email protected]>
---
 drivers/base/power/wakeup_stats.c | 63 ++++++++++++++++++++++++++++++-
 1 file changed, 62 insertions(+), 1 deletion(-)
drivers/base/power/wakeup_stats.c +1 -0
index 924fac493c4f..ad30e97f168b 100644
@@ -15,6 +15,7 @@
 #include <linux/kobject.h>
 #include <linux/slab.h>
 #include <linux/timekeeping.h>
+#include <linux/uaccess.h>
 
 #include "power.h"
 
@@ -208,9 +209,69 @@ void wakeup_source_sysfs_remove(struct wakeup_source *ws)
 	device_unregister(ws->dev);

drivers/base/power/wakeup_stats.c void wakeup_source_sysfs_remove(struct wakeup_source *ws) · line 219

Quoted 10 lines

drivers/base/power/wakeup_stats.c +8 -0

 }
 
+static ssize_t disable_ws_store(struct class *class,
+				struct class_attribute *attr,
+				const char *buf, size_t len)
+{
+	struct device		*dev;
+	struct wakeup_source	*ws;
+	char                    *ws_name;
+	int                     status;

Please don't pad these out to be in line like this, one space is all that is needed.

drivers/base/power/wakeup_stats.c void wakeup_source_sysfs_remove(struct wakeup_source *ws) · line 221

Quoted 2 lines

drivers/base/power/wakeup_stats.c +2 -0
+
+	ws_name = kzalloc(sizeof(*(buf)), GFP_KERNEL);

Are you sure this does what you think it does? You are allocating 8 bytes?

drivers/base/power/wakeup_stats.c void wakeup_source_sysfs_remove(struct wakeup_source *ws) · line 225

Quoted 4 lines

drivers/base/power/wakeup_stats.c +4 -0

+	if (!ws_name)
+		return -ENOMEM;
+
+	if (copy_from_user(ws_name, buf, sizeof(*(buf))))

Why are you doing this in a sysfs callback?

Did you test this code? How? Can you provide a test script for it also?

This does not look correct at all :(

thanks,

greg k-h

Vimal Kumar

author

2022-08-27 11:40 UTC #3 lore raw

Quoted 58 lines · Greg KH

Quoted 43 lines · Vimal Kumar
User could find many wakeup sources available in the bsp, which they won't be using. Currently users can only get the status and list of enabled wakeup sources, but users can't disable it runtime. It's very difficult to find the driver for each wakeup sources from where it's getting enabled and make the changes for disabling it.
This will help users to disable any wakeup sources at runtime, avoiding any code change and re-compilation. A new class attribute "disable_ws" will be added in the wakeup calss. If user want to disable any wakeup sources, user need to find the wakeup dev node associated with the particular wakeup source and write the devnode name to the class attribute "disable_ws".
Example:
Need to disable the wakeup source '1c08000.qcom,pcie'. The dev node associated with this wakeup source is:
cat /sys/class/wakeup3/name ==> "1c08000.qcom,pcie", then for disabling this wakeup source :
echo wakeup3 > /sys/class/wakeup/disable_ws
Co-developed-by Mintu Patel <[email protected]>
Signed-off-by Mintu Patel <[email protected]>
Co-developed-by Vishal Badole <[email protected]>
Signed-off-by Vishal Badole <[email protected]>
Signed-off-by Vimal Kumar <[email protected]>
---
 drivers/base/power/wakeup_stats.c | 63 ++++++++++++++++++++++++++++++-
 1 file changed, 62 insertions(+), 1 deletion(-)
drivers/base/power/wakeup_stats.c +1 -0
index 924fac493c4f..ad30e97f168b 100644
@@ -15,6 +15,7 @@
 #include <linux/kobject.h>
 #include <linux/slab.h>
 #include <linux/timekeeping.h>
+#include <linux/uaccess.h>
 
 #include "power.h"
 
@@ -208,9 +209,69 @@ void wakeup_source_sysfs_remove(struct wakeup_source *ws)
 	device_unregister(ws->dev);
drivers/base/power/wakeup_stats.c void wakeup_source_sysfs_remove(struct wakeup_source *ws) · line 219
Quoted 10 lines
drivers/base/power/wakeup_stats.c +8 -0
 }
 
+static ssize_t disable_ws_store(struct class *class,
+				struct class_attribute *attr,
+				const char *buf, size_t len)
+{
+	struct device		*dev;
+	struct wakeup_source	*ws;
+	char                    *ws_name;
+	int                     status;
Please don't pad these out to be in line like this, one space is all that is needed.

Quoted 5 lines

drivers/base/power/wakeup_stats.c void wakeup_source_sysfs_remove(struct wakeup_source *ws) · line 221
Quoted 2 lines
drivers/base/power/wakeup_stats.c +2 -0
+
+	ws_name = kzalloc(sizeof(*(buf)), GFP_KERNEL);
Are you sure this does what you think it does? You are allocating 8 bytes?

When I checked later, It was allocating 1 byte. My intension was to get the length on string wrriten from user. The parameter "size_t len" can be directly used adding one more byte.

Quoted 8 lines

drivers/base/power/wakeup_stats.c void wakeup_source_sysfs_remove(struct wakeup_source *ws) · line 225
Quoted 5 lines
drivers/base/power/wakeup_stats.c +4 -0
+	if (!ws_name)
+		return -ENOMEM;
+
+	if (copy_from_user(ws_name, buf, sizeof(*(buf))))
Why are you doing this in a sysfs callback?

Quoted 8 lines

Did you test this code? How? Can you provide a test script for it also?
This does not look correct at all :(
thanks,
greg k-h

I tested this code by manually wrriting wakeup device name from /sys/class/wakeup/ like "wakeup0", "wakeup13" etc to class attribute "disable_ws". Although, the code implementation using copy_from_user is wrong, I end up testing this code by directly using "buf" in the next call class_find_device_by_name like:

	class_find_device_by_name(wakeup_class, buf);

I will provide the next version of patch taking care of the review comments.

Warm Regards,
Vimal Kumar

Greg KH

maintainer

2022-09-01 15:56 UTC #4 lore raw

Quoted 8 lines · Vimal Kumar

Quoted 7 lines · Greg KH
Quoted 6 lines · Vimal Kumar
User could find many wakeup sources available in the bsp, which they won't be using. Currently users can only get the status and list of enabled wakeup sources, but users can't disable it runtime. It's very difficult to find the driver for each wakeup sources from where it's getting enabled and make the changes for disabling it.

Quoted 6 lines

Quoted 6 lines
Quoted 6 lines
This will help users to disable any wakeup sources at runtime, avoiding any code change and re-compilation. A new class attribute "disable_ws" will be added in the wakeup calss. If user want to disable any wakeup sources, user need to find the wakeup dev node associated with the particular wakeup source and write the devnode name to the class attribute "disable_ws".

What userspace tool will use this new interface?

Who is supposed to interact with it? Why is this even needed? Who would disable this dynamically, shouldn't the kernel handle this all automatically with no need for usersapce to get involved?

What is the root problem here you are trying to solve?

thanks,

greg k-h

kernel test robot

build bot

2022-08-21 20:07 UTC #5 lore raw

In reply to Vimal Kumar

Thank you for the patch! Perhaps something to improve:

Hi Vimal,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on rafael-pm/linux-next]
[also build test WARNING on linus/master v6.0-rc1 next-20220819] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Vimal-Kumar/PM-runtime-Add-support-to-disable-wakeup-sources/20220821-214614
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
config: i386-randconfig-s003 (https://download.01.org/0day-ci/archive/20220822/[email protected]/config)
compiler: gcc-11 (Debian 11.3.0-5) 11.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.4-39-gce1a6720-dirty
        # https://github.com/intel-lab-lkp/linux/commit/dee2f4d4c4b79cbfc7b2c792294b5137872d7c0c
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Vimal-Kumar/PM-runtime-Add-support-to-disable-wakeup-sources/20220821-214614
        git checkout dee2f4d4c4b79cbfc7b2c792294b5137872d7c0c
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=i386 SHELL=/bin/bash drivers/base/power/

If you fix the issue, kindly add following tag where applicable

Reported-by kernel test robot <[email protected]>

sparse warnings: (new ones prefixed by >>)

>> drivers/base/power/wakeup_stats.c:225:37: sparse: sparse: incorrect type in argument 2 (different address spaces) @@     expected void const [noderef] __user *from @@     got char const *buf @@

drivers/base/power/wakeup_stats.c:225:37: sparse: expected void const [noderef] __user *from
drivers/base/power/wakeup_stats.c:225:37: sparse: got char const *buf

vim +225 drivers/base/power/wakeup_stats.c

211
212 static ssize_t disable_ws_store(struct class *class,
213 struct class_attribute *attr,
214 const char *buf, size_t len)
215 {
216 struct device *dev;
217 struct wakeup_source *ws;
218 char *ws_name;
219 int status;
220
221 ws_name = kzalloc(sizeof(*(buf)), GFP_KERNEL);
222 if (!ws_name)
223 return -ENOMEM;
224

 > 225		if (copy_from_user(ws_name, buf, sizeof(*(buf))))

226 return -EFAULT;
227
228 dev = class_find_device_by_name(wakeup_class, ws_name);
229 if (!dev)
230 pr_err("%s : %s dev not found\n", __func__, ws_name);
231
232 ws = dev_get_drvdata(dev);
233 if (ws->dev->parent != NULL) {
234
235 status = device_wakeup_disable(ws->dev->parent);
236 if (status < 0) {
237 /* In case of virtual device, return code will be -EINVAL
238 * then unregister the wakeup source associated with it
239 */
240 wakeup_source_unregister(ws);
241 }
242 } else
243 /* If the parent device is NULL, just unregister the wakeup source */
244 wakeup_source_unregister(ws);
245
246 return len;
247 }
248

Signature · 2 lines

0-DAY CI Kernel Test Service
https://01.org/lkp

kernel test robot

build bot

2022-08-21 20:18 UTC #6 lore raw

In reply to Vimal Kumar

Thank you for the patch! Perhaps something to improve:

Hi Vimal,

Thank you for the patch! Perhaps something to improve:

url:    https://github.com/intel-lab-lkp/linux/commits/Vimal-Kumar/PM-runtime-Add-support-to-disable-wakeup-sources/20220821-214614
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
config: i386-randconfig-c021 (https://download.01.org/0day-ci/archive/20220822/[email protected]/config)
compiler: gcc-11 (Debian 11.3.0-5) 11.3.0

If you fix the issue, kindly add following tag where applicable

Reported-by kernel test robot <[email protected]>

cocci warnings: (new ones prefixed by >>)

>> drivers/base/power/wakeup_stats.c:221:11-18: WARNING opportunity for memdup_user

Signature · 2 lines

0-DAY CI Kernel Test Service
https://01.org/lkp