[PATCH v3] staging: gpib: Change return type of fluke_get_dma_residue

[PATCH v3] staging: gpib: Change return type of fluke_get_dma_residue

[Kees Bakker]

The function fluke_get_dma_residue returns an error as a negative value.
So the return type must not be unsigned.

This was detected by Coverity, CID 1600782

Signed-off-by: Kees Bakker <kees@ijzerbout.nl>
---
 v1 -> v2: change type of `residue` var; add note about Coverity CID in commit message
 v2 -> v3: add version in the subject (sorry Greg)

 drivers/staging/gpib/eastwood/fluke_gpib.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/gpib/eastwood/fluke_gpib.c b/drivers/staging/gpib/eastwood/fluke_gpib.c
index f9f149db222d..3843e986f104 100644
--- a/drivers/staging/gpib/eastwood/fluke_gpib.c
+++ b/drivers/staging/gpib/eastwood/fluke_gpib.c
@@ -536,7 +536,7 @@ static int fluke_accel_write(gpib_board_t *board, uint8_t *buffer, size_t length
 	return 0;
 }
 
-static unsigned int fluke_get_dma_residue(struct dma_chan *chan, dma_cookie_t cookie)
+static int fluke_get_dma_residue(struct dma_chan *chan, dma_cookie_t cookie)
 {
 	struct dma_tx_state state;
 	int result;
@@ -549,7 +549,7 @@ static unsigned int fluke_get_dma_residue(struct dma_chan *chan, dma_cookie_t co
 	dmaengine_tx_status(chan, cookie, &state);
 	// hardware doesn't support resume, so dont call this
 	// method unless the dma transfer is done.
-	return state.residue;
+	return (int)state.residue;
 }
 
 static int fluke_dma_read(gpib_board_t *board, uint8_t *buffer,
@@ -559,7 +559,7 @@ static int fluke_dma_read(gpib_board_t *board, uint8_t *buffer,
 	struct nec7210_priv *nec_priv = &e_priv->nec7210_priv;
 	int retval = 0;
 	unsigned long flags;
-	unsigned int residue;
+	int residue;
 	dma_addr_t bus_address;
 	struct dma_async_tx_descriptor *tx_desc;
 	dma_cookie_t dma_cookie;
-- 
2.47.0

Re: [PATCH v3] staging: gpib: Change return type of fluke_get_dma_residue

[Greg KH]

On Tue, Oct 15, 2024 at 10:33:30PM +0200, Kees Bakker wrote:
> The function fluke_get_dma_residue returns an error as a negative value.
> So the return type must not be unsigned.
> 
> This was detected by Coverity, CID 1600782
> 
> Signed-off-by: Kees Bakker <kees@ijzerbout.nl>
> ---
>  v1 -> v2: change type of `residue` var; add note about Coverity CID in commit message
>  v2 -> v3: add version in the subject (sorry Greg)
> 
>  drivers/staging/gpib/eastwood/fluke_gpib.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/gpib/eastwood/fluke_gpib.c b/drivers/staging/gpib/eastwood/fluke_gpib.c
> index f9f149db222d..3843e986f104 100644
> --- a/drivers/staging/gpib/eastwood/fluke_gpib.c
> +++ b/drivers/staging/gpib/eastwood/fluke_gpib.c
> @@ -536,7 +536,7 @@ static int fluke_accel_write(gpib_board_t *board, uint8_t *buffer, size_t length
>  	return 0;
>  }
>  
> -static unsigned int fluke_get_dma_residue(struct dma_chan *chan, dma_cookie_t cookie)
> +static int fluke_get_dma_residue(struct dma_chan *chan, dma_cookie_t cookie)
>  {
>  	struct dma_tx_state state;
>  	int result;
> @@ -549,7 +549,7 @@ static unsigned int fluke_get_dma_residue(struct dma_chan *chan, dma_cookie_t co
>  	dmaengine_tx_status(chan, cookie, &state);
>  	// hardware doesn't support resume, so dont call this
>  	// method unless the dma transfer is done.
> -	return state.residue;
> +	return (int)state.residue;

Shouldn't you be checking the result of dmaengine_tx_status instead?
residue is a u32 and is NOT an error here so I think the unsigned value
here is correct as that's not going to give you what you expect it to
give you.

thanks,

greg k-h

Re: [PATCH v3] staging: gpib: Change return type of fluke_get_dma_residue

[Kees Bakker]

Op 07-11-2024 om 09:25 schreef Greg KH:
> On Tue, Oct 15, 2024 at 10:33:30PM +0200, Kees Bakker wrote:
>> The function fluke_get_dma_residue returns an error as a negative value.
>> So the return type must not be unsigned.
>>
>> This was detected by Coverity, CID 1600782
>>
>> Signed-off-by: Kees Bakker <kees@ijzerbout.nl>
>> ---
>>   v1 -> v2: change type of `residue` var; add note about Coverity CID in commit message
>>   v2 -> v3: add version in the subject (sorry Greg)
>>
>>   drivers/staging/gpib/eastwood/fluke_gpib.c | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/staging/gpib/eastwood/fluke_gpib.c b/drivers/staging/gpib/eastwood/fluke_gpib.c
>> index f9f149db222d..3843e986f104 100644
>> --- a/drivers/staging/gpib/eastwood/fluke_gpib.c
>> +++ b/drivers/staging/gpib/eastwood/fluke_gpib.c
>> @@ -536,7 +536,7 @@ static int fluke_accel_write(gpib_board_t *board, uint8_t *buffer, size_t length
>>   	return 0;
>>   }
>>   
>> -static unsigned int fluke_get_dma_residue(struct dma_chan *chan, dma_cookie_t cookie)
>> +static int fluke_get_dma_residue(struct dma_chan *chan, dma_cookie_t cookie)
>>   {
>>   	struct dma_tx_state state;
>>   	int result;
>> @@ -549,7 +549,7 @@ static unsigned int fluke_get_dma_residue(struct dma_chan *chan, dma_cookie_t co
>>   	dmaengine_tx_status(chan, cookie, &state);
>>   	// hardware doesn't support resume, so dont call this
>>   	// method unless the dma transfer is done.
>> -	return state.residue;
>> +	return (int)state.residue;
> Shouldn't you be checking the result of dmaengine_tx_status instead?
> residue is a u32 and is NOT an error here so I think the unsigned value
> here is correct as that's not going to give you what you expect it to
> give you.
I'm not the author, so I leave that up to David.

However, in the mean time you have already signed off on a similar
change in commit 14bcf831f0d [1], so my patch won't apply anymore.

Note that there is a function fmh_gpib_get_dma_residue which is
identical to fluke_get_dma_residue.
That one was changed in the same way, commit 039beaa5ace1 [2]

[1] 
https://lore.kernel.org/r/20241017092511.17621-1-everestkc@everestkc.com.np
[2] 
https://lore.kernel.org/r/20241017220740.30370-1-everestkc@everestkc.com.np
>
> thanks,
>
> greg k-h

Re: [PATCH v3] staging: gpib: Change return type of fluke_get_dma_residue

[Dan Carpenter]

On Fri, Nov 08, 2024 at 08:37:59PM +0100, Kees Bakker wrote:
> Op 07-11-2024 om 09:25 schreef Greg KH:
> > On Tue, Oct 15, 2024 at 10:33:30PM +0200, Kees Bakker wrote:
> > > The function fluke_get_dma_residue returns an error as a negative value.
> > > So the return type must not be unsigned.
> > > 
> > > This was detected by Coverity, CID 1600782
> > > 
> > > Signed-off-by: Kees Bakker <kees@ijzerbout.nl>
> > > ---
> > >   v1 -> v2: change type of `residue` var; add note about Coverity CID in commit message
> > >   v2 -> v3: add version in the subject (sorry Greg)
> > > 
> > >   drivers/staging/gpib/eastwood/fluke_gpib.c | 6 +++---
> > >   1 file changed, 3 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/staging/gpib/eastwood/fluke_gpib.c b/drivers/staging/gpib/eastwood/fluke_gpib.c
> > > index f9f149db222d..3843e986f104 100644
> > > --- a/drivers/staging/gpib/eastwood/fluke_gpib.c
> > > +++ b/drivers/staging/gpib/eastwood/fluke_gpib.c
> > > @@ -536,7 +536,7 @@ static int fluke_accel_write(gpib_board_t *board, uint8_t *buffer, size_t length
> > >   	return 0;
> > >   }
> > > -static unsigned int fluke_get_dma_residue(struct dma_chan *chan, dma_cookie_t cookie)
> > > +static int fluke_get_dma_residue(struct dma_chan *chan, dma_cookie_t cookie)
> > >   {
> > >   	struct dma_tx_state state;
> > >   	int result;
> > > @@ -549,7 +549,7 @@ static unsigned int fluke_get_dma_residue(struct dma_chan *chan, dma_cookie_t co
> > >   	dmaengine_tx_status(chan, cookie, &state);
> > >   	// hardware doesn't support resume, so dont call this
> > >   	// method unless the dma transfer is done.
> > > -	return state.residue;
> > > +	return (int)state.residue;
> > Shouldn't you be checking the result of dmaengine_tx_status instead?
> > residue is a u32 and is NOT an error here so I think the unsigned value
> > here is correct as that's not going to give you what you expect it to
> > give you.
> I'm not the author, so I leave that up to David.
> 
> However, in the mean time you have already signed off on a similar
> change in commit 14bcf831f0d [1], so my patch won't apply anymore.
> 
> Note that there is a function fmh_gpib_get_dma_residue which is
> identical to fluke_get_dma_residue.
> That one was changed in the same way, commit 039beaa5ace1 [2]
> 
> [1]
> https://lore.kernel.org/r/20241017092511.17621-1-everestkc@everestkc.com.np
> [2]
> https://lore.kernel.org/r/20241017220740.30370-1-everestkc@everestkc.com.np
> > 

state.residue is going to be a very small positive number, so adding the cast
is unnecessary.  The rule is that we should pointless casts.

Checking the result from dmaengine_tx_status() is probably a good idea just
based on the fact that it's Greg suggesting it.  :P

regards,
dan carpenter

Re: [PATCH v3] staging: gpib: Change return type of fluke_get_dma_residue

[Dan Carpenter]

On Mon, Nov 11, 2024 at 01:59:35PM +0300, Dan Carpenter wrote:
> The rule is that we should pointless casts.

This sentence no verb.  The verb was supposed to be "avoid".

regards,
dan carpenter