Messages in this thread | | | Date | Sat, 29 Jul 2023 06:44:32 -0700 | From | Guenter Roeck <> | Subject | Re: [PATCH] hwmon: (aquacomputer_d5next) Add selective 200ms delay after sending ctrl report |
| |
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@vger.kernel.org > Signed-off-by: Aleksa Savic <savicaleksa83@gmail.com> > --- > 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
| |