Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 30 May 2022 19:33:06 +0200
From:   =?iso-8859-1?Q?Jos=E9_Exp=F3sito?= <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
Message-ID: <20220530173306.GA20330@elementary>
References: <20220518224702.1409327-1-jose.exposito89@gmail.com>
 <20220518224702.1409327-4-jose.exposito89@gmail.com>
 <17153eb3-0eb9-cc05-4b65-9c0f4e8d3c90@gmail.com>
 <20220530065452.GA39613@elementary>
 <a402fa65-e575-6c8d-90ff-c404143d947b@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <a402fa65-e575-6c8d-90ff-c404143d947b@gmail.com>
Precedence: bulk

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, &params->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, &params->frame_list[i],
> >> +				data, size);
> >> +		}
> >>  	}
> >>  
> >>  	return 0;
> >> -- 
> >> 2.36.1
> >>
> >>
> 
> Bye bye
> 
> Stefan Berzl