* your patch "x86, 8042: Enable A20 using KBC to fix S3 resume on some MSI laptops"
@ 2013-01-14 8:08 Jan Beulich
2013-01-14 8:29 ` Ondrej Zary
0 siblings, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2013-01-14 8:08 UTC (permalink / raw)
To: linux; +Cc: dmitry.torokhov, hpa, Alan Cox, Rafael J. Wysocki, linux-kernel
Ondrej,
I see two problems with this patch: For one, on a system without
i8042 the code at the place it got inserted ought to incur a stall of
1s (50us * I8042_CTL_TIMEOUT [10000] * 2). I believe that this
code should not be run before i8042_controller_check() completed
successfully, but at the very least the second call to
i8042_command() should be conditional upon the first being
successful (effectively halving the stall).
Second, considering that enabling A20 (even if just in a fake way),
is a core system operation, I don't think it belongs into a driver
that is only optionally present in the kernel.
Jan
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: your patch "x86, 8042: Enable A20 using KBC to fix S3 resume on some MSI laptops"
2013-01-14 8:08 your patch "x86, 8042: Enable A20 using KBC to fix S3 resume on some MSI laptops" Jan Beulich
@ 2013-01-14 8:29 ` Ondrej Zary
2013-01-14 8:37 ` Jan Beulich
0 siblings, 1 reply; 6+ messages in thread
From: Ondrej Zary @ 2013-01-14 8:29 UTC (permalink / raw)
To: Jan Beulich
Cc: dmitry.torokhov, hpa, Alan Cox, Rafael J. Wysocki, linux-kernel
On Monday 14 January 2013, Jan Beulich wrote:
> Ondrej,
>
> I see two problems with this patch: For one, on a system without
> i8042 the code at the place it got inserted ought to incur a stall of
> 1s (50us * I8042_CTL_TIMEOUT [10000] * 2). I believe that this
> code should not be run before i8042_controller_check() completed
> successfully, but at the very least the second call to
> i8042_command() should be conditional upon the first being
> successful (effectively halving the stall).
I believe that all PnP-capable systems without 8042 will exit with -ENODEV
after x86_platform.i8042_detect(). Old non-PnP systems usually have 8042.
> Second, considering that enabling A20 (even if just in a fake way),
> is a core system operation, I don't think it belongs into a driver
> that is only optionally present in the kernel.
The first version of this patch added A20 enabling to early init code. But
that could be dangerous as it was run before any 8042 detection, possibly
breaking systems without 8042. I haven't found a better place for this.
--
Ondrej Zary
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: your patch "x86, 8042: Enable A20 using KBC to fix S3 resume on some MSI laptops"
2013-01-14 8:29 ` Ondrej Zary
@ 2013-01-14 8:37 ` Jan Beulich
2013-01-14 8:54 ` Ondrej Zary
0 siblings, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2013-01-14 8:37 UTC (permalink / raw)
To: Ondrej Zary
Cc: dmitry.torokhov, hpa, Alan Cox, Rafael J. Wysocki, linux-kernel
>>> On 14.01.13 at 09:29, Ondrej Zary <linux@rainbow-software.org> wrote:
> On Monday 14 January 2013, Jan Beulich wrote:
>> Ondrej,
>>
>> I see two problems with this patch: For one, on a system without
>> i8042 the code at the place it got inserted ought to incur a stall of
>> 1s (50us * I8042_CTL_TIMEOUT [10000] * 2). I believe that this
>> code should not be run before i8042_controller_check() completed
>> successfully, but at the very least the second call to
>> i8042_command() should be conditional upon the first being
>> successful (effectively halving the stall).
>
> I believe that all PnP-capable systems without 8042 will exit with -ENODEV
> after x86_platform.i8042_detect().
How that, with default_i8042_detect() being just "return 1;"?
> Old non-PnP systems usually have 8042.
Sure.
>> Second, considering that enabling A20 (even if just in a fake way),
>> is a core system operation, I don't think it belongs into a driver
>> that is only optionally present in the kernel.
>
> The first version of this patch added A20 enabling to early init code. But
> that could be dangerous as it was run before any 8042 detection, possibly
> breaking systems without 8042. I haven't found a better place for this.
I realize that the change would be more intrusive, but imo that's
not an excuse for not doing it properly.
Jan
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: your patch "x86, 8042: Enable A20 using KBC to fix S3 resume on some MSI laptops"
2013-01-14 8:37 ` Jan Beulich
@ 2013-01-14 8:54 ` Ondrej Zary
2013-01-14 9:02 ` Jan Beulich
0 siblings, 1 reply; 6+ messages in thread
From: Ondrej Zary @ 2013-01-14 8:54 UTC (permalink / raw)
To: Jan Beulich
Cc: dmitry.torokhov, hpa, Alan Cox, Rafael J. Wysocki, linux-kernel
On Monday 14 January 2013, Jan Beulich wrote:
> >>> On 14.01.13 at 09:29, Ondrej Zary <linux@rainbow-software.org> wrote:
> >
> > On Monday 14 January 2013, Jan Beulich wrote:
> >> Ondrej,
> >>
> >> I see two problems with this patch: For one, on a system without
> >> i8042 the code at the place it got inserted ought to incur a stall of
> >> 1s (50us * I8042_CTL_TIMEOUT [10000] * 2). I believe that this
> >> code should not be run before i8042_controller_check() completed
> >> successfully, but at the very least the second call to
> >> i8042_command() should be conditional upon the first being
> >> successful (effectively halving the stall).
> >
> > I believe that all PnP-capable systems without 8042 will exit with
> > -ENODEV after x86_platform.i8042_detect().
>
> How that, with default_i8042_detect() being just "return 1;"?
Oops, I meant i8042_pnp_init(). But looking at the code again, it always
returns 0, even when the controller is not found.
So probably the right place is after i8042_controller_check().
> > Old non-PnP systems usually have 8042.
>
> Sure.
>
> >> Second, considering that enabling A20 (even if just in a fake way),
> >> is a core system operation, I don't think it belongs into a driver
> >> that is only optionally present in the kernel.
> >
> > The first version of this patch added A20 enabling to early init code.
> > But that could be dangerous as it was run before any 8042 detection,
> > possibly breaking systems without 8042. I haven't found a better place
> > for this.
>
> I realize that the change would be more intrusive, but imo that's
> not an excuse for not doing it properly.
What's "properly"? Doing extra 8042 detection somewhere else does not look
right.
--
Ondrej Zary
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: your patch "x86, 8042: Enable A20 using KBC to fix S3 resume on some MSI laptops"
2013-01-14 8:54 ` Ondrej Zary
@ 2013-01-14 9:02 ` Jan Beulich
2013-01-16 1:16 ` H. Peter Anvin
0 siblings, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2013-01-14 9:02 UTC (permalink / raw)
To: Ondrej Zary
Cc: dmitry.torokhov, hpa, Alan Cox, Rafael J. Wysocki, linux-kernel
>>> On 14.01.13 at 09:54, Ondrej Zary <linux@rainbow-software.org> wrote:
> On Monday 14 January 2013, Jan Beulich wrote:
>> >>> On 14.01.13 at 09:29, Ondrej Zary <linux@rainbow-software.org> wrote:
>> > On Monday 14 January 2013, Jan Beulich wrote:
>> >> Second, considering that enabling A20 (even if just in a fake way),
>> >> is a core system operation, I don't think it belongs into a driver
>> >> that is only optionally present in the kernel.
>> >
>> > The first version of this patch added A20 enabling to early init code.
>> > But that could be dangerous as it was run before any 8042 detection,
>> > possibly breaking systems without 8042. I haven't found a better place
>> > for this.
>>
>> I realize that the change would be more intrusive, but imo that's
>> not an excuse for not doing it properly.
>
> What's "properly"? Doing extra 8042 detection somewhere else does not look
> right.
Why?
Jan
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: your patch "x86, 8042: Enable A20 using KBC to fix S3 resume on some MSI laptops"
2013-01-14 9:02 ` Jan Beulich
@ 2013-01-16 1:16 ` H. Peter Anvin
0 siblings, 0 replies; 6+ messages in thread
From: H. Peter Anvin @ 2013-01-16 1:16 UTC (permalink / raw)
To: Jan Beulich
Cc: Ondrej Zary, dmitry.torokhov, Alan Cox, Rafael J. Wysocki, linux-kernel
On 01/14/2013 01:02 AM, Jan Beulich wrote:
>>
>> What's "properly"? Doing extra 8042 detection somewhere else does not look
>> right.
>
> Why?
>
Because it is time consuming, and threatens to step on things.
Enabling A20 is one thing, but this is something else... if we have the
KBC, we want to enable A20 *again* just in case. Having the i8042 code
in one place makes sense to me. This doesn't have to be done early.
The affected systems are *extremely* unlikely to ever be run without the
i8042 driver loaded, and the failure mode benign enough (if you actually
want to run a specific laptop without the keyboard driver, you can't
suspend it) that there really is no point to have yet another driver for
the same hardware that would stomp on each other.
-hpa
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-01-16 1:16 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-01-14 8:08 your patch "x86, 8042: Enable A20 using KBC to fix S3 resume on some MSI laptops" Jan Beulich
2013-01-14 8:29 ` Ondrej Zary
2013-01-14 8:37 ` Jan Beulich
2013-01-14 8:54 ` Ondrej Zary
2013-01-14 9:02 ` Jan Beulich
2013-01-16 1:16 ` H. Peter Anvin
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.