Patchwork IS_ERR_VALUE()

login
register
mail settings
Submitter Russell King - ARM Linux
Date March 12, 2013, 2:16 p.m.
Message ID <20130312141604.GV4977@n2100.arm.linux.org.uk>
Download mbox | patch
Permalink /patch/227032/
State New
Headers show

Comments

Russell King - ARM Linux - March 12, 2013, 2:16 p.m.
I am removing almost all references to the above macro from arch/arm.
Many of them are wrong.  Some of them are buggy.

For instance:

int gpmc_cs_set_timings(int cs, const struct gpmc_timings *t)
{
        int div;
        div = gpmc_calc_divider(t->sync_clk);
        if (div < 0)
                return div;
static int gpmc_set_async_mode(int cs, struct gpmc_timings *t)
{
...
        return gpmc_cs_set_timings(cs, t);

.....
        ret = gpmc_set_async_mode(gpmc_onenand_data->cs, &t);
        if (IS_ERR_VALUE(ret))
                return ret;

So, gpmc_cs_set_timings() thinks any negative return value is an error,
but where we check that in higher levels, only a limited range are
errors... seriously?  Come on guys, get with the program.  Get your
error checking in order.

There is only _one_ use of IS_ERR_VALUE() in arch/arm which is correct,
and that is in arch/arm/include/asm/syscall.h:

static inline long syscall_get_error(struct task_struct *task,
                                     struct pt_regs *regs)
{
        unsigned long error = regs->ARM_r0;
        return IS_ERR_VALUE(error) ? error : 0;
}

So, here's a patch to remove them all, except for the above.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
This patch will be going into my "cleanup" branch along with the removal
of many wrong IS_ERR_OR_NULL() uses - with any errors anyone spots fixed.

 arch/arm/mach-exynos/mach-nuri.c        |    2 +-
 arch/arm/mach-imx/devices/devices.c     |    2 +-
 arch/arm/mach-omap2/board-omap3beagle.c |    2 +-
 arch/arm/mach-omap2/clock.c             |    2 +-
 arch/arm/mach-omap2/gpmc-onenand.c      |    4 ++--
 arch/arm/mach-omap2/gpmc.c              |    8 ++++----
 arch/arm/mach-omap2/omap_device.c       |    2 +-
 arch/arm/mach-omap2/omap_hwmod.c        |    4 ++--
 arch/arm/mach-omap2/timer.c             |    2 +-
 arch/arm/plat-omap/dmtimer.c            |    2 +-
 10 files changed, 15 insertions(+), 15 deletions(-)
Tony Lindgren - March 12, 2013, 4:34 p.m.
* Russell King - ARM Linux <linux@arm.linux.org.uk> [130312 07:25]:
> I am removing almost all references to the above macro from arch/arm.
> Many of them are wrong.  Some of them are buggy.
> 
> For instance:
> 
> int gpmc_cs_set_timings(int cs, const struct gpmc_timings *t)
> {
>         int div;
>         div = gpmc_calc_divider(t->sync_clk);
>         if (div < 0)
>                 return div;
> static int gpmc_set_async_mode(int cs, struct gpmc_timings *t)
> {
> ...
>         return gpmc_cs_set_timings(cs, t);
> 
> .....
>         ret = gpmc_set_async_mode(gpmc_onenand_data->cs, &t);
>         if (IS_ERR_VALUE(ret))
>                 return ret;
> 
> So, gpmc_cs_set_timings() thinks any negative return value is an error,
> but where we check that in higher levels, only a limited range are
> errors... seriously?  Come on guys, get with the program.  Get your
> error checking in order.
> 
> There is only _one_ use of IS_ERR_VALUE() in arch/arm which is correct,
> and that is in arch/arm/include/asm/syscall.h:
> 
> static inline long syscall_get_error(struct task_struct *task,
>                                      struct pt_regs *regs)
> {
>         unsigned long error = regs->ARM_r0;
>         return IS_ERR_VALUE(error) ? error : 0;
> }
> 
> So, here's a patch to remove them all, except for the above.
> 
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> ---
> This patch will be going into my "cleanup" branch along with the removal
> of many wrong IS_ERR_OR_NULL() uses - with any errors anyone spots fixed.

Looks good to me. Can you please also let us know some immutable
commit for your cleanup branch? I'd like to use that as a base
to pull in further GPMC changes to avoid unnecessary merge conflicts.

Acked-by: Tony Lindgren <tony@atomide.com>
Uwe Kleine-König - March 12, 2013, 7:56 p.m.
Hello,

On Tue, Mar 12, 2013 at 02:16:04PM +0000, Russell King - ARM Linux wrote:
> I am removing almost all references to the above macro from arch/arm.
> Many of them are wrong.  Some of them are buggy.
> 
> For instance:
> 
> int gpmc_cs_set_timings(int cs, const struct gpmc_timings *t)
> {
>         int div;
>         div = gpmc_calc_divider(t->sync_clk);
>         if (div < 0)
>                 return div;
> static int gpmc_set_async_mode(int cs, struct gpmc_timings *t)
> {
> ...
>         return gpmc_cs_set_timings(cs, t);
> 
> .....
>         ret = gpmc_set_async_mode(gpmc_onenand_data->cs, &t);
>         if (IS_ERR_VALUE(ret))
>                 return ret;

Well, gpmc_calc_divider returns -1 on failure, so I agree that it's
wrong to use IS_ERR_VALUE on it.
 
> So, gpmc_cs_set_timings() thinks any negative return value is an error,
> but where we check that in higher levels, only a limited range are
> errors... seriously?  Come on guys, get with the program.  Get your
> error checking in order.
 
Still I don't get what is really wrong in your examples (but I didn't
check all). If a function returning an int is supposed to return 0 on
success and -ESOMEERROR on failure it doesn't matter much if I do (ret <
0) or IS_ERR_VALUE(ret). (Even if ret is signed, the check (x) >=
(unsigned long)-MAX_ERRNO in IS_ERR_VALUE does the right thing.)

There is only a semantical difference between the two checks for
(unsigned)ret being between MAX_INT+1 and (unsigned)-MAX_ERRNO. But a
value in this range isn't allowed for such a function. So you could also
just check using "if (ret)". All but the previous sentence also apply to
functions returning an int >=0 on success and -ESOMEERROR on failure.

So unless I missed something---and if I do, please tell me---your patch
is not about correctness, but "only" about style and consistency?!

Best regards
Uwe
Russell King - ARM Linux - March 12, 2013, 8:22 p.m.
On Tue, Mar 12, 2013 at 08:56:59PM +0100, Uwe Kleine-König wrote:
> Hello,
> 
> On Tue, Mar 12, 2013 at 02:16:04PM +0000, Russell King - ARM Linux wrote:
> > I am removing almost all references to the above macro from arch/arm.
> > Many of them are wrong.  Some of them are buggy.
> > 
> > For instance:
> > 
> > int gpmc_cs_set_timings(int cs, const struct gpmc_timings *t)
> > {
> >         int div;
> >         div = gpmc_calc_divider(t->sync_clk);
> >         if (div < 0)
> >                 return div;
> > static int gpmc_set_async_mode(int cs, struct gpmc_timings *t)
> > {
> > ...
> >         return gpmc_cs_set_timings(cs, t);
> > 
> > .....
> >         ret = gpmc_set_async_mode(gpmc_onenand_data->cs, &t);
> >         if (IS_ERR_VALUE(ret))
> >                 return ret;
> 
> Well, gpmc_calc_divider returns -1 on failure, so I agree that it's
> wrong to use IS_ERR_VALUE on it.

So what you've just told me is that on error, it returns -1, which is
-EPERM.  So we end up passing an "Operation not permitted" error up?
Yes, really, -EPERM all the way up and returning it as an error code
for an out of range divisor value.

Is that really appropriate.  How many times have I said to never use
-1 as an error value?  See what a screwed situation this causes?  See
what *not* having a clear policy on this in your mind causes?

If you always treat -ve values as errors, and _always_ return a -ve
error number as defined in the errno.h headers, then you won't ever
make this mistake.

> Still I don't get what is really wrong in your examples (but I didn't
> check all). If a function returning an int is supposed to return 0 on
> success and -ESOMEERROR on failure it doesn't matter much if I do (ret <
> 0) or IS_ERR_VALUE(ret). (Even if ret is signed, the check (x) >=
> (unsigned long)-MAX_ERRNO in IS_ERR_VALUE does the right thing.)
> 
> There is only a semantical difference between the two checks for
> (unsigned)ret being between MAX_INT+1 and (unsigned)-MAX_ERRNO. But a
> value in this range isn't allowed for such a function. So you could also
> just check using "if (ret)". All but the previous sentence also apply to
> functions returning an int >=0 on success and -ESOMEERROR on failure.
> 
> So unless I missed something---and if I do, please tell me---your patch
> is not about correctness, but "only" about style and consistency?!

So what about if the function returns a -ve number outside of MAX_ERRNO?
We have an inconsistent situation then - we have some functions
believing that it's an error, others believing it's not an error.

So, it _is_ about correctness _and_ consistency.  Not style.  Style
doesn't come into this.  See where I've replied above about the
importance of having this stuff clear and always doing it the same
way.

As soon as you start messing around doing crap like this, then you
start allowing minor bugs to creap in.

If you want buggy software, go and develop for Windows or something
else.

You clearly don't know me either.  I strive for correctness.  When
fixing a bug, I want the bug fixed, not some sticky plaster over it.
I hate crap like this where there's stupid idiotic differences just
because of no good reason what so ever, which then lead to subtle
bugs.  If you start out with a clear idea at the beginning then
you'll be a far better coder and you'll make less mistakes.  And you
won't mess up your error checking.

It's people with my kind of attitude on this issue which keeps stuff
sane and maintainable in the kernel.
Uwe Kleine-König - March 12, 2013, 9:18 p.m.
Hello Russell,

On Tue, Mar 12, 2013 at 08:22:38PM +0000, Russell King - ARM Linux wrote:
> On Tue, Mar 12, 2013 at 08:56:59PM +0100, Uwe Kleine-König wrote:
> > Hello,
> > 
> > On Tue, Mar 12, 2013 at 02:16:04PM +0000, Russell King - ARM Linux wrote:
> > > I am removing almost all references to the above macro from arch/arm.
> > > Many of them are wrong.  Some of them are buggy.
> > > 
> > > For instance:
> > > 
> > > int gpmc_cs_set_timings(int cs, const struct gpmc_timings *t)
> > > {
> > >         int div;
> > >         div = gpmc_calc_divider(t->sync_clk);
> > >         if (div < 0)
> > >                 return div;
> > > static int gpmc_set_async_mode(int cs, struct gpmc_timings *t)
> > > {
> > > ...
> > >         return gpmc_cs_set_timings(cs, t);
> > > 
> > > .....
> > >         ret = gpmc_set_async_mode(gpmc_onenand_data->cs, &t);
> > >         if (IS_ERR_VALUE(ret))
> > >                 return ret;
> > 
> > Well, gpmc_calc_divider returns -1 on failure, so I agree that it's
> > wrong to use IS_ERR_VALUE on it.
> 
> So what you've just told me is that on error, it returns -1, which is
> -EPERM.  So we end up passing an "Operation not permitted" error up?
> Yes, really, -EPERM all the way up and returning it as an error code
> for an out of range divisor value.
> 
> Is that really appropriate.  How many times have I said to never use
> -1 as an error value?  See what a screwed situation this causes?  See
> what *not* having a clear policy on this in your mind causes?
We agree here that -1 is a bad idea. So I take this "your" as "the
author's".

> If you always treat -ve values as errors, and _always_ return a -ve
> error number as defined in the errno.h headers, then you won't ever
> make this mistake.
> 
> > Still I don't get what is really wrong in your examples (but I didn't
> > check all). If a function returning an int is supposed to return 0 on
> > success and -ESOMEERROR on failure it doesn't matter much if I do (ret <
> > 0) or IS_ERR_VALUE(ret). (Even if ret is signed, the check (x) >=
> > (unsigned long)-MAX_ERRNO in IS_ERR_VALUE does the right thing.)
> > 
> > There is only a semantical difference between the two checks for
> > (unsigned)ret being between MAX_INT+1 and (unsigned)-MAX_ERRNO. But a
> > value in this range isn't allowed for such a function. So you could also
> > just check using "if (ret)". All but the previous sentence also apply to
> > functions returning an int >=0 on success and -ESOMEERROR on failure.
> > 
> > So unless I missed something---and if I do, please tell me---your patch
> > is not about correctness, but "only" about style and consistency?!
> 
> So what about if the function returns a -ve number outside of MAX_ERRNO?
Then it's a bug in this function.

> We have an inconsistent situation then - we have some functions
> believing that it's an error, others believing it's not an error.
If my function calls a buggy function I think it doesn't matter for the
correctness of my function if there is an inconsistent situation after
the buggy call.

So in my opinion the fix goes to the called function. Still this doesn't
(and shouldn't) stop a patch like yours that adds consistency.

> So, it _is_ about correctness _and_ consistency.  Not style.  Style
> doesn't come into this.  See where I've replied above about the
> importance of having this stuff clear and always doing it the same
> way.
> 
> As soon as you start messing around doing crap like this, then you
> start allowing minor bugs to creap in.
> 
> If you want buggy software, go and develop for Windows or something
> else.
> 
> You clearly don't know me either.  I strive for correctness.  When
> fixing a bug, I want the bug fixed, not some sticky plaster over it.
> I hate crap like this where there's stupid idiotic differences just
> because of no good reason what so ever, which then lead to subtle
> bugs.  If you start out with a clear idea at the beginning then
> you'll be a far better coder and you'll make less mistakes.  And you
> won't mess up your error checking.
> 
> It's people with my kind of attitude on this issue which keeps stuff
> sane and maintainable in the kernel.
I think to understand that your goal is to use the same pattern for all
functions unless it's not sensible. And yes, that's important for a
project with that many contributors.

Still if I had authored your patch I wouldn't call the usage "wrong" as
you did in your changelog. I would have written about consistency.

For writing my mail I looked around a bit in other areas of the kernel.
I think there are many more instances to fix. E.g. drivers/base/core.c
is full of "if (retval)", "if (error)" and "if (err)".

Best regards
Uwe

Patch

diff --git a/arch/arm/mach-exynos/mach-nuri.c b/arch/arm/mach-exynos/mach-nuri.c
index 1ea7973..688991a 100644
--- a/arch/arm/mach-exynos/mach-nuri.c
+++ b/arch/arm/mach-exynos/mach-nuri.c
@@ -1251,7 +1251,7 @@  static void __init nuri_camera_init(void)
 	}
 
 	m5mols_board_info.irq = s5p_register_gpio_interrupt(GPIO_CAM_8M_ISP_INT);
-	if (!IS_ERR_VALUE(m5mols_board_info.irq))
+	if (m5mols_board_info.irq >= 0)
 		s3c_gpio_cfgpin(GPIO_CAM_8M_ISP_INT, S3C_GPIO_SFN(0xF));
 	else
 		pr_err("%s: Failed to configure 8M_ISP_INT GPIO\n", __func__);
diff --git a/arch/arm/mach-imx/devices/devices.c b/arch/arm/mach-imx/devices/devices.c
index 1b37482..1b4366a 100644
--- a/arch/arm/mach-imx/devices/devices.c
+++ b/arch/arm/mach-imx/devices/devices.c
@@ -37,7 +37,7 @@  int __init mxc_device_init(void)
 	int ret;
 
 	ret = device_register(&mxc_aips_bus);
-	if (IS_ERR_VALUE(ret))
+	if (ret < 0)
 		goto done;
 
 	ret = device_register(&mxc_ahb_bus);
diff --git a/arch/arm/mach-omap2/board-omap3beagle.c b/arch/arm/mach-omap2/board-omap3beagle.c
index c3558f9..6c949bc 100644
--- a/arch/arm/mach-omap2/board-omap3beagle.c
+++ b/arch/arm/mach-omap2/board-omap3beagle.c
@@ -479,7 +479,7 @@  static int __init beagle_opp_init(void)
 
 	/* Initialize the omap3 opp table if not already created. */
 	r = omap3_opp_init();
-	if (IS_ERR_VALUE(r) && (r != -EEXIST)) {
+	if (r < 0 && (r != -EEXIST)) {
 		pr_err("%s: opp default init failed\n", __func__);
 		return r;
 	}
diff --git a/arch/arm/mach-omap2/clock.c b/arch/arm/mach-omap2/clock.c
index e4ec3a6..2191f25 100644
--- a/arch/arm/mach-omap2/clock.c
+++ b/arch/arm/mach-omap2/clock.c
@@ -596,7 +596,7 @@  int __init omap2_clk_switch_mpurate_at_boot(const char *mpurate_ck_name)
 		return -ENOENT;
 
 	r = clk_set_rate(mpurate_ck, mpurate);
-	if (IS_ERR_VALUE(r)) {
+	if (r < 0) {
 		WARN(1, "clock: %s: unable to set MPU rate to %d: %d\n",
 		     mpurate_ck_name, mpurate, r);
 		clk_put(mpurate_ck);
diff --git a/arch/arm/mach-omap2/gpmc-onenand.c b/arch/arm/mach-omap2/gpmc-onenand.c
index fadd8743..0d75889 100644
--- a/arch/arm/mach-omap2/gpmc-onenand.c
+++ b/arch/arm/mach-omap2/gpmc-onenand.c
@@ -303,7 +303,7 @@  static int omap2_onenand_setup_async(void __iomem *onenand_base)
 	t = omap2_onenand_calc_async_timings();
 
 	ret = gpmc_set_async_mode(gpmc_onenand_data->cs, &t);
-	if (IS_ERR_VALUE(ret))
+	if (ret < 0)
 		return ret;
 
 	omap2_onenand_set_async_mode(onenand_base);
@@ -325,7 +325,7 @@  static int omap2_onenand_setup_sync(void __iomem *onenand_base, int *freq_ptr)
 	t = omap2_onenand_calc_sync_timings(gpmc_onenand_data, freq);
 
 	ret = gpmc_set_sync_mode(gpmc_onenand_data->cs, &t);
-	if (IS_ERR_VALUE(ret))
+	if (ret < 0)
 		return ret;
 
 	set_onenand_cfg(onenand_base);
diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
index e4b16c8..0011116 100644
--- a/arch/arm/mach-omap2/gpmc.c
+++ b/arch/arm/mach-omap2/gpmc.c
@@ -716,7 +716,7 @@  static int gpmc_setup_irq(void)
 		return -EINVAL;
 
 	gpmc_irq_start = irq_alloc_descs(-1, 0, GPMC_NR_IRQ, 0);
-	if (IS_ERR_VALUE(gpmc_irq_start)) {
+	if (gpmc_irq_start < 0) {
 		pr_err("irq_alloc_descs failed\n");
 		return gpmc_irq_start;
 	}
@@ -801,7 +801,7 @@  static int gpmc_mem_init(void)
 			continue;
 		gpmc_cs_get_memconf(cs, &base, &size);
 		rc = gpmc_cs_insert_mem(cs, base, size);
-		if (IS_ERR_VALUE(rc)) {
+		if (rc < 0) {
 			while (--cs >= 0)
 				if (gpmc_cs_mem_enabled(cs))
 					gpmc_cs_delete_mem(cs);
@@ -1373,14 +1373,14 @@  static int gpmc_probe(struct platform_device *pdev)
 		 GPMC_REVISION_MINOR(l));
 
 	rc = gpmc_mem_init();
-	if (IS_ERR_VALUE(rc)) {
+	if (rc < 0) {
 		clk_disable_unprepare(gpmc_l3_clk);
 		clk_put(gpmc_l3_clk);
 		dev_err(gpmc_dev, "failed to reserve memory\n");
 		return rc;
 	}
 
-	if (IS_ERR_VALUE(gpmc_setup_irq()))
+	if (gpmc_setup_irq() < 0)
 		dev_warn(gpmc_dev, "gpmc_setup_irq failed\n");
 
 	rc = gpmc_probe_dt(pdev);
diff --git a/arch/arm/mach-omap2/omap_device.c b/arch/arm/mach-omap2/omap_device.c
index 381be7a..72b09e0 100644
--- a/arch/arm/mach-omap2/omap_device.c
+++ b/arch/arm/mach-omap2/omap_device.c
@@ -131,7 +131,7 @@  static int omap_device_build_from_dt(struct platform_device *pdev)
 	int oh_cnt, i, ret = 0;
 
 	oh_cnt = of_property_count_strings(node, "ti,hwmods");
-	if (!oh_cnt || IS_ERR_VALUE(oh_cnt)) {
+	if (oh_cnt <= 0) {
 		dev_dbg(&pdev->dev, "No 'hwmods' to build omap_device\n");
 		return -ENODEV;
 	}
diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
index c2c798c..a62213e 100644
--- a/arch/arm/mach-omap2/omap_hwmod.c
+++ b/arch/arm/mach-omap2/omap_hwmod.c
@@ -1663,7 +1663,7 @@  static int _deassert_hardreset(struct omap_hwmod *oh, const char *name)
 		return -ENOSYS;
 
 	ret = _lookup_hardreset(oh, name, &ohri);
-	if (IS_ERR_VALUE(ret))
+	if (ret < 0)
 		return ret;
 
 	if (oh->clkdm) {
@@ -2413,7 +2413,7 @@  static int __init _init(struct omap_hwmod *oh, void *data)
 	_init_mpu_rt_base(oh, NULL);
 
 	r = _init_clocks(oh, NULL);
-	if (IS_ERR_VALUE(r)) {
+	if (r < 0) {
 		WARN(1, "omap_hwmod: %s: couldn't init clocks\n", oh->name);
 		return -EINVAL;
 	}
diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c
index 2bdd4cf..5bde35b 100644
--- a/arch/arm/mach-omap2/timer.c
+++ b/arch/arm/mach-omap2/timer.c
@@ -287,7 +287,7 @@  static int __init omap_dm_timer_init_one(struct omap_dm_timer *timer,
 			r = -EINVAL;
 		} else {
 			r = clk_set_parent(timer->fclk, src);
-			if (IS_ERR_VALUE(r))
+			if (r < 0)
 				pr_warn("%s: %s cannot set source\n",
 					__func__, oh->name);
 			clk_put(src);
diff --git a/arch/arm/plat-omap/dmtimer.c b/arch/arm/plat-omap/dmtimer.c
index a0daa2f..17f21c2 100644
--- a/arch/arm/plat-omap/dmtimer.c
+++ b/arch/arm/plat-omap/dmtimer.c
@@ -506,7 +506,7 @@  int omap_dm_timer_set_source(struct omap_dm_timer *timer, int source)
 	}
 
 	ret = clk_set_parent(timer->fclk, parent);
-	if (IS_ERR_VALUE(ret))
+	if (ret < 0)
 		pr_err("%s: failed to set %s as parent\n", __func__,
 			parent_name);