Re: [PATCH v2] media: rc: ir-spi: reallocate buffer dynamically

[Sean Young <sean@mess.org>]

On Sun, Jun 08, 2025 at 10:15:33PM +0300, Cosmin Tanislav wrote:
> Replace the static transmit buffer with a dynamically allocated one,
> allowing the buffer to grow as needed based on the length of the
> message being transmitted.
> 
> Introduce a helper function ir_buf_realloc() to manage the allocation
> and reallocation of the buffer. Use it during probe to preallocate
> a buffer matching the original static buffer, then reallocate it as
> needed, with an overhead to avoid frequent reallocations.
> 
> Signed-off-by: Cosmin Tanislav <demonsingur@gmail.com>
> ---
> V2:
>  * use devm_krealloc_array
> 
>  drivers/media/rc/ir-spi.c | 32 +++++++++++++++++++++++++++++---
>  1 file changed, 29 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/rc/ir-spi.c b/drivers/media/rc/ir-spi.c
> index 8fc8e496e6aa..2f931950e107 100644
> --- a/drivers/media/rc/ir-spi.c
> +++ b/drivers/media/rc/ir-spi.c
> @@ -27,7 +27,8 @@ struct ir_spi_data {
>  	u32 freq;
>  	bool negated;
>  
> -	u16 tx_buf[IR_SPI_MAX_BUFSIZE];
> +	u16 *tx_buf;
> +	size_t tx_len;
>  	u16 pulse;
>  	u16 space;
>  
> @@ -36,6 +37,26 @@ struct ir_spi_data {
>  	struct regulator *regulator;
>  };
>  
> +static int ir_buf_realloc(struct ir_spi_data *idata, size_t len)
> +{
> +	u16 *tx_buf;
> +
> +	if (len <= idata->tx_len)
> +		return 0;
> +
> +	len = max(len, idata->tx_len + IR_SPI_MAX_BUFSIZE);
> +
> +	tx_buf = devm_krealloc_array(&idata->spi->dev, idata->tx_buf, len,
> +				     sizeof(*idata->tx_buf), GFP_KERNEL);
> +	if (!tx_buf)
> +		return -ENOMEM;
> +
> +	idata->tx_buf = tx_buf;
> +	idata->tx_len = len;
> +
> +	return 0;
> +}
> +
>  static int ir_spi_tx(struct rc_dev *dev, unsigned int *buffer, unsigned int count)
>  {
>  	int i;
> @@ -52,8 +73,9 @@ static int ir_spi_tx(struct rc_dev *dev, unsigned int *buffer, unsigned int coun
>  
>  		periods = DIV_ROUND_CLOSEST(buffer[i] * idata->freq, 1000000);
>  
> -		if (len + periods >= IR_SPI_MAX_BUFSIZE)
> -			return -EINVAL;
> +		ret = ir_buf_realloc(idata, len + periods);

You're reallocating in a loop. That causes a lot of churn.

> +		if (ret)
> +			return ret;
>  
>  		/*
>  		 * The first value in buffer is a pulse, so that 0, 2, 4, ...
> @@ -153,6 +175,10 @@ static int ir_spi_probe(struct spi_device *spi)
>  
>  	idata->freq = IR_SPI_DEFAULT_FREQUENCY;
>  
> +	ret = ir_buf_realloc(idata, IR_SPI_MAX_BUFSIZE);
> +	if (ret)
> +		return ret;
> +

By default, you're allocating IR_SPI_MAX_BUFSIZE already at probe time. So
until someone does a transmit, you haven't saved any memory compared to
before. In fact, the text size will be more so things are worse.

It might make sense to allocate IR_SPI_MAX_BUFSIZE once for each transmit;
most drivers do an allocation per transmit which is perfectly acceptable.


Thanks,

Sean

>  	return devm_rc_register_device(dev, idata->rc);
>  }
>  
> -- 
> 2.49.0