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 =====
--- 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(¶m, 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);
}
Hi,
On Fri, 21 Jan 2005 10:14:46 -0500, Prarit Bhargava <[email protected]> wrote:
> Hi,
>
> The following patch cleans up resource allocations in the i8042 driver
> when initialization fails.
>
...
>
> if (i8042_command(¶m, 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
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 <[email protected]> wrote:
> > Hi,
> >
> > The following patch cleans up resource allocations in the i8042 driver
> > when initialization fails.
> >
> ...
> >
> > if (i8042_command(¶m, 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
On Fri, 21 Jan 2005 17:35:40 +0100, Vojtech Pavlik <[email protected]> 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 <[email protected]> wrote:
> > > Hi,
> > >
> > > The following patch cleans up resource allocations in the i8042 driver
> > > when initialization fails.
> > >
> > ...
> > >
> > > if (i8042_command(¶m, 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
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
===== 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(¶m, 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);
}
On Fri, Jan 21, 2005 at 10:14:46AM -0500, Prarit Bhargava wrote:
> Signed-off-by: Prarit Bhargava <[email protected]>
>
> ===== 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(¶m, 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
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------
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 [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 =====
>--- 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(¶m, 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);
> }
>
>
>
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
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 [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 =====
> >--- 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(¶m, 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