[PATCH][RFC]: Clean up resource allocation in i8042 driver

[PATCH][RFC]: Clean up resource allocation in i8042 driver

[Prarit Bhargava]

Hi,

The following patch cleans up resource allocations in the i8042 driver 
when initialization fails.

Please consider for tree application.  Patch is generated against 
current bk pull.

Thanks,

P.

Signed-off-by: Prarit Bhargava <prarit@sgi.com>

===== i8042.c 1.71 vs edited =====
--- 1.71/drivers/input/serio/i8042.c    2005-01-03 08:11:49 -05:00
+++ edited/i8042.c      2005-01-21 10:02:20 -05:00
@@ -696,7 +696,10 @@
                unsigned char param;
 
                if (i8042_command(&param, I8042_CMD_CTL_TEST)) {
-                       printk(KERN_ERR "i8042.c: i8042 controller self test timeout.\n");
+                       if (i8042_read_status() != 0xFF)
+                               printk(KERN_ERR "i8042.c: i8042 controller self test timeout.\n");
+                       else
+                               printk(KERN_ERR "i8042.c: no i8042 controller found.\n");
                        return -1;
                }
 
                }

@@ -1011,21 +1014,34 @@
        i8042_timer.function = i8042_timer_func;
 
        if (i8042_platform_init())
+       {
+               del_timer_sync(&i8042_timer);
                return -EBUSY;
+       }
 
        i8042_aux_values.irq = I8042_AUX_IRQ;
        i8042_kbd_values.irq = I8042_KBD_IRQ;
 
        if (i8042_controller_init())
+       {
+               i8042_platform_exit();
+               del_timer_sync(&i8042_timer);
                return -ENODEV;
+       }
 
        err = driver_register(&i8042_driver);
        if (err)
+       {
+               i8042_platform_exit();
+               del_timer_sync(&i8042_timer);
                return err;
+       }

        i8042_platform_device = platform_device_register_simple("i8042", -1, NULL, 0);
        if (IS_ERR(i8042_platform_device)) {
                driver_unregister(&i8042_driver);
+               i8042_platform_exit();
+               del_timer_sync(&i8042_timer);
                return PTR_ERR(i8042_platform_device);
        }

Re: [PATCH][RFC]: Clean up resource allocation in i8042 driver

[Dmitry Torokhov]

Hi,

