[PATCH 26/28] staging: most: rearrange function aim_write

[Dan Carpenter]

On Wed, Nov 18, 2015 at 01:43:50PM +0100, Christian Gromm wrote:
> This patch rearranges the code of function aim_write() of module aim-cdev.
> It is needed to remove the error lable and make the code straighter.
> 

I like error labels.

> Signed-off-by: Christian Gromm <christian.gromm at microchip.com>
> ---
>  drivers/staging/most/aim-cdev/cdev.c |   22 ++++++----------------
>  1 file changed, 6 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/staging/most/aim-cdev/cdev.c b/drivers/staging/most/aim-cdev/cdev.c
> index 0ee2f08..e5ceb82 100644
> --- a/drivers/staging/most/aim-cdev/cdev.c
> +++ b/drivers/staging/most/aim-cdev/cdev.c
> @@ -183,7 +183,6 @@ static int aim_close(struct inode *inode, struct file *filp)
>  static ssize_t aim_write(struct file *filp, const char __user *buf,
>  			 size_t count, loff_t *offset)
>  {
> -	int ret, err;

Keep ret.  Get rid of err.

>  	size_t actual_len;
>  	size_t max_len;
>  	ssize_t retval;

Get rid of retval.

The first part of this function uses direct returns and that's fine.


> @@ -202,8 +201,8 @@ static ssize_t aim_write(struct file *filp, const char __user *buf,
>  	}
>  
>  	if (unlikely(!c->dev)) {
> -		err = -EPIPE;
> -		goto error;
> +		mutex_unlock(&c->io_mutex);
> +		return -EPIPE;

	if (!c->dev) {
		ret = -EPIPE;
		goto unlock;
	}


>  	}
>  
>  	max_len = c->cfg->buffer_size;
> @@ -212,23 +211,14 @@ static ssize_t aim_write(struct file *filp, const char __user *buf,
>  
>  	retval = copy_from_user(mbo->virt_address, buf, mbo->buffer_length);
>  	if (retval) {
> -		err = -EIO;
> -		goto error;
> +		most_put_mbo(mbo);
> +		mutex_unlock(&c->io_mutex);
> +		return -EIO;
>  	}

The error code should be -EFAULT instead of -EIO.  It's better to not
assign the "retval = " because we don't care about the actual value, we
only care if it's non-zero.  Also it's a few characters less to write it
the normal way.

	if (copy_from_user(mbo->virt_address, buf, mbo->buffer_length) {
		ret = -EFAULT;
		goto put_mbo;
	}

>  
> -	ret = most_submit_mbo(mbo);
> -	if (ret) {
> -		pr_info("submitting MBO to core failed\n");
> -		err = ret;
> -		goto error;
> -	}
> +	most_submit_mbo(mbo);

Why remove the error handling?  Anyway,

	ret = most_submit_mbo(mbo);
	if (ret)
		goto put_mbo;

>  	mutex_unlock(&c->io_mutex);
>  	return actual_len - retval;

retval is always zero here.

> -error:
> -	if (mbo)
> -		most_put_mbo(mbo);
> -	mutex_unlock(&c->io_mutex);
> -	return err;

	mutex_unlock(&c->io_mutex);

	return actual_len;

put_mbo:
	most_put_mbo(mbo);
unlock:
	mutex_unlock(&c->io_mutex);

	return ret;


regards,
dan carpenter