| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <69901d1c-4e48-0bcc-7716-f1d88953968d@redhat.com>
Date: Mon, 21 Mar 2022 15:51:06 +0100
From: Hans de Goede <hdegoede@...hat.com>
To: Werner Sembach <wse@...edocomputers.com>,
dmitry.torokhov@...il.com, tiwai@...e.de, mpdesouza@...e.com,
arnd@...db.de, samuel@...oj.net, linux-input@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 1/3] Input/i8042: Merge quirk tables
Hi,
On 3/21/22 15:25, Werner Sembach wrote:
>
> Am 09.03.22 um 18:16 schrieb Hans de Goede:
>> Hi,
>>
>> On 3/8/22 18:05, Werner Sembach wrote:
>>> Merge i8042 quirk tables to reduce code duplication for devices that need
>>> more than one quirk.
>>>
>>> Also align quirkable options with command line parameters and make vendor
>>> wide quirks per device overwriteable on a per device basis.
>>>
>>> Some duplication on the ASUS devices is required to mirror the exact
>>> behaviour of the previous code.
>> Can you explain this a bit more ?
> Yes, see next patch
Next patch as in:
https://lore.kernel.org/linux-input/20220308170523.783284-3-wse@tuxedocomputers.com/
? Or do you mean the next version of this patch-set ?
>>
>>
>>
>>> Signed-off-by: Werner Sembach <wse@...edocomputers.com>
>>> Cc: stable@...r.kernel.org
>>> ---
>>> drivers/input/serio/i8042-x86ia64io.h | 1125 ++++++++++++++-----------
>>> 1 file changed, 640 insertions(+), 485 deletions(-)
>>>
>>> diff --git a/drivers/input/serio/i8042-x86ia64io.h b/drivers/input/serio/i8042-x86ia64io.h
>>> index 148a7c5fd0e2..689b9ee3e742 100644
>>> --- a/drivers/input/serio/i8042-x86ia64io.h
>>> +++ b/drivers/input/serio/i8042-x86ia64io.h
>>> @@ -67,675 +67,821 @@ static inline void i8042_write_command(int val)
>>>
>>> #include <linux/dmi.h>
>>>
>>> -static const struct dmi_system_id __initconst i8042_dmi_noloop_table[] = {
>>> +#define SERIO_QUIRK_NOKBD BIT(0)
>>> +#define SERIO_QUIRK_NOAUX BIT(1)
>>> +#define SERIO_QUIRK_NOMUX BIT(2)
>>> +#define SERIO_QUIRK_FORCEMUX BIT(3)
>>> +#define SERIO_QUIRK_UNLOCK BIT(4)
>>> +#define SERIO_QUIRK_PROBE_DEFER BIT(5)
>>> +#define SERIO_QUIRK_RESET_ALWAYS BIT(6)
>>> +#define SERIO_QUIRK_RESET_NEVER BIT(7)
>>> +#define SERIO_QUIRK_DIECT BIT(8)
>>> +#define SERIO_QUIRK_DUMBKBD BIT(9)
>>> +#define SERIO_QUIRK_NOLOOP BIT(10)
>>> +#define SERIO_QUIRK_NOTIMEOUT BIT(11)
>>> +#define SERIO_QUIRK_KBDRESET BIT(12)
>>> +#define SERIO_QUIRK_DRITEK BIT(13)
>>> +#define SERIO_QUIRK_NOPNP BIT(14)
>>> +
>>> +/* Quirk table for different mainboards. Options similar or identical to i8042
>>> + * module parameters.
>>> + * ORDERING IS IMPORTANT! The first match will be apllied and the rest ignored.
>>> + * This allows entries to overwrite vendor wide quirks on a per device basis.
>>> + * Where this is irrelevant, entries are sorted case sensitive by DMI_SYS_VENDOR
>>> + * and/or DMI_BOARD_VENDOR to make it easier to avoid dublicate entries.
>>> + */
>>> +static const struct dmi_system_id i8042_dmi_quirk_table[] __initconst = {
>> <snip>
>>
>>> @@ -1167,11 +1307,6 @@ static int __init i8042_pnp_init(void)
>>> bool pnp_data_busted = false;
>>> int err;
>>>
>>> -#ifdef CONFIG_X86
>>> - if (dmi_check_system(i8042_dmi_nopnp_table))
>>> - i8042_nopnp = true;
>>> -#endif
>>> -
>>> if (i8042_nopnp) {
>>> pr_info("PNP detection disabled\n");
>>> return 0;
>> have you checked that i8042_platform_init() *always*
>> gets called before i8042_pnp_init()?
> As far as i can tell i8042_pnp_init() is only ever called inside i8042_platform_init() so moving this check from pnp
> init to platform init should be no problem.
Ok.
>>
>> Maybe just add something like this:
>>
>> #ifdef CONFIG_X86
>> static void __init i8042_check_quirks(void)
>> {
>> const struct dmi_system_id *device_quirk_info;
>> uintptr_t quirks;
>>
>> device_quirk_info = dmi_first_match(i8042_dmi_quirk_table);
>> if (!device_quirk_info)
>> return;
>>
>> quirks = (uintptr_t)device_quirk_info->driver_data;
>>
>> if (i8042_reset == I8042_RESET_DEFAULT) {
>> if (quirks & SERIO_QUIRK_RESET)
>> i8042_reset = I8042_RESET_ALWAYS;
>> if (quirks & SERIO_QUIRK_NOSELFTEST)
>> i8042_reset = I8042_RESET_NEVER;
>> }
>>
>> /* Do more quirk checks */
>> }
>> #else
>> static inline void i8042_check_quirks(void) {}
>> #endif
>>
>> (above the declaration of i8042_pnp_init())
>>
>> And call i8042_check_quirks() in both
>> i8042_platform_init() and i8042_platform_init() ?
>>
>> This also abstracts some of the CONFIG_X86
>> ifdef-ery out of the other functions.
>>
>>
>>> @@ -1277,6 +1412,9 @@ static inline void i8042_pnp_exit(void) { }
>>>
>>> static int __init i8042_platform_init(void)
>>> {
>>> + bool i8042_reset_always_quirk = false;
>>> + bool i8042_reset_never_quirk = false;
>> I'm not a fan of these 2 helper variables, you can do this directly,
>> see above.
>>
>>> + const struct dmi_system_id *device_quirk_info;
>> All 3 these variables will trigger unused variable
>> settings when compiling without CONFIG_X86 set. Note
>> introducing the i8042_check_quirks() helper as I suggest
>> above avoids this without needing more #ifdef-ery.
>
> Fixed by moving it to its own function as you suggested.
>
> My original reasoning for the helper variables was that i didn't want to move the i8042_reset evaluation, but then did
> it anyways in the next patch after checking that pnp_init doesn't use the variable.
Ok.
Regards,
Hans
>
>>
>>> int retval;
>>>
>>> #ifdef CONFIG_X86
>>> @@ -1297,6 +1435,44 @@ static int __init i8042_platform_init(void)
>>> i8042_kbd_irq = I8042_MAP_IRQ(1);
>>> i8042_aux_irq = I8042_MAP_IRQ(12);
>>>
>>> +#ifdef CONFIG_X86
>>> + device_quirk_info = dmi_first_match(i8042_dmi_quirk_table);
>>> + if (device_quirk_info) {
>>> + if ((uintptr_t)device_quirk_info->driver_data & SERIO_QUIRK_NOKBD)
>>> + i8042_nokbd = true;
>>> + if ((uintptr_t)device_quirk_info->driver_data & SERIO_QUIRK_NOAUX)
>>> + i8042_noaux = true;
>>> + if ((uintptr_t)device_quirk_info->driver_data & SERIO_QUIRK_NOMUX)
>>> + i8042_nomux = true;
>>> + if ((uintptr_t)device_quirk_info->driver_data & SERIO_QUIRK_FORCEMUX)
>>> + i8042_nomux = false;
>>> + if ((uintptr_t)device_quirk_info->driver_data & SERIO_QUIRK_UNLOCK)
>>> + i8042_unlock = true;
>>> + if ((uintptr_t)device_quirk_info->driver_data & SERIO_QUIRK_PROBE_DEFER)
>>> + i8042_probe_defer = true;
>>> + if ((uintptr_t)device_quirk_info->driver_data & SERIO_QUIRK_RESET_ALWAYS)
>>> + i8042_reset_always_quirk = true;
>>> + if ((uintptr_t)device_quirk_info->driver_data & SERIO_QUIRK_RESET_NEVER)
>>> + i8042_reset_never_quirk = true;
>>> + if ((uintptr_t)device_quirk_info->driver_data & SERIO_QUIRK_DIECT)
>>> + i8042_direct = true;
>>> + if ((uintptr_t)device_quirk_info->driver_data & SERIO_QUIRK_DUMBKBD)
>>> + i8042_dumbkbd = true;
>>> + if ((uintptr_t)device_quirk_info->driver_data & SERIO_QUIRK_NOLOOP)
>>> + i8042_noloop = true;
>>> + if ((uintptr_t)device_quirk_info->driver_data & SERIO_QUIRK_NOTIMEOUT)
>>> + i8042_notimeout = true;
>>> + if ((uintptr_t)device_quirk_info->driver_data & SERIO_QUIRK_KBDRESET)
>>> + i8042_kbdreset = true;
>>> + if ((uintptr_t)device_quirk_info->driver_data & SERIO_QUIRK_DRITEK)
>>> + i8042_dritek = true;
>>> +#ifdef CONFIG_PNP
>>> + if ((uintptr_t)device_quirk_info->driver_data & SERIO_QUIRK_NOPNP)
>>> + i8042_nopnp = true;
>>> +#endif
>>> + }
>>> +#endif
>>> +
>>> retval = i8042_pnp_init();
>>> if (retval)
>>> return retval;
>>> @@ -1308,34 +1484,13 @@ static int __init i8042_platform_init(void)
>>> #ifdef CONFIG_X86
>>> /* Honor module parameter when value is not default */
>>> if (i8042_reset == I8042_RESET_DEFAULT) {
>>> - if (dmi_check_system(i8042_dmi_reset_table))
>>> + if (i8042_reset_always_quirk)
>>> i8042_reset = I8042_RESET_ALWAYS;
>>>
>>> - if (dmi_check_system(i8042_dmi_noselftest_table))
>>> + if (i8042_reset_never_quirk)
>>> i8042_reset = I8042_RESET_NEVER;
>>> }
>>>
>>> - if (dmi_check_system(i8042_dmi_noloop_table))
>>> - i8042_noloop = true;
>>> -
>>> - if (dmi_check_system(i8042_dmi_nomux_table))
>>> - i8042_nomux = true;
>>> -
>>> - if (dmi_check_system(i8042_dmi_forcemux_table))
>>> - i8042_nomux = false;
>>> -
>>> - if (dmi_check_system(i8042_dmi_notimeout_table))
>>> - i8042_notimeout = true;
>>> -
>>> - if (dmi_check_system(i8042_dmi_dritek_table))
>>> - i8042_dritek = true;
>>> -
>>> - if (dmi_check_system(i8042_dmi_kbdreset_table))
>>> - i8042_kbdreset = true;
>>> -
>>> - if (dmi_check_system(i8042_dmi_probe_defer_table))
>>> - i8042_probe_defer = true;
>>> -
>>> /*
>>> * A20 was already enabled during early kernel init. But some buggy
>>> * BIOSes (in MSI Laptops) require A20 to be enabled using 8042 to
>> Regards,
>>
>> Hans
>>
>>
> Regards,
>
> Werner
>
Powered by blists - more mailing lists