| Message ID | 20130312141604.GV4977@n2100.arm.linux.org.uk |
|---|---|
| State | New |
| Headers | show |
* 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>
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
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.
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
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);
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(-)