Re: [PATCH 1/3] fsi: occ: Force sequence numbering per OCC

[Joel Stanley <joel@jms.id.au>]

On Fri, 16 Jul 2021 at 15:19, Eddie James <eajames@linux.ibm.com> wrote:
>
> Set and increment the sequence number during the submit operation.
> This prevents sequence number conflicts between different users of
> the interface. A sequence number conflict may result in a user
> getting an OCC response meant for a different command. Since the
> sequence number is now modified, the checksum must be calculated and
> set before submitting the command.
>
> Signed-off-by: Eddie James <eajames@linux.ibm.com>

Reviewed-by: Joel Stanley <joel@jms.id.au>

> @@ -479,11 +483,26 @@ int fsi_occ_submit(struct device *dev, const void *request, size_t req_len,
>                 return -EINVAL;
>         }
>
> +       /* Checksum the request, ignoring first byte (sequence number). */
> +       for (i = 1; i < req_len - 2; ++i)
> +               checksum += byte_request[i];
> +

This could go below, after you've got the sequence number, so the
checksumming all happens in the same spot?

The driver has become a bit of a maze, I can't tell how you're
deciding what goes in fsi_occ_submit vs occ_write vs occ_putsram. If
oyu have some ideas on how to simplify it then I would welcome those
changes.



>         mutex_lock(&occ->occ_lock);
>
> -       /* Extract the seq_no from the command (first byte) */
> -       seq_no = *(const u8 *)request;
> -       rc = occ_putsram(occ, request, req_len);
> +       /*
> +        * Get a sequence number and update the counter. Avoid a sequence
> +        * number of 0 which would pass the response check below even if the
> +        * OCC response is uninitialized. Any sequence number the user is
> +        * trying to send is overwritten since this function is the only common
> +        * interface to the OCC and therefore the only place we can guarantee
> +        * unique sequence numbers.
> +        */
> +       seq_no = occ->sequence_number++;
> +       if (!occ->sequence_number)
> +               occ->sequence_number = 1;
> +       checksum += seq_no;
> +
> +       rc = occ_putsram(occ, request, req_len, seq_no, checksum);
>         if (rc)
>                 goto done;