| Message ID | 20230206221256.129198-2-hadess@hadess.net |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,1/3] HID: logitech-hidpp: Add more debug statements | expand |
On Feb 06 2023, Bastien Nocera wrote: > Handle the busy error coming from the device or receiver. The > documentation says a busy error can be returned when: > " > Device (or receiver) cannot answer immediately to this request > for any reason i.e: > - already processing a request from the same or another SW > - pipe full > " > > Signed-off-by: Bastien Nocera <hadess@hadess.net> > --- > > Same as v1 > > drivers/hid/hid-logitech-hidpp.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c > index 1952d8d3b6b2..9e94026de437 100644 > --- a/drivers/hid/hid-logitech-hidpp.c > +++ b/drivers/hid/hid-logitech-hidpp.c > @@ -295,6 +295,7 @@ static int hidpp_send_message_sync(struct hidpp_device *hidpp, > */ > *response = *message; > > +retry: > ret = __hidpp_send_report(hidpp->hid_dev, message); > > if (ret) { > @@ -321,6 +322,10 @@ static int hidpp_send_message_sync(struct hidpp_device *hidpp, > response->report_id == REPORT_ID_HIDPP_VERY_LONG) && > response->fap.feature_index == HIDPP20_ERROR) { > ret = response->fap.params[1]; > + if (ret == HIDPP20_ERROR_BUSY) { > + dbg_hid("%s:got busy hidpp 2.0 error %02X, retrying\n", __func__, ret); > + goto retry; I must confess, I blocked a little bit there to decide whether or not using goto here was OK. But then I reliazed that there is no way to leave that function if the device is buggy and constantly sends back ERROR_BUSY. So I am not very found of the idea of having that got after all. Would you mind respinning that patch with a bounded loop for the retries instead of using a goto? I'd like the driver to give up after a few retries if the device is not fair. Cheers, Benjamin > + } > dbg_hid("%s:got hidpp 2.0 error %02X\n", __func__, ret); > goto exit; > } > -- > 2.39.1 >
On Thu, 2023-02-09 at 15:50 +0100, Benjamin Tissoires wrote: > On Feb 06 2023, Bastien Nocera wrote: > > Handle the busy error coming from the device or receiver. The > > documentation says a busy error can be returned when: > > " > > Device (or receiver) cannot answer immediately to this request > > for any reason i.e: > > - already processing a request from the same or another SW > > - pipe full > > " > > > > Signed-off-by: Bastien Nocera <hadess@hadess.net> > > --- > > > > Same as v1 > > > > drivers/hid/hid-logitech-hidpp.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid- > > logitech-hidpp.c > > index 1952d8d3b6b2..9e94026de437 100644 > > --- a/drivers/hid/hid-logitech-hidpp.c > > +++ b/drivers/hid/hid-logitech-hidpp.c > > @@ -295,6 +295,7 @@ static int hidpp_send_message_sync(struct > > hidpp_device *hidpp, > > */ > > *response = *message; > > > > +retry: > > ret = __hidpp_send_report(hidpp->hid_dev, message); > > > > if (ret) { > > @@ -321,6 +322,10 @@ static int hidpp_send_message_sync(struct > > hidpp_device *hidpp, > > response->report_id == > > REPORT_ID_HIDPP_VERY_LONG) && > > response->fap.feature_index == > > HIDPP20_ERROR) { > > ret = response->fap.params[1]; > > + if (ret == HIDPP20_ERROR_BUSY) { > > + dbg_hid("%s:got busy hidpp 2.0 error %02X, > > retrying\n", __func__, ret); > > + goto retry; > > I must confess, I blocked a little bit there to decide whether or not > using goto here was OK. > > But then I reliazed that there is no way to leave that function if > the > device is buggy and constantly sends back ERROR_BUSY. So I am not > very > found of the idea of having that got after all. > > Would you mind respinning that patch with a bounded loop for the > retries > instead of using a goto? I'd like the driver to give up after a few > retries if the device is not fair. Done in v3.
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c index 1952d8d3b6b2..9e94026de437 100644 --- a/drivers/hid/hid-logitech-hidpp.c +++ b/drivers/hid/hid-logitech-hidpp.c @@ -295,6 +295,7 @@ static int hidpp_send_message_sync(struct hidpp_device *hidpp, */ *response = *message; +retry: ret = __hidpp_send_report(hidpp->hid_dev, message); if (ret) { @@ -321,6 +322,10 @@ static int hidpp_send_message_sync(struct hidpp_device *hidpp, response->report_id == REPORT_ID_HIDPP_VERY_LONG) && response->fap.feature_index == HIDPP20_ERROR) { ret = response->fap.params[1]; + if (ret == HIDPP20_ERROR_BUSY) { + dbg_hid("%s:got busy hidpp 2.0 error %02X, retrying\n", __func__, ret); + goto retry; + } dbg_hid("%s:got hidpp 2.0 error %02X\n", __func__, ret); goto exit; }
Handle the busy error coming from the device or receiver. The documentation says a busy error can be returned when: " Device (or receiver) cannot answer immediately to this request for any reason i.e: - already processing a request from the same or another SW - pipe full " Signed-off-by: Bastien Nocera <hadess@hadess.net> --- Same as v1 drivers/hid/hid-logitech-hidpp.c | 5 +++++ 1 file changed, 5 insertions(+)