| Message ID | 20250208173628.5734-3-tomasz.pakula.oficjalny@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | HID: pidff: Compatibility update and new devices | expand |
On Sat, 8 Feb 2025, Tomasz Pakuła wrote: > As noted by Anssi some 20 years ago, pool report is sometimes messed up. > This worked fine on many devices but casued oops on VRS DirectForce PRO. > > Here, we're making sure pool report is refetched before trying to access > any of it's fields. While loop was replaced with a for loop + exit > conditions were moved aroud to decrease the possibility of creating an > infinite loop scenario. > > Signed-off-by: Tomasz Pakuła <tomasz.pakula.oficjalny@gmail.com> > --- > drivers/hid/usbhid/hid-pidff.c | 34 ++++++++++++++++------------------ > 1 file changed, 16 insertions(+), 18 deletions(-) > > diff --git a/drivers/hid/usbhid/hid-pidff.c b/drivers/hid/usbhid/hid-pidff.c > index f23381b6e344..acdcc0af86ba 100644 > --- a/drivers/hid/usbhid/hid-pidff.c > +++ b/drivers/hid/usbhid/hid-pidff.c > @@ -604,28 +604,26 @@ static void pidff_reset(struct pidff_device *pidff) > } > > /* > - * Refetch pool report > + * Fetch pool report > */ > static void pidff_fetch_pool(struct pidff_device *pidff) > { > - if (!pidff->pool[PID_SIMULTANEOUS_MAX].value) > - return; > - > - int i = 0; > - while (pidff->pool[PID_SIMULTANEOUS_MAX].value[0] < 2) { > - hid_dbg(pidff->hid, "pid_pool requested again\n"); > - hid_hw_request(pidff->hid, pidff->reports[PID_POOL], > - HID_REQ_GET_REPORT); > - hid_hw_wait(pidff->hid); > - > - /* break after 20 tries with SIMULTANEOUS_MAX < 2 */ > - if (i++ > 20) { > - hid_warn(pidff->hid, > - "device reports %d simultaneous effects\n", > - pidff->pool[PID_SIMULTANEOUS_MAX].value[0]); > - break; > - } > + int i; > + struct hid_device *hid = pidff->hid; > + > + /* Try 20 times if PID_SIMULTANEOUS_MAX < 2. > + We must make sure this isn't just an error */ Sorry for annoying nit: this is not really consistent with Kernel / HID comment style :)
On Mon, 10 Feb 2025 at 09:52, Jiri Kosina <jikos@kernel.org> wrote: > On Sat, 8 Feb 2025, Tomasz Pakuła wrote: > > + /* Try 20 times if PID_SIMULTANEOUS_MAX < 2. > > + We must make sure this isn't just an error */ > > Sorry for annoying nit: this is not really consistent with Kernel / HID > comment style :) > > -- > Jiri Kosina > SUSE Labs No problem! I must say I did think this might seem janky as simple two-line comment didn't feel right when it took up 4 lines, but on the other hand, this looks just a bit out of place as well. I'll send out an improved v2 today. Thanks, Tomasz P.S. Thank you for merging this. I worried we'd have to wait for Anssi to leave his comments/reviews as he told me recently he's quite busy and doesn't have much time to go over all this PID work but he appreciates the CCs.
diff --git a/drivers/hid/usbhid/hid-pidff.c b/drivers/hid/usbhid/hid-pidff.c index f23381b6e344..acdcc0af86ba 100644 --- a/drivers/hid/usbhid/hid-pidff.c +++ b/drivers/hid/usbhid/hid-pidff.c @@ -604,28 +604,26 @@ static void pidff_reset(struct pidff_device *pidff) } /* - * Refetch pool report + * Fetch pool report */ static void pidff_fetch_pool(struct pidff_device *pidff) { - if (!pidff->pool[PID_SIMULTANEOUS_MAX].value) - return; - - int i = 0; - while (pidff->pool[PID_SIMULTANEOUS_MAX].value[0] < 2) { - hid_dbg(pidff->hid, "pid_pool requested again\n"); - hid_hw_request(pidff->hid, pidff->reports[PID_POOL], - HID_REQ_GET_REPORT); - hid_hw_wait(pidff->hid); - - /* break after 20 tries with SIMULTANEOUS_MAX < 2 */ - if (i++ > 20) { - hid_warn(pidff->hid, - "device reports %d simultaneous effects\n", - pidff->pool[PID_SIMULTANEOUS_MAX].value[0]); - break; - } + int i; + struct hid_device *hid = pidff->hid; + + /* Try 20 times if PID_SIMULTANEOUS_MAX < 2. + We must make sure this isn't just an error */ + for(i = 0; i < 20; i++) { + hid_hw_request(hid, pidff->reports[PID_POOL], HID_REQ_GET_REPORT); + hid_hw_wait(hid); + + if (!pidff->pool[PID_SIMULTANEOUS_MAX].value) + return; + if (pidff->pool[PID_SIMULTANEOUS_MAX].value[0] >= 2) + return; } + hid_warn(hid, "device reports %d simultaneous effects\n", + pidff->pool[PID_SIMULTANEOUS_MAX].value[0]); } /*
As noted by Anssi some 20 years ago, pool report is sometimes messed up. This worked fine on many devices but casued oops on VRS DirectForce PRO. Here, we're making sure pool report is refetched before trying to access any of it's fields. While loop was replaced with a for loop + exit conditions were moved aroud to decrease the possibility of creating an infinite loop scenario. Signed-off-by: Tomasz Pakuła <tomasz.pakula.oficjalny@gmail.com> --- drivers/hid/usbhid/hid-pidff.c | 34 ++++++++++++++++------------------ 1 file changed, 16 insertions(+), 18 deletions(-)