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@xxxxxxxxxx>
> Fixes: c167b9c7e3d6 ("platform/surface: Add Surface Aggregator subsystem")
> Signed-off-by: Maximilian Luz <luzmaximilian@xxxxxxxxx>
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");
>
> /*
>
