[PATCH V2] staging: gpib: Remove a dead condition in if statement

[PATCH V2] staging: gpib: Remove a dead condition in if statement

[Everest K.C.]

The variable `residue` is an unsigned int, also the function
`fluke_get_dma_residue` returns an unsigned int. The value of
an unsigned int can only be 0 at minimum.
The less-than-zero comparison can never be true.
Fix it by removing the dead condition in the if statement.

This issue was reported by Coverity Scan.
Report:
CID 1600782: (#1 of 1): Macro compares unsigned to 0 (NO_EFFECT)
unsigned_compare: This less-than-zero comparison of an unsigned value
is never true. residue < 0U.

Signed-off-by: Everest K.C. <everestkc@everestkc.com.np>
---
V1 -> V2: - Fixed typo of comparison in changelog
          - Removed Fixes tag 

 drivers/staging/gpib/eastwood/fluke_gpib.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/gpib/eastwood/fluke_gpib.c b/drivers/staging/gpib/eastwood/fluke_gpib.c
index f9f149db222d..51b4f9891a34 100644
--- a/drivers/staging/gpib/eastwood/fluke_gpib.c
+++ b/drivers/staging/gpib/eastwood/fluke_gpib.c
@@ -644,7 +644,7 @@ static int fluke_dma_read(gpib_board_t *board, uint8_t *buffer,
 	 */
 	usleep_range(10, 15);
 	residue = fluke_get_dma_residue(e_priv->dma_channel, dma_cookie);
-	if (WARN_ON_ONCE(residue > length || residue < 0))
+	if (WARN_ON_ONCE(residue > length))
 		return -EFAULT;
 	*bytes_read += length - residue;
 	dmaengine_terminate_all(e_priv->dma_channel);
-- 
2.43.0

Re: [PATCH V2] staging: gpib: Remove a dead condition in if statement

[Greg KH]

On Wed, Oct 16, 2024 at 01:53:18AM -0600, Everest K.C. wrote:
> The variable `residue` is an unsigned int, also the function
> `fluke_get_dma_residue` returns an unsigned int. The value of
> an unsigned int can only be 0 at minimum.
> The less-than-zero comparison can never be true.
> Fix it by removing the dead condition in the if statement.
> 
> This issue was reported by Coverity Scan.
> Report:
> CID 1600782: (#1 of 1): Macro compares unsigned to 0 (NO_EFFECT)
> unsigned_compare: This less-than-zero comparison of an unsigned value
> is never true. residue < 0U.
> 
> Signed-off-by: Everest K.C. <everestkc@everestkc.com.np>
> ---
> V1 -> V2: - Fixed typo of comparison in changelog
>           - Removed Fixes tag 
> 
>  drivers/staging/gpib/eastwood/fluke_gpib.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/gpib/eastwood/fluke_gpib.c b/drivers/staging/gpib/eastwood/fluke_gpib.c
> index f9f149db222d..51b4f9891a34 100644
> --- a/drivers/staging/gpib/eastwood/fluke_gpib.c
> +++ b/drivers/staging/gpib/eastwood/fluke_gpib.c
> @@ -644,7 +644,7 @@ static int fluke_dma_read(gpib_board_t *board, uint8_t *buffer,
>  	 */
>  	usleep_range(10, 15);
>  	residue = fluke_get_dma_residue(e_priv->dma_channel, dma_cookie);
> -	if (WARN_ON_ONCE(residue > length || residue < 0))
> +	if (WARN_ON_ONCE(residue > length))

No, this is incorrect, now we never notice is the call to
fluke_get_dma_residue() has failed.  Please fix that bug instead (hint,
Covertity is giving you a pointer to where something might be wrong, but
this change is NOT how to fix it.)

thanks,

greg k-h

Re: [PATCH V2] staging: gpib: Remove a dead condition in if statement

[Everest K.C.]

On Wed, Oct 16, 2024 at 2:04 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Wed, Oct 16, 2024 at 01:53:18AM -0600, Everest K.C. wrote:
> > The variable `residue` is an unsigned int, also the function
> > `fluke_get_dma_residue` returns an unsigned int. The value of
> > an unsigned int can only be 0 at minimum.
> > The less-than-zero comparison can never be true.
> > Fix it by removing the dead condition in the if statement.
> >
> > This issue was reported by Coverity Scan.
> > Report:
> > CID 1600782: (#1 of 1): Macro compares unsigned to 0 (NO_EFFECT)
> > unsigned_compare: This less-than-zero comparison of an unsigned value
> > is never true. residue < 0U.
> >
> > Signed-off-by: Everest K.C. <everestkc@everestkc.com.np>
> > ---
> > V1 -> V2: - Fixed typo of comparison in changelog
> >           - Removed Fixes tag
> >
> >  drivers/staging/gpib/eastwood/fluke_gpib.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/staging/gpib/eastwood/fluke_gpib.c b/drivers/staging/gpib/eastwood/fluke_gpib.c
> > index f9f149db222d..51b4f9891a34 100644
> > --- a/drivers/staging/gpib/eastwood/fluke_gpib.c
> > +++ b/drivers/staging/gpib/eastwood/fluke_gpib.c
> > @@ -644,7 +644,7 @@ static int fluke_dma_read(gpib_board_t *board, uint8_t *buffer,
> >        */
> >       usleep_range(10, 15);
> >       residue = fluke_get_dma_residue(e_priv->dma_channel, dma_cookie);
> > -     if (WARN_ON_ONCE(residue > length || residue < 0))
> > +     if (WARN_ON_ONCE(residue > length))
>
> No, this is incorrect, now we never notice is the call to
> fluke_get_dma_residue() has failed.  Please fix that bug instead (hint,
> Covertity is giving you a pointer to where something might be wrong, but
> this change is NOT how to fix it.)
I need a little guidance here.
My best guess to fix the bug would be to make fluke_get_dma_residue()
return an int instead of unsigned int or size_t. But theoretically the
maximum value of residue can be UINT_MAX, and casting it to int will
result in a negative number, which in turn will cause  the error check
condition to evaluate to true.
The best solution I see would be to make fluke_get_dma_residue() return
an int (-1 for error and 0 for success). Then pass the address of residue
reference to fluke_get_dma_residue() to be updated.
Am I on the right track ?

Also,I searched for the functions with names that match get_dma_residue
in the kernel source code and found that they return unsigned int. I also
noticed that no error checks have been made to check if get_dma_residue
was successful.
> thanks,
>
> greg k-h
Thanks,
Everest K.C.

Re: [PATCH V2] staging: gpib: Remove a dead condition in if statement

[Dan Carpenter]

On Wed, Oct 16, 2024 at 06:54:00AM -0600, Everest K.C. wrote:
> > > diff --git a/drivers/staging/gpib/eastwood/fluke_gpib.c b/drivers/staging/gpib/eastwood/fluke_gpib.c
> > > index f9f149db222d..51b4f9891a34 100644
> > > --- a/drivers/staging/gpib/eastwood/fluke_gpib.c
> > > +++ b/drivers/staging/gpib/eastwood/fluke_gpib.c
> > > @@ -644,7 +644,7 @@ static int fluke_dma_read(gpib_board_t *board, uint8_t *buffer,
> > >        */
> > >       usleep_range(10, 15);
> > >       residue = fluke_get_dma_residue(e_priv->dma_channel, dma_cookie);
> > > -     if (WARN_ON_ONCE(residue > length || residue < 0))
> > > +     if (WARN_ON_ONCE(residue > length))
> >
> > No, this is incorrect, now we never notice is the call to
> > fluke_get_dma_residue() has failed.  Please fix that bug instead (hint,
> > Covertity is giving you a pointer to where something might be wrong, but
> > this change is NOT how to fix it.)
> I need a little guidance here.
> My best guess to fix the bug would be to make fluke_get_dma_residue()
> return an int instead of unsigned int or size_t. But theoretically the
> maximum value of residue can be UINT_MAX, and casting it to int will
> result in a negative number, which in turn will cause  the error check
> condition to evaluate to true.
> The best solution I see would be to make fluke_get_dma_residue() return
> an int (-1 for error and 0 for success). Then pass the address of residue
> reference to fluke_get_dma_residue() to be updated.
> Am I on the right track ?

Functions shouldn't return -1 on error, they should return proper error codes.

> 
> Also,I searched for the functions with names that match get_dma_residue
> in the kernel source code and found that they return unsigned int. I also
> noticed that no error checks have been made to check if get_dma_residue
> was successful.

$ git grep get_dma_residue | grep static | grep -v gpib

arch/alpha/include/asm/dma.h:static __inline__ int get_dma_residue(unsigned int dmanr)
arch/arm/mach-footbridge/dma-isa.c:static int isa_get_dma_residue(unsigned int chan, dma_t *dma)
arch/m68k/include/asm/floppy.h:static int vdma_get_dma_residue(unsigned int dummy)
arch/mips/include/asm/dma.h:static __inline__ int get_dma_residue(unsigned int dmanr)
arch/mips/include/asm/mach-au1x00/au1000_dma.h:static inline int get_dma_residue(unsigned int dmanr)
arch/mips/include/asm/mach-generic/floppy.h:static inline int fd_get_dma_residue(void)
arch/mips/include/asm/mach-jazz/floppy.h:static inline int fd_get_dma_residue(void)
arch/parisc/include/asm/dma.h:static __inline__ int get_dma_residue(unsigned int dmanr)
arch/parisc/include/asm/floppy.h:static int vdma_get_dma_residue(unsigned int dummy)
arch/powerpc/include/asm/dma.h:static __inline__ int get_dma_residue(unsigned int dmanr)
arch/powerpc/include/asm/floppy.h:static int vdma_get_dma_residue(unsigned int dummy)
arch/sh/drivers/dma/dma-pvr2.c:static int pvr2_get_dma_residue(struct dma_channel *chan)
arch/sh/drivers/dma/dma-sh.c:static int sh_dmac_get_dma_residue(struct dma_channel *chan)
arch/sparc/include/asm/floppy_64.h:static unsigned int sun_get_dma_residue(void)
arch/sparc/include/asm/floppy_64.h:static unsigned int sun_pci_get_dma_residue(void)
arch/sparc/include/asm/parport_64.h:static inline unsigned int get_dma_residue(unsigned int dmanr)
arch/x86/include/asm/dma.h:static inline int get_dma_residue(unsigned int dmanr)
arch/x86/include/asm/floppy.h:static int vdma_get_dma_residue(unsigned int dummy)

Only the Sparc functions return unsigned int.  The rest return int.

The return value is going to be between 0 and priv->dma_buffer_size (0x7ff).
It's set in fluke_allocate_private().

Just make the return value an int.

regards,
dan carpenter

Re: [PATCH V2] staging: gpib: Remove a dead condition in if statement

[Everest K.C.]

On Wed, Oct 16, 2024 at 9:00 AM Dan Carpenter <dan.carpenter@linaro.org> wrote:
>
> On Wed, Oct 16, 2024 at 06:54:00AM -0600, Everest K.C. wrote:
> > > > diff --git a/drivers/staging/gpib/eastwood/fluke_gpib.c b/drivers/staging/gpib/eastwood/fluke_gpib.c
> > > > index f9f149db222d..51b4f9891a34 100644
> > > > --- a/drivers/staging/gpib/eastwood/fluke_gpib.c
> > > > +++ b/drivers/staging/gpib/eastwood/fluke_gpib.c
> > > > @@ -644,7 +644,7 @@ static int fluke_dma_read(gpib_board_t *board, uint8_t *buffer,
> > > >        */
> > > >       usleep_range(10, 15);
> > > >       residue = fluke_get_dma_residue(e_priv->dma_channel, dma_cookie);
> > > > -     if (WARN_ON_ONCE(residue > length || residue < 0))
> > > > +     if (WARN_ON_ONCE(residue > length))
> > >
> > > No, this is incorrect, now we never notice is the call to
> > > fluke_get_dma_residue() has failed.  Please fix that bug instead (hint,
> > > Covertity is giving you a pointer to where something might be wrong, but
> > > this change is NOT how to fix it.)
> > I need a little guidance here.
> > My best guess to fix the bug would be to make fluke_get_dma_residue()
> > return an int instead of unsigned int or size_t. But theoretically the
> > maximum value of residue can be UINT_MAX, and casting it to int will
> > result in a negative number, which in turn will cause  the error check
> > condition to evaluate to true.
> > The best solution I see would be to make fluke_get_dma_residue() return
> > an int (-1 for error and 0 for success). Then pass the address of residue
> > reference to fluke_get_dma_residue() to be updated.
> > Am I on the right track ?
>
> Functions shouldn't return -1 on error, they should return proper error codes.
Noted.
> >
> > Also,I searched for the functions with names that match get_dma_residue
> > in the kernel source code and found that they return unsigned int. I also
> > noticed that no error checks have been made to check if get_dma_residue
> > was successful.
>
> $ git grep get_dma_residue | grep static | grep -v gpib
>
> arch/alpha/include/asm/dma.h:static __inline__ int get_dma_residue(unsigned int dmanr)
> arch/arm/mach-footbridge/dma-isa.c:static int isa_get_dma_residue(unsigned int chan, dma_t *dma)
> arch/m68k/include/asm/floppy.h:static int vdma_get_dma_residue(unsigned int dummy)
> arch/mips/include/asm/dma.h:static __inline__ int get_dma_residue(unsigned int dmanr)
> arch/mips/include/asm/mach-au1x00/au1000_dma.h:static inline int get_dma_residue(unsigned int dmanr)
> arch/mips/include/asm/mach-generic/floppy.h:static inline int fd_get_dma_residue(void)
> arch/mips/include/asm/mach-jazz/floppy.h:static inline int fd_get_dma_residue(void)
> arch/parisc/include/asm/dma.h:static __inline__ int get_dma_residue(unsigned int dmanr)
> arch/parisc/include/asm/floppy.h:static int vdma_get_dma_residue(unsigned int dummy)
> arch/powerpc/include/asm/dma.h:static __inline__ int get_dma_residue(unsigned int dmanr)
> arch/powerpc/include/asm/floppy.h:static int vdma_get_dma_residue(unsigned int dummy)
> arch/sh/drivers/dma/dma-pvr2.c:static int pvr2_get_dma_residue(struct dma_channel *chan)
> arch/sh/drivers/dma/dma-sh.c:static int sh_dmac_get_dma_residue(struct dma_channel *chan)
> arch/sparc/include/asm/floppy_64.h:static unsigned int sun_get_dma_residue(void)
> arch/sparc/include/asm/floppy_64.h:static unsigned int sun_pci_get_dma_residue(void)
> arch/sparc/include/asm/parport_64.h:static inline unsigned int get_dma_residue(unsigned int dmanr)
> arch/x86/include/asm/dma.h:static inline int get_dma_residue(unsigned int dmanr)
> arch/x86/include/asm/floppy.h:static int vdma_get_dma_residue(unsigned int dummy)
>
> Only the Sparc functions return unsigned int.  The rest return int.
Why is it so ? Are there any resources I could go through to
understand it better?
> The return value is going to be between 0 and priv->dma_buffer_size (0x7ff).
> It's set in fluke_allocate_private().
Thank you for pointing it out.
> Just make the return value an int.
Noted.

> regards,
> dan carpenter
Thanks,
Everest K.C.

Re: [PATCH V2] staging: gpib: Remove a dead condition in if statement

[Dan Carpenter]

On Wed, Oct 16, 2024 at 08:47:21PM -0600, Everest K.C. wrote:
> > $ git grep get_dma_residue | grep static | grep -v gpib
> >
> > arch/alpha/include/asm/dma.h:static __inline__ int get_dma_residue(unsigned int dmanr)
> > arch/arm/mach-footbridge/dma-isa.c:static int isa_get_dma_residue(unsigned int chan, dma_t *dma)
> > arch/m68k/include/asm/floppy.h:static int vdma_get_dma_residue(unsigned int dummy)
> > arch/mips/include/asm/dma.h:static __inline__ int get_dma_residue(unsigned int dmanr)
> > arch/mips/include/asm/mach-au1x00/au1000_dma.h:static inline int get_dma_residue(unsigned int dmanr)
> > arch/mips/include/asm/mach-generic/floppy.h:static inline int fd_get_dma_residue(void)
> > arch/mips/include/asm/mach-jazz/floppy.h:static inline int fd_get_dma_residue(void)
> > arch/parisc/include/asm/dma.h:static __inline__ int get_dma_residue(unsigned int dmanr)
> > arch/parisc/include/asm/floppy.h:static int vdma_get_dma_residue(unsigned int dummy)
> > arch/powerpc/include/asm/dma.h:static __inline__ int get_dma_residue(unsigned int dmanr)
> > arch/powerpc/include/asm/floppy.h:static int vdma_get_dma_residue(unsigned int dummy)
> > arch/sh/drivers/dma/dma-pvr2.c:static int pvr2_get_dma_residue(struct dma_channel *chan)
> > arch/sh/drivers/dma/dma-sh.c:static int sh_dmac_get_dma_residue(struct dma_channel *chan)
> > arch/sparc/include/asm/floppy_64.h:static unsigned int sun_get_dma_residue(void)
> > arch/sparc/include/asm/floppy_64.h:static unsigned int sun_pci_get_dma_residue(void)
> > arch/sparc/include/asm/parport_64.h:static inline unsigned int get_dma_residue(unsigned int dmanr)
> > arch/x86/include/asm/dma.h:static inline int get_dma_residue(unsigned int dmanr)
> > arch/x86/include/asm/floppy.h:static int vdma_get_dma_residue(unsigned int dummy)
> >
> > Only the Sparc functions return unsigned int.  The rest return int.
> Why is it so ? Are there any resources I could go through to
> understand it better?

There isn't a reason for it.  Programmers make millions of little choices and
some don't matter so it's just a roll of the dice which way things go.

regards,
dan carpenter

Re: [PATCH V2] staging: gpib: Remove a dead condition in if statement

[Greg KH]

On Wed, Oct 16, 2024 at 06:54:00AM -0600, Everest K.C. wrote:
> On Wed, Oct 16, 2024 at 2:04 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Wed, Oct 16, 2024 at 01:53:18AM -0600, Everest K.C. wrote:
> > > The variable `residue` is an unsigned int, also the function
> > > `fluke_get_dma_residue` returns an unsigned int. The value of
> > > an unsigned int can only be 0 at minimum.
> > > The less-than-zero comparison can never be true.
> > > Fix it by removing the dead condition in the if statement.
> > >
> > > This issue was reported by Coverity Scan.
> > > Report:
> > > CID 1600782: (#1 of 1): Macro compares unsigned to 0 (NO_EFFECT)
> > > unsigned_compare: This less-than-zero comparison of an unsigned value
> > > is never true. residue < 0U.
> > >
> > > Signed-off-by: Everest K.C. <everestkc@everestkc.com.np>
> > > ---
> > > V1 -> V2: - Fixed typo of comparison in changelog
> > >           - Removed Fixes tag
> > >
> > >  drivers/staging/gpib/eastwood/fluke_gpib.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/staging/gpib/eastwood/fluke_gpib.c b/drivers/staging/gpib/eastwood/fluke_gpib.c
> > > index f9f149db222d..51b4f9891a34 100644
> > > --- a/drivers/staging/gpib/eastwood/fluke_gpib.c
> > > +++ b/drivers/staging/gpib/eastwood/fluke_gpib.c
> > > @@ -644,7 +644,7 @@ static int fluke_dma_read(gpib_board_t *board, uint8_t *buffer,
> > >        */
> > >       usleep_range(10, 15);
> > >       residue = fluke_get_dma_residue(e_priv->dma_channel, dma_cookie);
> > > -     if (WARN_ON_ONCE(residue > length || residue < 0))
> > > +     if (WARN_ON_ONCE(residue > length))
> >
> > No, this is incorrect, now we never notice is the call to
> > fluke_get_dma_residue() has failed.  Please fix that bug instead (hint,
> > Covertity is giving you a pointer to where something might be wrong, but
> > this change is NOT how to fix it.)
> I need a little guidance here.
> My best guess to fix the bug would be to make fluke_get_dma_residue()
> return an int instead of unsigned int or size_t. But theoretically the
> maximum value of residue can be UINT_MAX, and casting it to int will
> result in a negative number, which in turn will cause  the error check
> condition to evaluate to true.

Look at the code to see what it does.

> The best solution I see would be to make fluke_get_dma_residue() return
> an int (-1 for error and 0 for success). Then pass the address of residue
> reference to fluke_get_dma_residue() to be updated.
> Am I on the right track ?

Close, yes.  "-1" is not a valid error, so that needs to be fixed at the
least here, as it's obviously not returning an error that gets caught
today :)

good luck!

greg k-h

Re: [PATCH V2] staging: gpib: Remove a dead condition in if statement

[Everest K.C.]

On Wed, Oct 16, 2024 at 9:12 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Wed, Oct 16, 2024 at 06:54:00AM -0600, Everest K.C. wrote:
> > On Wed, Oct 16, 2024 at 2:04 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Wed, Oct 16, 2024 at 01:53:18AM -0600, Everest K.C. wrote:
> > > > The variable `residue` is an unsigned int, also the function
> > > > `fluke_get_dma_residue` returns an unsigned int. The value of
> > > > an unsigned int can only be 0 at minimum.
> > > > The less-than-zero comparison can never be true.
> > > > Fix it by removing the dead condition in the if statement.
> > > >
> > > > This issue was reported by Coverity Scan.
> > > > Report:
> > > > CID 1600782: (#1 of 1): Macro compares unsigned to 0 (NO_EFFECT)
> > > > unsigned_compare: This less-than-zero comparison of an unsigned value
> > > > is never true. residue < 0U.
> > > >
> > > > Signed-off-by: Everest K.C. <everestkc@everestkc.com.np>
> > > > ---
> > > > V1 -> V2: - Fixed typo of comparison in changelog
> > > >           - Removed Fixes tag
> > > >
> > > >  drivers/staging/gpib/eastwood/fluke_gpib.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/staging/gpib/eastwood/fluke_gpib.c b/drivers/staging/gpib/eastwood/fluke_gpib.c
> > > > index f9f149db222d..51b4f9891a34 100644
> > > > --- a/drivers/staging/gpib/eastwood/fluke_gpib.c
> > > > +++ b/drivers/staging/gpib/eastwood/fluke_gpib.c
> > > > @@ -644,7 +644,7 @@ static int fluke_dma_read(gpib_board_t *board, uint8_t *buffer,
> > > >        */
> > > >       usleep_range(10, 15);
> > > >       residue = fluke_get_dma_residue(e_priv->dma_channel, dma_cookie);
> > > > -     if (WARN_ON_ONCE(residue > length || residue < 0))
> > > > +     if (WARN_ON_ONCE(residue > length))
> > >
> > > No, this is incorrect, now we never notice is the call to
> > > fluke_get_dma_residue() has failed.  Please fix that bug instead (hint,
> > > Covertity is giving you a pointer to where something might be wrong, but
> > > this change is NOT how to fix it.)
> > I need a little guidance here.
> > My best guess to fix the bug would be to make fluke_get_dma_residue()
> > return an int instead of unsigned int or size_t. But theoretically the
> > maximum value of residue can be UINT_MAX, and casting it to int will
> > result in a negative number, which in turn will cause  the error check
> > condition to evaluate to true.
>
> Look at the code to see what it does.
>
> > The best solution I see would be to make fluke_get_dma_residue() return
> > an int (-1 for error and 0 for success). Then pass the address of residue
> > reference to fluke_get_dma_residue() to be updated.
> > Am I on the right track ?
>
> Close, yes.  "-1" is not a valid error, so that needs to be fixed at the
> least here, as it's obviously not returning an error that gets caught
> today :)
Noted. Thank you very much.
I have a question though. Since, the file I had previously fixed (which
was incorrect) and the file I now need to fix are different. Should I create
a new patch that would be of version 1, or should I send a V2 ?
I went through the "Submitting patches" documentation but it does not
clearly explain whether I need to send a new patch or the revision ?
> good luck!
>
> greg k-h
Thanks,
Everest K.C.

Re: [PATCH V2] staging: gpib: Remove a dead condition in if statement

[Everest K.C.]

On Wed, Oct 16, 2024 at 8:50 PM Everest K.C. <everestkc@everestkc.com.np> wrote:
>
> On Wed, Oct 16, 2024 at 9:12 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Wed, Oct 16, 2024 at 06:54:00AM -0600, Everest K.C. wrote:
> > > On Wed, Oct 16, 2024 at 2:04 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > >
> > > > On Wed, Oct 16, 2024 at 01:53:18AM -0600, Everest K.C. wrote:
> > > > > The variable `residue` is an unsigned int, also the function
> > > > > `fluke_get_dma_residue` returns an unsigned int. The value of
> > > > > an unsigned int can only be 0 at minimum.
> > > > > The less-than-zero comparison can never be true.
> > > > > Fix it by removing the dead condition in the if statement.
> > > > >
> > > > > This issue was reported by Coverity Scan.
> > > > > Report:
> > > > > CID 1600782: (#1 of 1): Macro compares unsigned to 0 (NO_EFFECT)
> > > > > unsigned_compare: This less-than-zero comparison of an unsigned value
> > > > > is never true. residue < 0U.
> > > > >
> > > > > Signed-off-by: Everest K.C. <everestkc@everestkc.com.np>
> > > > > ---
> > > > > V1 -> V2: - Fixed typo of comparison in changelog
> > > > >           - Removed Fixes tag
> > > > >
> > > > >  drivers/staging/gpib/eastwood/fluke_gpib.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/staging/gpib/eastwood/fluke_gpib.c b/drivers/staging/gpib/eastwood/fluke_gpib.c
> > > > > index f9f149db222d..51b4f9891a34 100644
> > > > > --- a/drivers/staging/gpib/eastwood/fluke_gpib.c
> > > > > +++ b/drivers/staging/gpib/eastwood/fluke_gpib.c
> > > > > @@ -644,7 +644,7 @@ static int fluke_dma_read(gpib_board_t *board, uint8_t *buffer,
> > > > >        */
> > > > >       usleep_range(10, 15);
> > > > >       residue = fluke_get_dma_residue(e_priv->dma_channel, dma_cookie);
> > > > > -     if (WARN_ON_ONCE(residue > length || residue < 0))
> > > > > +     if (WARN_ON_ONCE(residue > length))
> > > >
> > > > No, this is incorrect, now we never notice is the call to
> > > > fluke_get_dma_residue() has failed.  Please fix that bug instead (hint,
> > > > Covertity is giving you a pointer to where something might be wrong, but
> > > > this change is NOT how to fix it.)
> > > I need a little guidance here.
> > > My best guess to fix the bug would be to make fluke_get_dma_residue()
> > > return an int instead of unsigned int or size_t. But theoretically the
> > > maximum value of residue can be UINT_MAX, and casting it to int will
> > > result in a negative number, which in turn will cause  the error check
> > > condition to evaluate to true.
> >
> > Look at the code to see what it does.
> >
> > > The best solution I see would be to make fluke_get_dma_residue() return
> > > an int (-1 for error and 0 for success). Then pass the address of residue
> > > reference to fluke_get_dma_residue() to be updated.
> > > Am I on the right track ?
> >
> > Close, yes.  "-1" is not a valid error, so that needs to be fixed at the
> > least here, as it's obviously not returning an error that gets caught
> > today :)
> Noted. Thank you very much.
> I have a question though. Since, the file I had previously fixed (which
> was incorrect) and the file I now need to fix are different. Should I create
> a new patch that would be of version 1, or should I send a V2 ?
Oops, it's in the same file but my question still stands, should I send a new
patch or V2 revision ?
> I went through the "Submitting patches" documentation but it does not
> clearly explain whether I need to send a new patch or the revision ?
> > good luck!
> >
> > greg k-h
> Thanks,
> Everest K.C.

Re: [PATCH V2] staging: gpib: Remove a dead condition in if statement

[Greg KH]

On Wed, Oct 16, 2024 at 10:42:03PM -0600, Everest K.C. wrote:
> On Wed, Oct 16, 2024 at 8:50 PM Everest K.C. <everestkc@everestkc.com.np> wrote:
> >
> > On Wed, Oct 16, 2024 at 9:12 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Wed, Oct 16, 2024 at 06:54:00AM -0600, Everest K.C. wrote:
> > > > On Wed, Oct 16, 2024 at 2:04 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > >
> > > > > On Wed, Oct 16, 2024 at 01:53:18AM -0600, Everest K.C. wrote:
> > > > > > The variable `residue` is an unsigned int, also the function
> > > > > > `fluke_get_dma_residue` returns an unsigned int. The value of
> > > > > > an unsigned int can only be 0 at minimum.
> > > > > > The less-than-zero comparison can never be true.
> > > > > > Fix it by removing the dead condition in the if statement.
> > > > > >
> > > > > > This issue was reported by Coverity Scan.
> > > > > > Report:
> > > > > > CID 1600782: (#1 of 1): Macro compares unsigned to 0 (NO_EFFECT)
> > > > > > unsigned_compare: This less-than-zero comparison of an unsigned value
> > > > > > is never true. residue < 0U.
> > > > > >
> > > > > > Signed-off-by: Everest K.C. <everestkc@everestkc.com.np>
> > > > > > ---
> > > > > > V1 -> V2: - Fixed typo of comparison in changelog
> > > > > >           - Removed Fixes tag
> > > > > >
> > > > > >  drivers/staging/gpib/eastwood/fluke_gpib.c | 2 +-
> > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/staging/gpib/eastwood/fluke_gpib.c b/drivers/staging/gpib/eastwood/fluke_gpib.c
> > > > > > index f9f149db222d..51b4f9891a34 100644
> > > > > > --- a/drivers/staging/gpib/eastwood/fluke_gpib.c
> > > > > > +++ b/drivers/staging/gpib/eastwood/fluke_gpib.c
> > > > > > @@ -644,7 +644,7 @@ static int fluke_dma_read(gpib_board_t *board, uint8_t *buffer,
> > > > > >        */
> > > > > >       usleep_range(10, 15);
> > > > > >       residue = fluke_get_dma_residue(e_priv->dma_channel, dma_cookie);
> > > > > > -     if (WARN_ON_ONCE(residue > length || residue < 0))
> > > > > > +     if (WARN_ON_ONCE(residue > length))
> > > > >
> > > > > No, this is incorrect, now we never notice is the call to
> > > > > fluke_get_dma_residue() has failed.  Please fix that bug instead (hint,
> > > > > Covertity is giving you a pointer to where something might be wrong, but
> > > > > this change is NOT how to fix it.)
> > > > I need a little guidance here.
> > > > My best guess to fix the bug would be to make fluke_get_dma_residue()
> > > > return an int instead of unsigned int or size_t. But theoretically the
> > > > maximum value of residue can be UINT_MAX, and casting it to int will
> > > > result in a negative number, which in turn will cause  the error check
> > > > condition to evaluate to true.
> > >
> > > Look at the code to see what it does.
> > >
> > > > The best solution I see would be to make fluke_get_dma_residue() return
> > > > an int (-1 for error and 0 for success). Then pass the address of residue
> > > > reference to fluke_get_dma_residue() to be updated.
> > > > Am I on the right track ?
> > >
> > > Close, yes.  "-1" is not a valid error, so that needs to be fixed at the
> > > least here, as it's obviously not returning an error that gets caught
> > > today :)
> > Noted. Thank you very much.
> > I have a question though. Since, the file I had previously fixed (which
> > was incorrect) and the file I now need to fix are different. Should I create
> > a new patch that would be of version 1, or should I send a V2 ?
> Oops, it's in the same file but my question still stands, should I send a new
> patch or V2 revision ?

Probably a new patch if it has a totally different subject line.

thanks,

greg k-h