Linux-PM Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 1/2] PM / devfreq: Fix governor module load failure
@ 2019-06-05 19:00 ` Ezequiel Garcia
  2019-06-05 19:00   ` [PATCH 2/2] PM / devfreq: Sanitize prints Ezequiel Garcia
                     ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Ezequiel Garcia @ 2019-06-05 19:00 UTC (permalink / raw)
  To: Kyungmin Park, MyungJoo Ham
  Cc: kernel, linux-pm, Enric Balletbo i Serra, Ezequiel Garcia

A bit unexpectedly (but still documented), request_module may
return a positive value, in case of a modprobe error.
This is currently causing issues in the devfreq framework.

When a request_module exits with a positive value, we currently
return that via ERR_PTR. However, because the value is positive,
it's not a ERR_VALUE proper, and is therefore treated as a
valid struct devfreq_governor pointer, leading to a kernel oops.

The right way to fix this is hinted in __request_module documentation:

"""
[snip] The function returns
zero on success or a negative errno code or positive exit code from
"modprobe" on failure. Note that a successful module load does not mean
the module did not then unload and exit on an error of its own. Callers
must check that the service they requested is now available not blindly
invoke it.
"""

Therefore, drop the return value check, which is not useful, and instead
just re-try to find the (hopefully now loaded) governor.

