linux-kernel - Re: [PATCH v1 1/2] HID: i2c-hid: introduce re-power-on quirk

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2xb4vqlt2gdrmioyx7tjaw2vfw55pmhvz54q7f2ldrkikzzxge@737bp5ms6gwc>
Date: Wed, 25 Sep 2024 13:54:20 +0200
From: Benjamin Tissoires <bentiss@...nel.org>
To: Aleksandrs Vinarskis <alex.vinarskis@...il.com>
Cc: Jiri Kosina <jikos@...nel.org>, linux-input@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 1/2] HID: i2c-hid: introduce re-power-on quirk

On Sep 25 2024, Aleksandrs Vinarskis wrote:
> It appears some keyboards from vendor 'QTEC' will not work properly until
> suspend & resume.
> 
> Empirically narrowed down to solution of re-sending power on command
> _after_ initialization was completed before the end of initial probing.
> 
> Signed-off-by: Aleksandrs Vinarskis <alex.vinarskis@...il.com>
> ---
>  drivers/hid/i2c-hid/i2c-hid-core.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
> index 632eaf9e11a6..087ca2474176 100644
> --- a/drivers/hid/i2c-hid/i2c-hid-core.c
> +++ b/drivers/hid/i2c-hid/i2c-hid-core.c
> @@ -50,6 +50,7 @@
>  #define I2C_HID_QUIRK_BAD_INPUT_SIZE		BIT(3)
>  #define I2C_HID_QUIRK_NO_WAKEUP_AFTER_RESET	BIT(4)
>  #define I2C_HID_QUIRK_NO_SLEEP_ON_SUSPEND	BIT(5)
> +#define I2C_HID_QUIRK_RE_POWER_ON		BIT(6)
>  
>  /* Command opcodes */
>  #define I2C_HID_OPCODE_RESET			0x01
> @@ -1048,7 +1049,11 @@ static int i2c_hid_core_register_hid(struct i2c_hid *ihid)
>  		return ret;
>  	}
>  
> -	return 0;
> +	/* At least some QTEC devices need this after initialization */
> +	if (ihid->quirks & I2C_HID_QUIRK_RE_POWER_ON)
> +		ret = i2c_hid_set_power(ihid, I2C_HID_PWR_ON);

I'd rather not have this in i2c-hid-core.c, TBH.

We do have a nice split separation of i2c-hid which allows to add vendor
specific i2c-hid-of drivers. We currently have 2 (goodix and elan) and a
third wouldn't be much of an issue.

I'm not really happy of this admittely simple solution in this patch
because:
- what if QTEC "fixes" that behavior in the future?
- what if you actually need to enable/disable regulators like goodix and
  elan do

So to me, a better solution would be to create a i2c-hid-of-qtec.c,
assign a new compatible for this keyboard, and try to fix up the initial
powerup in .power_up in that particular driver. This way, we can extend
the driver for the regulators, and we can also fix this issue while being
sure we do not touch at anything else.

Anyway, glad to see the bringup of the new arm based XPS-13 taking
shape!

Cheers,
Benjamin


> +
> +	return ret;
>  }
>  
>  static int i2c_hid_core_probe_panel_follower(struct i2c_hid *ihid)
> -- 
> 2.43.0
>