2024-04-01 00:48:24

by Stefan Berzl

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

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 <[email protected]>
---
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

2024-04-12 15:52:26

by Jiri Kosina

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

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 <[email protected]>

Let me CC Nicolai, the author of the code of question (8b013098be2c9).

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

--
Jiri Kosina
SUSE Labs

2024-04-18 13:34:56

by Stefan Berzl

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

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 <[email protected]>
>
> 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
>>
>

2024-04-18 17:04:32

by Nikolai Kondrashov

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

Hi Jiri, Stefan,

On 4/18/24 4:31 PM, Stefan Berzl wrote:
>
> 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 <[email protected]>
>>
>> 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.

Yes, Stefan is right. I was trying to implement general rewrite logic, and if
we really had that, then the fix would need to be checking that the new ID is
different. As such there's really no need, and Stefan's fix is fine.

Only perhaps amend that comment to something like

/* Change to the (non-pen) subreport ID, and continue */

Or at least remove ", and restart".

Thank you, Stefan and Jiri!
Nick

2024-04-18 18:46:23

by Stefan Berzl

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

Hi!

On 18/04/2024 19:04, Nikolai Kondrashov wrote:
> Hi Jiri, Stefan,
>
> On 4/18/24 4:31 PM, Stefan Berzl wrote:
>>
>> 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 <[email protected]>
>>>
>>> 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.
>
> Yes, Stefan is right. I was trying to implement general rewrite logic, and if
> we really had that, then the fix would need to be checking that the new ID is
> different. As such there's really no need, and Stefan's fix is fine.
>
> Only perhaps amend that comment to something like
>
> /* Change to the (non-pen) subreport ID, and continue */
>
> Or at least remove ", and restart".
>

Will do! I'll send a v2 with the comment updated.

Regards