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

[Dan Carpenter <dan.carpenter@oracle.com>]

On Mon, Jan 25, 2021 at 01:12:06PM +0100, Maximilian Luz wrote:
> 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?
> 

I think I understand, but can you send the patch for this.  Why not just
make the condition:

	if (aligned.ptr != source->ptr) {

Anyway, I assume you know what you're doing.  :)

regards,
dan carepnter