Messages in this thread | | | Date | Mon, 10 Jul 2023 14:25:59 +0300 | Subject | Re: [PATCH v4 06/21] thermal/drivers/db8500: convert to use devm_request*_irq_probe() | From | Dmitry Baryshkov <> |
| |
On 10/07/2023 12:59, Yangtao Li wrote: > There are more than 700 calls to devm_request_threaded_irq method and > more than 1000 calls to devm_request_irq method. Most drivers only > request one interrupt resource, and these error messages are basically > the same. If error messages are printed everywhere, more than 2000 lines > of code can be saved by removing the msg in the driver. > > And tglx point out that: > > If we actually look at the call sites of > devm_request_threaded_irq() then the vast majority of them print more or > less lousy error messages. A quick grep/sed/awk/sort/uniq revealed > > 519 messages total (there are probably more) > > 352 unique messages > > 323 unique messages after lower casing > > Those 323 are mostly just variants of the same patterns with > slight modifications in formatting and information provided. > > 186 of these messages do not deliver any useful information, > e.g. "no irq", " > > The most useful one of all is: "could request wakeup irq: %d" > > So there is certainly an argument to be made that this particular > function should print a well formatted and informative error message. > > It's not a general allocator like kmalloc(). It's specialized and in the > vast majority of cases failing to request the interrupt causes the > device probe to fail. So having proper and consistent information why > the device cannot be used _is_ useful. > > So convert to use devm_request*_irq_probe() API, which ensure that all > error handling branches print error information. > > In this way, when this function fails, the upper-layer functions can > directly return an error code without missing debugging information. > Otherwise, the error message will be printed redundantly or missing. > > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Krzysztof Kozlowski <krzk@kernel.org> > Cc: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de> > Cc: Jonathan Cameron <Jonathan.Cameron@Huawei.com> > Cc: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> > Signed-off-by: Yangtao Li <frank.li@vivo.com> > --- > drivers/thermal/db8500_thermal.c | 16 ++++++---------- > drivers/thermal/qcom/lmh.c | 7 +++----
Please split LMH to a separate driver.
> 2 files changed, 9 insertions(+), 14 deletions(-) > > diff --git a/drivers/thermal/db8500_thermal.c b/drivers/thermal/db8500_thermal.c > index fca5c2c93bf9..0ef8fc2eb4a1 100644 > --- a/drivers/thermal/db8500_thermal.c > +++ b/drivers/thermal/db8500_thermal.c > @@ -164,25 +164,21 @@ static int db8500_thermal_probe(struct platform_device *pdev) > if (low_irq < 0) > return low_irq; > > - ret = devm_request_threaded_irq(dev, low_irq, NULL, > + ret = devm_request_threaded_irq_probe(dev, low_irq, NULL, > prcmu_low_irq_handler, IRQF_NO_SUSPEND | IRQF_ONESHOT, > - "dbx500_temp_low", th); > - if (ret < 0) { > - dev_err(dev, "failed to allocate temp low irq\n"); > + "dbx500_temp_low", th, "temp low"); > + if (ret < 0) > return ret; > - } > > high_irq = platform_get_irq_byname(pdev, "IRQ_HOTMON_HIGH"); > if (high_irq < 0) > return high_irq; > > - ret = devm_request_threaded_irq(dev, high_irq, NULL, > + ret = devm_request_threaded_irq_probe(dev, high_irq, NULL, > prcmu_high_irq_handler, IRQF_NO_SUSPEND | IRQF_ONESHOT, > - "dbx500_temp_high", th); > - if (ret < 0) { > - dev_err(dev, "failed to allocate temp high irq\n"); > + "dbx500_temp_high", th, "temp high"); > + if (ret < 0) > return ret; > - } > > /* register of thermal sensor and get info from DT */ > th->tz = devm_thermal_of_zone_register(dev, 0, th, &thdev_ops); > diff --git a/drivers/thermal/qcom/lmh.c b/drivers/thermal/qcom/lmh.c > index f6edb12ec004..48a14d7e8bf5 100644 > --- a/drivers/thermal/qcom/lmh.c > +++ b/drivers/thermal/qcom/lmh.c > @@ -207,11 +207,10 @@ static int lmh_probe(struct platform_device *pdev) > > /* Disable the irq and let cpufreq enable it when ready to handle the interrupt */ > irq_set_status_flags(lmh_data->irq, IRQ_NOAUTOEN); > - ret = devm_request_irq(dev, lmh_data->irq, lmh_handle_irq, > - IRQF_ONESHOT | IRQF_NO_SUSPEND, > - "lmh-irq", lmh_data); > + ret = devm_request_irq_probe(dev, lmh_data->irq, lmh_handle_irq, > + IRQF_ONESHOT | IRQF_NO_SUSPEND, > + "lmh-irq", lmh_data, NULL); > if (ret) { > - dev_err(dev, "Error %d registering irq %x\n", ret, lmh_data->irq); > irq_domain_remove(lmh_data->domain); > return ret; > }
-- With best wishes Dmitry
| |