| Message ID | 20240401004757.22708-1-stefanberzl@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | HID: uclogic: Remove useless loop | expand |
On Mon, 1 Apr 2024, Stefan Berzl wrote: > The while in question does nothing except provide the possibility > to have an infinite loop in case the subreport id is actually the same > as the pen id. > > Signed-off-by: Stefan Berzl <stefanberzl@gmail.com> Let me CC Nicolai, the author of the code of question (8b013098be2c9). > --- > drivers/hid/hid-uclogic-core.c | 55 ++++++++++++++++------------------ > 1 file changed, 25 insertions(+), 30 deletions(-) > > diff --git a/drivers/hid/hid-uclogic-core.c b/drivers/hid/hid-uclogic-core.c > index ad74cbc9a0aa..a56f4de216de 100644 > --- a/drivers/hid/hid-uclogic-core.c > +++ b/drivers/hid/hid-uclogic-core.c > @@ -431,40 +431,35 @@ static int uclogic_raw_event(struct hid_device *hdev, > if (uclogic_exec_event_hook(params, data, size)) > return 0; > > - while (true) { > - /* Tweak pen reports, if necessary */ > - if ((report_id == params->pen.id) && (size >= 2)) { > - subreport_list_end = > - params->pen.subreport_list + > - ARRAY_SIZE(params->pen.subreport_list); > - /* Try to match a subreport */ > - for (subreport = params->pen.subreport_list; > - subreport < subreport_list_end; subreport++) { > - if (subreport->value != 0 && > - subreport->value == data[1]) { > - break; > - } > - } > - /* If a subreport matched */ > - if (subreport < subreport_list_end) { > - /* Change to subreport ID, and restart */ > - report_id = data[0] = subreport->id; > - continue; > - } else { > - return uclogic_raw_event_pen(drvdata, data, size); > + /* Tweak pen reports, if necessary */ > + if ((report_id == params->pen.id) && (size >= 2)) { > + subreport_list_end = > + params->pen.subreport_list + > + ARRAY_SIZE(params->pen.subreport_list); > + /* Try to match a subreport */ > + for (subreport = params->pen.subreport_list; > + subreport < subreport_list_end; subreport++) { > + if (subreport->value != 0 && > + subreport->value == data[1]) { > + break; > } > } > - > - /* Tweak frame control reports, if necessary */ > - for (i = 0; i < ARRAY_SIZE(params->frame_list); i++) { > - if (report_id == params->frame_list[i].id) { > - return uclogic_raw_event_frame( > - drvdata, ¶ms->frame_list[i], > - data, size); > - } > + /* If a subreport matched */ > + if (subreport < subreport_list_end) { > + /* Change to subreport ID, and restart */ > + report_id = data[0] = subreport->id; > + } else { > + return uclogic_raw_event_pen(drvdata, data, size); > } > + } > > - break; > + /* Tweak frame control reports, if necessary */ > + for (i = 0; i < ARRAY_SIZE(params->frame_list); i++) { > + if (report_id == params->frame_list[i].id) { > + return uclogic_raw_event_frame( > + drvdata, ¶ms->frame_list[i], > + data, size); > + } > } > > return 0; > -- > 2.43.0 >
On 12/04/2024 17:52, Jiri Kosina wrote: > On Mon, 1 Apr 2024, Stefan Berzl wrote: > >> The while in question does nothing except provide the possibility >> to have an infinite loop in case the subreport id is actually the same >> as the pen id. >> >> Signed-off-by: Stefan Berzl <stefanberzl@gmail.com> > > Let me CC Nicolai, the author of the code of question (8b013098be2c9). I agree that Nicolai's opinion would be invaluable, but even without it, the patch is trivially correct. If we have a subreport that matches the packet, we change the report_id accordingly. If we then loop back to the beginning, either the report_id is different or we are caught in an infinite loop. None of these are hardware registers where the access itself would matter. >> --- >> drivers/hid/hid-uclogic-core.c | 55 ++++++++++++++++------------------ >> 1 file changed, 25 insertions(+), 30 deletions(-) >> >> diff --git a/drivers/hid/hid-uclogic-core.c b/drivers/hid/hid-uclogic-core.c >> index ad74cbc9a0aa..a56f4de216de 100644 >> --- a/drivers/hid/hid-uclogic-core.c >> +++ b/drivers/hid/hid-uclogic-core.c >> @@ -431,40 +431,35 @@ static int uclogic_raw_event(struct hid_device *hdev, >> if (uclogic_exec_event_hook(params, data, size)) >> return 0; >> >> - while (true) { >> - /* Tweak pen reports, if necessary */ >> - if ((report_id == params->pen.id) && (size >= 2)) { >> - subreport_list_end = >> - params->pen.subreport_list + >> - ARRAY_SIZE(params->pen.subreport_list); >> - /* Try to match a subreport */ >> - for (subreport = params->pen.subreport_list; >> - subreport < subreport_list_end; subreport++) { >> - if (subreport->value != 0 && >> - subreport->value == data[1]) { >> - break; >> - } >> - } >> - /* If a subreport matched */ >> - if (subreport < subreport_list_end) { >> - /* Change to subreport ID, and restart */ >> - report_id = data[0] = subreport->id; >> - continue; >> - } else { >> - return uclogic_raw_event_pen(drvdata, data, size); >> + /* Tweak pen reports, if necessary */ >> + if ((report_id == params->pen.id) && (size >= 2)) { >> + subreport_list_end = >> + params->pen.subreport_list + >> + ARRAY_SIZE(params->pen.subreport_list); >> + /* Try to match a subreport */ >> + for (subreport = params->pen.subreport_list; >> + subreport < subreport_list_end; subreport++) { >> + if (subreport->value != 0 && >> + subreport->value == data[1]) { >> + break; >> } >> } >> - >> - /* Tweak frame control reports, if necessary */ >> - for (i = 0; i < ARRAY_SIZE(params->frame_list); i++) { >> - if (report_id == params->frame_list[i].id) { >> - return uclogic_raw_event_frame( >> - drvdata, ¶ms->frame_list[i], >> - data, size); >> - } >> + /* If a subreport matched */ >> + if (subreport < subreport_list_end) { >> + /* Change to subreport ID, and restart */ >> + report_id = data[0] = subreport->id; >> + } else { >> + return uclogic_raw_event_pen(drvdata, data, size); >> } >> + } >> >> - break; >> + /* Tweak frame control reports, if necessary */ >> + for (i = 0; i < ARRAY_SIZE(params->frame_list); i++) { >> + if (report_id == params->frame_list[i].id) { >> + return uclogic_raw_event_frame( >> + drvdata, ¶ms->frame_list[i], >> + data, size); >> + } >> } >> >> return 0; >> -- >> 2.43.0 >> >
Hi Jiri, Stefan, On 4/18/24 4:31 PM, Stefan Berzl wrote: > > On 12/04/2024 17:52, Jiri Kosina wrote: >> On Mon, 1 Apr 2024, Stefan Berzl wrote: >> >>> The while in question does nothing except provide the possibility >>> to have an infinite loop in case the subreport id is actually the same >>> as the pen id. >>> >>> Signed-off-by: Stefan Berzl <stefanberzl@gmail.com> >> >> Let me CC Nicolai, the author of the code of question (8b013098be2c9). > > I agree that Nicolai's opinion would be invaluable, but even without it, > the patch is trivially correct. If we have a subreport that matches the > packet, we change the report_id accordingly. If we then loop back to the > beginning, either the report_id is different or we are caught in an > infinite loop. None of these are hardware registers where the access > itself would matter. Yes, Stefan is right. I was trying to implement general rewrite logic, and if we really had that, then the fix would need to be checking that the new ID is different. As such there's really no need, and Stefan's fix is fine. Only perhaps amend that comment to something like /* Change to the (non-pen) subreport ID, and continue */ Or at least remove ", and restart". Thank you, Stefan and Jiri! Nick
Hi! On 18/04/2024 19:04, Nikolai Kondrashov wrote: > Hi Jiri, Stefan, > > On 4/18/24 4:31 PM, Stefan Berzl wrote: >> >> On 12/04/2024 17:52, Jiri Kosina wrote: >>> On Mon, 1 Apr 2024, Stefan Berzl wrote: >>> >>>> The while in question does nothing except provide the possibility >>>> to have an infinite loop in case the subreport id is actually the same >>>> as the pen id. >>>> >>>> Signed-off-by: Stefan Berzl <stefanberzl@gmail.com> >>> >>> Let me CC Nicolai, the author of the code of question (8b013098be2c9). >> >> I agree that Nicolai's opinion would be invaluable, but even without it, >> the patch is trivially correct. If we have a subreport that matches the >> packet, we change the report_id accordingly. If we then loop back to the >> beginning, either the report_id is different or we are caught in an >> infinite loop. None of these are hardware registers where the access >> itself would matter. > > Yes, Stefan is right. I was trying to implement general rewrite logic, and if > we really had that, then the fix would need to be checking that the new ID is > different. As such there's really no need, and Stefan's fix is fine. > > Only perhaps amend that comment to something like > > /* Change to the (non-pen) subreport ID, and continue */ > > Or at least remove ", and restart". > Will do! I'll send a v2 with the comment updated. Regards
diff --git a/drivers/hid/hid-uclogic-core.c b/drivers/hid/hid-uclogic-core.c index ad74cbc9a0aa..a56f4de216de 100644 --- a/drivers/hid/hid-uclogic-core.c +++ b/drivers/hid/hid-uclogic-core.c @@ -431,40 +431,35 @@ static int uclogic_raw_event(struct hid_device *hdev, if (uclogic_exec_event_hook(params, data, size)) return 0; - while (true) { - /* Tweak pen reports, if necessary */ - if ((report_id == params->pen.id) && (size >= 2)) { - subreport_list_end = - params->pen.subreport_list + - ARRAY_SIZE(params->pen.subreport_list); - /* Try to match a subreport */ - for (subreport = params->pen.subreport_list; - subreport < subreport_list_end; subreport++) { - if (subreport->value != 0 && - subreport->value == data[1]) { - break; - } - } - /* If a subreport matched */ - if (subreport < subreport_list_end) { - /* Change to subreport ID, and restart */ - report_id = data[0] = subreport->id; - continue; - } else { - return uclogic_raw_event_pen(drvdata, data, size); + /* Tweak pen reports, if necessary */ + if ((report_id == params->pen.id) && (size >= 2)) { + subreport_list_end = + params->pen.subreport_list + + ARRAY_SIZE(params->pen.subreport_list); + /* Try to match a subreport */ + for (subreport = params->pen.subreport_list; + subreport < subreport_list_end; subreport++) { + if (subreport->value != 0 && + subreport->value == data[1]) { + break; } } - - /* Tweak frame control reports, if necessary */ - for (i = 0; i < ARRAY_SIZE(params->frame_list); i++) { - if (report_id == params->frame_list[i].id) { - return uclogic_raw_event_frame( - drvdata, ¶ms->frame_list[i], - data, size); - } + /* If a subreport matched */ + if (subreport < subreport_list_end) { + /* Change to subreport ID, and restart */ + report_id = data[0] = subreport->id; + } else { + return uclogic_raw_event_pen(drvdata, data, size); } + } - break; + /* Tweak frame control reports, if necessary */ + for (i = 0; i < ARRAY_SIZE(params->frame_list); i++) { + if (report_id == params->frame_list[i].id) { + return uclogic_raw_event_frame( + drvdata, ¶ms->frame_list[i], + data, size); + } } return 0;
The while in question does nothing except provide the possibility to have an infinite loop in case the subreport id is actually the same as the pen id. Signed-off-by: Stefan Berzl <stefanberzl@gmail.com> --- drivers/hid/hid-uclogic-core.c | 55 ++++++++++++++++------------------ 1 file changed, 25 insertions(+), 30 deletions(-)