Skip to contentSkip to conversation

: Clean up resource allocation in i8042 driver

RFC PATCH In review Prarit Bhargava 7 participants 11 messages Jan 21, 2005 – Feb 15, 2005 Input drivers

lore
Share & tools
lore

Prarit Bhargava

author
#0 lore raw

RFC PATCH 1/1 : Clean up resource allocation in i8042 driver

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 <[email protected]>

===== i8042.c 1.71 vs edited =====

i8042.c 2005-01-21 10:02:20 -05:00 renamed from drivers/input/serio/i8042.c 2005-01-03 08:11:49 -05:00 +4 -1
@@ -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);
       }
maintainer
#1 lore raw

Hi,

Quoted 5 lines · Prarit Bhargava

Hi,

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

...

i8042.c 2005-01-21 10:02:20 -05:00 line 702

Quoted 7 lines
               if (i8042_command(&param, I8042_CMD_CTL_TEST)) {
i8042.c 2005-01-21 10:02:20 -05:00 renamed from drivers/input/serio/i8042.c 2005-01-03 08:11:49 -05:00 +4 -1
-                       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?

Review

Quoted 7 lines
       if (i8042_platform_init())
(quoted diff) +3 -0
+       {
+               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!

Signature · 1 line
Dmitry
#2 lore raw
Quoted 10 lines · Dmitry Torokhov

Hi,

Quoted 5 lines · Prarit Bhargava

Hi,

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

...

Quoted 1 line
Quoted 8 lines

i8042.c 2005-01-21 10:02:20 -05:00 line 702

Quoted 6 lines
               if (i8042_command(&param, I8042_CMD_CTL_TEST)) {
i8042.c 2005-01-21 10:02:20 -05:00 renamed from drivers/input/serio/i8042.c 2005-01-03 08:11:49 -05:00 +4 -1
-                       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.

Quoted 10 lines

Review

Quoted 6 lines
       if (i8042_platform_init())
(quoted diff) +3 -0
+       {
+               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.

Quoted 4 lines

- 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!

Signature · 2 lines
Vojtech Pavlik
SuSE Labs, SuSE CR
maintainer
#3 lore raw
Quoted 18 lines · Vojtech Pavlik
Quoted 17 lines · Dmitry Torokhov

Hi,

Quoted 5 lines · Prarit Bhargava

Hi,

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

...

Quoted 7 lines
               if (i8042_command(&param, I8042_CMD_CTL_TEST)) {
i8042.c 2005-01-21 10:02:20 -05:00 renamed from drivers/input/serio/i8042.c 2005-01-03 08:11:49 -05:00 +4 -1
-                       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");
Quoted 8 lines
Quoted 1 line

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...

Signature · 1 line
Dmitry
#4 lore raw
In reply to Vojtech Pavlik
Quoted 3 lines · Vojtech Pavlik

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

author
#5 lore raw

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

Thanks,

P.

Quoted 9 lines · Jesse Barnes
Quoted 5 lines · Vojtech Pavlik

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.

Quoted 11 lines

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 [email protected]
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 =====

i8042.c renamed from drivers/input/serio/i8042.c +12 -3
@@ -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);
 	}
#9 lore raw
In reply to Prarit Bhargava

i8042.c 2005-01-21 10:02:20 -05:00 line 699

Quoted 11 lines · Prarit Bhargava
Signed-off-by Prarit Bhargava <[email protected]>

===== i8042.c 1.71 vs edited =====

i8042.c 2005-01-21 10:02:20 -05:00 renamed from drivers/input/serio/i8042.c 2005-01-03 08:11:49 -05:00 +0 -1
@@ -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.

Quoted 10 lines
i8042.c 2005-01-21 10:02:20 -05:00 renamed from drivers/input/serio/i8042.c 2005-01-03 08:11:49 -05:00 +4 -0
+                       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 ?

i8042.c 2005-01-21 10:02:20 -05:00 line 1020

Quoted 8 lines
i8042.c 2005-01-21 10:02:20 -05:00 renamed from drivers/input/serio/i8042.c 2005-01-03 08:11:49 -05:00 +3 -0
@@ -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

#10 lore raw
Quoted 4 lines · Dave Jones
Quoted 3 lines

- 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.

Quoted 4 lines
Quoted 3 lines
               }

               }

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------

author
#6 lore raw
In reply to Prarit Bhargava

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

P.

Quoted 71 lines · Prarit Bhargava

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

Thanks,

P.

Quoted 9 lines · Jesse Barnes
Quoted 4 lines · Vojtech Pavlik

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.

Quoted 11 lines

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 [email protected]
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 =====

i8042.c renamed from drivers/input/serio/i8042.c +12 -3
@@ -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);
 	}
#7 lore raw
Quoted 3 lines · Prarit Bhargava

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.

Signature · 1 line
Dmitry
#8 lore raw
In reply to Prarit Bhargava
Quoted 1 line · Prarit Bhargava

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.

Quoted 84 lines

P.

Quoted 79 lines · Prarit Bhargava

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

Thanks,

P.

Quoted 13 lines · Jesse Barnes
Quoted 4 lines · Vojtech Pavlik

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).

Quoted 10 lines

Jesse
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
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 =====

i8042.c renamed from drivers/input/serio/i8042.c +4 -1
@@ -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;
		}
i8042.c renamed from drivers/input/serio/i8042.c +8 -2
@@ -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);
 	}
Signature · 2 lines
Vojtech Pavlik
SuSE Labs, SuSE CR