On Sat, Apr 27, 2019 at 11:14:34AM +0200, Nicholas Mc Guire wrote:
> weit_for_completion_interruptible returns in (0 on completion and 
   ^
  wait_for_completion_interruptible

> -ERESTARTSYS on interruption) - so use an int not long for API conformance
> and simplify the logic here a bit: need not check explicitly for == 0 as
> this is either -ERESTARTSYS or 0.
> 
> Signed-off-by: Nicholas Mc Guire <hof...@osadl.org>
> ---
> 
> Problem located with experimental API conformance checking cocci script
> 
> V2: kbuild reported a missing closing comment - seems that I managed
>     send out the the initial version before compile testing/checkpatching
>     sorry for the noise.
> 
> Not sure if making such point-wise fixes makes much sense - this driver has
> a number of issues both style-wise and API compliance wise.
> 
> Note that kpc_dma_transfer() returns int not long - currently rv (long) is
> being returned in most places (some places do return int) - so the return
> handling here is a bit inconsistent.
> 
> Patch was compile-tested with: x86_64_defconfig + KPC2000=y, KPC2000_DMA=y
> (with a number of unrelated sparse warnings about non-declared symbols, and
>  smatch warnings about overflowing constants as well as coccicheck warning
>  about usless casting)
> 
> Patch is against 5.1-rc6 (localversion-next is next-20190426)
> 
>  drivers/staging/kpc2000/kpc_dma/fileops.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/staging/kpc2000/kpc_dma/fileops.c 
> b/drivers/staging/kpc2000/kpc_dma/fileops.c
> index 5741d2b..66f0d5a 100644
> --- a/drivers/staging/kpc2000/kpc_dma/fileops.c
> +++ b/drivers/staging/kpc2000/kpc_dma/fileops.c
> @@ -38,6 +38,7 @@ int  kpc_dma_transfer(struct dev_private_data *priv, struct 
> kiocb *kcb, unsigned
>  {
>       unsigned int i = 0;
>       long rv = 0;
> +     int ret = 0;

This assignment is never used.  It just turns static checking for
uninitialized variable bugs.

>       struct kpc_dma_device *ldev;
>       struct aio_cb_data *acd;
>       DECLARE_COMPLETION_ONSTACK(done);
> @@ -180,16 +181,17 @@ int  kpc_dma_transfer(struct dev_private_data *priv, 
> struct kiocb *kcb, unsigned
>       
>       // If this is a synchronous kiocb, we need to put the calling process 
> to sleep until the transfer is complete
>       if (kcb == NULL || is_sync_kiocb(kcb)){
> -             rv = wait_for_completion_interruptible(&done);
> -             // If the user aborted (rv == -ERESTARTSYS), we're no longer 
> responsible for cleaning up the acd
> -             if (rv == -ERESTARTSYS){
> +             ret = wait_for_completion_interruptible(&done);
> +             /* If the user aborted (ret == -ERESTARTSYS), we're
> +              * no longer responsible for cleaning up the acd
> +              */
> +             if (ret) {
>                       acd->cpl = NULL;
> -             }
> -             if (rv == 0){
> -                     rv = acd->len;
> +             } else {
> +                     ret = acd->len;

"acd->len" is a size_t (unsigned long) so probably we should just
change the type of kpc_dma_transfer to ssize_t actually.

regards,
dan carpenter

_______________________________________________
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Reply via email to