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
