Re: [bug report] platform/surface: Add Surface Aggregator subsystem

[Maximilian Luz <luzmaximilian@gmail.com>]

On 1/25/21 12:42 PM, Dan Carpenter wrote:
> Hello Maximilian Luz,
> 
> The patch c167b9c7e3d6: "platform/surface: Add Surface Aggregator
> subsystem" from Dec 21, 2020, leads to the following static checker
> warning:
> 
> 	drivers/platform/surface/aggregator/ssh_packet_layer.c:1697 ssh_ptl_rx_eval()
> 	warn: check likely/unlikely parens
> 
> drivers/platform/surface/aggregator/ssh_packet_layer.c
>    1683  static size_t ssh_ptl_rx_eval(struct ssh_ptl *ptl, struct ssam_span *source)
>    1684  {
>    1685          struct ssh_frame *frame;
>    1686          struct ssam_span payload;
>    1687          struct ssam_span aligned;
>    1688          bool syn_found;
>    1689          int status;
>    1690
>    1691          /* Error injection: Modify data to simulate corrupt SYN bytes. */
>    1692          ssh_ptl_rx_inject_invalid_syn(ptl, source);
>    1693
>    1694          /* Find SYN. */
>    1695          syn_found = sshp_find_syn(source, &aligned);
>    1696
>    1697          if (unlikely(aligned.ptr - source->ptr) > 0) {
> 
> The unlikely() macro returns 0/1.  Smatch is suggesting that this should
> just be "if (unlikely((aligned.ptr - source->ptr) > 0)) {" but I'm not
> at all sure that that's correct.  Isn't aligned being higher than source
> the normal case?

Hi,

the suggestion by Smatch, i.e.

     if (unlikely((aligned.ptr - source->ptr) > 0)

should be correct. The normal case is aligned.ptr equal to source->ptr.

Let me elaborate a bit for detail: SSH messages all start with a "SYN"
sequence and are frame based. So once we've parsed one message, we
expect it to be followed up directly by the next message. So, here, we
are directly expecting a new message to start, meaning the SYN should be
found at the start of the buffer, or, closer to the code, aligned.ptr
(the start of the message frame) should equal source->ptr. If this is
not the case, this indicates that some unexpected bytes are in-between
the last successfully parsed message and the next message. Usually
that's caused by when a previous message has failed one of the CRC
checks (thus we can't rely on the length encoded in the frame) and we're
actively searching for the next message (via this call here).

Thanks for the report!

Do you want to submit a patch for this or should I do that?

Regards,
Max

> 
>    1698                  ptl_warn(ptl, "rx: parser: invalid start of frame, skipping\n");
>    1699
>    1700                  /*
>    1701                   * Notes:
>    1702                   * - This might send multiple NAKs in case the communication
> 
> regards,
> dan carpenter
>