2005-12-15 09:19:03

by Al Viro

Subject: [PATCH] tlclk.c: pointers are handled by %p

Signed-off-by: Al Viro <[email protected]>


---

drivers/char/tlclk.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)

f1bbf945c86b729c20199133daba358284fa3c32
diff --git a/drivers/char/tlclk.c b/drivers/char/tlclk.c
index 12167c0..e8467dc 100644
--- a/drivers/char/tlclk.c
+++ b/drivers/char/tlclk.c
@@ -776,8 +776,8 @@ static int __init tlclk_init(void)
tlclk_device = platform_device_register_simple("telco_clock",
-1, NULL, 0);
if (!tlclk_device) {
- printk(KERN_ERR " platform_device_register retruns 0x%X\n",
- (unsigned int) tlclk_device);
+ printk(KERN_ERR " platform_device_register retruns 0x%p\n",
+ tlclk_device);
ret = -EBUSY;
goto out4;
}
--
0.99.9.GIT

2005-12-15 09:58:01

by Russell King

Subject: Re: [PATCH] tlclk.c: pointers are handled by %p

On Thu, Dec 15, 2005 at 09:18:35AM +0000, Al Viro wrote:
> diff --git a/drivers/char/tlclk.c b/drivers/char/tlclk.c
> index 12167c0..e8467dc 100644
> --- a/drivers/char/tlclk.c
> +++ b/drivers/char/tlclk.c
> @@ -776,8 +776,8 @@ static int __init tlclk_init(void)
> tlclk_device = platform_device_register_simple("telco_clock",
> -1, NULL, 0);
> if (!tlclk_device) {
> - printk(KERN_ERR " platform_device_register retruns 0x%X\n",
> - (unsigned int) tlclk_device);
> + printk(KERN_ERR " platform_device_register retruns 0x%p\n",
> + tlclk_device);

This looks really strange - we know what tlclk_device will be at that
printk - it'll be NULL because if it's anything different we wouldn't
be inside this if(){ }.

Moreover, this code is obviously bogus. platform_device_register_simple
does not return NULL for the error case. It should be something like:

if (IS_ERR(tlclk_device)) {
ret = PTR_ERR(tlclk_device);
printk(KERN_ERR "platform_device_register returns %d\n",
ret);
goto out4;
}

--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of: 2.6 Serial core

2005-12-16 06:55:55

by Dmitry Torokhov

Subject: Re: [PATCH] tlclk.c: pointers are handled by %p

On Thursday 15 December 2005 04:57, Russell King wrote:
> On Thu, Dec 15, 2005 at 09:18:35AM +0000, Al Viro wrote:
> > diff --git a/drivers/char/tlclk.c b/drivers/char/tlclk.c
> > index 12167c0..e8467dc 100644
> > --- a/drivers/char/tlclk.c
> > +++ b/drivers/char/tlclk.c
> > @@ -776,8 +776,8 @@ static int __init tlclk_init(void)
> > tlclk_device = platform_device_register_simple("telco_clock",
> > -1, NULL, 0);
> > if (!tlclk_device) {
> > - printk(KERN_ERR " platform_device_register retruns 0x%X\n",
> > - (unsigned int) tlclk_device);
> > + printk(KERN_ERR " platform_device_register retruns 0x%p\n",
> > + tlclk_device);
>
> This looks really strange - we know what tlclk_device will be at that
> printk - it'll be NULL because if it's anything different we wouldn't
> be inside this if(){ }.
>
> Moreover, this code is obviously bogus. platform_device_register_simple
> does not return NULL for the error case. It should be something like:
>
> if (IS_ERR(tlclk_device)) {
> ret = PTR_ERR(tlclk_device);
> printk(KERN_ERR "platform_device_register returns %d\n",
> ret);
> goto out4;
> }
>

I have a patch killing usage of platform_register_device_simple in this
driver (converting to platform_device_alloc() + _add()). Will post in a
minute.

--
Dmitry