Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH] platform/surface: aggregator: Fix braces in if condition
 with unlikely() macro
To:     Maximilian Luz <luzmaximilian@gmail.com>,
        platform-driver-x86@vger.kernel.org
Cc:     Mark Gross <mgross@linux.intel.com>,
        Dan Carpenter <dan.carpenter@oracle.com>,
        linux-kernel@vger.kernel.org
References: <20210126172202.1428367-1-luzmaximilian@gmail.com>
From:   Hans de Goede <hdegoede@redhat.com>
Message-ID: <81c1f69c-30ba-d01f-002f-41da5cb670aa@redhat.com>
Date:   Wed, 3 Feb 2021 12:00:29 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.6.1
MIME-Version: 1.0
In-Reply-To: <20210126172202.1428367-1-luzmaximilian@gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi,

On 1/26/21 6:22 PM, Maximilian Luz wrote:
> The braces of the unlikely() macro inside the if condition only cover
> the subtraction part, not the whole statement. This causes the result of
> the subtraction to be converted to zero or one. While that still works
> in this context, it causes static analysis tools to complain (and is
> just plain wrong).
> 
> Fix the bracket placement and, while at it, simplify the if-condition.
> Also add a comment to the if-condition explaining what we expect the
> result to be and what happens on the failure path, as it seems to have
> caused a bit of confusion.
> 
> This commit should not cause any difference in behavior or generated
> code.
> 
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Fixes: c167b9c7e3d6 ("platform/surface: Add Surface Aggregator subsystem")
> Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>

Thank you for your patch, I've applied this patch to my review-hans 
branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.

Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.

Regards,

Hans


> ---
>  .../surface/aggregator/ssh_packet_layer.c     | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/surface/aggregator/ssh_packet_layer.c b/drivers/platform/surface/aggregator/ssh_packet_layer.c
> index 74f0faaa2b27..583315db8b02 100644
> --- a/drivers/platform/surface/aggregator/ssh_packet_layer.c
> +++ b/drivers/platform/surface/aggregator/ssh_packet_layer.c
> @@ -1694,7 +1694,24 @@ static size_t ssh_ptl_rx_eval(struct ssh_ptl *ptl, struct ssam_span *source)
>  	/* Find SYN. */
>  	syn_found = sshp_find_syn(source, &aligned);
>  
> -	if (unlikely(aligned.ptr - source->ptr) > 0) {
> +	if (unlikely(aligned.ptr != source->ptr)) {
> +		/*
> +		 * We expect aligned.ptr == source->ptr. If this is not the
> +		 * case, then aligned.ptr > source->ptr and we've encountered
> +		 * some unexpected data where we'd expect the start of a new
> +		 * message (i.e. the SYN sequence).
> +		 *
> +		 * This can happen when a CRC check for the previous message
> +		 * failed and we start actively searching for the next one
> +		 * (via the call to sshp_find_syn() above), or the first bytes
> +		 * of a message got dropped or corrupted.
> +		 *
> +		 * In any case, we issue a warning, send a NAK to the EC to
> +		 * request re-transmission of any data we haven't acknowledged
> +		 * yet, and finally, skip everything up to the next SYN
> +		 * sequence.
> +		 */
> +
>  		ptl_warn(ptl, "rx: parser: invalid start of frame, skipping\n");
>  
>  		/*
>