| Message ID | 20240423122518.34811-2-kl@kl.wtf (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | HID: i2c-hid: Probe and wake device with HID descriptor fetch | expand |
Hi, On Tue, Apr 23, 2024 at 5:26 AM Kenny Levinsen <kl@kl.wtf> 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 with > EREMOTEIO. Propagate the error up so the caller can handle EREMOTEIO > gracefully, and remove the probe as it is no longer necessary. > > Signed-off-by: Kenny Levinsen <kl@kl.wtf> > --- > drivers/hid/i2c-hid/i2c-hid-core.c | 20 ++++++-------------- > 1 file changed, 6 insertions(+), 14 deletions(-) > > diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c > index 2df1ab3c31cc..515a80dbf6c7 100644 > --- a/drivers/hid/i2c-hid/i2c-hid-core.c > +++ b/drivers/hid/i2c-hid/i2c-hid-core.c > @@ -894,12 +894,8 @@ 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; > - } > + if (error) > + return error; > } > > /* Validate the length of HID descriptor, the 4 first bytes: > @@ -1014,17 +1010,13 @@ 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) { > + ret = i2c_hid_fetch_hid_descriptor(ihid); > + if (ret == -EREMOTEIO) { I worry a little bit about keying just off of -EREMOTEIO. If I'm skimming the code properly it's up to the different i2c bus controller to decide which error code to return here. Looking at, for instance, "i2c-qcom-geni.c", I see: [NACK] = {-ENXIO, "NACK: slv unresponsive, check its power/reset-ln"}, Maybe we should just use dev_dbg() in all cases here when we fail to fetch the descriptor? ...otherwise I think some boards will start getting a noisy error message. ...and confirmed that my skim seemed to be accurate. i put your patches on my system and then changed the system to think my hid-over-i2c device was at 0x11 instead of the normal 0x10. Now, I get: [ 5.973417] i2c_hid_of 4-0011: failed to fetch HID descriptor: -6 [ 5.979701] i2c_hid_of 4-0011: Power on failed: -6 -Doug
On 4/24/24 7:00 PM, Doug Anderson wrote: > I worry a little bit about keying just off of -EREMOTEIO. If I'm > skimming the code properly it's up to the different i2c bus controller > to decide which error code to return here. Looking at, for instance, > "i2c-qcom-geni.c", I see: > > [NACK] = {-ENXIO, "NACK: slv unresponsive, check its power/reset-ln"}, Hmm, good point. I decided to go through the drivers and check their behavior on NACK, and based on my quick glance I found (insert accuracy disclaimer): - 52 drivers emitting ENXIO - 14 drivers emitting EREMOTEIO - 11 driver emitting EIO - 5 drivers emitting ETIMEDOUT - 1 driver emitting EAGAIN - 1 driver emitting I2C_ERR_BERR (???) So just EREMOTEIO is definitely not good enough. Looking at the drivers, it seems like the majority of drivers emitting generic error codes could just as well emit ENXIO on NACK. Room for improvement. > Maybe we should just use dev_dbg() in all cases here when we fail to > fetch the descriptor? ...otherwise I think some boards will start > getting a noisy error message. I'm okay with that. I don't like hiding a useful error message, but the smbus probe would also have hidden bus errors. I'll send a v3 with just dev_dbg, then if I (or someone else) end up aligning more i2c drivers on their NACK error we can go to the stricter check and incentivize the drivers to give meaningful error values...
diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c index 2df1ab3c31cc..515a80dbf6c7 100644 --- a/drivers/hid/i2c-hid/i2c-hid-core.c +++ b/drivers/hid/i2c-hid/i2c-hid-core.c @@ -894,12 +894,8 @@ 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; - } + if (error) + return error; } /* Validate the length of HID descriptor, the 4 first bytes: @@ -1014,17 +1010,13 @@ 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) { + ret = i2c_hid_fetch_hid_descriptor(ihid); + if (ret == -EREMOTEIO) { i2c_hid_dbg(ihid, "nothing at this address: %d\n", ret); return -ENXIO; - } - - ret = i2c_hid_fetch_hid_descriptor(ihid); - if (ret < 0) { + } else if (ret < 0) { dev_err(&client->dev, - "Failed to fetch the HID Descriptor\n"); + "failed to fetch HID descriptor: %d\n", ret); return ret; }
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 with EREMOTEIO. Propagate the error up so the caller can handle EREMOTEIO gracefully, and remove the probe as it is no longer necessary. Signed-off-by: Kenny Levinsen <kl@kl.wtf> --- drivers/hid/i2c-hid/i2c-hid-core.c | 20 ++++++-------------- 1 file changed, 6 insertions(+), 14 deletions(-)