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: <87ikuawlm1.ffs@tglx>
Date: Wed, 02 Oct 2024 17:49:42 +0200
From: Thomas Gleixner <tglx@...utronix.de>
To: Bart Van Assche <bvanassche@....org>
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 Tue, Oct 01 2024 at 13:12, Bart Van Assche wrote:
> 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>

Yes.

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

Can you please add that to the change log?

>>>   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()?

Sure.

>>> +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'.

Thank you!

      tglx