Hi,
Please find my response inline.
> -----Original Message-----
> From: Xu Yilun <yilun.xu@xxxxxxxxx>
> Sent: Monday, May 9, 2022 10:25 PM
> To: Tom Rix <trix@xxxxxxxxxx>
> Cc: Dan Carpenter <dan.carpenter@xxxxxxxxxx>; Moritz Fischer
> <mdf@xxxxxxxxxx>; Nava kishore Manne <navam@xxxxxxxxxx>; Wu Hao
> <hao.wu@xxxxxxxxx>; Michal Simek <michals@xxxxxxxxxx>; linux-
> fpga@xxxxxxxxxxxxxxx; kernel-janitors@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH] fpga: zynq: fix zynq_fpga_has_sync()
>
> On Mon, May 09, 2022 at 05:43:58AM -0700, Tom Rix wrote:
> >
> > 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.
>
> I don't think it's necessary to change the write_init(), const char *buf is fine.
>
> For this case, if the cleanup is necessary, just type casting the buf.
>
> zynq_fpga_has_sync((const u8 *)buf, count)
>
zynq_fpga_ops_write_init() and fpga_manager_ops ideally handle the binary files so I feel 'const u8 *' would be a better option here.
In the Initial version of my patches I have done only type cast. But base on the comments I have changed zynq_fpga_has_sync() API args type.
For more details please refer the below links
https://patchwork.kernel.org/project/linux-fpga/patch/20220308094519.1816649-2-nava.manne@xxxxxxxxxx/
https://patchwork.kernel.org/project/linux-fpga/patch/20220322082202.2007321-2-nava.manne@xxxxxxxxxx/
Regards,
Navakishore.
