linux-kernel - Re: [PATCH 01/21] genirq: Introduce number_of_interrupts() and set_number_of

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] [day] [month] [year] [list]

Message-ID: <bd377620-7781-4b93-98c1-93f778b74724@acm.org>
Date: Tue, 1 Oct 2024 13:12:26 -0700
From: Bart Van Assche <bvanassche@....org>
To: Thomas Gleixner <tglx@...utronix.de>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 Russell King <linux@...linux.org.uk>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 01/21] genirq: Introduce number_of_interrupts() and
 set_number_of_interrupts()

On 10/1/24 5:33 AM, Thomas Gleixner wrote:
> On Mon, Sep 30 2024 at 11:15, Bart Van Assche wrote:
>> This patch prepares for changing 'nr_irqs' from an exported global
>> variable
> 
> git grep 'This patch' Documentation/process/

Is this the documentation that you are referring to? Anyway, I will 
change the patch description into the imperative mood. <quote>Describe
your changes in imperative mood, e.g. "make xyzzy do frotz"
instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy
to do frotz", as if you are giving orders to the codebase to change
its behaviour.</quote>

>> into a variable with file scope.
> 
> Also what's the rationale for this?

Suppose that a patch would be submitted for review that removes a
declaration of a local variable with the name 'nr_irqs' and that does
not remove all assignments to that local variable. Such a patch converts
an assignment to a local variable into an assignment into a global
variable. If the 'nr_irqs' assignment is more than three lines away from 
other changes, the assignment won't be included in the diff context 
lines and hence won't be visible without inspecting the modified file.
This is why I mentioned in the cover letter that this change makes
patches easier to review. With this patch series applied, such
accidental conversions from assignments to a local variable into an
assignment to a global variable are converted into a compilation error.

>>   extern int nr_irqs;
>> +int number_of_interrupts(void) __pure;
>> +int set_number_of_interrupts(int nr);
> 
> Please use a proper name space prefix for the functions
> irq_.....(). These random names are horrible.

How about irq_count() and irq_set_count()?

>> +int number_of_interrupts(void)
>> +{
>> +	return nr_irqs;
> 
> Why is this int? The number of interrupts is strictly positive, no?

Yes, the number of interrupts is strictly positive. The return type
comes from the type of 'nr_irqs' and been chosen to minimize the risk of
the changes in this patch series. Anyway, I will audit the code that
reads and sets the global 'nr_irqs' variable to see whether its type can
be changed safely into 'unsigned int'.

Thanks,

Bart.