[patch] bus: sunxi-rsb: unlock on error in sunxi_rsb_read()

[patch] bus: sunxi-rsb: unlock on error in sunxi_rsb_read()

[Dan Carpenter]

Don't forget to unlock before returning an error code.

Fixes: d787dcdb9c8f ('bus: sunxi-rsb: Add driver for Allwinner Reduced Serial Bus')
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/drivers/bus/sunxi-rsb.c b/drivers/bus/sunxi-rsb.c
index 846bc29c..0cfcb39 100644
--- a/drivers/bus/sunxi-rsb.c
+++ b/drivers/bus/sunxi-rsb.c
@@ -342,13 +342,13 @@ static int sunxi_rsb_read(struct sunxi_rsb *rsb, u8 rtaddr, u8 addr,
 
 	ret = _sunxi_rsb_run_xfer(rsb);
 	if (ret)
-		goto out;
+		goto unlock;
 
 	*buf = readl(rsb->regs + RSB_DATA);
 
+unlock:
 	mutex_unlock(&rsb->lock);
 
-out:
 	return ret;
 }
 

[patch] bus: sunxi-rsb: unlock on error in sunxi_rsb_read()

[walter harms]

Am 03.11.2015 23:02, schrieb Dan Carpenter:
> Don't forget to unlock before returning an error code.
> 
> Fixes: d787dcdb9c8f ('bus: sunxi-rsb: Add driver for Allwinner Reduced Serial Bus')
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> diff --git a/drivers/bus/sunxi-rsb.c b/drivers/bus/sunxi-rsb.c
> index 846bc29c..0cfcb39 100644
> --- a/drivers/bus/sunxi-rsb.c
> +++ b/drivers/bus/sunxi-rsb.c
> @@ -342,13 +342,13 @@ static int sunxi_rsb_read(struct sunxi_rsb *rsb, u8 rtaddr, u8 addr,
>  
>  	ret = _sunxi_rsb_run_xfer(rsb);
>  	if (ret)
> -		goto out;
> +		goto unlock;
>  
>  	*buf = readl(rsb->regs + RSB_DATA);
>  
> +unlock:
>  	mutex_unlock(&rsb->lock);
>  
> -out:
>  	return ret;
>  }
>  

microoptimisation:
You can remove the goto.

if (!ret)
	*buf = readl(rsb->regs + RSB_DATA);

mutex_unlock(&rsb->lock);
return ret;

just my 2 cents,

re,
 wh

[patch] bus: sunxi-rsb: unlock on error in sunxi_rsb_read()

[Julia Lawall]

On Wed, 4 Nov 2015, walter harms wrote:

>
>
> Am 03.11.2015 23:02, schrieb Dan Carpenter:
> > Don't forget to unlock before returning an error code.
> >
> > Fixes: d787dcdb9c8f ('bus: sunxi-rsb: Add driver for Allwinner Reduced Serial Bus')
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> >
> > diff --git a/drivers/bus/sunxi-rsb.c b/drivers/bus/sunxi-rsb.c
> > index 846bc29c..0cfcb39 100644
> > --- a/drivers/bus/sunxi-rsb.c
> > +++ b/drivers/bus/sunxi-rsb.c
> > @@ -342,13 +342,13 @@ static int sunxi_rsb_read(struct sunxi_rsb *rsb, u8 rtaddr, u8 addr,
> >
> >  	ret = _sunxi_rsb_run_xfer(rsb);
> >  	if (ret)
> > -		goto out;
> > +		goto unlock;
> >
> >  	*buf = readl(rsb->regs + RSB_DATA);
> >
> > +unlock:
> >  	mutex_unlock(&rsb->lock);
> >
> > -out:
> >  	return ret;
> >  }
> >
>
> microoptimisation:
> You can remove the goto.
>
> if (!ret)
> 	*buf = readl(rsb->regs + RSB_DATA);
>
> mutex_unlock(&rsb->lock);
> return ret;

I think the goto is nicer.  Failure => goto.

