Re: [PATCH v9 1/1] genirq/devres: Add dev_err_probe() in devm_request_threaded_irq() and devm_request_any_context_irq()

[Thomas Gleixner <tglx@linutronix.de>]

On Wed, Jul 30 2025 at 18:48, Christophe JAILLET wrote:
> Le 30/07/2025 à 08:25, Pan Chuang a écrit :
>> +int devm_request_any_context_irq(struct device *dev, unsigned int irq,
>> +				 irq_handler_t handler, unsigned long irqflags,
>> +				 const char *devname, void *dev_id)
>> +{
>> +	int rc = __devm_request_any_context_irq(dev, irq, handler, irqflags,
>> +						devname, dev_id);
>> +	if (rc < 0) {
>> +		return dev_err_probe(dev, rc, "request_irq(%u) %ps %s\n",
>> +				     irq, handler, devname ? : "");
>> +	}
>
> Extra { } should be removed.

No. Even when C does not require it, brackets should only be omitted
when there is truly a single line after the condition. That's just
simpler to read because w/o brackets the brain pattern recognition
mechanism expects _one_ line not two or more and if there are more the
reading flow is interrupted as you have to parse it. With the brackets
it's obvious that there are more lines to read than one.

>  From my PoV, it would look more logical to have the same logic in 
> devm_request_threaded_irq() and in devm_request_any_context_irq().

As they print the same thing the right thing to do is:

        int rc = __devm_request_any_context_irq(....);

        return devm_request_result(dev, rc, irq, handler, NULL, devname);

and in devm_request_threaded_irq() invoke it with:

        return devm_request_result(dev, rc, irq, handler, thread_fn, devname);

and let that function return rc if (rc >= 0), which handles both cases.

>>   EXPORT_SYMBOL(devm_request_any_context_irq);
>
> On version 5 of the patch, there was a comment related to using 
> EXPORT_SYMBOL_GPL(), does it still make sense?
> (no strong opinion from me, just noted that and wondered if done on purpose)

That's an existing export. If at all that needs to be a seperate patch.

Thanks,

        tglx