Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sat, 27 Aug 2022 17:10:28 +0530
From:   Vimal Kumar <vimal.kumar32@gmail.com>
To:     Greg KH <gregkh@linuxfoundation.org>
Cc:     chinmoyghosh2001@gmail.com, Mintu Patel <mintupatel89@gmail.com>,
        Vishal Badole <badolevishal1116@gmail.com>,
        "Rafael J. Wysocki" <rafael@kernel.org>,
        Pavel Machek <pavel@ucw.cz>, Len Brown <len.brown@intel.com>,
        linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v1] PM: runtime: Add support to disable wakeup sources
Message-ID: <20220827114028.GA258@DESKTOP-KA7F9LU.localdomain>
References: <20220821134533.22901-1-vimal.kumar32@gmail.com>
 <YwI7PM9wVF/H9hQL@kroah.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YwI7PM9wVF/H9hQL@kroah.com>
Precedence: bulk

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