linux-kernel - Re: [PATCH v9 1/1] genirq/devres: Add dev_err_probe() in devm_request_threaded_irq() and devm_request_any_context

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87ms8lio7i.ffs@tglx>
Date: Wed, 30 Jul 2025 19:27:13 +0200
From: Thomas Gleixner <tglx@...utronix.de>
To: Christophe JAILLET <christophe.jaillet@...adoo.fr>, Pan Chuang
 <panchuang@...o.com>
Cc: kernel-janitors@...r.kernel.org, linux-kernel@...r.kernel.org,
 miquel.raynal@...tlin.com, Jonathan.Cameron@...wei.com,
 u.kleine-koenig@...gutronix.de, angeg.delregno@...labora.com,
 krzk@...nel.org, a.fatoum@...gutronix.de, frank.li@...o.com
Subject: Re: [PATCH v9 1/1] genirq/devres: Add dev_err_probe() in
 devm_request_threaded_irq() and devm_request_any_context_irq()

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