Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 30 Apr 2019 12:52:55 +0300
From:   Dan Carpenter <dan.carpenter@oracle.com>
To:     Nicholas Mc Guire <hofrat@osadl.org>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        devel@driverdev.osuosl.org,
        Matt Sickler <Matt.Sickler@daktronics.com>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH RFC] staging: kpc2000: use int for
 wait_for_completion_interruptible
Message-ID: <20190430095254.GC2269@kadam>
References: <1556332725-7391-1-git-send-email-hofrat@osadl.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1556332725-7391-1-git-send-email-hofrat@osadl.org>
User-Agent: Mutt/1.9.4 (2018-02-28)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Sat, Apr 27, 2019 at 04:38:45AM +0200, Nicholas Mc Guire wrote:
> weit_for_completion_interruptible returns in (0 on completion and 
> -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 <hofrat@osadl.org>
> ---
> 
> Problem located with experimental API conformance checking cocci script
> 
> 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)

The patch must have got corrupted or something.  Or maybe the code was
ifdeffed out.  It won't compile.

> 
> 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;
>  	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
> +		 *

This comment is never closed off with a "*/".

> +		if (ret){
                       ^^
Missing space.  Please use checkpatch.pl.

>  			acd->cpl = NULL;
> -		}
> -		if (rv == 0){
> -			rv = acd->len;
> +		} else {
> +			ret = acd->len;
>  			kfree(acd);
>  		}
> -		return rv;
> +		return ret

I don't really see an advantage with introducing a "ret" variable
instead of using "rv".

regards,
dan carpenter