julia

[patch] bus: sunxi-rsb: unlock on error in sunxi_rsb_read()

[Dan Carpenter]

On Wed, Nov 04, 2015 at 01:19:55PM +0100, walter harms wrote:
> 
> 
> Am 03.11.2015 23:02, schrieb Dan Carpenter:
> > Don't forget to unlock before returning an error code.
> > 
> > Fixes: d787dcdb9c8f ('bus: sunxi-rsb: Add driver for Allwinner Reduced Serial Bus')
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > 
> > diff --git a/drivers/bus/sunxi-rsb.c b/drivers/bus/sunxi-rsb.c
> > index 846bc29c..0cfcb39 100644
> > --- a/drivers/bus/sunxi-rsb.c
> > +++ b/drivers/bus/sunxi-rsb.c
> > @@ -342,13 +342,13 @@ static int sunxi_rsb_read(struct sunxi_rsb *rsb, u8 rtaddr, u8 addr,
> >  
> >  	ret = _sunxi_rsb_run_xfer(rsb);
> >  	if (ret)
> > -		goto out;
> > +		goto unlock;
> >  
> >  	*buf = readl(rsb->regs + RSB_DATA);
> >  
> > +unlock:
> >  	mutex_unlock(&rsb->lock);
> >  
> > -out:
> >  	return ret;
> >  }
> >  
> 
> microoptimisation:
> You can remove the goto.
> 
> if (!ret)
> 	*buf = readl(rsb->regs + RSB_DATA);

Success handling is an anti-pattern.

People normally don't do success handling because it leads to a lot of
nesting and spaghetti code.

	ret = one();
	if (!ret) {
		ret = two();
		if (!ret) {
			ret = three();
			if (!ret)
				return four();
		}
	}

But then they get to the last condition or the last two in a function
and they switch to success handling.

	ret = one();
	if (ret)
		handle_error;

	ret = two();
	if (ret)
		handle_error;

	ret = three();
	if (ret)
		handle_error;

	ret = four();
	if (!ret)
		handle_success;

	return ret;

Code like this drives me nuts.  It's often bug prone.  Keep it
consistent.  Always do error handling instead of success handling.
Don't worry about the extra line.  Worry more about keeping it simple.

regards,
dan carpenter

[patch] bus: sunxi-rsb: unlock on error in sunxi_rsb_read()

[Chen-Yu Tsai]

On Wed, Nov 4, 2015 at 6:02 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> Don't forget to unlock before returning an error code.
>
> Fixes: d787dcdb9c8f ('bus: sunxi-rsb: Add driver for Allwinner Reduced Serial Bus')
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Acked-by: Chen-Yu Tsai <wens@csie.org>

>
> diff --git a/drivers/bus/sunxi-rsb.c b/drivers/bus/sunxi-rsb.c
> index 846bc29c..0cfcb39 100644
> --- a/drivers/bus/sunxi-rsb.c
> +++ b/drivers/bus/sunxi-rsb.c
> @@ -342,13 +342,13 @@ static int sunxi_rsb_read(struct sunxi_rsb *rsb, u8 rtaddr, u8 addr,
>
>         ret = _sunxi_rsb_run_xfer(rsb);
>         if (ret)
> -               goto out;
> +               goto unlock;
>
>         *buf = readl(rsb->regs + RSB_DATA);
>
> +unlock:
>         mutex_unlock(&rsb->lock);
>
> -out:
>         return ret;
>  }
>

[patch] bus: sunxi-rsb: unlock on error in sunxi_rsb_read()

[Maxime Ripard]

Hi Dan,

On Wed, Nov 04, 2015 at 01:02:44AM +0300, Dan Carpenter wrote:
> Don't forget to unlock before returning an error code.
> 
> Fixes: d787dcdb9c8f ('bus: sunxi-rsb: Add driver for Allwinner Reduced Serial Bus')
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

I just queued this for 4.4, thanks!

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20151104/43b0f17f/attachment.sig>