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

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

[Advait Dhamorikar]

This patch updates the type of hw_irq to unsigned long to 
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.

Signed-off-by: Advait Dhamorikar <advaitdhamorikar@gmail.com>
---
 drivers/irqchip/irq-renesas-rzv2h.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/irqchip/irq-renesas-rzv2h.c b/drivers/irqchip/irq-renesas-rzv2h.c
index fe2d29e91026..f7f27ee5c732 100644
--- a/drivers/irqchip/irq-renesas-rzv2h.c
+++ b/drivers/irqchip/irq-renesas-rzv2h.c
@@ -102,7 +102,7 @@ static inline struct rzv2h_icu_priv *irq_data_to_priv(struct irq_data *data)
 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;
 	u32 bit;
 
@@ -128,7 +128,7 @@ static void rzv2h_icu_eoi(struct irq_data *d)
 static void rzv2h_tint_irq_endisable(struct irq_data *d, bool enable)
 {
 	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);
 	u32 tint_nr, tssel_n, k, tssr;
 
 	if (hw_irq < ICU_TINT_START)
@@ -184,7 +184,7 @@ static int rzv2h_nmi_set_type(struct irq_data *d, unsigned int type)
 
 static void rzv2h_clear_irq_int(struct rzv2h_icu_priv *priv, unsigned int hwirq)
 {
-	unsigned int irq_nr = hwirq - ICU_IRQ_START;
+	unsigned long irq_nr = hwirq - ICU_IRQ_START;
 	u32 isctr, iitsr, iitsel;
 	u32 bit = BIT(irq_nr);
 
@@ -204,8 +204,8 @@ static void rzv2h_clear_irq_int(struct rzv2h_icu_priv *priv, unsigned int hwirq)
 static int rzv2h_irq_set_type(struct irq_data *d, unsigned int type)
 {
 	struct rzv2h_icu_priv *priv = irq_data_to_priv(d);
-	unsigned int hwirq = irqd_to_hwirq(d);
-	u32 irq_nr = hwirq - ICU_IRQ_START;
+	unsigned long hwirq = irqd_to_hwirq(d);
+	unsigned long irq_nr = hwirq - ICU_IRQ_START;
 	u32 iitsr, sense;
 
 	switch (type & IRQ_TYPE_SENSE_MASK) {
@@ -241,7 +241,7 @@ static int rzv2h_irq_set_type(struct irq_data *d, unsigned int type)
 
 static void rzv2h_clear_tint_int(struct rzv2h_icu_priv *priv, unsigned int hwirq)
 {
-	unsigned int tint_nr = hwirq - ICU_TINT_START;
+	unsigned long tint_nr = hwirq - ICU_TINT_START;
 	int titsel_n = ICU_TITSR_TITSEL_N(tint_nr);
 	u32 tsctr, titsr, titsel;
 	u32 bit = BIT(tint_nr);
@@ -265,9 +265,9 @@ static int rzv2h_tint_set_type(struct irq_data *d, unsigned int type)
 	u32 titsr, titsr_k, titsel_n, tien;
 	struct rzv2h_icu_priv *priv;
 	u32 tssr, tssr_k, tssel_n;
-	unsigned int hwirq;
+	unsigned long hwirq;
 	u32 tint, sense;
-	int tint_nr;
+	unsigned long tint_nr;
 
 	switch (type & IRQ_TYPE_SENSE_MASK) {
 	case IRQ_TYPE_LEVEL_LOW:
@@ -329,7 +329,7 @@ static int rzv2h_tint_set_type(struct irq_data *d, unsigned int type)
 
 static int rzv2h_icu_set_type(struct irq_data *d, unsigned int type)
 {
-	unsigned int hw_irq = irqd_to_hwirq(d);
+	unsigned long hw_irq = irqd_to_hwirq(d);
 	int ret;
 
 	if (hw_irq >= ICU_TINT_START)
-- 
2.34.1

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

[Thomas Gleixner]

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

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

[Advait Dhamorikar]

Hello Thomas,

> and read through the matching documentation.
My bad, I will be more imperative next time :)

> 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 did not know about the interrupt range of the chip, so I
assumed the truncation from 8 bytes to 4 might pose a problem.

>If at all, then the proper change is either
>1) to make the related variables type irq_hw_number_t
This seems like the better option to me. If it is needed,
I will submit a patch v2 after waiting for some more feedback, if there's any.

I have one question, static analyzers report an issue of a bad bit
shift operation
on line 307: tien = ICU_TSSR_TIEN(titsel_n);
#define ICU_TSSR_TIEN(n) (BIT(7) << ((n) * 8))

From what I understand hwirq can possibly have values from 0 to 31
If titsel_n ends up being a large remainder say 5, we can have a bad
bitshift operation
exceeding 64 bits.
My humble apologies if my observations are completely off, I'm a
beginner trying to learn
Linux driver dev by looking at how other drivers work.
If this is an issue what could be a possible method to fix this?
I would be grateful if you or someone could point me to some relevant docs.

Thank you for your time and feedback,

Best regards,
Advait

On Fri, 1 Nov 2024 at 02:54, Thomas Gleixner <tglx@linutronix.de> wrote:
>
> 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

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

[Fabrizio Castro]

Hello Advait,

Thanks for your email.

> From: Advait Dhamorikar <advaitdhamorikar@gmail.com>
> Subject: Re: [PATCH-next] irqchip/renesas-rzv2h: Fix potentially mismatched datatype
> 
> Hello Thomas,
> 
> > and read through the matching documentation.
> My bad, I will be more imperative next time :)
> 
> > 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 did not know about the interrupt range of the chip, so I
> assumed the truncation from 8 bytes to 4 might pose a problem.
> 
> >If at all, then the proper change is either
> >1) to make the related variables type irq_hw_number_t
> This seems like the better option to me. If it is needed,
> I will submit a patch v2 after waiting for some more feedback, if there's any.
> 
> I have one question, static analyzers report an issue of a bad bit
> shift operation
> on line 307: tien = ICU_TSSR_TIEN(titsel_n);
> #define ICU_TSSR_TIEN(n) (BIT(7) << ((n) * 8))

Which static analyzers did you use?

> 
> From what I understand hwirq can possibly have values from 0 to 31
> If titsel_n ends up being a large remainder say 5, we can have a bad
> bitshift operation
> exceeding 64 bits.

In this case it's not a bad bit shift operation. The code never passes a parameter that makes it exceed the 64 bit boundary.

There is nothing to fix in this case.

Remember that analysers are not always right, you still need to read and understand the code.

Kind regards,
Fab

> My humble apologies if my observations are completely off, I'm a
> beginner trying to learn
> Linux driver dev by looking at how other drivers work.
> If this is an issue what could be a possible method to fix this?
> I would be grateful if you or someone could point me to some relevant docs.
> 
> Thank you for your time and feedback,
> 
> Best regards,
> Advait
> 
> On Fri, 1 Nov 2024 at 02:54, Thomas Gleixner <tglx@linutronix.de> wrote:
> >
> > 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

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

[Advait Dhamorikar]

Hello Fabrizio,

> Which static analyzers did you use?
I used the Synopsys blackduck coverity scanner.

> In this case it's not a bad bit shift operation. The code never passes a parameter that makes it exceed the 64 bit boundary.
> There is nothing to fix in this case.
> Remember that analysers are not always right, you still need to read and understand the code.
Yes, I needed clarification because I wasn't so sure what range of
values irqd_to_hwirq() returned to hwirq.

Thanks for your time and the insights,

Kind regards,
Advait

On Fri, 1 Nov 2024 at 15:18, Fabrizio Castro
<fabrizio.castro.jz@renesas.com> wrote:
>
> Hello Advait,
>
> Thanks for your email.
>
> > From: Advait Dhamorikar <advaitdhamorikar@gmail.com>
> > Subject: Re: [PATCH-next] irqchip/renesas-rzv2h: Fix potentially mismatched datatype
> >
> > Hello Thomas,
> >
> > > and read through the matching documentation.
> > My bad, I will be more imperative next time :)
> >
> > > 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 did not know about the interrupt range of the chip, so I
> > assumed the truncation from 8 bytes to 4 might pose a problem.
> >
> > >If at all, then the proper change is either
> > >1) to make the related variables type irq_hw_number_t
> > This seems like the better option to me. If it is needed,
> > I will submit a patch v2 after waiting for some more feedback, if there's any.
> >
> > I have one question, static analyzers report an issue of a bad bit
> > shift operation
> > on line 307: tien = ICU_TSSR_TIEN(titsel_n);
> > #define ICU_TSSR_TIEN(n) (BIT(7) << ((n) * 8))
>
> Which static analyzers did you use?
>
> >
> > From what I understand hwirq can possibly have values from 0 to 31
> > If titsel_n ends up being a large remainder say 5, we can have a bad
> > bitshift operation
> > exceeding 64 bits.
>
> In this case it's not a bad bit shift operation. The code never passes a parameter that makes it exceed the 64 bit boundary.
>
> There is nothing to fix in this case.
>
> Remember that analysers are not always right, you still need to read and understand the code.
>
> Kind regards,
> Fab
>
> > My humble apologies if my observations are completely off, I'm a
> > beginner trying to learn
> > Linux driver dev by looking at how other drivers work.
> > If this is an issue what could be a possible method to fix this?
> > I would be grateful if you or someone could point me to some relevant docs.
> >
> > Thank you for your time and feedback,
> >
> > Best regards,
> > Advait
> >
> > On Fri, 1 Nov 2024 at 02:54, Thomas Gleixner <tglx@linutronix.de> wrote:
> > >
> > > 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

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

[Jiri Slaby]

Hi,

On 31. 10. 24, 20:36, Advait Dhamorikar wrote:
> This patch updates the type of hw_irq to unsigned long to
> 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.

"in turn"

But what's the purpose of this? First, why wouldn't you use 
irq_hw_number_t then?

Nevertheless, the HW does not support hw irqs > uint (it supports 
1+16+32, actually). So why all this in the first place?

> @@ -265,9 +265,9 @@ static int rzv2h_tint_set_type(struct irq_data *d, unsigned int type)
>   	u32 titsr, titsr_k, titsel_n, tien;
>   	struct rzv2h_icu_priv *priv;
>   	u32 tssr, tssr_k, tssel_n;
> -	unsigned int hwirq;
> +	unsigned long hwirq;
>   	u32 tint, sense;
> -	int tint_nr;
> +	unsigned long tint_nr;

Switching tint_nr to unsigned might still be a good thing to avoid weird 
signed overflows in the future. But I don't think it warrants for a patch...

thanks,
-- 
js
suse labs