All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v1] PM: runtime: Add support to disable wakeup sources
@ 2022-08-21 13:45 Vimal Kumar
  2022-08-21 14:01 ` Greg KH
                   ` (3 more replies)
  0 siblings, 4 replies; 7+ messages in thread
From: Vimal Kumar @ 2022-08-21 13:45 UTC (permalink / raw)
  To: gregkh
  Cc: chinmoyghosh2001, Vimal Kumar, Mintu Patel, Vishal Badole,
	Rafael J. Wysocki, Pavel Machek, Len Brown, linux-pm,
	linux-kernel

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 <mintupatel89@gmail.com>
Signed-off-by: Mintu Patel <mintupatel89@gmail.com>
Co-developed-by: Vishal Badole <badolevishal1116@gmail.com>
Signed-off-by: Vishal Badole <badolevishal1116@gmail.com>
Signed-off-by: Vimal Kumar <vimal.kumar32@gmail.com>
---
 drivers/base/power/wakeup_stats.c | 63 ++++++++++++++++++++++++++++++-
 1 file changed, 62 insertions(+), 1 deletion(-)

diff --git a/drivers/base/power/wakeup_stats.c b/drivers/base/power/wakeup_stats.c
index 924fac493c4f..ad30e97f168b 100644
--- a/drivers/base/power/wakeup_stats.c
+++ b/drivers/base/power/wakeup_stats.c
@@ -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);
 }
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH v1] PM: runtime: Add support to disable wakeup sources
  2022-08-21 13:45 [PATCH v1] PM: runtime: Add support to disable wakeup sources Vimal Kumar
@ 2022-08-21 14:01 ` Greg KH
  2022-08-21 14:03 ` Greg KH
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 7+ messages in thread
From: Greg KH @ 2022-08-21 14:01 UTC (permalink / raw)
  To: Vimal Kumar
  Cc: chinmoyghosh2001, Mintu Patel, Vishal Badole, Rafael J. Wysocki,
	Pavel Machek, Len Brown, linux-pm, linux-kernel

On Sun, Aug 21, 2022 at 07:15:32PM +0530, Vimal Kumar wrote:
> 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 <mintupatel89@gmail.com>
> Signed-off-by: Mintu Patel <mintupatel89@gmail.com>
> Co-developed-by: Vishal Badole <badolevishal1116@gmail.com>
> Signed-off-by: Vishal Badole <badolevishal1116@gmail.com>
> Signed-off-by: Vimal Kumar <vimal.kumar32@gmail.com>

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

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v1] PM: runtime: Add support to disable wakeup sources
  2022-08-21 13:45 [PATCH v1] PM: runtime: Add support to disable wakeup sources Vimal Kumar
  2022-08-21 14:01 ` Greg KH
@ 2022-08-21 14:03 ` Greg KH
  2022-08-27 11:40   ` Vimal Kumar
  2022-08-21 20:06 ` kernel test robot
  2022-08-21 20:17 ` kernel test robot
  3 siblings, 1 reply; 7+ messages in thread
From: Greg KH @ 2022-08-21 14:03 UTC (permalink / raw)
  To: Vimal Kumar
  Cc: chinmoyghosh2001, Mintu Patel, Vishal Badole, Rafael J. Wysocki,
	Pavel Machek, Len Brown, linux-pm, linux-kernel

On Sun, Aug 21, 2022 at 07:15:32PM +0530, Vimal Kumar wrote:
> 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 <mintupatel89@gmail.com>
> Signed-off-by: Mintu Patel <mintupatel89@gmail.com>
> Co-developed-by: Vishal Badole <badolevishal1116@gmail.com>
> Signed-off-by: Vishal Badole <badolevishal1116@gmail.com>
> Signed-off-by: Vimal Kumar <vimal.kumar32@gmail.com>
> ---
>  drivers/base/power/wakeup_stats.c | 63 ++++++++++++++++++++++++++++++-
>  1 file changed, 62 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/base/power/wakeup_stats.c b/drivers/base/power/wakeup_stats.c
> index 924fac493c4f..ad30e97f168b 100644
> --- a/drivers/base/power/wakeup_stats.c
> +++ b/drivers/base/power/wakeup_stats.c
> @@ -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;

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

> +
> +	ws_name = kzalloc(sizeof(*(buf)), GFP_KERNEL);

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

> +	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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v1] PM: runtime: Add support to disable wakeup sources
  2022-08-21 13:45 [PATCH v1] PM: runtime: Add support to disable wakeup sources Vimal Kumar
  2022-08-21 14:01 ` Greg KH
  2022-08-21 14:03 ` Greg KH
@ 2022-08-21 20:06 ` kernel test robot
  2022-08-21 20:17 ` kernel test robot
  3 siblings, 0 replies; 7+ messages in thread
From: kernel test robot @ 2022-08-21 20:06 UTC (permalink / raw)
  To: Vimal Kumar, gregkh
  Cc: kbuild-all, chinmoyghosh2001, Vimal Kumar, Mintu Patel,
	Vishal Badole, Rafael J. Wysocki, Pavel Machek, Len Brown,
	linux-pm, linux-kernel

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/202208220446.3Qzss7sC-lkp@intel.com/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 <lkp@intel.com>

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	

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v1] PM: runtime: Add support to disable wakeup sources
  2022-08-21 13:45 [PATCH v1] PM: runtime: Add support to disable wakeup sources Vimal Kumar
                   ` (2 preceding siblings ...)
  2022-08-21 20:06 ` kernel test robot
@ 2022-08-21 20:17 ` kernel test robot
  3 siblings, 0 replies; 7+ messages in thread
