From: "José Expósito" <jose.exposito89@gmail.com>
To: Stefan Berzl <stefanberzl@gmail.com>
Cc: jikos@kernel.org, benjamin.tissoires@redhat.com,
spbnick@gmail.com, linux-input@vger.kernel.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH for-5.19/uclogic] HID: uclogic: Remove useless loop
Date: Mon, 30 May 2022 19:33:06 +0200 [thread overview]
Message-ID: <20220530173306.GA20330@elementary> (raw)
In-Reply-To: <a402fa65-e575-6c8d-90ff-c404143d947b@gmail.com>
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
next prev parent reply other threads:[~2022-05-30 17:33 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-05-18 22:46 [PATCH for-5.19/uclogic v2 0/4] Add support for XP-PEN Deco L José Expósito
2022-05-18 22:46 ` [PATCH for-5.19/uclogic v2 1/4] HID: uclogic: Make template placeholder IDs generic José Expósito
2022-05-18 22:47 ` [PATCH for-5.19/uclogic v2 2/4] HID: uclogic: Add KUnit tests for uclogic_rdesc_template_apply() José Expósito
2022-05-18 22:47 ` [PATCH for-5.19/uclogic v2 3/4] HID: uclogic: Allow to generate frame templates José Expósito
2022-05-29 21:49 ` [PATCH for-5.19/uclogic] HID: uclogic: Remove useless loop Stefan Berzl
2022-05-30 6:54 ` José Expósito
2022-05-30 16:46 ` Stefan Berzl
2022-05-30 17:33 ` José Expósito [this message]
2022-05-18 22:47 ` [PATCH for-5.19/uclogic v2 4/4] HID: uclogic: Add support for XP-PEN Deco L José Expósito
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20220530173306.GA20330@elementary \
--to=jose.exposito89@gmail.com \
--cc=benjamin.tissoires@redhat.com \
--cc=jikos@kernel.org \
--cc=linux-input@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=spbnick@gmail.com \
--cc=stefanberzl@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).