Re: [PATCH 01/15] genirq/devres: Add error information printing for devm_request_threaded_irq()

Re: [PATCH 01/15] genirq/devres: Add error information printing for devm_request_threaded_irq()

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

Re: [PATCH 01/15] genirq/devres: Add error information printing for devm_request_threaded_irq()

[Krzysztof Kozlowski]

On 03/07/2023 11:10, Yangtao Li wrote:
> 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.

Where did tglx suggest printing devres allocation ENOMEM errors?

Best regards,
Krzysztof