Re: [PATCH v3 1/4] fsi: occ: Use a large buffer for responses

[Joel Stanley]

On Mon, 27 Sept 2021 at 15:59, Eddie James <eajames@xxxxxxxxxxxxx> wrote:
>
> Allocate a large buffer for each OCC to handle response data. This
> removes memory allocation during an operation, and also allows for
> the maximum amount of SBE FFDC.

Why do we need this change? (is it fixing a bug, did the host change,
is it an unimplemented feature, etc)

>
> Signed-off-by: Eddie James <eajames@xxxxxxxxxxxxx>
> ---
>  drivers/fsi/fsi-occ.c   | 109 ++++++++++++++++------------------------
>  include/linux/fsi-occ.h |   2 +
>  2 files changed, 45 insertions(+), 66 deletions(-)
>
> diff --git a/drivers/fsi/fsi-occ.c b/drivers/fsi/fsi-occ.c
> index b0c9322078a1..ace3ec7767e5 100644
> --- a/drivers/fsi/fsi-occ.c
> +++ b/drivers/fsi/fsi-occ.c
> @@ -10,6 +10,7 @@
>  #include <linux/kernel.h>
>  #include <linux/list.h>
>  #include <linux/miscdevice.h>
> +#include <linux/mm.h>
>  #include <linux/module.h>
>  #include <linux/mutex.h>
>  #include <linux/fsi-occ.h>
> @@ -42,13 +43,6 @@
>
>  #define OCC_P10_SRAM_MODE      0x58    /* Normal mode, OCB channel 2 */
>
> -/*
> - * Assume we don't have much FFDC, if we do we'll overflow and
> - * fail the command. This needs to be big enough for simple
> - * commands as well.
> - */
> -#define OCC_SBE_STATUS_WORDS   32
> -
>  #define OCC_TIMEOUT_MS         1000
>  #define OCC_CMD_IN_PRG_WAIT_MS 50
>
> @@ -60,6 +54,7 @@ struct occ {
>         char name[32];
>         int idx;
>         u8 sequence_number;
> +       void *buffer;
>         enum versions version;
>         struct miscdevice mdev;
>         struct mutex occ_lock;
> @@ -250,8 +245,10 @@ static int occ_verify_checksum(struct occ *occ, struct occ_response *resp,
>  static int occ_getsram(struct occ *occ, u32 offset, void *data, ssize_t len)
>  {
>         u32 data_len = ((len + 7) / 8) * 8;     /* must be multiples of 8 B */
> -       size_t cmd_len, resp_len, resp_data_len;
> -       __be32 *resp, cmd[6];
> +       size_t cmd_len, resp_data_len;
> +       size_t resp_len = OCC_MAX_RESP_WORDS;
> +       __be32 *resp = occ->buffer;
> +       __be32 cmd[6];
>         int idx = 0, rc;
>
>         /*
> @@ -278,19 +275,19 @@ static int occ_getsram(struct occ *occ, u32 offset, void *data, ssize_t len)
>         cmd[1] = cpu_to_be32(SBEFIFO_CMD_GET_OCC_SRAM);
>         cmd[4 + idx] = cpu_to_be32(data_len);
>
> -       resp_len = (data_len >> 2) + OCC_SBE_STATUS_WORDS;
> -       resp = kzalloc(resp_len << 2, GFP_KERNEL);

Previously the driver would zero the buffer before using it. Should
you add a memset here?

> @@ -635,6 +605,10 @@ static int occ_probe(struct platform_device *pdev)
>         if (!occ)
>                 return -ENOMEM;
>
> +       occ->buffer = kvmalloc(OCC_MAX_RESP_WORDS * 4, GFP_KERNEL);

Why do you allocate WORDS * 4?

> diff --git a/include/linux/fsi-occ.h b/include/linux/fsi-occ.h
> index d4cdc2aa6e33..7ee3dbd7f4b3 100644
> --- a/include/linux/fsi-occ.h
> +++ b/include/linux/fsi-occ.h
> @@ -19,6 +19,8 @@ struct device;
>  #define OCC_RESP_CRIT_OCB              0xE3
>  #define OCC_RESP_CRIT_HW               0xE4
>
> +#define OCC_MAX_RESP_WORDS             2048

Does this need to go in the header?