On Fri, 21 Jan 2005 10:14:46 -0500, Prarit Bhargava <prarit@sgi.com> wrote:
> Hi,
> 
> The following patch cleans up resource allocations in the i8042 driver
> when initialization fails.
> 
...
> 
>                if (i8042_command(&param, I8042_CMD_CTL_TEST)) {
> -                       printk(KERN_ERR "i8042.c: i8042 controller self test timeout.\n");
> +                       if (i8042_read_status() != 0xFF)
> +                               printk(KERN_ERR "i8042.c: i8042 controller self test timeout.\n");
> +                       else
> +                               printk(KERN_ERR "i8042.c: no i8042 controller found.\n");

Is this documented somewhere?

> 
>        if (i8042_platform_init())
> +       {
> +               del_timer_sync(&i8042_timer);
>                return -EBUSY;
> +       }
> 

Couple of comments:
 - i8042_timer has not been started yet so there is no need to delete
it in either of the chinks.
- opening brace placement does not follow Linux coding style.

I think I have some changes to i8042 in my tree, I will add
i8042_platform_exit calls to the init routine. Thanks for noticing it!

-- 
Dmitry

Re: [PATCH][RFC]: Clean up resource allocation in i8042 driver

[Vojtech Pavlik]

On Fri, Jan 21, 2005 at 10:43:36AM -0500, Dmitry Torokhov wrote:
> Hi,
> 
> On Fri, 21 Jan 2005 10:14:46 -0500, Prarit Bhargava <prarit@sgi.com> wrote:
> > Hi,
> > 
> > The following patch cleans up resource allocations in the i8042 driver
> > when initialization fails.
> > 
> ...
> > 
> >                if (i8042_command(&param, I8042_CMD_CTL_TEST)) {
> > -                       printk(KERN_ERR "i8042.c: i8042 controller self test timeout.\n");
> > +                       if (i8042_read_status() != 0xFF)
> > +                               printk(KERN_ERR "i8042.c: i8042 controller self test timeout.\n");
> > +                       else
> > +                               printk(KERN_ERR "i8042.c: no i8042 controller found.\n");
> 
> Is this documented somewhere?

No. But vacant ports usually return 0xff. The problem here is that 0xff
is a valid value for the status register, too. Fortunately this patch
checks for 0xff only after the timeout failed.

Anyway, I suppose we could fail silently here on ia64 machines where
ACPI is present.

> >        if (i8042_platform_init())
> > +       {
> > +               del_timer_sync(&i8042_timer);
> >                return -EBUSY;
> > +       }
> > 
> 
> Couple of comments:
>  - i8042_timer has not been started yet so there is no need to delete
> it in either of the chinks.

Indeed.

> - opening brace placement does not follow Linux coding style.
> 
> I think I have some changes to i8042 in my tree, I will add
> i8042_platform_exit calls to the init routine. Thanks for noticing it!

-- 
Vojtech Pavlik
SuSE Labs, SuSE CR

Re: [PATCH][RFC]: Clean up resource allocation in i8042 driver

[Dmitry Torokhov]

On Fri, 21 Jan 2005 17:35:40 +0100, Vojtech Pavlik <vojtech@suse.cz> wrote:
> On Fri, Jan 21, 2005 at 10:43:36AM -0500, Dmitry Torokhov wrote:
> > Hi,
> >
> > On Fri, 21 Jan 2005 10:14:46 -0500, Prarit Bhargava <prarit@sgi.com> wrote:
> > > Hi,
> > >
> > > The following patch cleans up resource allocations in the i8042 driver
> > > when initialization fails.
> > >
> > ...
> > >
> > >                if (i8042_command(&param, I8042_CMD_CTL_TEST)) {
> > > -                       printk(KERN_ERR "i8042.c: i8042 controller self test timeout.\n");
> > > +                       if (i8042_read_status() != 0xFF)
> > > +                               printk(KERN_ERR "i8042.c: i8042 controller self test timeout.\n");
> > > +                       else
> > > +                               printk(KERN_ERR "i8042.c: no i8042 controller found.\n");
> >
> > Is this documented somewhere?
> 
> No. But vacant ports usually return 0xff. The problem here is that 0xff
> is a valid value for the status register, too. Fortunately this patch
> checks for 0xff only after the timeout failed.
> 
> Anyway, I suppose we could fail silently here on ia64 machines where
> ACPI is present.

But it ACPI is present but neither KBD nor PS mouse port is defined in
DSDT (or they not active as far as _STR goes) i8042_plantorm_init will
fail and we won't even get there...

-- 
Dmitry

Re: [PATCH][RFC]: Clean up resource allocation in i8042 driver

[Jesse Barnes]

On Friday, January 21, 2005 8:35 am, Vojtech Pavlik wrote:
> No. But vacant ports usually return 0xff. The problem here is that 0xff
> is a valid value for the status register, too. Fortunately this patch
> checks for 0xff only after the timeout failed.

On PCs you'll get all 1s, but on some ia64 platforms and others, you'll take a 
hard machine check exception if you try to access non-existent memory (mmio, 
port space, or otherwise).

Jesse

Re: [PATCH][RFC]: Clean up resource allocation in i8042 driver

[Prarit Bhargava]

[-- Attachment #1: Type: text/plain, Size: 848 bytes --]

I've taken into account Dmitry's comments (thanks Dmitry!) and generated 
a new patch.

Thanks,

P.
Jesse Barnes wrote:

>On Friday, January 21, 2005 8:35 am, Vojtech Pavlik wrote:
>  
>
>>No. But vacant ports usually return 0xff. The problem here is that 0xff
>>is a valid value for the status register, too. Fortunately this patch
>>checks for 0xff only after the timeout failed.
>>    
>>
>
>On PCs you'll get all 1s, but on some ia64 platforms and others, you'll take a 
>hard machine check exception if you try to access non-existent memory (mmio, 
>port space, or otherwise).
>
>Jesse
>-
>To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html
>Please read the FAQ at  http://www.tux.org/lkml/
>
>  
>

[-- Attachment #2: i8042.c.diff --]
[-- Type: text/plain, Size: 1114 bytes --]

===== i8042.c 1.71 vs edited =====
--- 1.71/drivers/input/serio/i8042.c	2005-01-03 08:11:49 -05:00
+++ edited/i8042.c	2005-01-21 11:50:11 -05:00
@@ -696,7 +696,10 @@
 		unsigned char param;
 
 		if (i8042_command(&param, I8042_CMD_CTL_TEST)) {
-			printk(KERN_ERR "i8042.c: i8042 controller self test timeout.\n");
+			if (i8042_read_status() != 0xFF)
+				printk(KERN_ERR "i8042.c: i8042 controller self test timeout.\n");
+			else
+				printk(KERN_ERR "i8042.c: no i8042 controller found.\n");
 			return -1;
 		}
 
@@ -1016,16 +1019,22 @@
 	i8042_aux_values.irq = I8042_AUX_IRQ;
 	i8042_kbd_values.irq = I8042_KBD_IRQ;
 
-	if (i8042_controller_init())
+	if (i8042_controller_init()) {
+		i8042_platform_exit();
 		return -ENODEV;
+	}
 
 	err = driver_register(&i8042_driver);
-	if (err)
+	if (err) {
+		i8042_platform_exit();
 		return err;
+	}
 
 	i8042_platform_device = platform_device_register_simple("i8042", -1, NULL, 0);
 	if (IS_ERR(i8042_platform_device)) {
 		driver_unregister(&i8042_driver);
+		i8042_platform_exit();
+		del_timer_sync(&i8042_timer);
 		return PTR_ERR(i8042_platform_device);
 	}
 

Re: [PATCH][RFC]: Clean up resource allocation in i8042 driver

[Prarit Bhargava]

I didn't see a final ACK on this patch -- just checking for one :)

P.

Prarit Bhargava wrote:

> I've taken into account Dmitry's comments (thanks Dmitry!) and 
> generated a new patch.
>
> Thanks,
>
> P.
> Jesse Barnes wrote:
>
>> On Friday, January 21, 2005 8:35 am, Vojtech Pavlik wrote:
>>  
>>
>>> No. But vacant ports usually return 0xff. The problem here is that 0xff
>>> is a valid value for the status register, too. Fortunately this patch
>>> checks for 0xff only after the timeout failed.
>>>   
>>
>>
>> On PCs you'll get all 1s, but on some ia64 platforms and others, 
>> you'll take a hard machine check exception if you try to access 
>> non-existent memory (mmio, port space, or otherwise).
>>
>> Jesse
>> -
>> To unsubscribe from this list: send the line "unsubscribe 
>> linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/
>>
>>  
>>
>------------------------------------------------------------------------
>
>===== i8042.c 1.71 vs edited =====
>--- 1.71/drivers/input/serio/i8042.c	2005-01-03 08:11:49 -05:00
>+++ edited/i8042.c	2005-01-21 11:50:11 -05:00
>@@ -696,7 +696,10 @@
> 		unsigned char param;
> 
> 		if (i8042_command(&param, I8042_CMD_CTL_TEST)) {
>-			printk(KERN_ERR "i8042.c: i8042 controller self test timeout.\n");
>+			if (i8042_read_status() != 0xFF)
>+				printk(KERN_ERR "i8042.c: i8042 controller self test timeout.\n");
>+			else
>+				printk(KERN_ERR "i8042.c: no i8042 controller found.\n");
> 			return -1;
> 		}
> 
>@@ -1016,16 +1019,22 @@
> 	i8042_aux_values.irq = I8042_AUX_IRQ;
> 	i8042_kbd_values.irq = I8042_KBD_IRQ;
> 
>-	if (i8042_controller_init())
>+	if (i8042_controller_init()) {
>+		i8042_platform_exit();
> 		return -ENODEV;
>+	}
> 
> 	err = driver_register(&i8042_driver);
>-	if (err)
>+	if (err) {
>+		i8042_platform_exit();
> 		return err;
>+	}
> 
> 	i8042_platform_device = platform_device_register_simple("i8042", -1, NULL, 0);
> 	if (IS_ERR(i8042_platform_device)) {
> 		driver_unregister(&i8042_driver);
>+		i8042_platform_exit();
>+		del_timer_sync(&i8042_timer);
> 		return PTR_ERR(i8042_platform_device);
> 	}
> 
>  
>

Re: [PATCH][RFC]: Clean up resource allocation in i8042 driver

[Dmitry Torokhov]

On Monday 14 February 2005 11:32, Prarit Bhargava wrote:
> I didn't see a final ACK on this patch -- just checking for one :)
> 
> P.

I see that resource allocation part is in Vojtech's tree now but the
part changing timeout message was dropped.

-- 
Dmitry

Re: [PATCH][RFC]: Clean up resource allocation in i8042 driver

[Vojtech Pavlik]

On Mon, Feb 14, 2005 at 11:32:30AM -0500, Prarit Bhargava wrote:

> I didn't see a final ACK on this patch -- just checking for one :)

The patch was redone - we now check for error from the 'flush' command
before sending CTL_TEST.

> P.
> 
> Prarit Bhargava wrote:
> 
> >I've taken into account Dmitry's comments (thanks Dmitry!) and 
> >generated a new patch.
> >
> >Thanks,
> >
> >P.
> >Jesse Barnes wrote:
> >
> >>On Friday, January 21, 2005 8:35 am, Vojtech Pavlik wrote:
> >> 
> >>
> >>>No. But vacant ports usually return 0xff. The problem here is that 0xff
> >>>is a valid value for the status register, too. Fortunately this patch
> >>>checks for 0xff only after the timeout failed.
> >>>  
> >>
> >>
> >>On PCs you'll get all 1s, but on some ia64 platforms and others, 
> >>you'll take a hard machine check exception if you try to access 
> >>non-existent memory (mmio, port space, or otherwise).
> >>
> >>Jesse
> >>-
> >>To unsubscribe from this list: send the line "unsubscribe 
> >>linux-kernel" in
> >>the body of a message to majordomo@vger.kernel.org
> >>More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >>Please read the FAQ at  http://www.tux.org/lkml/
> >>
> >> 
> >>
> >------------------------------------------------------------------------
> >
> >===== i8042.c 1.71 vs edited =====
> >--- 1.71/drivers/input/serio/i8042.c	2005-01-03 08:11:49 -05:00
> >+++ edited/i8042.c	2005-01-21 11:50:11 -05:00
> >@@ -696,7 +696,10 @@
> >		unsigned char param;
> >
> >		if (i8042_command(&param, I8042_CMD_CTL_TEST)) {
> >-			printk(KERN_ERR "i8042.c: i8042 controller self test 
> >timeout.\n");
> >+			if (i8042_read_status() != 0xFF)
> >+				printk(KERN_ERR "i8042.c: i8042 controller 
> >self test timeout.\n");
> >+			else
> >+				printk(KERN_ERR "i8042.c: no i8042 
> >controller found.\n");
> >			return -1;
> >		}
> >
> >@@ -1016,16 +1019,22 @@
> >	i8042_aux_values.irq = I8042_AUX_IRQ;
> >	i8042_kbd_values.irq = I8042_KBD_IRQ;
> >
> >-	if (i8042_controller_init())
> >+	if (i8042_controller_init()) {
> >+		i8042_platform_exit();
> >		return -ENODEV;
> >+	}
> >
> >	err = driver_register(&i8042_driver);
> >-	if (err)
> >+	if (err) {
> >+		i8042_platform_exit();
> >		return err;
> >+	}
> >
> >	i8042_platform_device = platform_device_register_simple("i8042", -1, 
> >	NULL, 0);
> >	if (IS_ERR(i8042_platform_device)) {
> >		driver_unregister(&i8042_driver);
> >+		i8042_platform_exit();
> >+		del_timer_sync(&i8042_timer);
> >		return PTR_ERR(i8042_platform_device);
> >	}
> >
> > 
> >
> 

-- 
Vojtech Pavlik
SuSE Labs, SuSE CR

Re: [PATCH][RFC]: Clean up resource allocation in i8042 driver

[Dave Jones]

On Fri, Jan 21, 2005 at 10:14:46AM -0500, Prarit Bhargava wrote:

 > Signed-off-by: Prarit Bhargava <prarit@sgi.com>
 > 
 > ===== i8042.c 1.71 vs edited =====
 > --- 1.71/drivers/input/serio/i8042.c    2005-01-03 08:11:49 -05:00
 > +++ edited/i8042.c      2005-01-21 10:02:20 -05:00
 > @@ -696,7 +696,10 @@
 >                unsigned char param;
 > 
 >                if (i8042_command(&param, I8042_CMD_CTL_TEST)) {
 > -                       printk(KERN_ERR "i8042.c: i8042 controller self 
 > test timeout.\n");

wordwrapped patch.
		
 > +                       if (i8042_read_status() != 0xFF)
 > +                               printk(KERN_ERR "i8042.c: i8042 controller 
 > self test timeout.\n");
 > +                       else
 > +                               printk(KERN_ERR "i8042.c: no i8042 
 > controller found.\n");
 >                        return -1;
 >                }
 > 
 >                }

I doubt these }'s should line up, broken indentation ?
 
 > @@ -1011,21 +1014,34 @@
 >        i8042_timer.function = i8042_timer_func;
 > 
 >        if (i8042_platform_init())
 > +       {
 > +               del_timer_sync(&i8042_timer);
 >                return -EBUSY;
 > +       }

Spaces instead of tabs. Oops.
(Also the { should be on the same line as the if)

Nitpicking..

		Dave

Re: [PATCH][RFC]: Clean up resource allocation in i8042 driver

[Kyle Moffett]

On Jan 21, 2005, at 14:26, Dave Jones wrote:
>> -                       printk(KERN_ERR "i8042.c: i8042 controller 
>> self
>> test timeout.\n");
> wordwrapped patch.

Err, it showed up fine for me.  I suspect your mail client or server is
mangling emails.

>>                }
>>
>>                }
> I doubt these }'s should line up, broken indentation ?

Likewise, looks fine to me.

Cheers,
Kyle Moffett

-----BEGIN GEEK CODE BLOCK-----
Version: 3.12
GCM/CS/IT/U d- s++: a18 C++++>$ UB/L/X/*++++(+)>$ P+++(++++)>$
L++++(+++) E W++(+) N+++(++) o? K? w--- O? M++ V? PS+() PE+(-) Y+
PGP+++ t+(+++) 5 X R? tv-(--) b++++(++) DI+ D+ G e->++++$ h!*()>++$ r  
!y?(-)
------END GEEK CODE BLOCK------