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, ¶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.43.0
>>
>
