| Message ID | 1363075109-27038-1-git-send-email-silviupopescu1990@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Mar 12, 2013 at 09:58:29AM +0200, Silviu-Mihai Popescu wrote: > This uses PTR_RET instead of IS_ERR and PTR_ERR in order to increase > readability. > > Signed-off-by: Silviu-Mihai Popescu <silviupopescu1990@gmail.com> > --- > arch/arm/mach-omap2/devices.c | 4 ++-- > arch/arm/mach-omap2/fb.c | 5 +---- > arch/arm/mach-omap2/gpmc.c | 2 +- > arch/arm/mach-omap2/pmu.c | 5 +---- > 4 files changed, 5 insertions(+), 11 deletions(-) > > diff --git a/arch/arm/mach-omap2/devices.c b/arch/arm/mach-omap2/devices.c > index 1ec7f05..2a0816e 100644 > --- a/arch/arm/mach-omap2/devices.c > +++ b/arch/arm/mach-omap2/devices.c > @@ -66,7 +66,7 @@ static int __init omap3_l3_init(void) > > WARN(IS_ERR(pdev), "could not build omap_device for %s\n", oh_name); > > - return IS_ERR(pdev) ? PTR_ERR(pdev) : 0; > + return PTR_RET(pdev); This is incorrect. The return value will be tested for < 0. Kernel pointers in general are all above 3GB, and so are all "< 0". I'm afraid none of these changes stuff is an improvement - they all introduce bugs.
On 03/12/2013 06:05 AM, Russell King - ARM Linux wrote: > On Tue, Mar 12, 2013 at 09:58:29AM +0200, Silviu-Mihai Popescu wrote: >> This uses PTR_RET instead of IS_ERR and PTR_ERR in order to increase >> readability. >> >> Signed-off-by: Silviu-Mihai Popescu <silviupopescu1990@gmail.com> >> --- >> arch/arm/mach-omap2/devices.c | 4 ++-- >> arch/arm/mach-omap2/fb.c | 5 +---- >> arch/arm/mach-omap2/gpmc.c | 2 +- >> arch/arm/mach-omap2/pmu.c | 5 +---- >> 4 files changed, 5 insertions(+), 11 deletions(-) >> >> diff --git a/arch/arm/mach-omap2/devices.c b/arch/arm/mach-omap2/devices.c >> index 1ec7f05..2a0816e 100644 >> --- a/arch/arm/mach-omap2/devices.c >> +++ b/arch/arm/mach-omap2/devices.c >> @@ -66,7 +66,7 @@ static int __init omap3_l3_init(void) >> >> WARN(IS_ERR(pdev), "could not build omap_device for %s\n", oh_name); >> >> - return IS_ERR(pdev) ? PTR_ERR(pdev) : 0; >> + return PTR_RET(pdev); > > This is incorrect. > > The return value will be tested for < 0. Kernel pointers in general are > all above 3GB, and so are all "< 0". > > I'm afraid none of these changes stuff is an improvement - they all > introduce bugs. Sorry I am now not sure I follow you here. Someone just pointed out to me that PTR_RET() is defined as ... static inline int __must_check PTR_RET(const void *ptr) { if (IS_ERR(ptr)) return PTR_ERR(ptr); else return 0; } So the above change appears to be equivalent. Is there something that is wrong with the current implementation that needs to be fixed? Jon
On Wed, Mar 20, 2013 at 8:28 PM, Jon Hunter <jon-hunter@ti.com> wrote: > > On 03/12/2013 06:05 AM, Russell King - ARM Linux wrote: >> On Tue, Mar 12, 2013 at 09:58:29AM +0200, Silviu-Mihai Popescu wrote: >>> This uses PTR_RET instead of IS_ERR and PTR_ERR in order to increase >>> readability. >>> >>> Signed-off-by: Silviu-Mihai Popescu <silviupopescu1990@gmail.com> >>> --- >>> arch/arm/mach-omap2/devices.c | 4 ++-- >>> arch/arm/mach-omap2/fb.c | 5 +---- >>> arch/arm/mach-omap2/gpmc.c | 2 +- >>> arch/arm/mach-omap2/pmu.c | 5 +---- >>> 4 files changed, 5 insertions(+), 11 deletions(-) >>> >>> diff --git a/arch/arm/mach-omap2/devices.c b/arch/arm/mach-omap2/devices.c >>> index 1ec7f05..2a0816e 100644 >>> --- a/arch/arm/mach-omap2/devices.c >>> +++ b/arch/arm/mach-omap2/devices.c >>> @@ -66,7 +66,7 @@ static int __init omap3_l3_init(void) >>> >>> WARN(IS_ERR(pdev), "could not build omap_device for %s\n", oh_name); >>> >>> - return IS_ERR(pdev) ? PTR_ERR(pdev) : 0; >>> + return PTR_RET(pdev); >> >> This is incorrect. >> >> The return value will be tested for < 0. Kernel pointers in general are >> all above 3GB, and so are all "< 0". >> >> I'm afraid none of these changes stuff is an improvement - they all >> introduce bugs. > > Sorry I am now not sure I follow you here. Someone just pointed out to > me that PTR_RET() is defined as ... > > static inline int __must_check PTR_RET(const void *ptr) > { > if (IS_ERR(ptr)) > return PTR_ERR(ptr); > else > return 0; > } > > So the above change appears to be equivalent. Is there something that is > wrong with the current implementation that needs to be fixed? As the patch message says, it's just for readability purposes. I used make coccicheck and it suggested this minor change. -- Silviu Popescu
On Wed, Mar 20, 2013 at 01:28:47PM -0500, Jon Hunter wrote: > Sorry I am now not sure I follow you here. Someone just pointed out to > me that PTR_RET() is defined as ... > > static inline int __must_check PTR_RET(const void *ptr) > { > if (IS_ERR(ptr)) > return PTR_ERR(ptr); > else > return 0; > } > > So the above change appears to be equivalent. Is there something that is > wrong with the current implementation that needs to be fixed? No - I misread it as PTR_ERR not PTR_RET. Your patch is fine.
On 03/22/2013 11:36 AM, Russell King - ARM Linux wrote: > On Wed, Mar 20, 2013 at 01:28:47PM -0500, Jon Hunter wrote: >> Sorry I am now not sure I follow you here. Someone just pointed out to >> me that PTR_RET() is defined as ... >> >> static inline int __must_check PTR_RET(const void *ptr) >> { >> if (IS_ERR(ptr)) >> return PTR_ERR(ptr); >> else >> return 0; >> } >> >> So the above change appears to be equivalent. Is there something that is >> wrong with the current implementation that needs to be fixed? > > No - I misread it as PTR_ERR not PTR_RET. Your patch is fine. Thanks for confirming. I had made the same mistake recently too! Jon
diff --git a/arch/arm/mach-omap2/devices.c b/arch/arm/mach-omap2/devices.c index 1ec7f05..2a0816e 100644 --- a/arch/arm/mach-omap2/devices.c +++ b/arch/arm/mach-omap2/devices.c @@ -66,7 +66,7 @@ static int __init omap3_l3_init(void) WARN(IS_ERR(pdev), "could not build omap_device for %s\n", oh_name); - return IS_ERR(pdev) ? PTR_ERR(pdev) : 0; + return PTR_RET(pdev); } omap_postcore_initcall(omap3_l3_init); @@ -100,7 +100,7 @@ static int __init omap4_l3_init(void) WARN(IS_ERR(pdev), "could not build omap_device for %s\n", oh_name); - return IS_ERR(pdev) ? PTR_ERR(pdev) : 0; + return PTR_RET(pdev); } omap_postcore_initcall(omap4_l3_init); diff --git a/arch/arm/mach-omap2/fb.c b/arch/arm/mach-omap2/fb.c index 190ae49..2ca33cc 100644 --- a/arch/arm/mach-omap2/fb.c +++ b/arch/arm/mach-omap2/fb.c @@ -83,10 +83,7 @@ static int __init omap_init_vrfb(void) pdev = platform_device_register_resndata(NULL, "omapvrfb", -1, res, num_res, NULL, 0); - if (IS_ERR(pdev)) - return PTR_ERR(pdev); - else - return 0; + return PTR_RET(pdev); } omap_arch_initcall(omap_init_vrfb); diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c index 410e1ba..c665721 100644 --- a/arch/arm/mach-omap2/gpmc.c +++ b/arch/arm/mach-omap2/gpmc.c @@ -1448,7 +1448,7 @@ static int __init omap_gpmc_init(void) pdev = omap_device_build(DEVICE_NAME, -1, oh, NULL, 0); WARN(IS_ERR(pdev), "could not build omap_device for %s\n", oh_name); - return IS_ERR(pdev) ? PTR_ERR(pdev) : 0; + return PTR_RET(pdev); } omap_postcore_initcall(omap_gpmc_init); diff --git a/arch/arm/mach-omap2/pmu.c b/arch/arm/mach-omap2/pmu.c index 9debf82..13ff2f5 100644 --- a/arch/arm/mach-omap2/pmu.c +++ b/arch/arm/mach-omap2/pmu.c @@ -52,10 +52,7 @@ static int __init omap2_init_pmu(unsigned oh_num, char *oh_names[]) WARN(IS_ERR(omap_pmu_dev), "Can't build omap_device for %s.\n", dev_name); - if (IS_ERR(omap_pmu_dev)) - return PTR_ERR(omap_pmu_dev); - - return 0; + return PTR_RET(omap_pmu_dev); } static int __init omap_init_pmu(void)
This uses PTR_RET instead of IS_ERR and PTR_ERR in order to increase readability. Signed-off-by: Silviu-Mihai Popescu <silviupopescu1990@gmail.com> --- arch/arm/mach-omap2/devices.c | 4 ++-- arch/arm/mach-omap2/fb.c | 5 +---- arch/arm/mach-omap2/gpmc.c | 2 +- arch/arm/mach-omap2/pmu.c | 5 +---- 4 files changed, 5 insertions(+), 11 deletions(-)