This patch prepares for changing 'nr_irqs' from an exported global variable
into a variable with file scope.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 include/linux/irqnr.h |  2 ++
 kernel/irq/irqdesc.c  | 14 ++++++++++++++
 2 files changed, 16 insertions(+)

diff --git a/include/linux/irqnr.h b/include/linux/irqnr.h
index 3496baa0b07f..a8b2cb6146e8 100644
--- a/include/linux/irqnr.h
+++ b/include/linux/irqnr.h
@@ -6,6 +6,8 @@
 
 
 extern int nr_irqs;
+int number_of_interrupts(void) __pure;
+int set_number_of_interrupts(int nr);
 extern struct irq_desc *irq_to_desc(unsigned int irq);
 unsigned int irq_get_next_irq(unsigned int offset);
 
diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
index 1dee88ba0ae4..8c6280843964 100644
--- a/kernel/irq/irqdesc.c
+++ b/kernel/irq/irqdesc.c
@@ -141,6 +141,20 @@ static void desc_set_defaults(unsigned int irq, struct irq_desc *desc, int node,
 int nr_irqs = NR_IRQS;
 EXPORT_SYMBOL_GPL(nr_irqs);
 
+int number_of_interrupts(void)
+{
+	return nr_irqs;
+}
+EXPORT_SYMBOL_GPL(number_of_interrupts);
+
+int set_number_of_interrupts(int nr)
+{
+	nr_irqs = nr;
+
+	return nr;
+}
+EXPORT_SYMBOL_GPL(set_number_of_interrupts);
+
 static DEFINE_MUTEX(sparse_irq_lock);
 static struct maple_tree sparse_irqs = MTREE_INIT_EXT(sparse_irqs,
 					MT_FLAGS_ALLOC_RANGE |

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/

> into a variable with file scope.

Also what's the rationale for this?

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

>  extern struct irq_desc *irq_to_desc(unsigned int irq);
>  unsigned int irq_get_next_irq(unsigned int offset);
>  
> diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
> index 1dee88ba0ae4..8c6280843964 100644
> --- a/kernel/irq/irqdesc.c
> +++ b/kernel/irq/irqdesc.c
> @@ -141,6 +141,20 @@ static void desc_set_defaults(unsigned int irq, struct irq_desc *desc, int node,
>  int nr_irqs = NR_IRQS;
>  EXPORT_SYMBOL_GPL(nr_irqs);
>  
> +int number_of_interrupts(void)
> +{
> +	return nr_irqs;

Why is this int? The number of interrupts is strictly positive, no?

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.

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

On 10/2/24 8:49 AM, Thomas Gleixner wrote:
> 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:
>>>> 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?

I will do that.

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

I just noticed that a macro with the name irq_count() already exists.
How about the names irq_get_nr_irqs() and irq_set_nr_irqs() instead?

Thanks,

Bart.

On Thu, Oct 03 2024 at 14:01, Bart Van Assche wrote:
> On 10/2/24 8:49 AM, Thomas Gleixner wrote:
>>> How about irq_count() and irq_set_count()?
>> 
>> Sure.
>
> I just noticed that a macro with the name irq_count() already exists.
> How about the names irq_get_nr_irqs() and irq_set_nr_irqs() instead?

Sounds good to me.

Thanks,

        tglx