Fixes: 23c7b54ca1cd1 ("PM / devfreq: Fix devfreq_add_device() when drivers are built as modules.")
Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 drivers/devfreq/devfreq.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 6b6991f0e873..8868ad9472d2 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -236,7 +236,6 @@ static struct devfreq_governor *find_devfreq_governor(const char *name)
 static struct devfreq_governor *try_then_request_governor(const char *name)
 {
 	struct devfreq_governor *governor;
-	int err = 0;
 
 	if (IS_ERR_OR_NULL(name)) {
 		pr_err("DEVFREQ: %s: Invalid parameters\n", __func__);
@@ -251,13 +250,10 @@ static struct devfreq_governor *try_then_request_governor(const char *name)
 
 		if (!strncmp(name, DEVFREQ_GOV_SIMPLE_ONDEMAND,
 			     DEVFREQ_NAME_LEN))
-			err = request_module("governor_%s", "simpleondemand");
+			request_module("governor_%s", "simpleondemand");
 		else
-			err = request_module("governor_%s", name);
-		/* Restore previous state before return */
+			request_module("governor_%s", name);
 		mutex_lock(&devfreq_list_lock);
-		if (err)
-			return ERR_PTR(err);
 
 		governor = find_devfreq_governor(name);
 	}
-- 
2.20.1


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

* [PATCH 2/2] PM / devfreq: Sanitize prints
  2019-06-05 19:00 ` [PATCH 1/2] PM / devfreq: Fix governor module load failure Ezequiel Garcia
@ 2019-06-05 19:00   ` Ezequiel Garcia
  2019-06-20  7:23     ` Chanwoo Choi
  2019-06-05 21:46   ` [PATCH 1/2] PM / devfreq: Fix governor module load failure Enric Balletbo Serra
  2019-06-20  7:59   ` Chanwoo Choi
  2 siblings, 1 reply; 11+ messages in thread
From: Ezequiel Garcia @ 2019-06-05 19:00 UTC (permalink / raw)
  To: Kyungmin Park, MyungJoo Ham
  Cc: kernel, linux-pm, Enric Balletbo i Serra, Ezequiel Garcia

This commit is a simple cosmetic cleanup, where pr_fmt is used to avoid
the "DEVFREQ" prefix in some prints.

Also, messages are changed to not start with a capital. This is just
a cosmetic change, meant to sanitize all prints from this file.

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 drivers/devfreq/devfreq.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 8868ad9472d2..44392fa1c570 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -10,6 +10,8 @@
  * published by the Free Software Foundation.
  */
 
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
 #include <linux/kernel.h>
 #include <linux/kmod.h>
 #include <linux/sched.h>
@@ -59,7 +61,7 @@ static struct devfreq *find_device_devfreq(struct device *dev)
 	struct devfreq *tmp_devfreq;
 
 	if (IS_ERR_OR_NULL(dev)) {
-		pr_err("DEVFREQ: %s: Invalid parameters\n", __func__);
+		pr_err("%s: invalid parameters\n", __func__);
 		return ERR_PTR(-EINVAL);
 	}
 	WARN(!mutex_is_locked(&devfreq_list_lock),
@@ -208,7 +210,7 @@ static struct devfreq_governor *find_devfreq_governor(const char *name)
 	struct devfreq_governor *tmp_governor;
 
 	if (IS_ERR_OR_NULL(name)) {
-		pr_err("DEVFREQ: %s: Invalid parameters\n", __func__);
+		pr_err("%s: invalid parameters\n", __func__);
 		return ERR_PTR(-EINVAL);
 	}
 	WARN(!mutex_is_locked(&devfreq_list_lock),
@@ -238,7 +240,7 @@ static struct devfreq_governor *try_then_request_governor(const char *name)
 	struct devfreq_governor *governor;
 
 	if (IS_ERR_OR_NULL(name)) {
-		pr_err("DEVFREQ: %s: Invalid parameters\n", __func__);
+		pr_err("%s: invalid parameters\n", __func__);
 		return ERR_PTR(-EINVAL);
 	}
 	WARN(!mutex_is_locked(&devfreq_list_lock),
@@ -1001,7 +1003,7 @@ int devfreq_add_governor(struct devfreq_governor *governor)
 	int err = 0;
 
 	if (!governor) {
-		pr_err("%s: Invalid parameters.\n", __func__);
+		pr_err("%s: invalid parameters.\n", __func__);
 		return -EINVAL;
 	}
 
@@ -1066,7 +1068,7 @@ int devfreq_remove_governor(struct devfreq_governor *governor)
 	int err = 0;
 
 	if (!governor) {
-		pr_err("%s: Invalid parameters.\n", __func__);
+		pr_err("%s: invalid parameters.\n", __func__);
 		return -EINVAL;
 	}
 
-- 
2.20.1


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

* Re: [PATCH 1/2] PM / devfreq: Fix governor module load failure
  2019-06-05 19:00 ` [PATCH 1/2] PM / devfreq: Fix governor module load failure Ezequiel Garcia
  2019-06-05 19:00   ` [PATCH 2/2] PM / devfreq: Sanitize prints Ezequiel Garcia
@ 2019-06-05 21:46   ` Enric Balletbo Serra
  2019-06-06 13:42     ` Ezequiel Garcia
  2019-06-20  7:59   ` Chanwoo Choi
  2 siblings, 1 reply; 11+ messages in thread
From: Enric Balletbo Serra @ 2019-06-05 21:46 UTC (permalink / raw)
  To: Ezequiel Garcia
  Cc: Kyungmin Park, MyungJoo Ham, kernel, Linux PM list,
	Enric Balletbo i Serra

Hi Ezequiel,

Missatge de Ezequiel Garcia <ezequiel@collabora.com> del dia dc., 5 de
juny 2019 a les 21:06:
>
> A bit unexpectedly (but still documented), request_module may
> return a positive value, in case of a modprobe error.
> This is currently causing issues in the devfreq framework.
>
> When a request_module exits with a positive value, we currently
> return that via ERR_PTR. However, because the value is positive,
> it's not a ERR_VALUE proper, and is therefore treated as a
> valid struct devfreq_governor pointer, leading to a kernel oops.
>
> The right way to fix this is hinted in __request_module documentation:
>
> """
> [snip] The function returns
> zero on success or a negative errno code or positive exit code from
> "modprobe" on failure. Note that a successful module load does not mean
> the module did not then unload and exit on an error of its own. Callers
> must check that the service they requested is now available not blindly
> invoke it.
> """
>
> Therefore, drop the return value check, which is not useful, and instead
> just re-try to find the (hopefully now loaded) governor.
>
> Fixes: 23c7b54ca1cd1 ("PM / devfreq: Fix devfreq_add_device() when drivers are built as modules.")

I think that what you really fixed is a bug introduced by:

b53b0128052ff ("PM / devfreq: Fix static checker warning in
try_then_request_governor")

not the above commit.

> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> ---
>  drivers/devfreq/devfreq.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 6b6991f0e873..8868ad9472d2 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -236,7 +236,6 @@ static struct devfreq_governor *find_devfreq_governor(const char *name)
>  static struct devfreq_governor *try_then_request_governor(const char *name)
>  {
>         struct devfreq_governor *governor;
> -       int err = 0;
>
>         if (IS_ERR_OR_NULL(name)) {
>                 pr_err("DEVFREQ: %s: Invalid parameters\n", __func__);
> @@ -251,13 +250,10 @@ static struct devfreq_governor *try_then_request_governor(const char *name)
>
>                 if (!strncmp(name, DEVFREQ_GOV_SIMPLE_ONDEMAND,
>                              DEVFREQ_NAME_LEN))
> -                       err = request_module("governor_%s", "simpleondemand");
> +                       request_module("governor_%s", "simpleondemand");
>                 else
> -                       err = request_module("governor_%s", name);
> -               /* Restore previous state before return */
> +                       request_module("governor_%s", name);
>                 mutex_lock(&devfreq_list_lock);
> -               if (err)

If you remove this check you'll iterate always over the full devfreq
list of governors, I know should be quick and is not too long but ...

> -                       return ERR_PTR(err);

The fix can be simply:

return ERR_PTR(-EINVAL);

I don't think overlap the real error is a problem here.

Thanks,
 Enric

>
>                 governor = find_devfreq_governor(name);
>         }
> --
> 2.20.1
>

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

* Re: [PATCH 1/2] PM / devfreq: Fix governor module load failure
  2019-06-05 21:46   ` [PATCH 1/2] PM / devfreq: Fix governor module load failure Enric Balletbo Serra
@ 2019-06-06 13:42     ` Ezequiel Garcia
  2019-06-06 13:48       ` Enric Balletbo i Serra
  0 siblings, 1 reply; 11+ messages in thread
From: Ezequiel Garcia @ 2019-06-06 13:42 UTC (permalink / raw)
  To: Enric Balletbo Serra
  Cc: Kyungmin Park, MyungJoo Ham, kernel, Linux PM list,
	Enric Balletbo i Serra

On Wed, 2019-06-05 at 23:46 +0200, Enric Balletbo Serra wrote:
> Hi Ezequiel,
> 
> Missatge de Ezequiel Garcia <ezequiel@collabora.com> del dia dc., 5 de
> juny 2019 a les 21:06:
> > A bit unexpectedly (but still documented), request_module may
> > return a positive value, in case of a modprobe error.
> > This is currently causing issues in the devfreq framework.
> > 
> > When a request_module exits with a positive value, we currently
> > return that via ERR_PTR. However, because the value is positive,
> > it's not a ERR_VALUE proper, and is therefore treated as a
> > valid struct devfreq_governor pointer, leading to a kernel oops.
> > 
> > The right way to fix this is hinted in __request_module documentation:
> > 
> > """
> > [snip] The function returns
> > zero on success or a negative errno code or positive exit code from
> > "modprobe" on failure. Note that a successful module load does not mean
> > the module did not then unload and exit on an error of its own. Callers
> > must check that the service they requested is now available not blindly
> > invoke it.
> > """
> > 
> > Therefore, drop the return value check, which is not useful, and instead
> > just re-try to find the (hopefully now loaded) governor.
> > 
> > Fixes: 23c7b54ca1cd1 ("PM / devfreq: Fix devfreq_add_device() when drivers are built as modules.")
> 
> I think that what you really fixed is a bug introduced by:
> 
> b53b0128052ff ("PM / devfreq: Fix static checker warning in
> try_then_request_governor")
> 
> not the above commit.
> 

Oh, you are right of course. I looked for the commit introducing the
request_module usage, and thought it was the culprit.

> > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> > ---
> >  drivers/devfreq/devfreq.c | 8 ++------
> >  1 file changed, 2 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> > index 6b6991f0e873..8868ad9472d2 100644
> > --- a/drivers/devfreq/devfreq.c
> > +++ b/drivers/devfreq/devfreq.c
> > @@ -236,7 +236,6 @@ static struct devfreq_governor *find_devfreq_governor(const char *name)
> >  static struct devfreq_governor *try_then_request_governor(const char *name)
> >  {
> >         struct devfreq_governor *governor;
> > -       int err = 0;
> > 
> >         if (IS_ERR_OR_NULL(name)) {
> >                 pr_err("DEVFREQ: %s: Invalid parameters\n", __func__);
> > @@ -251,13 +250,10 @@ static struct devfreq_governor *try_then_request_governor(const char *name)
> > 
> >                 if (!strncmp(name, DEVFREQ_GOV_SIMPLE_ONDEMAND,
> >                              DEVFREQ_NAME_LEN))
> > -                       err = request_module("governor_%s", "simpleondemand");
> > +                       request_module("governor_%s", "simpleondemand");
> >                 else
> > -                       err = request_module("governor_%s", name);
> > -               /* Restore previous state before return */
> > +                       request_module("governor_%s", name);
> >                 mutex_lock(&devfreq_list_lock);
> > -               if (err)
> 
> If you remove this check you'll iterate always over the full devfreq
> list of governors, I know should be quick and is not too long but ...
> 

Keep in mind that when the request_module succeeds, we need
to iterate anyways to find the governor.

> > -                       return ERR_PTR(err);
> 
> The fix can be simply:
> 
> return ERR_PTR(-EINVAL);
> 
> I don't think overlap the real error is a problem here.
> 

Yeah, I also thought about this, but somehow thought this
was simpler.

I don't have a strong opinion, so whatever you prefer is fine.

Thanks,
Eze

> Thanks,
>  Enric
> 
> >                 governor = find_devfreq_governor(name);
> >         }
> > --
> > 2.20.1
> > 



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

* Re: [PATCH 1/2] PM / devfreq: Fix governor module load failure
  2019-06-06 13:42     ` Ezequiel Garcia
@ 2019-06-06 13:48       ` Enric Balletbo i Serra
  0 siblings, 0 replies; 11+ messages in thread
From: Enric Balletbo i Serra @ 2019-06-06 13:48 UTC (permalink / raw)
  To: Ezequiel Garcia, Enric Balletbo Serra
  Cc: Kyungmin Park, MyungJoo Ham, kernel, Linux PM list



On 6/6/19 15:42, Ezequiel Garcia wrote:
> On Wed, 2019-06-05 at 23:46 +0200, Enric Balletbo Serra wrote:
>> Hi Ezequiel,
>>
>> Missatge de Ezequiel Garcia <ezequiel@collabora.com> del dia dc., 5 de
>> juny 2019 a les 21:06:
>>> A bit unexpectedly (but still documented), request_module may
>>> return a positive value, in case of a modprobe error.
>>> This is currently causing issues in the devfreq framework.
>>>
>>> When a request_module exits with a positive value, we currently
>>> return that via ERR_PTR. However, because the value is positive,
>>> it's not a ERR_VALUE proper, and is therefore treated as a
>>> valid struct devfreq_governor pointer, leading to a kernel oops.
>>>
>>> The right way to fix this is hinted in __request_module documentation:
>>>
>>> """
>>> [snip] The function returns
>>> zero on success or a negative errno code or positive exit code from
>>> "modprobe" on failure. Note that a successful module load does not mean
>>> the module did not then unload and exit on an error of its own. Callers
>>> must check that the service they requested is now available not blindly
>>> invoke it.
>>> """
>>>
>>> Therefore, drop the return value check, which is not useful, and instead
>>> just re-try to find the (hopefully now loaded) governor.
>>>
>>> Fixes: 23c7b54ca1cd1 ("PM / devfreq: Fix devfreq_add_device() when drivers are built as modules.")
>>
>> I think that what you really fixed is a bug introduced by:
>>
>> b53b0128052ff ("PM / devfreq: Fix static checker warning in
>> try_then_request_governor")
>>
>> not the above commit.
>>
> 
> Oh, you are right of course. I looked for the commit introducing the
> request_module usage, and thought it was the culprit.
> 
>>> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
>>> ---
>>>  drivers/devfreq/devfreq.c | 8 ++------
>>>  1 file changed, 2 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>>> index 6b6991f0e873..8868ad9472d2 100644
>>> --- a/drivers/devfreq/devfreq.c
>>> +++ b/drivers/devfreq/devfreq.c
>>> @@ -236,7 +236,6 @@ static struct devfreq_governor *find_devfreq_governor(const char *name)
>>>  static struct devfreq_governor *try_then_request_governor(const char *name)
>>>  {
>>>         struct devfreq_governor *governor;
>>> -       int err = 0;
>>>
>>>         if (IS_ERR_OR_NULL(name)) {
>>>                 pr_err("DEVFREQ: %s: Invalid parameters\n", __func__);
>>> @@ -251,13 +250,10 @@ static struct devfreq_governor *try_then_request_governor(const char *name)
>>>
>>>                 if (!strncmp(name, DEVFREQ_GOV_SIMPLE_ONDEMAND,
>>>                              DEVFREQ_NAME_LEN))
>>> -                       err = request_module("governor_%s", "simpleondemand");
>>> +                       request_module("governor_%s", "simpleondemand");
>>>                 else
>>> -                       err = request_module("governor_%s", name);
>>> -               /* Restore previous state before return */
>>> +                       request_module("governor_%s", name);
>>>                 mutex_lock(&devfreq_list_lock);
>>> -               if (err)
>>
>> If you remove this check you'll iterate always over the full devfreq
>> list of governors, I know should be quick and is not too long but ...
>>
> 
> Keep in mind that when the request_module succeeds, we need
> to iterate anyways to find the governor.
> 

Well, the error path will be a micro-bit faster :-)

>>> -                       return ERR_PTR(err);
>>
>> The fix can be simply:
>>
>> return ERR_PTR(-EINVAL);
>>
>> I don't think overlap the real error is a problem here.
>>
> 
> Yeah, I also thought about this, but somehow thought this
> was simpler.
> 
> I don't have a strong opinion, so whatever you prefer is fine.
> 

Me neither, I think is more up to the maintainer :-)

BTW, with the Fixes tag fixed

Reviewed-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>

> Thanks,
> Eze
> 
>> Thanks,
>>  Enric
>>
>>>                 governor = find_devfreq_governor(name);
>>>         }
>>> --
>>> 2.20.1
>>>
> 
> 

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

* Re: [PATCH 2/2] PM / devfreq: Sanitize prints
  2019-06-05 19:00   ` [PATCH 2/2] PM / devfreq: Sanitize prints Ezequiel Garcia
@ 2019-06-20  7:23     ` Chanwoo Choi
  2019-06-20 14:41       ` Ezequiel Garcia
  0 siblings, 1 reply; 11+ messages in thread
From: Chanwoo Choi @ 2019-06-20  7:23 UTC (permalink / raw)
  To: Ezequiel Garcia, Kyungmin Park, MyungJoo Ham
  Cc: kernel, linux-pm, Enric Balletbo i Serra

Hi,

Frankly, I don't like the existing 'DEVFREQ: ' prefix 
because it is not used on all error log and it is not necessary.

Instead of this patch, I just prefer to delete the 'DEVFREQ: ' prefix

On 19. 6. 6. 오전 4:00, Ezequiel Garcia wrote:
> This commit is a simple cosmetic cleanup, where pr_fmt is used to avoid
> the "DEVFREQ" prefix in some prints.
> 
> Also, messages are changed to not start with a capital. This is just
> a cosmetic change, meant to sanitize all prints from this file.
> 
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> ---
>  drivers/devfreq/devfreq.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 8868ad9472d2..44392fa1c570 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -10,6 +10,8 @@
>   * published by the Free Software Foundation.
>   */
>  
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
>  #include <linux/kernel.h>
>  #include <linux/kmod.h>
>  #include <linux/sched.h>
> @@ -59,7 +61,7 @@ static struct devfreq *find_device_devfreq(struct device *dev)
>  	struct devfreq *tmp_devfreq;
>  
>  	if (IS_ERR_OR_NULL(dev)) {
> -		pr_err("DEVFREQ: %s: Invalid parameters\n", __func__);
> +		pr_err("%s: invalid parameters\n", __func__);
>  		return ERR_PTR(-EINVAL);
>  	}
>  	WARN(!mutex_is_locked(&devfreq_list_lock),
> @@ -208,7 +210,7 @@ static struct devfreq_governor *find_devfreq_governor(const char *name)
>  	struct devfreq_governor *tmp_governor;
>  
>  	if (IS_ERR_OR_NULL(name)) {
> -		pr_err("DEVFREQ: %s: Invalid parameters\n", __func__);
> +		pr_err("%s: invalid parameters\n", __func__);
>  		return ERR_PTR(-EINVAL);
>  	}
>  	WARN(!mutex_is_locked(&devfreq_list_lock),
> @@ -238,7 +240,7 @@ static struct devfreq_governor *try_then_request_governor(const char *name)
>  	struct devfreq_governor *governor;
>  
>  	if (IS_ERR_OR_NULL(name)) {
> -		pr_err("DEVFREQ: %s: Invalid parameters\n", __func__);
> +		pr_err("%s: invalid parameters\n", __func__);
>  		return ERR_PTR(-EINVAL);
>  	}
>  	WARN(!mutex_is_locked(&devfreq_list_lock),
> @@ -1001,7 +1003,7 @@ int devfreq_add_governor(struct devfreq_governor *governor)
>  	int err = 0;
>  
>  	if (!governor) {
> -		pr_err("%s: Invalid parameters.\n", __func__);
> +		pr_err("%s: invalid parameters.\n", __func__);
>  		return -EINVAL;
>  	}
>  
> @@ -1066,7 +1068,7 @@ int devfreq_remove_governor(struct devfreq_governor *governor)
>  	int err = 0;
>  
>  	if (!governor) {
> -		pr_err("%s: Invalid parameters.\n", __func__);
> +		pr_err("%s: invalid parameters.\n", __func__);
>  		return -EINVAL;
>  	}
>  
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

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

* Re: [PATCH 1/2] PM / devfreq: Fix governor module load failure
  2019-06-05 19:00 ` [PATCH 1/2] PM / devfreq: Fix governor module load failure Ezequiel Garcia
  2019-06-05 19:00   ` [PATCH 2/2] PM / devfreq: Sanitize prints Ezequiel Garcia
  2019-06-05 21:46   ` [PATCH 1/2] PM / devfreq: Fix governor module load failure Enric Balletbo Serra
@ 2019-06-20  7:59   ` Chanwoo Choi
  2019-06-20  8:04     ` Chanwoo Choi
  2 siblings, 1 reply; 11+ messages in thread
From: Chanwoo Choi @ 2019-06-20  7:59 UTC (permalink / raw)
  To: Ezequiel Garcia, Kyungmin Park, MyungJoo Ham
  Cc: kernel, linux-pm, Enric Balletbo i Serra

Hi,

On 19. 6. 6. 오전 4:00, Ezequiel Garcia wrote:
> A bit unexpectedly (but still documented), request_module may
> return a positive value, in case of a modprobe error.
> This is currently causing issues in the devfreq framework.
> 
> When a request_module exits with a positive value, we currently
> return that via ERR_PTR. However, because the value is positive,
> it's not a ERR_VALUE proper, and is therefore treated as a
> valid struct devfreq_governor pointer, leading to a kernel oops.
> 
> The right way to fix this is hinted in __request_module documentation:
> 
> """
> [snip] The function returns
> zero on success or a negative errno code or positive exit code from
> "modprobe" on failure. Note that a successful module load does not mean
> the module did not then unload and exit on an error of its own. Callers
> must check that the service they requested is now available not blindly
> invoke it.
> """
> 
> Therefore, drop the return value check, which is not useful, and instead
> just re-try to find the (hopefully now loaded) governor.
> 
> Fixes: 23c7b54ca1cd1 ("PM / devfreq: Fix devfreq_add_device() when drivers are built as modules.")
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> ---
>  drivers/devfreq/devfreq.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 6b6991f0e873..8868ad9472d2 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -236,7 +236,6 @@ static struct devfreq_governor *find_devfreq_governor(const char *name)
>  static struct devfreq_governor *try_then_request_governor(const char *name)
>  {
>  	struct devfreq_governor *governor;
> -	int err = 0;
>  
>  	if (IS_ERR_OR_NULL(name)) {
>  		pr_err("DEVFREQ: %s: Invalid parameters\n", __func__);
> @@ -251,13 +250,10 @@ static struct devfreq_governor *try_then_request_governor(const char *name)
>  
>  		if (!strncmp(name, DEVFREQ_GOV_SIMPLE_ONDEMAND,
>  			     DEVFREQ_NAME_LEN))
> -			err = request_module("governor_%s", "simpleondemand");
> +			request_module("governor_%s", "simpleondemand");

I don't agree to remove the exception handling. Even if request_module()
returns positive value, it have to be handled the negative code for error case.

>  		else
> -			err = request_module("governor_%s", name);
> -		/* Restore previous state before return */
> +			request_module("governor_%s", name);

ditto.

>  		mutex_lock(&devfreq_list_lock);
> -		if (err)> -			return ERR_PTR(err);

You better to modify it as following:

if (err < 0)
	return ERR_PTR(err);
else if (err > 0)
	return ERR_PTR(-EINVAL);


>  
>  		governor = find_devfreq_governor(name);
>  	}
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

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

* Re: [PATCH 1/2] PM / devfreq: Fix governor module load failure
  2019-06-20  7:59   ` Chanwoo Choi
@ 2019-06-20  8:04     ` Chanwoo Choi
  2019-06-20 15:31       ` Ezequiel Garcia
  0 siblings, 1 reply; 11+ messages in thread
From: Chanwoo Choi @ 2019-06-20  8:04 UTC (permalink / raw)
  To: Ezequiel Garcia, Kyungmin Park, MyungJoo Ham
  Cc: kernel, linux-pm, Enric Balletbo i Serra

On 19. 6. 20. 오후 4:59, Chanwoo Choi wrote:
> Hi,
> 
> On 19. 6. 6. 오전 4:00, Ezequiel Garcia wrote:
>> A bit unexpectedly (but still documented), request_module may
>> return a positive value, in case of a modprobe error.
>> This is currently causing issues in the devfreq framework.
>>
>> When a request_module exits with a positive value, we currently
>> return that via ERR_PTR. However, because the value is positive,
>> it's not a ERR_VALUE proper, and is therefore treated as a
>> valid struct devfreq_governor pointer, leading to a kernel oops.
>>
>> The right way to fix this is hinted in __request_module documentation:
>>
>> """
>> [snip] The function returns
>> zero on success or a negative errno code or positive exit code from
>> "modprobe" on failure. Note that a successful module load does not mean
>> the module did not then unload and exit on an error of its own. Callers
>> must check that the service they requested is now available not blindly
>> invoke it.
>> """
>>
>> Therefore, drop the return value check, which is not useful, and instead
>> just re-try to find the (hopefully now loaded) governor.
>>
>> Fixes: 23c7b54ca1cd1 ("PM / devfreq: Fix devfreq_add_device() when drivers are built as modules.")
>> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
>> ---
>>  drivers/devfreq/devfreq.c | 8 ++------
>>  1 file changed, 2 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>> index 6b6991f0e873..8868ad9472d2 100644
>> --- a/drivers/devfreq/devfreq.c
>> +++ b/drivers/devfreq/devfreq.c
>> @@ -236,7 +236,6 @@ static struct devfreq_governor *find_devfreq_governor(const char *name)
>>  static struct devfreq_governor *try_then_request_governor(const char *name)
>>  {
>>  	struct devfreq_governor *governor;
>> -	int err = 0;
>>  
>>  	if (IS_ERR_OR_NULL(name)) {
>>  		pr_err("DEVFREQ: %s: Invalid parameters\n", __func__);
>> @@ -251,13 +250,10 @@ static struct devfreq_governor *try_then_request_governor(const char *name)
>>  
>>  		if (!strncmp(name, DEVFREQ_GOV_SIMPLE_ONDEMAND,
>>  			     DEVFREQ_NAME_LEN))
>> -			err = request_module("governor_%s", "simpleondemand");
>> +			request_module("governor_%s", "simpleondemand");
> 
> I don't agree to remove the exception handling. Even if request_module()
> returns positive value, 

Sorry, I wrote the wrong comment. It have to handle the positive return value
for exception handling.

> 
>>  		else
>> -			err = request_module("governor_%s", name);
>> -		/* Restore previous state before return */
>> +			request_module("governor_%s", name);
> 
> ditto.
> 
>>  		mutex_lock(&devfreq_list_lock);
>> -		if (err)> -			return ERR_PTR(err);
> 
> You better to modify it as following:
> 
> if (err < 0)
> 	return ERR_PTR(err);
> else if (err > 0)
> 	return ERR_PTR(-EINVAL);
> 
> 
>>  
>>  		governor = find_devfreq_governor(name);
>>  	}
>>
> 
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

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

* Re: [PATCH 2/2] PM / devfreq: Sanitize prints
  2019-06-20  7:23     ` Chanwoo Choi
@ 2019-06-20 14:41       ` Ezequiel Garcia
  2019-06-21  2:53         ` Chanwoo Choi
  0 siblings, 1 reply; 11+ messages in thread
From: Ezequiel Garcia @ 2019-06-20 14:41 UTC (permalink / raw)
  To: Chanwoo Choi, Kyungmin Park, MyungJoo Ham
  Cc: kernel, linux-pm, Enric Balletbo i Serra

On Thu, 2019-06-20 at 16:23 +0900, Chanwoo Choi wrote:
> Hi,
> 
> Frankly, I don't like the existing 'DEVFREQ: ' prefix 
> because it is not used on all error log and it is not necessary.
> 
> Instead of this patch, I just prefer to delete the 'DEVFREQ: ' prefix
> 

Hm, I have to disagree. Having naked pr_{} with just the __func__
reducess logging consistency.

Thanks,
Ezequiel


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

* Re: [PATCH 1/2] PM / devfreq: Fix governor module load failure
  2019-06-20  8:04     ` Chanwoo Choi
@ 2019-06-20 15:31       ` Ezequiel Garcia
  0 siblings, 0 replies; 11+ messages in thread
From: Ezequiel Garcia @ 2019-06-20 15:31 UTC (permalink / raw)
  To: Chanwoo Choi, Kyungmin Park, MyungJoo Ham
  Cc: kernel, linux-pm, Enric Balletbo i Serra

On Thu, 2019-06-20 at 17:04 +0900, Chanwoo Choi wrote:
> On 19. 6. 20. 오후 4:59, Chanwoo Choi wrote:
> > Hi,
> > 
> > On 19. 6. 6. 오전 4:00, Ezequiel Garcia wrote:
> > > A bit unexpectedly (but still documented), request_module may
> > > return a positive value, in case of a modprobe error.
> > > This is currently causing issues in the devfreq framework.
> > > 
> > > When a request_module exits with a positive value, we currently
> > > return that via ERR_PTR. However, because the value is positive,
> > > it's not a ERR_VALUE proper, and is therefore treated as a
> > > valid struct devfreq_governor pointer, leading to a kernel oops.
> > > 
> > > The right way to fix this is hinted in __request_module documentation:
> > > 
> > > """
> > > [snip] The function returns
> > > zero on success or a negative errno code or positive exit code from
> > > "modprobe" on failure. Note that a successful module load does not mean
> > > the module did not then unload and exit on an error of its own. Callers
> > > must check that the service they requested is now available not blindly
> > > invoke it.
> > > """
> > > 
> > > Therefore, drop the return value check, which is not useful, and instead
> > > just re-try to find the (hopefully now loaded) governor.
> > > 
> > > Fixes: 23c7b54ca1cd1 ("PM / devfreq: Fix devfreq_add_device() when drivers are built as modules.")
> > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> > > ---
> > >  drivers/devfreq/devfreq.c | 8 ++------
> > >  1 file changed, 2 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> > > index 6b6991f0e873..8868ad9472d2 100644
> > > --- a/drivers/devfreq/devfreq.c
> > > +++ b/drivers/devfreq/devfreq.c
> > > @@ -236,7 +236,6 @@ static struct devfreq_governor *find_devfreq_governor(const char *name)
> > >  static struct devfreq_governor *try_then_request_governor(const char *name)
> > >  {
> > >  	struct devfreq_governor *governor;
> > > -	int err = 0;
> > >  
> > >  	if (IS_ERR_OR_NULL(name)) {
> > >  		pr_err("DEVFREQ: %s: Invalid parameters\n", __func__);
> > > @@ -251,13 +250,10 @@ static struct devfreq_governor *try_then_request_governor(const char *name)
> > >  
> > >  		if (!strncmp(name, DEVFREQ_GOV_SIMPLE_ONDEMAND,
> > >  			     DEVFREQ_NAME_LEN))
> > > -			err = request_module("governor_%s", "simpleondemand");
> > > +			request_module("governor_%s", "simpleondemand");
> > 
> > I don't agree to remove the exception handling. Even if request_module()
> > returns positive value, 
> 
> Sorry, I wrote the wrong comment. It have to handle the positive return value
> for exception handling.
> 

OK, let me give this a new try.

Thanks,
Ezequiel


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

* Re: [PATCH 2/2] PM / devfreq: Sanitize prints
  2019-06-20 14:41       ` Ezequiel Garcia
@ 2019-06-21  2:53         ` Chanwoo Choi
  0 siblings, 0 replies; 11+ messages in thread
From: Chanwoo Choi @ 2019-06-21  2:53 UTC (permalink / raw)
  To: Ezequiel Garcia, Kyungmin Park, MyungJoo Ham
  Cc: kernel, linux-pm, Enric Balletbo i Serra

Hi,

On 19. 6. 20. 오후 11:41, Ezequiel Garcia wrote:
> On Thu, 2019-06-20 at 16:23 +0900, Chanwoo Choi wrote:
>> Hi,
>>
>> Frankly, I don't like the existing 'DEVFREQ: ' prefix 
>> because it is not used on all error log and it is not necessary.
>>
>> Instead of this patch, I just prefer to delete the 'DEVFREQ: ' prefix
>>
> 
> Hm, I have to disagree. Having naked pr_{} with just the __func__
> reducess logging consistency.

Actually, it is minor clean-up and there are no any benefits.

As I said, 'DEVFREQ: ' prefix was not used on all devfreq log.
If you don't agree to remove 'DEVFREQ: ', no problem.
IMO, just better to keep the current style without
any addition changes.


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

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

end of thread, back to index

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <CGME20190605190147epcas1p3e74fba524dfcfc87f7ce3c9569ffaa3f@epcas1p3.samsung.com>
2019-06-05 19:00 ` [PATCH 1/2] PM / devfreq: Fix governor module load failure Ezequiel Garcia
2019-06-05 19:00   ` [PATCH 2/2] PM / devfreq: Sanitize prints Ezequiel Garcia
2019-06-20  7:23     ` Chanwoo Choi
2019-06-20 14:41       ` Ezequiel Garcia
2019-06-21  2:53         ` Chanwoo Choi
2019-06-05 21:46   ` [PATCH 1/2] PM / devfreq: Fix governor module load failure Enric Balletbo Serra
2019-06-06 13:42     ` Ezequiel Garcia
2019-06-06 13:48       ` Enric Balletbo i Serra
2019-06-20  7:59   ` Chanwoo Choi
2019-06-20  8:04     ` Chanwoo Choi
2019-06-20 15:31       ` Ezequiel Garcia

Linux-PM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-pm/0 linux-pm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-pm linux-pm/ https://lore.kernel.org/linux-pm \
		linux-pm@vger.kernel.org linux-pm@archiver.kernel.org
	public-inbox-index linux-pm


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-pm


AGPL code for this site: git clone https://public-inbox.org/ public-inbox