bus: sunxi-rsb: unlock on error in sunxi_rsb_read()
Subsystems (2)
PATCH 1/1 bus: sunxi-rsb: unlock on error in sunxi_rsb_read()
Don't forget to unlock before returning an error code.
drivers/bus/sunxi-rsb.c +2 -2
@@ -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; }
Am 03.11.2015 23:02, schrieb Dan Carpenter:
Quoted 15 lines
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 <[email protected]>drivers/bus/sunxi-rsb.c +1
-1@@ -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;
drivers/bus/sunxi-rsb.c static int sunxi_rsb_read(struct sunxi_rsb *rsb, u8 rtaddr, u8 addr, · line 354
Quoted 10 lines
drivers/bus/sunxi-rsb.c +1
-1*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
Quoted 32 lines · walter harms
Am 03.11.2015 23:02, schrieb Dan Carpenter:
Quoted 15 lines
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 <[email protected]>drivers/bus/sunxi-rsb.c +1
-1@@ -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;drivers/bus/sunxi-rsb.c static int sunxi_rsb_read(struct sunxi_rsb *rsb, u8 rtaddr, u8 addr, · line 354
Quoted 10 lines
drivers/bus/sunxi-rsb.c +1
-1*buf =readl (rsb->regs +RSB_DATA ); +unlock:mutex_unlock (&rsb->lock); -out:return ret; }microoptimisation:
You can remove the goto.
Quoted 5 lines
if (!ret)
*buf = readl(rsb->regs + RSB_DATA);
mutex_unlock(&rsb->lock);
return ret;
I think the goto is nicer. Failure => goto.
julia
Quoted 32 lines · walter harms
Am 03.11.2015 23:02, schrieb Dan Carpenter:
Quoted 15 lines
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 <[email protected]>drivers/bus/sunxi-rsb.c +1
-1@@ -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;drivers/bus/sunxi-rsb.c static int sunxi_rsb_read(struct sunxi_rsb *rsb, u8 rtaddr, u8 addr, · line 354
Quoted 10 lines
drivers/bus/sunxi-rsb.c +1
-1*buf =readl (rsb->regs +RSB_DATA ); +unlock:mutex_unlock (&rsb->lock); -out:return ret; }microoptimisation:
You can remove the goto.
Quoted 2 lines
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
Quoted 4 lines · 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 <[email protected]>
Quoted 21 lines
drivers/bus/sunxi-rsb.c +2
-2@@ -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; }
Hi Dan,
Quoted 4 lines · 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 <[email protected]>
I just queued this for 4.4, thanks!
Maxime
Signature · 3 lines
Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com
Am 04.11.2015 13:46, schrieb Dan Carpenter:
Quoted 72 lines · walter harms
Quoted 32 lines
Am 03.11.2015 23:02, schrieb Dan Carpenter:
Quoted 15 lines
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 <[email protected]>drivers/bus/sunxi-rsb.c +1
-1@@ -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;drivers/bus/sunxi-rsb.c static int sunxi_rsb_read(struct sunxi_rsb *rsb, u8 rtaddr, u8 addr, · line 354
Quoted 10 lines
drivers/bus/sunxi-rsb.c +1
-1*buf =readl (rsb->regs +RSB_DATA ); +unlock:mutex_unlock (&rsb->lock); -out:return ret; }microoptimisation:
You can remove the goto.Quoted 2 lines
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;
Quoted 4 lines
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
Quoted 3 lines · walter harms
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