Re: [PATCH v3 1/3] HID: i2c-hid: Rely on HID descriptor fetch to probe

[Dmitry Torokhov]

Hi Kenny, Lukasz,

On Sat, Apr 27, 2024 at 12:47:07AM +0200, Kenny Levinsen wrote:
> To avoid error messages when a device is not present, b3a81b6c4fc6 added
> an initial bus probe using a dummy i2c_smbus_read_byte() call.
> 
> Without this probe, i2c_hid_fetch_hid_descriptor() will fail internally
> on a bus error and log. Treat the bus error as a missing device and
> remove the error log so we can do away with the probe.
> 
> Tested-by: Lukasz Majczak <lma@xxxxxxxxxxxx>
> Reviewed-by: Lukasz Majczak <lma@xxxxxxxxxxxx>
> Signed-off-by: Kenny Levinsen <kl@xxxxxx>
> ---
>  drivers/hid/i2c-hid/i2c-hid-core.c | 21 ++++++---------------
>  1 file changed, 6 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
> index d965382196c6..6ffa43d245b4 100644
> --- a/drivers/hid/i2c-hid/i2c-hid-core.c
> +++ b/drivers/hid/i2c-hid/i2c-hid-core.c
> @@ -872,12 +872,11 @@ static int i2c_hid_fetch_hid_descriptor(struct i2c_hid *ihid)
>  					      ihid->wHIDDescRegister,
>  					      &ihid->hdesc,
>  					      sizeof(ihid->hdesc));
> -		if (error) {
> -			dev_err(&ihid->client->dev,
> -				"failed to fetch HID descriptor: %d\n",
> -				error);
> -			return -ENODEV;
> -		}
> +
> +		/* The i2c drivers are a bit inconsistent with their error
> +		 * codes, so treat everything as -ENXIO for now. */
> +		if (error)
> +			return -ENXIO;

I really think we should differentiate the cases "we do not know if
there is a device" vs "we do known that there is a device and we have
strong expectation of what that device is, and we do not expect
communication to fail".

Because of that I think we should have separate retry for the original
i2c_smbus_read_byte() (if you want you can make a wrapper around it,
something like i2c_hid_check_device_present()", and if there is a
concern that we will run into similar issue on resume (either from
suspend or from hibernate) then we can have similar retry in
i2c_hid_fetch_hid_descriptor().

But I do think that i2c_hid_fetch_hid_descriptor() should not try to
clobber the error but rather log the true one.

>  	}
>  
>  	/* Validate the length of HID descriptor, the 4 first bytes:
> @@ -992,17 +991,9 @@ static int __i2c_hid_core_probe(struct i2c_hid *ihid)
>  	struct hid_device *hid = ihid->hid;
>  	int ret;
>  
> -	/* Make sure there is something at this address */
> -	ret = i2c_smbus_read_byte(client);
> -	if (ret < 0) {
> -		i2c_hid_dbg(ihid, "nothing at this address: %d\n", ret);
> -		return -ENXIO;
> -	}
> -
>  	ret = i2c_hid_fetch_hid_descriptor(ihid);
>  	if (ret < 0) {
> -		dev_err(&client->dev,
> -			"Failed to fetch the HID Descriptor\n");
> +		i2c_hid_dbg(ihid, "failed to fetch HID descriptor: %d\n", ret);
>  		return ret;
>  	}
>  
> -- 
> 2.44.0
> 
> 

Thanks.

-- 
Dmitry