Message-ID: <CAJ7bep+ras3xdWfH-7YLbVha0QVN-ONmp39Pvf_TpYhh9A=o-g@mail.gmail.com>
Date: Fri, 1 Nov 2024 15:48:11 +0530
From: Advait Dhamorikar <advaitdhamorikar@...il.com>
To: Fabrizio Castro <fabrizio.castro.jz@...esas.com>
Cc: Thomas Gleixner <tglx@...utronix.de>, 
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, 
	"skhan@...uxfoundation.org" <skhan@...uxfoundation.org>, 
	"anupnewsmail@...il.com" <anupnewsmail@...il.com>
Subject: Re: [PATCH-next] irqchip/renesas-rzv2h: Fix potentially mismatched datatype

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@...esas.com> wrote:
>
> Hello Advait,
>
> Thanks for your email.
>
> > From: Advait Dhamorikar <advaitdhamorikar@...il.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@...utronix.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

Powered by blists - more mailing lists