Messages in this thread | | | Date | Mon, 3 Jul 2023 17:13:29 +0800 | Subject | Re: [PATCH v2 01/15] genirq/devres: Add error information printing for devm_request_threaded_irq() | From | Yangtao Li <> |
| |
Hi,
On 2023/6/30 19:11, Thomas Gleixner wrote: > On Tue, Jun 27 2023 at 13:00, Uwe Kleine-König wrote: >> On Tue, Jun 27, 2023 at 06:12:01PM +0800, Yangtao Li wrote: >> >> While I assume changing to dev_err_probe is a result of my concern that >> no error should be printed when rc=-EPROBEDEFER, my other concern that >> adding an error message to a generic allocation function is a bad idea >> still stands. > I agree in general, but if you 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. > > Yangtao: The way how this is attempted is not useful at all. > > 1) The changelog is word salad and provides 0 rationale > > Also such series require a cover letter... > > 2) The dev_err() which is added is not informative at all and cannot > replace actually useful error messages. It's not that hard to > make it useful. > > 2) Adding the printks unconditionally first will emit two messages > with different content. > > This is not how such changes are done. > > The proper approach is to create a wrapper function which emits > the error message: > > wrapper(....., const char *info) > { > ret = devm_request_threaded_irq(....); > if (ret < 0) { > dev_err(dev, "Failed to request %sinterrupt %u %s %s: %d\n, > thread_fn ? "threaded " : "", > irq, devname, info ? : "", ret); > } > return ret; > } > > Then convert the callsites over one by one with proper > changelogs and justification. > > See?
Yes, thanks a lot for the suggestion, and v3 has been sent.
MBR,
Yangtao
> > Thanks, > > tglx
| |