Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Message-ID: <a402fa65-e575-6c8d-90ff-c404143d947b@gmail.com>
Date:   Mon, 30 May 2022 18:46:38 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.9.0
Subject: Re: [PATCH for-5.19/uclogic] HID: uclogic: Remove useless loop
Content-Language: en-US
To:     =?UTF-8?B?Sm9zw6kgRXhww7NzaXRv?= <jose.exposito89@gmail.com>
Cc:     jikos@kernel.org, benjamin.tissoires@redhat.com, spbnick@gmail.com,
        linux-input@vger.kernel.org, linux-kernel@vger.kernel.org
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>
From:   Stefan Berzl <stefanberzl@gmail.com>
In-Reply-To: <20220530065452.GA39613@elementary>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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, &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