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@xxxxxxxxxxxx>
> > > ---
> > > 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@xxxxxxxxxxxxxxxx
> [2]
> https://lore.kernel.org/r/20241017220740.30370-1-everestkc@xxxxxxxxxxxxxxxx
> >
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
