[PATCH 1/2] PM / devfreq: Fix governor module load failure

[PATCH 1/2] PM / devfreq: Fix governor module load failure

[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

[PATCH 2/2] PM / devfreq: Sanitize prints

[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

Re: [PATCH 2/2] PM / devfreq: Sanitize prints

[Chanwoo Choi]

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

Re: [PATCH 2/2] PM / devfreq: Sanitize prints

[Ezequiel Garcia]

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

Re: [PATCH 2/2] PM / devfreq: Sanitize prints

[Chanwoo Choi]

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

Re: [PATCH 1/2] PM / devfreq: Fix governor module load failure

[Enric Balletbo 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
>

Re: [PATCH 1/2] PM / devfreq: Fix governor module load failure

[Ezequiel Garcia]

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

Re: [PATCH 1/2] PM / devfreq: Fix governor module load failure

[Enric Balletbo i Serra]

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

Re: [PATCH 1/2] PM / devfreq: Fix governor module load failure

[Chanwoo Choi]

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

Re: [PATCH 1/2] PM / devfreq: Fix governor module load failure

[Chanwoo Choi]

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

Re: [PATCH 1/2] PM / devfreq: Fix governor module load failure

[Ezequiel Garcia]

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