From mboxrd@z Thu Jan  1 00:00:00 1970
From: Urja Rannikko <urjaman@gmail.com>
Date: Sun, 19 May 2019 17:04:45 +0000
Subject: [U-Boot] [PATCH 17/17] spi: Avoid using malloc() in a critical
	function
In-Reply-To: <20190518175954.262021-18-sjg@chromium.org>
References: <20190518175954.262021-1-sjg@chromium.org>
 <20190518175954.262021-18-sjg@chromium.org>
Message-ID: <CAPCnQJ=utRAsB17xBtt0fw3etxstobbsq8RgM9ekB7UV7gSYdg@mail.gmail.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Hi,

On Sat, May 18, 2019 at 6:05 PM Simon Glass <sjg@chromium.org> wrote:
>
> In general we should avoid calling malloc() and free() repeatedly in
> U-Boot lest we turn it into tianocore. In SPL this can make SPI flash
> unusable since free() is often a nop and allocation space is limited.
>
> In any case, these seems no need for malloc() since the number of bytes
> is very small, perhaps less than 8.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Fixes: d13f5b254a (spi: Extend the core to ease integration of SPI
>         memory controllers)
>
> ---
>
>  drivers/spi/spi-mem.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
> index b86eee75bcb..7aabebeff5f 100644
> --- a/drivers/spi/spi-mem.c
> +++ b/drivers/spi/spi-mem.c
> @@ -201,7 +201,6 @@ int spi_mem_exec_op(struct spi_slave *slave, const struct spi_mem_op *op)
>         unsigned int pos = 0;
>         const u8 *tx_buf = NULL;
>         u8 *rx_buf = NULL;
> -       u8 *op_buf;
>         int op_len;
>         u32 flag;
>         int ret;
> @@ -338,7 +337,17 @@ int spi_mem_exec_op(struct spi_slave *slave, const struct spi_mem_op *op)
>         }
>
>         op_len = sizeof(op->cmd.opcode) + op->addr.nbytes + op->dummy.nbytes;
> -       op_buf = calloc(1, op_len);
> +
> +       /*
> +        * Avoid using malloc() here so that we can use this code in SPL where
> +        * simple malloc may be used. That implementation does not allow free()
> +        * so repeated calls to this code can exhaust the space.
> +        *
> +        * The value of op_len is small, since it does not include the actual
> +        * data being sent, only the op-code and address. In fact, it should be
> +        * possible to just use a small fixed value here instead of op_len.
> +        */
> +       u8 op_buf[op_len];

I'd say just make this a fixed buffer instead of a VLA - less code
space bloat and potential stack problems in case of nonsensical
inputs.

As for the size, 8 bytes would be fine and actually leave some margin:
the most i would expect for op_len is 1 + 4 + 1 = 6 bytes (the lowest
would be 1+3+0 and the usual 1+3+1).

-- 
Urja Rannikko