Messages in this thread | | | Date | Mon, 3 Jul 2023 17:10:22 +0800 | Subject | Re: [PATCH 01/15] genirq/devres: Add error information printing for devm_request_threaded_irq() | From | Yangtao Li <> |
| |
On 2023/6/27 15:43, Krzysztof Kozlowski wrote:
> On 27/06/2023 09:16, Yangtao Li wrote: >> 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. >> >> There are more than 700 calls to the devm_request_threaded_irq method. >> If error messages are printed everywhere, more than 1000 lines of code >> can be saved by removing the msg in the driver. >> >> Signed-off-by: Yangtao Li <frank.li@vivo.com> >> --- >> kernel/irq/devres.c | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/kernel/irq/devres.c b/kernel/irq/devres.c >> index f6e5515ee077..94039a915218 100644 >> --- a/kernel/irq/devres.c >> +++ b/kernel/irq/devres.c >> @@ -58,8 +58,10 @@ int devm_request_threaded_irq(struct device *dev, unsigned int irq, >> >> dr = devres_alloc(devm_irq_release, sizeof(struct irq_devres), >> GFP_KERNEL); >> - if (!dr) >> + if (!dr) { >> + dev_err(dev, "Failed to allocate device resource data\n"); > Just like any memory allocation, I don't think we print anything for > devres failures. Why do you think we should start doing it?
And tglx point out that:
Having proper and consistent information why the device cannot be used _is_ useful.
> >> return -ENOMEM; >> + } >> >> if (!devname) >> devname = dev_name(dev); >> @@ -67,6 +69,7 @@ int devm_request_threaded_irq(struct device *dev, unsigned int irq, >> rc = request_threaded_irq(irq, handler, thread_fn, irqflags, devname, >> dev_id); >> if (rc) { >> + dev_err(dev, "Failed to request threaded irq\n"); > I don't like that one path - devm() managed - prints error, but regular > path does not. Code should be here consistent. Also error message is too > generic. You need to print at least irq number, maybe also devname?
v3 has been added.
Thx,
Yangtao
> > Best regards, > Krzysztof >
| |