lkml.org 
[lkml]   [2022]   [Mar]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH v3 1/3] Input/i8042: Merge quirk tables
    From

    Am 21.03.22 um 15:51 schrieb Hans de Goede:
    > 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 ?
    Next patch as in next revision of this patch set. Sorry for causing confusion.
    >
    >
    >>>
    >>>
    >>>> Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
    >>>> Cc: stable@vger.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
    >>

    \
     
     \ /
      Last update: 2022-03-21 18:04    [W:2.581 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site