| Message ID | YnkE8AbimDa7sfN8@kili (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | fpga: zynq: fix zynq_fpga_has_sync() | expand |
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. 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 &&
Hi Tom, Please find my response inline. > -----Original Message----- > From: Tom Rix <trix@redhat.com> > Sent: Monday, May 9, 2022 6:14 PM > To: Dan Carpenter <dan.carpenter@oracle.com>; Moritz Fischer > <mdf@kernel.org>; Nava kishore Manne <navam@xilinx.com>; Xu Yilun > <yilun.xu@intel.com>; Wu Hao <hao.wu@intel.com> > Cc: Michal Simek <michals@xilinx.com>; linux-fpga@vger.kernel.org; kernel- > janitors@vger.kernel.org > 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@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. > > If folks agree, I'll make the change. > I agree, please change it to u8 Regards, Nava kishore.
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 &&
On Mon, May 09, 2022 at 03:11:28PM +0300, 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) Hi Dan & Moritz: Thanks for the patch. But it actually reverts Nava's patch. Since Nava's patch is not pushed to linux-next yet, could we just drop it from linux-fpga? Thanks, Yilun > { > for (; count >= 4; buf += 4, count -= 4) > if (buf[0] == 0x66 && buf[1] == 0x55 && buf[2] == 0x99 && > -- > 2.35.1
Hi, Please find my response inline. > -----Original Message----- > From: Xu Yilun <yilun.xu@intel.com> > Sent: Monday, May 9, 2022 10:25 PM > To: Tom Rix <trix@redhat.com> > Cc: Dan Carpenter <dan.carpenter@oracle.com>; Moritz Fischer > <mdf@kernel.org>; Nava kishore Manne <navam@xilinx.com>; Wu Hao > <hao.wu@intel.com>; Michal Simek <michals@xilinx.com>; linux- > fpga@vger.kernel.org; kernel-janitors@vger.kernel.org > 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@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) > 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@xilinx.com/ https://patchwork.kernel.org/project/linux-fpga/patch/20220322082202.2007321-2-nava.manne@xilinx.com/ Regards, Navakishore.
On 5/9/22 8:15 AM, Nava kishore Manne wrote: > Hi Tom, > > Please find my response inline. > >> -----Original Message----- >> From: Tom Rix <trix@redhat.com> >> Sent: Monday, May 9, 2022 6:14 PM >> To: Dan Carpenter <dan.carpenter@oracle.com>; Moritz Fischer >> <mdf@kernel.org>; Nava kishore Manne <navam@xilinx.com>; Xu Yilun >> <yilun.xu@intel.com>; Wu Hao <hao.wu@intel.com> >> Cc: Michal Simek <michals@xilinx.com>; linux-fpga@vger.kernel.org; kernel- >> janitors@vger.kernel.org >> 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@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. >> >> 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.
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@redhat.com> > > > Sent: Monday, May 9, 2022 6:14 PM > > > To: Dan Carpenter <dan.carpenter@oracle.com>; Moritz Fischer > > > <mdf@kernel.org>; Nava kishore Manne <navam@xilinx.com>; Xu Yilun > > > <yilun.xu@intel.com>; Wu Hao <hao.wu@intel.com> > > > Cc: Michal Simek <michals@xilinx.com>; linux-fpga@vger.kernel.org; kernel- > > > janitors@vger.kernel.org > > > 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@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. > > > > > > 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.
On Tue, May 10, 2022 at 01:00:15AM +0800, Xu Yilun wrote: > > Hi Dan & Moritz: > > Thanks for the patch. But it actually reverts Nava's patch. Since Nava's > patch is not pushed to linux-next yet, could we just drop it from > linux-fpga? I don't know if you meant linux-next. It's pushed to linux-next. But linux-next is rebased every day so reverting is fine as far as linux-next goes. regards, dan carpenter
On Wed, May 11, 2022 at 11:33:29AM +0300, Dan Carpenter wrote: > On Tue, May 10, 2022 at 01:00:15AM +0800, Xu Yilun wrote: > > > > Hi Dan & Moritz: > > > > Thanks for the patch. But it actually reverts Nava's patch. Since Nava's > > patch is not pushed to linux-next yet, could we just drop it from > > linux-fpga? > > I don't know if you meant linux-next. It's pushed to linux-next. But > linux-next is rebased every day so reverting is fine as far as linux-next > goes. Thanks for the info. I was not aware of that and may mess up something. I force pushed the linux-fpga yesterday for this patchset, then linux-next would be conflict with linux-fpga, is it? If it is the case, how could I remedy? Thanks, Yilun > > regards, > dan carpenter
On Wed, May 11, 2022 at 12:17:18PM +0300, Dan Carpenter wrote: > On Wed, May 11, 2022 at 04:48:38PM +0800, Xu Yilun wrote: > > On Wed, May 11, 2022 at 11:33:29AM +0300, Dan Carpenter wrote: > > > On Tue, May 10, 2022 at 01:00:15AM +0800, Xu Yilun wrote: > > > > > > > > Hi Dan & Moritz: > > > > > > > > Thanks for the patch. But it actually reverts Nava's patch. Since Nava's > > > > patch is not pushed to linux-next yet, could we just drop it from > > > > linux-fpga? > > > > > > I don't know if you meant linux-next. It's pushed to linux-next. But > > > linux-next is rebased every day so reverting is fine as far as linux-next > > > goes. > > > > Thanks for the info. I was not aware of that and may mess up something. > > I force pushed the linux-fpga yesterday for this patchset, then linux-next > > would be conflict with linux-fpga, is it? > > > > If it is the case, how could I remedy? > > No. Don't worry about linux-next. Stephen rebuilds it from scratch > every day. It's fine if you rebase. There are quite a few trees which > rebase. That's good new! Thanks again. > > regards, > dan carpenter
On Wed, May 11, 2022 at 04:48:38PM +0800, Xu Yilun wrote: > On Wed, May 11, 2022 at 11:33:29AM +0300, Dan Carpenter wrote: > > On Tue, May 10, 2022 at 01:00:15AM +0800, Xu Yilun wrote: > > > > > > Hi Dan & Moritz: > > > > > > Thanks for the patch. But it actually reverts Nava's patch. Since Nava's > > > patch is not pushed to linux-next yet, could we just drop it from > > > linux-fpga? > > > > I don't know if you meant linux-next. It's pushed to linux-next. But > > linux-next is rebased every day so reverting is fine as far as linux-next > > goes. > > Thanks for the info. I was not aware of that and may mess up something. > I force pushed the linux-fpga yesterday for this patchset, then linux-next > would be conflict with linux-fpga, is it? > > If it is the case, how could I remedy? No. Don't worry about linux-next. Stephen rebuilds it from scratch every day. It's fine if you rebase. There are quite a few trees which rebase. regards, dan carpenter
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) { for (; count >= 4; buf += 4, count -= 4) if (buf[0] == 0x66 && buf[1] == 0x55 && buf[2] == 0x99 &&
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(-)