From: kernel test robot @ 2022-08-21 20:17 UTC (permalink / raw)
  To: Vimal Kumar, gregkh
  Cc: kbuild-all, chinmoyghosh2001, Vimal Kumar, Mintu Patel,
	Vishal Badole, Rafael J. Wysocki, Pavel Machek, Len Brown,
	linux-pm, linux-kernel

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-c021 (https://download.01.org/0day-ci/archive/20220822/202208220446.ZLg90bEJ-lkp@intel.com/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 <lkp@intel.com>

cocci warnings: (new ones prefixed by >>)
>> drivers/base/power/wakeup_stats.c:221:11-18: WARNING opportunity for memdup_user

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v1] PM: runtime: Add support to disable wakeup sources
  2022-08-21 14:03 ` Greg KH
@ 2022-08-27 11:40   ` Vimal Kumar
  2022-09-01 15:56     ` Greg KH
  0 siblings, 1 reply; 7+ messages in thread
From: Vimal Kumar @ 2022-08-27 11:40 UTC (permalink / raw)
  To: Greg KH
  Cc: chinmoyghosh2001, Mintu Patel, Vishal Badole, Rafael J. Wysocki,
	Pavel Machek, Len Brown, linux-pm, linux-kernel

On Sun, Aug 21, 2022 at 04:03:40PM +0200, Greg KH wrote:
> On Sun, Aug 21, 2022 at 07:15:32PM +0530, Vimal Kumar wrote:
> > 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 <mintupatel89@gmail.com>
> > Signed-off-by: Mintu Patel <mintupatel89@gmail.com>
> > Co-developed-by: Vishal Badole <badolevishal1116@gmail.com>
> > Signed-off-by: Vishal Badole <badolevishal1116@gmail.com>
> > Signed-off-by: Vimal Kumar <vimal.kumar32@gmail.com>
> > ---
> >  drivers/base/power/wakeup_stats.c | 63 ++++++++++++++++++++++++++++++-
> >  1 file changed, 62 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/base/power/wakeup_stats.c b/drivers/base/power/wakeup_stats.c
> > index 924fac493c4f..ad30e97f168b 100644
> > --- a/drivers/base/power/wakeup_stats.c
> > +++ b/drivers/base/power/wakeup_stats.c
> > @@ -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;
> 
> Please don't pad these out to be in line like this, one space is all
> that is needed.
> 
> > +
> > +	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.

> 
> > +	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

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v1] PM: runtime: Add support to disable wakeup sources
  2022-08-27 11:40   ` Vimal Kumar
@ 2022-09-01 15:56     ` Greg KH
  0 siblings, 0 replies; 7+ messages in thread
From: Greg KH @ 2022-09-01 15:56 UTC (permalink / raw)
  To: Vimal Kumar
  Cc: chinmoyghosh2001, Mintu Patel, Vishal Badole, Rafael J. Wysocki,
	Pavel Machek, Len Brown, linux-pm, linux-kernel

On Sat, Aug 27, 2022 at 05:10:28PM +0530, Vimal Kumar wrote:
> On Sun, Aug 21, 2022 at 04:03:40PM +0200, Greg KH wrote:
> > On Sun, Aug 21, 2022 at 07:15:32PM +0530, Vimal Kumar wrote:
> > > 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".

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2022-09-01 15:56 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-21 13:45 [PATCH v1] PM: runtime: Add support to disable wakeup sources Vimal Kumar
2022-08-21 14:01 ` Greg KH
2022-08-21 14:03 ` Greg KH
2022-08-27 11:40   ` Vimal Kumar
2022-09-01 15:56     ` Greg KH
2022-08-21 20:06 ` kernel test robot
2022-08-21 20:17 ` kernel test robot

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.