Re: Re: [PATCH] HID: uclogic: Remove useless loop

[Stefan Berzl]

On 12/04/2024 17:52, Jiri Kosina wrote:
> On Mon, 1 Apr 2024, 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@xxxxxxxxx>
> 
> Let me CC Nicolai, the author of the code of question (8b013098be2c9).

I agree that Nicolai's opinion would be invaluable, but even without it,
the patch is trivially correct. If we have a subreport that matches the
packet, we change the report_id accordingly. If we then loop back to the
beginning, either the report_id is different or we are caught in an
infinite loop. None of these are hardware registers where the access
itself would matter.

>> ---
>>  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 ad74cbc9a0aa..a56f4de216de 100644
>> --- a/drivers/hid/hid-uclogic-core.c
>> +++ b/drivers/hid/hid-uclogic-core.c
>> @@ -431,40 +431,35 @@ static int uclogic_raw_event(struct hid_device *hdev,
>>  	if (uclogic_exec_event_hook(params, data, size))
>>  		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, &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.43.0
>>
>