| Message ID | 17153eb3-0eb9-cc05-4b65-9c0f4e8d3c90@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | [for-5.19/uclogic] HID: uclogic: Remove useless loop | expand |
Hi Stefan, Thanks for the patch. You can send it as an standalone patch rather than as a response to my patches, I don't know if it could be missed by maintaners this way. On Sun, May 29, 2022 at 11:49:46PM +0200, 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> > > --- > 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 c0fe66e50c58..1a6b941f3964 100644 > --- a/drivers/hid/hid-uclogic-core.c > +++ b/drivers/hid/hid-uclogic-core.c > @@ -423,40 +423,35 @@ static int uclogic_raw_event(struct hid_device *hdev, > if (report->type != HID_INPUT_REPORT) > 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; Here, in the previous code, the "report_id" is set to the subreport ID and the while loop is executed again with the new ID. The loop acts as a recursive function. Isn't this behaviour removed by your patch? Jose > - } 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.36.1 > >
Hello everynyan! > Hi Stefan, > > Thanks for the patch. You can send it as an standalone patch rather > than as a response to my patches, I don't know if it could be missed by > maintaners this way. You are right about that, I'll keep it in mind. But for consistency, I think it's better to reply here for now. >> 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(-) >> >> diff --git a/drivers/hid/hid-uclogic-core.c b/drivers/hid/hid-uclogic-core.c >> index c0fe66e50c58..1a6b941f3964 100644 >> --- a/drivers/hid/hid-uclogic-core.c >> +++ b/drivers/hid/hid-uclogic-core.c >> @@ -423,40 +423,35 @@ static int uclogic_raw_event(struct hid_device *hdev, >> if (report->type != HID_INPUT_REPORT) >> 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; > > Here, in the previous code, the "report_id" is set to the subreport ID > and the while loop is executed again with the new ID. The loop acts as > a recursive function. > > Isn't this behaviour removed by your patch? > > Jose Think about what this behavior really achieves. In the first iteration, we check if params->pen.id equals the report_id, which is the actual report id from the usb message. If that is the case, we check if the second byte of the message is such that we need an updated "subreport" for this particular message. Therefore, the report_id is set to the subreport->id. This subreport->id is by design supposed to be different from the original params->pen.id, because otherwise, why would we need this update? If we then "continue" with this useless loop, either one of two cases can happen: The best case is that the (report_id = subreport->id) != params->pen.id in which case the if-block won't be executed and we only wasted time. If the (report_id = subreport->id) == params->pen.id however, things get interesting. The "subreport_list_end" and "subreport" variables will again be set to entries based on "params->pen.subreport_list", which is totally unchanged from the last iteration. We will iterate the same subreports, find the same result, set report_id to the same subreport->id and, that's the beauty of it, "continue" this ingenious loop, creating an infinite loop. This contraption is in the best case only wasteful, yet it has been accepted all willy-nilly like. Really gets the noggin joggin. > >> - } 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.36.1 >> >> Bye bye Stefan Berzl
On Mon, May 30, 2022 at 06:46:38PM +0200, Stefan Berzl wrote: > Think about what this behavior really achieves. In the first iteration, > we check if params->pen.id equals the report_id, which is the actual > report id from the usb message. If that is the case, we check if the > second byte of the message is such that we need an updated "subreport" > for this particular message. Therefore, the report_id is set to the > subreport->id. This subreport->id is by design supposed to be different > from the original params->pen.id, because otherwise, why would we need > this update? If we then "continue" with this useless loop, either one of > two cases can happen: > > The best case is that the (report_id = subreport->id) != params->pen.id > in which case the if-block won't be executed and we only wasted time. > > If the (report_id = subreport->id) == params->pen.id however, things get > interesting. The "subreport_list_end" and "subreport" variables will > again be set to entries based on "params->pen.subreport_list", which is > totally unchanged from the last iteration. We will iterate the same > subreports, find the same result, set report_id to the same > subreport->id and, that's the beauty of it, "continue" this ingenious > loop, creating an infinite loop. True, data[1] doesn't change, so an extra if is executed for no reason. I mean, it is not dramatic, but I guess the while loop could be removed for clarity. I wonder why it was implemented in a loop though, check commit 8b013098be ("HID: uclogic: Replace pen_frame_flag with subreport_list"). The while loop is intrudeced there and I can imagine that for a good reason... However, I can not think in a case where removing the loop could cause issues. > This contraption is in the best case only wasteful, yet it has been > accepted all willy-nilly like. Really gets the noggin joggin. > > > > >> - } 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.36.1 > >> > >> > > Bye bye > > Stefan Berzl
diff --git a/drivers/hid/hid-uclogic-core.c b/drivers/hid/hid-uclogic-core.c index c0fe66e50c58..1a6b941f3964 100644 --- a/drivers/hid/hid-uclogic-core.c +++ b/drivers/hid/hid-uclogic-core.c @@ -423,40 +423,35 @@ static int uclogic_raw_event(struct hid_device *hdev, if (report->type != HID_INPUT_REPORT) 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(-)