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@xxxxxxxxxxxxx>
> ---
> 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
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
