Re: [PATCH v2] fsi: occ: Improve response status checking

[Joel Stanley]

On Tue, 8 Feb 2022 at 15:22, Eddie James <eajames@xxxxxxxxxxxxx> wrote:
>
> If the driver sequence number coincidentally equals the previous
> command response sequence number, the driver may proceed with
> fetching the entire buffer before the OCC has processed the current
> command. To be sure the correct response is obtained, check the
> command type and also retry if any of the response parameters have
> changed when the rest of the buffer is fetched. Also initialize the
> driver with a random sequence number in order to reduce the chances
> of this happening.
>
> Signed-off-by: Eddie James <eajames@xxxxxxxxxxxxx>

Reviewed-by: Joel Stanley <joel@xxxxxxxxx>

I've applied this for v5.18.

> ---
> Changes since v1:
>  - Refactor the retry into one loop
>  - Add a comment about the pseudo-random number
>
>  drivers/fsi/fsi-occ.c | 87 ++++++++++++++++++++++++++++---------------
>  1 file changed, 56 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/fsi/fsi-occ.c b/drivers/fsi/fsi-occ.c
> index 7eaab1be0aa4..c9cc75fbdfb9 100644
> --- a/drivers/fsi/fsi-occ.c
> +++ b/drivers/fsi/fsi-occ.c
> @@ -451,6 +451,14 @@ static int occ_trigger_attn(struct occ *occ)
>         return rc;
>  }
>
> +static bool fsi_occ_response_not_ready(struct occ_response *resp, u8 seq_no,
> +                                      u8 cmd_type)
> +{
> +       return resp->return_status == OCC_RESP_CMD_IN_PRG ||
> +               resp->return_status == OCC_RESP_CRIT_INIT ||
> +               resp->seq_no != seq_no || resp->cmd_type != cmd_type;
> +}
> +
>  int fsi_occ_submit(struct device *dev, const void *request, size_t req_len,
>                    void *response, size_t *resp_len)
>  {
> @@ -461,10 +469,11 @@ int fsi_occ_submit(struct device *dev, const void *request, size_t req_len,
>         struct occ_response *resp = response;
>         size_t user_resp_len = *resp_len;
>         u8 seq_no;
> +       u8 cmd_type;
>         u16 checksum = 0;
>         u16 resp_data_length;
>         const u8 *byte_request = (const u8 *)request;
> -       unsigned long start;
> +       unsigned long end;
>         int rc;
>         size_t i;
>
> @@ -478,6 +487,8 @@ int fsi_occ_submit(struct device *dev, const void *request, size_t req_len,
>                 return -EINVAL;
>         }
>
> +       cmd_type = byte_request[1];
> +
>         /* Checksum the request, ignoring first byte (sequence number). */
>         for (i = 1; i < req_len - 2; ++i)
>                 checksum += byte_request[i];
> @@ -509,51 +520,61 @@ int fsi_occ_submit(struct device *dev, const void *request, size_t req_len,
>         if (rc)
>                 goto done;
>
> -       /* Read occ response header */
> -       start = jiffies;
> -       do {
> +       end = jiffies + timeout;
> +       while (true) {
> +               /* Read occ response header */
>                 rc = occ_getsram(occ, 0, resp, 8);
>                 if (rc)
>                         goto done;
>
> -               if (resp->return_status == OCC_RESP_CMD_IN_PRG ||
> -                   resp->return_status == OCC_RESP_CRIT_INIT ||
> -                   resp->seq_no != seq_no) {
> -                       rc = -ETIMEDOUT;
> -
> -                       if (time_after(jiffies, start + timeout)) {
> -                               dev_err(occ->dev, "resp timeout status=%02x "
> -                                       "resp seq_no=%d our seq_no=%d\n",
> +               if (fsi_occ_response_not_ready(resp, seq_no, cmd_type)) {
> +                       if (time_after(jiffies, end)) {
> +                               dev_err(occ->dev,
> +                                       "resp timeout status=%02x seq=%d cmd=%d, our seq=%d cmd=%d\n",
>                                         resp->return_status, resp->seq_no,
> -                                       seq_no);
> +                                       resp->cmd_type, seq_no, cmd_type);
> +                               rc = -ETIMEDOUT;
>                                 goto done;
>                         }
>
>                         set_current_state(TASK_UNINTERRUPTIBLE);
>                         schedule_timeout(wait_time);
> -               }
> -       } while (rc);
> -
> -       /* Extract size of response data */
> -       resp_data_length = get_unaligned_be16(&resp->data_length);
> +               } else {
> +                       /* Extract size of response data */
> +                       resp_data_length =
> +                               get_unaligned_be16(&resp->data_length);
> +
> +                       /*
> +                        * Message size is data length + 5 bytes header + 2
> +                        * bytes checksum
> +                        */
> +                       if ((resp_data_length + 7) > user_resp_len) {
> +                               rc = -EMSGSIZE;
> +                               goto done;
> +                       }
>
> -       /* Message size is data length + 5 bytes header + 2 bytes checksum */
> -       if ((resp_data_length + 7) > user_resp_len) {
> -               rc = -EMSGSIZE;
> -               goto done;
> +                       /*
> +                        * Get the entire response including the header again,
> +                        * in case it changed
> +                        */
> +                       if (resp_data_length > 1) {
> +                               rc = occ_getsram(occ, 0, resp,
> +                                                resp_data_length + 7);
> +                               if (rc)
> +                                       goto done;
> +
> +                               if (!fsi_occ_response_not_ready(resp, seq_no,
> +                                                               cmd_type))
> +                                       break;
> +                       } else {
> +                               break;
> +                       }
> +               }
>         }
>
>         dev_dbg(dev, "resp_status=%02x resp_data_len=%d\n",
>                 resp->return_status, resp_data_length);
>
> -       /* Grab the rest */
> -       if (resp_data_length > 1) {
> -               /* already got 3 bytes resp, also need 2 bytes checksum */
> -               rc = occ_getsram(occ, 8, &resp->data[3], resp_data_length - 1);
> -               if (rc)
> -                       goto done;
> -       }
> -
>         occ->client_response_size = resp_data_length + 7;
>         rc = occ_verify_checksum(occ, resp, resp_data_length);
>
> @@ -598,7 +619,11 @@ static int occ_probe(struct platform_device *pdev)
>         occ->version = (uintptr_t)of_device_get_match_data(dev);
>         occ->dev = dev;
>         occ->sbefifo = dev->parent;
> -       occ->sequence_number = 1;
> +       /*
> +        * Quickly derive a pseudo-random number from jiffies so that
> +        * re-probing the driver doesn't accidentally overlap sequence numbers.
> +        */
> +       occ->sequence_number = (u8)((jiffies % 0xff) + 1);
>         mutex_init(&occ->occ_lock);
>
>         if (dev->of_node) {
> --
> 2.27.0
>