Re: [PATCH] fpga: zynq: fix zynq_fpga_has_sync()

[Xu Yilun <yilun.xu@intel.com>]

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@intel.com>
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > ---
> > 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)

Thanks,
Yilun

> 
> If folks agree, I'll make the change.
> 
> Tom
> 
> >   {
> >   	for (; count >= 4; buf += 4, count -= 4)
> >   		if (buf[0] == 0x66 && buf[1] == 0x55 && buf[2] == 0x99 &&