Re: [PATCH-next] irqchip/renesas-rzv2h: Fix potentially mismatched datatype

[Thomas Gleixner <tglx@linutronix.de>]

On Fri, Nov 01 2024 at 01:06, Advait Dhamorikar wrote:
> This patch updates the type of hw_irq to unsigned long to 

Please do:

git grep 'This patch' Documentation/process/

and read through the matching documentation.

> match irq_hw_number_t.
>
> The variable hw_irq is defined as unsigned int at places,
> However when it is initialized using irqd_to_hwirq(), it returns 
> an irq_hw_number_t, which inturn is a typedef for unsigned long.

We know that, but what is the problem this patch is actually solving?

>  static void rzv2h_icu_eoi(struct irq_data *d)
>  {
>  	struct rzv2h_icu_priv *priv = irq_data_to_priv(d);
> -	unsigned int hw_irq = irqd_to_hwirq(d);
> +	unsigned long hw_irq = irqd_to_hwirq(d);
>  	unsigned int tintirq_nr;

It moves the type mismatch and potential truncation a few lines further
down:

	tintirq_nr = hw_irq - ICU_TINT_START;

In fact there is no problem with the existing code because the hardware
interrupt number range for this interrupt chip is guaranteed to be
smaller than UINT_MAX. IOW, a truncation from unsigned long to unsigned
int (on a 64-bit system) does not matter at all.

I'm all for being type safe, but what you are doing is purely cosmetic.

If at all, then the proper change is either

 1) to make the related variables type irq_hw_number_t

    You cannot make assumptions about the type which is behind
    irq_hw_number_t today. The type can change tomorrow, no?

or

 2) Use a proper type cast which documents that the type conversion
    including the potential truncation is intentional and correct.

    This should not be an actual type cast, but a helper inline which
    has the cast and explicitely returns an unsigned int.

I leave it to you to decide which variant is the correct one, but I'm
happy to answer your questions.

Thanks,

        tglx