Re: [PATCH 01/21] genirq: Introduce number_of_interrupts() and set_number_of_interrupts()

[Thomas Gleixner <tglx@linutronix.de>]

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