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 08:54:52 +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: <20220530065452.GA39613@elementary>
References: <20220518224702.1409327-1-jose.exposito89@gmail.com>
 <20220518224702.1409327-4-jose.exposito89@gmail.com>
 <17153eb3-0eb9-cc05-4b65-9c0f4e8d3c90@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <17153eb3-0eb9-cc05-4b65-9c0f4e8d3c90@gmail.com>
Precedence: bulk

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.

On Sun, May 29, 2022 at 11:49:46PM +0200, 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@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

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