| Message ID | 20230621-logitech-fixes-v2-1-3635f7f9c8af@kernel.org |
|---|---|
| State | Accepted |
| Commit | 60165ab774cb0c509680a73cf826d0e158454653 |
| Headers | show |
| Series | [v2] HID: logitech-hidpp: rework one more time the retries attempts | expand |
Hi Benjamin, /me again! :-D On 12.07.23 17:02, bentiss@kernel.org wrote: > From: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > Extract the internal code inside a helper function, fix the > initialization of the parameters used in the helper function > (`hidpp->answer_available` was not reset and `*response` wasn't either), > and use a `do {...} while();` loop. > > Fixes: 586e8fede795 ("HID: logitech-hidpp: Retry commands when device is busy") > Cc: stable@vger.kernel.org
On Wed, 12 Jul 2023 17:02:34 +0200, bentiss@kernel.org wrote: > Extract the internal code inside a helper function, fix the > initialization of the parameters used in the helper function > (`hidpp->answer_available` was not reset and `*response` wasn't either), > and use a `do {...} while();` loop. > > Applied to hid/hid.git (for-6.6/logitech), thanks! [1/1] HID: logitech-hidpp: rework one more time the retries attempts https://git.kernel.org/hid/hid/c/60165ab774cb Cheers,
Hi Thorsten, On Fri, Aug 4, 2023 at 11:14 AM Linux regression tracking (Thorsten Leemhuis) <regressions@leemhuis.info> wrote: > > Hi Benjamin, /me again! :-D > > On 12.07.23 17:02, bentiss@kernel.org wrote: > > From: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > > > Extract the internal code inside a helper function, fix the > > initialization of the parameters used in the helper function > > (`hidpp->answer_available` was not reset and `*response` wasn't either), > > and use a `do {...} while();` loop. > > > > Fixes: 586e8fede795 ("HID: logitech-hidpp: Retry commands when device is busy") > > Cc: stable@vger.kernel.org > > From what I understood there was hope that this would cure the last > remains (occasional init problems iirc) of the recent regressions with > this driver and their fixes. But things look stalled. Is there a reason? > Lack of reviews? Is there nevertheless hope that this will make it at > least into 6.6? Well, mostly lack of testing. But given that nothing happened, I just sneaked it in the 6.6 work. I guess bots are happy enough by now. Cheers, Benjamin > > Ciao, Thorsten > > > Reviewed-by: Bastien Nocera <hadess@hadess.net> > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > --- > > as requested by https://lore.kernel.org/all/CAHk-=wiMbF38KCNhPFiargenpSBoecSXTLQACKS2UMyo_Vu2ww@mail.gmail.com/ > > This is a rewrite of that particular piece of code. > > --- > > Changes in v2: > > - added __must_hold() for KASAN > > - Reworked the comment describing the functions and their return values > > - Link to v1: https://lore.kernel.org/r/20230621-logitech-fixes-v1-1-32e70933c0b0@redhat.com > > --- > > drivers/hid/hid-logitech-hidpp.c | 115 +++++++++++++++++++++++++-------------- > > 1 file changed, 75 insertions(+), 40 deletions(-) > > > > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c > > index 129b01be488d..09ba2086c95c 100644 > > --- a/drivers/hid/hid-logitech-hidpp.c > > +++ b/drivers/hid/hid-logitech-hidpp.c > > @@ -275,21 +275,22 @@ static int __hidpp_send_report(struct hid_device *hdev, > > } > > > > /* > > - * hidpp_send_message_sync() returns 0 in case of success, and something else > > - * in case of a failure. > > - * - If ' something else' is positive, that means that an error has been raised > > - * by the protocol itself. > > - * - If ' something else' is negative, that means that we had a classic error > > - * (-ENOMEM, -EPIPE, etc...) > > + * Effectively send the message to the device, waiting for its answer. > > + * > > + * Must be called with hidpp->send_mutex locked > > + * > > + * Same return protocol than hidpp_send_message_sync(): > > + * - success on 0 > > + * - negative error means transport error > > + * - positive value means protocol error > > */ > > -static int hidpp_send_message_sync(struct hidpp_device *hidpp, > > +static int __do_hidpp_send_message_sync(struct hidpp_device *hidpp, > > struct hidpp_report *message, > > struct hidpp_report *response) > > { > > - int ret = -1; > > - int max_retries = 3; > > + int ret; > > > > - mutex_lock(&hidpp->send_mutex); > > + __must_hold(&hidpp->send_mutex); > > > > hidpp->send_receive_buf = response; > > hidpp->answer_available = false; > > @@ -300,47 +301,74 @@ static int hidpp_send_message_sync(struct hidpp_device *hidpp, > > */ > > *response = *message; > > > > - for (; max_retries != 0 && ret; max_retries--) { > > - ret = __hidpp_send_report(hidpp->hid_dev, message); > > + ret = __hidpp_send_report(hidpp->hid_dev, message); > > + if (ret) { > > + dbg_hid("__hidpp_send_report returned err: %d\n", ret); > > + memset(response, 0, sizeof(struct hidpp_report)); > > + return ret; > > + } > > > > - if (ret) { > > - dbg_hid("__hidpp_send_report returned err: %d\n", ret); > > - memset(response, 0, sizeof(struct hidpp_report)); > > - break; > > - } > > + if (!wait_event_timeout(hidpp->wait, hidpp->answer_available, > > + 5*HZ)) { > > + dbg_hid("%s:timeout waiting for response\n", __func__); > > + memset(response, 0, sizeof(struct hidpp_report)); > > + return -ETIMEDOUT; > > + } > > > > - if (!wait_event_timeout(hidpp->wait, hidpp->answer_available, > > - 5*HZ)) { > > - dbg_hid("%s:timeout waiting for response\n", __func__); > > - memset(response, 0, sizeof(struct hidpp_report)); > > - ret = -ETIMEDOUT; > > - break; > > - } > > + if (response->report_id == REPORT_ID_HIDPP_SHORT && > > + response->rap.sub_id == HIDPP_ERROR) { > > + ret = response->rap.params[1]; > > + dbg_hid("%s:got hidpp error %02X\n", __func__, ret); > > + return ret; > > + } > > > > - if (response->report_id == REPORT_ID_HIDPP_SHORT && > > - response->rap.sub_id == HIDPP_ERROR) { > > - ret = response->rap.params[1]; > > - dbg_hid("%s:got hidpp error %02X\n", __func__, ret); > > + if ((response->report_id == REPORT_ID_HIDPP_LONG || > > + response->report_id == REPORT_ID_HIDPP_VERY_LONG) && > > + response->fap.feature_index == HIDPP20_ERROR) { > > + ret = response->fap.params[1]; > > + dbg_hid("%s:got hidpp 2.0 error %02X\n", __func__, ret); > > + return ret; > > + } > > + > > + return 0; > > +} > > + > > +/* > > + * hidpp_send_message_sync() returns 0 in case of success, and something else > > + * in case of a failure. > > + * > > + * See __do_hidpp_send_message_sync() for a detailed explanation of the returned > > + * value. > > + */ > > +static int hidpp_send_message_sync(struct hidpp_device *hidpp, > > + struct hidpp_report *message, > > + struct hidpp_report *response) > > +{ > > + int ret; > > + int max_retries = 3; > > + > > + mutex_lock(&hidpp->send_mutex); > > + > > + do { > > + ret = __do_hidpp_send_message_sync(hidpp, message, response); > > + if (ret != HIDPP20_ERROR_BUSY) > > break; > > - } > > > > - if ((response->report_id == REPORT_ID_HIDPP_LONG || > > - 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 hidpp 2.0 error %02X\n", __func__, ret); > > - break; > > - } > > - dbg_hid("%s:got busy hidpp 2.0 error %02X, retrying\n", __func__, ret); > > - } > > - } > > + dbg_hid("%s:got busy hidpp 2.0 error %02X, retrying\n", __func__, ret); > > + } while (--max_retries); > > > > mutex_unlock(&hidpp->send_mutex); > > return ret; > > > > } > > > > +/* > > + * hidpp_send_fap_command_sync() returns 0 in case of success, and something else > > + * in case of a failure. > > + * > > + * See __do_hidpp_send_message_sync() for a detailed explanation of the returned > > + * value. > > + */ > > static int hidpp_send_fap_command_sync(struct hidpp_device *hidpp, > > u8 feat_index, u8 funcindex_clientid, u8 *params, int param_count, > > struct hidpp_report *response) > > @@ -373,6 +401,13 @@ static int hidpp_send_fap_command_sync(struct hidpp_device *hidpp, > > return ret; > > } > > > > +/* > > + * hidpp_send_rap_command_sync() returns 0 in case of success, and something else > > + * in case of a failure. > > + * > > + * See __do_hidpp_send_message_sync() for a detailed explanation of the returned > > + * value. > > + */ > > static int hidpp_send_rap_command_sync(struct hidpp_device *hidpp_dev, > > u8 report_id, u8 sub_id, u8 reg_address, u8 *params, int param_count, > > struct hidpp_report *response) > > > > --- > > base-commit: 87854366176403438d01f368b09de3ec2234e0f5 > > change-id: 20230621-logitech-fixes-a4c0e66ea2ad > > > > Best regards, >
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c index 129b01be488d..09ba2086c95c 100644 --- a/drivers/hid/hid-logitech-hidpp.c +++ b/drivers/hid/hid-logitech-hidpp.c @@ -275,21 +275,22 @@ static int __hidpp_send_report(struct hid_device *hdev, } /* - * hidpp_send_message_sync() returns 0 in case of success, and something else - * in case of a failure. - * - If ' something else' is positive, that means that an error has been raised - * by the protocol itself. - * - If ' something else' is negative, that means that we had a classic error - * (-ENOMEM, -EPIPE, etc...) + * Effectively send the message to the device, waiting for its answer. + * + * Must be called with hidpp->send_mutex locked + * + * Same return protocol than hidpp_send_message_sync(): + * - success on 0 + * - negative error means transport error + * - positive value means protocol error */ -static int hidpp_send_message_sync(struct hidpp_device *hidpp, +static int __do_hidpp_send_message_sync(struct hidpp_device *hidpp, struct hidpp_report *message, struct hidpp_report *response) { - int ret = -1; - int max_retries = 3; + int ret; - mutex_lock(&hidpp->send_mutex); + __must_hold(&hidpp->send_mutex); hidpp->send_receive_buf = response; hidpp->answer_available = false; @@ -300,47 +301,74 @@ static int hidpp_send_message_sync(struct hidpp_device *hidpp, */ *response = *message; - for (; max_retries != 0 && ret; max_retries--) { - ret = __hidpp_send_report(hidpp->hid_dev, message); + ret = __hidpp_send_report(hidpp->hid_dev, message); + if (ret) { + dbg_hid("__hidpp_send_report returned err: %d\n", ret); + memset(response, 0, sizeof(struct hidpp_report)); + return ret; + } - if (ret) { - dbg_hid("__hidpp_send_report returned err: %d\n", ret); - memset(response, 0, sizeof(struct hidpp_report)); - break; - } + if (!wait_event_timeout(hidpp->wait, hidpp->answer_available, + 5*HZ)) { + dbg_hid("%s:timeout waiting for response\n", __func__); + memset(response, 0, sizeof(struct hidpp_report)); + return -ETIMEDOUT; + } - if (!wait_event_timeout(hidpp->wait, hidpp->answer_available, - 5*HZ)) { - dbg_hid("%s:timeout waiting for response\n", __func__); - memset(response, 0, sizeof(struct hidpp_report)); - ret = -ETIMEDOUT; - break; - } + if (response->report_id == REPORT_ID_HIDPP_SHORT && + response->rap.sub_id == HIDPP_ERROR) { + ret = response->rap.params[1]; + dbg_hid("%s:got hidpp error %02X\n", __func__, ret); + return ret; + } - if (response->report_id == REPORT_ID_HIDPP_SHORT && - response->rap.sub_id == HIDPP_ERROR) { - ret = response->rap.params[1]; - dbg_hid("%s:got hidpp error %02X\n", __func__, ret); + if ((response->report_id == REPORT_ID_HIDPP_LONG || + response->report_id == REPORT_ID_HIDPP_VERY_LONG) && + response->fap.feature_index == HIDPP20_ERROR) { + ret = response->fap.params[1]; + dbg_hid("%s:got hidpp 2.0 error %02X\n", __func__, ret); + return ret; + } + + return 0; +} + +/* + * hidpp_send_message_sync() returns 0 in case of success, and something else + * in case of a failure. + * + * See __do_hidpp_send_message_sync() for a detailed explanation of the returned + * value. + */ +static int hidpp_send_message_sync(struct hidpp_device *hidpp, + struct hidpp_report *message, + struct hidpp_report *response) +{ + int ret; + int max_retries = 3; + + mutex_lock(&hidpp->send_mutex); + + do { + ret = __do_hidpp_send_message_sync(hidpp, message, response); + if (ret != HIDPP20_ERROR_BUSY) break; - } - if ((response->report_id == REPORT_ID_HIDPP_LONG || - 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 hidpp 2.0 error %02X\n", __func__, ret); - break; - } - dbg_hid("%s:got busy hidpp 2.0 error %02X, retrying\n", __func__, ret); - } - } + dbg_hid("%s:got busy hidpp 2.0 error %02X, retrying\n", __func__, ret); + } while (--max_retries); mutex_unlock(&hidpp->send_mutex); return ret; } +/* + * hidpp_send_fap_command_sync() returns 0 in case of success, and something else + * in case of a failure. + * + * See __do_hidpp_send_message_sync() for a detailed explanation of the returned + * value. + */ static int hidpp_send_fap_command_sync(struct hidpp_device *hidpp, u8 feat_index, u8 funcindex_clientid, u8 *params, int param_count, struct hidpp_report *response) @@ -373,6 +401,13 @@ static int hidpp_send_fap_command_sync(struct hidpp_device *hidpp, return ret; } +/* + * hidpp_send_rap_command_sync() returns 0 in case of success, and something else + * in case of a failure. + * + * See __do_hidpp_send_message_sync() for a detailed explanation of the returned + * value. + */ static int hidpp_send_rap_command_sync(struct hidpp_device *hidpp_dev, u8 report_id, u8 sub_id, u8 reg_address, u8 *params, int param_count, struct hidpp_report *response)