* [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 related [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 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 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
* 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 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 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