| Message ID | 20230621-logitech-fixes-v2-1-3635f7f9c8af@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Jiri Kosina |
| 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 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? 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,
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)