| Message ID | 20241030162930.2111255-1-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [v1,1/1] cpufreq: loongson: Check for error code from devm_mutex_init() call | expand |
Hi, Andy, On Thu, Oct 31, 2024 at 12:29 AM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > Even if it's not critical, the avoidance of checking the error code > from devm_mutex_init() call today diminishes the point of using devm > variant of it. Tomorrow it may even leak something. Add the missed > check. > > Fixes: ccf51454145b ("cpufreq: Add Loongson-3 CPUFreq driver support") > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/cpufreq/loongson3_cpufreq.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/cpufreq/loongson3_cpufreq.c b/drivers/cpufreq/loongson3_cpufreq.c > index 61ebebf69455..bd34bf0fafa5 100644 > --- a/drivers/cpufreq/loongson3_cpufreq.c > +++ b/drivers/cpufreq/loongson3_cpufreq.c > @@ -346,8 +346,11 @@ static int loongson3_cpufreq_probe(struct platform_device *pdev) > { > int i, ret; > > - for (i = 0; i < MAX_PACKAGES; i++) > - devm_mutex_init(&pdev->dev, &cpufreq_mutex[i]); > + for (i = 0; i < MAX_PACKAGES; i++) { > + ret = devm_mutex_init(&pdev->dev, &cpufreq_mutex[i]); > + if (ret) Good catch, but I think "if (ret < 0)" is better? Sometimes a positive return value is legal, even if not in this case. And it is better to use loongson3 rather than loongson because there is another loongson2 driver. Huacai > + return ret; > + } > > ret = do_service_request(0, 0, CMD_GET_VERSION, 0, 0); > if (ret <= 0) > -- > 2.43.0.rc1.1336.g36b5255a03ac > >
On Thu, Oct 31, 2024 at 09:29:52AM +0800, Huacai Chen wrote: > On Thu, Oct 31, 2024 at 12:29 AM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: ... > > - for (i = 0; i < MAX_PACKAGES; i++) > > - devm_mutex_init(&pdev->dev, &cpufreq_mutex[i]); > > + for (i = 0; i < MAX_PACKAGES; i++) { > > + ret = devm_mutex_init(&pdev->dev, &cpufreq_mutex[i]); > > + if (ret) > Good catch, but I think "if (ret < 0)" is better? Sometimes a positive > return value is legal, even if not in this case. I disagree on this. During a tons of reviews I have done in the past this kind of check is impediment and always rises the Q "why?" It means that the author hasn't fully thought through the code and most likely done something is a cargo cult. On top of that, if the callee is changed at some point to actually return a positive code(s), the caller most likely has to be at least aware of that change. The proposed modification makes this silently compile and hides possible important details from the caller(s). > And it is better to use loongson3 rather than loongson because there > is another loongson2 driver. Thanks, I will change that in v2 (I believe you are talking about Subject?). > > + return ret; > > + }
diff --git a/drivers/cpufreq/loongson3_cpufreq.c b/drivers/cpufreq/loongson3_cpufreq.c index 61ebebf69455..bd34bf0fafa5 100644 --- a/drivers/cpufreq/loongson3_cpufreq.c +++ b/drivers/cpufreq/loongson3_cpufreq.c @@ -346,8 +346,11 @@ static int loongson3_cpufreq_probe(struct platform_device *pdev) { int i, ret; - for (i = 0; i < MAX_PACKAGES; i++) - devm_mutex_init(&pdev->dev, &cpufreq_mutex[i]); + for (i = 0; i < MAX_PACKAGES; i++) { + ret = devm_mutex_init(&pdev->dev, &cpufreq_mutex[i]); + if (ret) + return ret; + } ret = do_service_request(0, 0, CMD_GET_VERSION, 0, 0); if (ret <= 0)
Even if it's not critical, the avoidance of checking the error code from devm_mutex_init() call today diminishes the point of using devm variant of it. Tomorrow it may even leak something. Add the missed check. Fixes: ccf51454145b ("cpufreq: Add Loongson-3 CPUFreq driver support") Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/cpufreq/loongson3_cpufreq.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)