* [patch] bus: sunxi-rsb: unlock on error in sunxi_rsb_read()
@ 2015-11-03 22:02 ` Dan Carpenter
0 siblings, 0 replies; 14+ messages in thread
From: Dan Carpenter @ 2015-11-03 22:02 UTC (permalink / raw)
To: linux-arm-kernel
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;
}
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [patch] bus: sunxi-rsb: unlock on error in sunxi_rsb_read()
2015-11-03 22:02 ` Dan Carpenter
@ 2015-11-04 12:19 ` walter harms
-1 siblings, 0 replies; 14+ messages in thread
From: walter harms @ 2015-11-04 12:19 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 14+ messages in thread* [patch] bus: sunxi-rsb: unlock on error in sunxi_rsb_read()
@ 2015-11-04 12:19 ` walter harms
0 siblings, 0 replies; 14+ messages in thread
From: walter harms @ 2015-11-04 12:19 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [patch] bus: sunxi-rsb: unlock on error in sunxi_rsb_read()
2015-11-04 12:19 ` walter harms
@ 2015-11-04 12:34 ` Julia Lawall
-1 siblings, 0 replies; 14+ messages in thread
From: Julia Lawall @ 2015-11-04 12:34 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 14+ messages in thread* [patch] bus: sunxi-rsb: unlock on error in sunxi_rsb_read()
@ 2015-11-04 12:34 ` Julia Lawall
0 siblings, 0 replies; 14+ messages in thread
From: Julia Lawall @ 2015-11-04 12:34 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [patch] bus: sunxi-rsb: unlock on error in sunxi_rsb_read()
2015-11-04 12:19 ` walter harms
@ 2015-11-04 12:46 ` Dan Carpenter
-1 siblings, 0 replies; 14+ messages in thread
From: Dan Carpenter @ 2015-11-04 12:46 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 14+ messages in thread* [patch] bus: sunxi-rsb: unlock on error in sunxi_rsb_read()
@ 2015-11-04 12:46 ` Dan Carpenter
0 siblings, 0 replies; 14+ messages in thread
From: Dan Carpenter @ 2015-11-04 12:46 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [patch] bus: sunxi-rsb: unlock on error in sunxi_rsb_read()
2015-11-03 22:02 ` Dan Carpenter
@ 2015-11-04 15:28 ` Chen-Yu Tsai
-1 siblings, 0 replies; 14+ messages in thread
From: Chen-Yu Tsai @ 2015-11-04 15:28 UTC (permalink / raw)
To: linux-arm-kernel
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;
> }
>
^ permalink raw reply [flat|nested] 14+ messages in thread* [patch] bus: sunxi-rsb: unlock on error in sunxi_rsb_read()
@ 2015-11-04 15:28 ` Chen-Yu Tsai
0 siblings, 0 replies; 14+ messages in thread
From: Chen-Yu Tsai @ 2015-11-04 15:28 UTC (permalink / raw)
To: linux-arm-kernel
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;
> }
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [patch] bus: sunxi-rsb: unlock on error in sunxi_rsb_read()
2015-11-03 22:02 ` Dan Carpenter
@ 2015-11-04 15:51 ` Maxime Ripard
-1 siblings, 0 replies; 14+ messages in thread
From: Maxime Ripard @ 2015-11-04 15:51 UTC (permalink / raw)
To: linux-arm-kernel
[-- Attachment #1: Type: text/plain, Size: 445 bytes --]
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
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [patch] bus: sunxi-rsb: unlock on error in sunxi_rsb_read()
2015-11-03 22:02 ` Dan Carpenter
` (3 preceding siblings ...)
(?)
@ 2015-11-04 21:22 ` walter harms
-1 siblings, 0 replies; 14+ messages in thread
From: walter harms @ 2015-11-04 21:22 UTC (permalink / raw)
To: kernel-janitors
Am 04.11.2015 13:46, schrieb 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.
>
It is not a problem with me, i only wanted to make my point since it
seemed obvious (i did not bother to look at the whole function). If you
does this intentionally its fine.
NTL one comment, if you looked it as success handling it may be uncommon
but i you see it as "proctector" it is more obvious like
if (f) free(f);
but problem is solved anyway and we all want to make things more easy not
more complicated.
re,
wh
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [patch] bus: sunxi-rsb: unlock on error in sunxi_rsb_read()
2015-11-03 22:02 ` Dan Carpenter
` (4 preceding siblings ...)
(?)
@ 2015-11-05 6:07 ` Dan Carpenter
-1 siblings, 0 replies; 14+ messages in thread
From: Dan Carpenter @ 2015-11-05 6:07 UTC (permalink / raw)
To: kernel-janitors
On Wed, Nov 04, 2015 at 10:22:35PM +0100, walter harms wrote:
> NTL one comment, if you looked it as success handling it may be uncommon
> but i you see it as "proctector" it is more obvious like
> if (f) free(f);
You shouldn't normally have those kinds of if statements because you
should handle the case of if "f" is NULL immediately. It shouldn't be a
question of "I'm half way through the function and I don't know what has
been allocated and what is free."
If you have an if condition like that it probably means you are doing
One Err style error handling and we all know my feelings about that. ;)
regards,
dan carpenter
^ permalink raw reply [flat|nested] 14+ messages in thread