On Tue, May 10, 2022 at 05:21:59AM -0700, Tom Rix wrote:
>
> On 5/9/22 8:15 AM, Nava kishore Manne wrote:
> > Hi Tom,
> >
> > Please find my response inline.
> >
> > > -----Original Message-----
> > > From: Tom Rix <trix@xxxxxxxxxx>
> > > Sent: Monday, May 9, 2022 6:14 PM
> > > To: Dan Carpenter <dan.carpenter@xxxxxxxxxx>; Moritz Fischer
> > > <mdf@xxxxxxxxxx>; Nava kishore Manne <navam@xxxxxxxxxx>; Xu Yilun
> > > <yilun.xu@xxxxxxxxx>; Wu Hao <hao.wu@xxxxxxxxx>
> > > Cc: Michal Simek <michals@xxxxxxxxxx>; linux-fpga@xxxxxxxxxxxxxxx; kernel-
> > > janitors@xxxxxxxxxxxxxxx
> > > Subject: Re: [PATCH] fpga: zynq: fix zynq_fpga_has_sync()
> > >
> > >
> > > On 5/9/22 5:11 AM, Dan Carpenter wrote:
> > > > The type needs to be u8. The type was accidentally changed to char as
> > > > a cleanup. Unfortunately, that meant that the zynq_fpga_has_sync()
> > > > function never returns true. This bug was detected by Smatch and Clang:
> > > >
> > > > drivers/fpga/zynq-fpga.c:245 zynq_fpga_has_sync() warn: impossible
> > > condition '(buf[2] == 153) => ((-128)-127 == 153)'
> > > > drivers/fpga/zynq-fpga.c:246 zynq_fpga_has_sync() warn: impossible
> > > condition '(buf[3] == 170) => ((-128)-127 == 170)'
> > > > drivers/fpga/zynq-fpga.c:246:14: warning: result of comparison of
> > > > constant 170 with expression of type 'const char' is always false
> > > > [-Wtautological-constant-out-of-range-compare]
> > > > buf[3] == 0xaa)
> > > > ~~~~~~ ^ ~~~~
> > > > drivers/fpga/zynq-fpga.c:245:50: warning: result of comparison of
> > > > constant 153 with expression of type 'const char' is always false
> > > > [-Wtautological-constant-out-of-range-compare]
> > > > if (buf[0] == 0x66 && buf[1] == 0x55 && buf[2] == 0x99 &&
> > > > ~~~~~~ ^
> > > > ~~~~
> > > >
> > > > Fixes: ada14a023a64 ("fpga: zynq: Fix incorrect variable type")
> > > > Reported-by: kernel test robot <lkp@xxxxxxxxx>
> > > > Signed-off-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> > > > ---
> > > > The ada14a023a64 ("fpga: zynq: Fix incorrect variable type") patch
> > > > went through six of revisions. The kbuild bug found this bug early on
> > > > but the author ingored kbuild-bot and kept resending the buggy patch
> > > > anyway.
> > > >
> > > > After the patch was merged then I sent a separate bug report and Xu
> > > > Yilun asked about why only the author was on the CC list for the first
> > > > bug reports. A valid question, definitely. I will poke the kbuild
> > > > devs about this.
> > > >
> > > > Hm... Actually looking through the list there have been a bunch of
> > > > bug reports about this because both Smatch and Clang complain so
> > > > kbuild sends duplicate warnings for this type of bug. And then kbuild
> > > > sends another to say "This issue is still remaining" warning. And
> > > > then Xu Yilun sent an email "Kbuild-bot is still complaining. Please
> > > > don't forget to fix this." So that's at least four public emails
> > > > about this and one or two private emails directly from kbuild-bot to the
> > > author.
> > > > The kbuild-bot wanted to send *another* warning today, but I decided
> > > > to send a fix instead.
> > > >
> > > > LOL.
> > > >
> > > > drivers/fpga/zynq-fpga.c | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/fpga/zynq-fpga.c b/drivers/fpga/zynq-fpga.c index
> > > > 6beaba9dfe97..426aa34c6a0d 100644
> > > > --- a/drivers/fpga/zynq-fpga.c
> > > > +++ b/drivers/fpga/zynq-fpga.c
> > > > @@ -239,7 +239,7 @@ static irqreturn_t zynq_fpga_isr(int irq, void *data)
> > > > * the correct byte order, and be dword aligned. The input is a Xilinx .bin
> > > > * file with every 32 bit quantity swapped.
> > > > */
> > > > -static bool zynq_fpga_has_sync(const char *buf, size_t count)
> > > > +static bool zynq_fpga_has_sync(const u8 *buf, size_t count)
> > > This is called from zynq_fpga_ops_write_init, a fpga_manager_ops function
> > > that
> > >
> > > uses 'const char *' as a type for its write() buf's.
> > >
> > > I think const u8 * would be a better type for all of the fpga_manager
> > > instances.
> > >
> > > If folks agree, I'll make the change.
> > >
> > I agree, please change it to u8
>
> I was hoping one of the fpga/ maintainers would chime in.
As I said before, I think const char *buf for write_init() is OK.
>
> Though it seems obvious that the type-specifier should be unsigned, this is
> a general interface change to the subsystem.
Searching over all kernel code, we can find many const char *buf/buf[], and
also many const u8 *buf/buf[], const unsigned char *buf/buf[]. I think
they are all good. They are just a block of data and people don't care
about their format. So signed or unsigned is not important.
If a specific driver have its own understanding of the data block and
want to parse it, just type casting it as needed to u8/s8/u16/s16 ...
So const char/u8/unsigned char * are all good to me, but for now I don't see
the necessity changing to one another for all fpga drivers.
Thanks,
Yilun
>
> I would rather have buy in before spending a couple of days doing the change
> and having it rejected.
>
> Tom
>
> >
> > Regards,
> > Nava kishore.
