Received-SPF: pass (google.com: domain of linux-kernel+bounces-160877-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Fri, 26 Apr 2024 20:21:52 -0700
From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
To: Kenny Levinsen <kl@kl.wtf>
Cc: Jiri Kosina <jikos@kernel.org>, Dmitry Torokhov <dtor@chromium.org>,
	Benjamin Tissoires <benjamin.tissoires@redhat.com>,
	Douglas Anderson <dianders@chromium.org>,
	Hans de Goede <hdegoede@redhat.com>,
	Maxime Ripard <mripard@kernel.org>,
	Kai-Heng Feng <kai.heng.feng@canonical.com>,
	Johan Hovold <johan+linaro@kernel.org>, linux-input@vger.kernel.org,
	linux-kernel@vger.kernel.org, Radoslaw Biernacki <rad@chromium.org>,
	Lukasz Majczak <lma@chromium.org>
Subject: Re: [PATCH v3 1/3] HID: i2c-hid: Rely on HID descriptor fetch to
 probe
Message-ID: <ZixvUNooESC02cJK@google.com>
References: <20240426225739.2166-1-kl@kl.wtf>
 <20240426225739.2166-2-kl@kl.wtf>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240426225739.2166-2-kl@kl.wtf>

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@chromium.org>
> Reviewed-by: Lukasz Majczak <lma@chromium.org>
> Signed-off-by: Kenny Levinsen <kl@kl.wtf>
> ---
>  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