On 2023-07-29 15:44:32 GMT+02:00, Guenter Roeck wrote: > On 7/29/23 04:27, Aleksa Savic wrote: >> Add a 200ms delay after sending a ctrl report to Quadro, >> Octo, D5 Next and Aquaero to give them enough time to >> process the request and save the data to memory. Otherwise, >> under heavier userspace loads where multiple sysfs entries >> are usually set in quick succession, a new ctrl report could >> be requested from the device while it's still processing the >> previous one and fail with -EPIPE. >> >> Reported by a user on Github [1] and tested by both of us. >> >> [1] https://github.com/aleksamagicka/aquacomputer_d5next-hwmon/issues/82 >> >> Cc: stable@xxxxxxxxxxxxxxx >> Signed-off-by: Aleksa Savic <savicaleksa83@xxxxxxxxx> >> --- >> drivers/hwmon/aquacomputer_d5next.c | 25 +++++++++++++++++++++++++ >> 1 file changed, 25 insertions(+) >> >> diff --git a/drivers/hwmon/aquacomputer_d5next.c b/drivers/hwmon/aquacomputer_d5next.c >> index a997dbcb563f..9cb55d51185a 100644 >> --- a/drivers/hwmon/aquacomputer_d5next.c >> +++ b/drivers/hwmon/aquacomputer_d5next.c >> @@ -652,6 +652,31 @@ static int aqc_send_ctrl_data(struct aqc_data *priv) >> ret = hid_hw_raw_request(priv->hdev, priv->secondary_ctrl_report_id, >> priv->secondary_ctrl_report, priv->secondary_ctrl_report_size, >> HID_FEATURE_REPORT, HID_REQ_SET_REPORT); >> + if (ret < 0) >> + return ret; >> + >> + /* >> + * Wait 200ms before returning to make sure that the device actually processed both reports >> + * and saved ctrl data to memory. Otherwise, an aqc_get_ctrl_data() call made shortly after >> + * may fail with -EPIPE because the device is still busy and can't provide data. This can >> + * happen when userspace tools, such as fancontrol or liquidctl, write to sysfs entries in >> + * quick succession. >> + * >> + * 200ms was found to be the sweet spot between fixing the issue and not significantly >> + * prolonging the call. Quadro, Octo, D5 Next and Aquaero are currently known to be >> + * affected. >> + */ >> + switch (priv->kind) { >> + case quadro: >> + case octo: >> + case d5next: >> + case aquaero: >> + msleep(200); >> + break; >> + default: >> + break; >> + } >> + >> return ret; >> } >> > > This would force writes to sleep even if there is no subsequent operation. > Please make this conditional by saving the most recent access time and wait > on the subsequent operation. I would also suggest to store the wait time > in struct aqc_data to avoid the switch statement in the data path. An example > for a driver doing something similar is drivers/hwmon/pmbus/zl6100.c. > > Thanks, > Guenter > Thanks, will take a look at it. Please ignore v2 of this patch in this case. Aleksa
