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.
Though it seems obvious that the type-specifier should be unsigned, this
is a general interface change to the subsystem.
I would rather have buy in before spending a couple of days doing the
change and having it rejected.
Tom
Regards,
Nava kishore.
