Re: [PATCH v3] media: rc: ir-spi: allocate buffer dynamically

[Cosmin Tanislav]

On 6/10/25 12:39 PM, Sean Young wrote:
> On Mon, Jun 09, 2025 at 02:17:13PM +0300, Cosmin Tanislav wrote:
> > Replace the static transmit buffer with a dynamically allocated one,
> > removing the limit imposed on the number of pulses to transmit.
> >
> > Calculate the number of pulses for each duration in the received buffer
> > ahead of time, while also adding up the total pulses, to be able to
> > allocate a buffer that perfectly fits the total number of pulses, then
> > populate it.
> >
> > Signed-off-by: Cosmin Tanislav <demonsingur@xxxxxxxxx>
> > ---
> > V3:
> >   * move the allocation to be done per-TX operation
> >
> > V2:
> >   * use devm_krealloc_array
> >
> >   drivers/media/rc/ir-spi.c | 33 ++++++++++++++++++++-------------
> >   1 file changed, 20 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/media/rc/ir-spi.c b/drivers/media/rc/ir-spi.c
> > index 8fc8e496e6aa..50e30e2fae22 100644
> > --- a/drivers/media/rc/ir-spi.c
> > +++ b/drivers/media/rc/ir-spi.c
> > @@ -21,13 +21,11 @@
> >   #define IR_SPI_DRIVER_NAME		"ir-spi"
> >   
> >   #define IR_SPI_DEFAULT_FREQUENCY	38000
> > -#define IR_SPI_MAX_BUFSIZE		 4096
> >   
> >   struct ir_spi_data {
> >   	u32 freq;
> >   	bool negated;
> >   
> > -	u16 tx_buf[IR_SPI_MAX_BUFSIZE];
> >   	u16 pulse;
> >   	u16 space;
> >   
> > @@ -43,37 +41,42 @@ static int ir_spi_tx(struct rc_dev *dev, unsigned int *buffer, unsigned int coun
> >   	unsigned int len = 0;
> >   	struct ir_spi_data *idata = dev->priv;
> >   	struct spi_transfer xfer;
> > +	u16 *tx_buf;
> >   
> >   	/* convert the pulse/space signal to raw binary signal */
> >   	for (i = 0; i < count; i++) {
> > -		unsigned int periods;
> > +		buffer[i] = DIV_ROUND_CLOSEST(buffer[i] * idata->freq, 1000000);
> > +		len += buffer[i];
> > +	}
>
> This looks great, thank you.
>
> I do have one concern though. If someone sets a carrier of U32_MAX - 1 then
> this code could be doing largish allocations, spending too long in kernel
> space filling them with data and spi can't send it anyway. Actually
> the kmalloc might fail which doesn't look good in the logs.
>
> We may have to constrain the carrier to something spi can handle.

The SPI device has a max_speed_hz, maybe we should check that?

It seems to be set based on the spi-max-frequency property in
the device tree node of the SPI device, and uses the max_speed_hz
of the SPI controller as a fallback.

Should I add a separate patch that adds a check in
ir_spi_set_tx_carrier?

if (carrier * 16 > idata->spi->max_speed_hz)
	return -EINVAL.

Something along these lines.

> Sean
>
> > +
> > +	tx_buf = kmalloc_array(len, sizeof(*tx_buf), GFP_KERNEL);
> > +	if (!tx_buf)
> > +		return -ENOMEM;
> > +
> > +	len = 0;
> > +	for (i = 0; i < count; i++) {
> >   		int j;
> >   		u16 val;
> >   
> > -		periods = DIV_ROUND_CLOSEST(buffer[i] * idata->freq, 1000000);
> > -
> > -		if (len + periods >= IR_SPI_MAX_BUFSIZE)
> > -			return -EINVAL;
> > -
> >   		/*
> >   		 * The first value in buffer is a pulse, so that 0, 2, 4, ...
> >   		 * contain a pulse duration. On the contrary, 1, 3, 5, ...
> >   		 * contain a space duration.
> >   		 */
> >   		val = (i % 2) ? idata->space : idata->pulse;
> > -		for (j = 0; j < periods; j++)
> > -			idata->tx_buf[len++] = val;
> > +		for (j = 0; j < buffer[i]; j++)
> > +			tx_buf[len++] = val;
> >   	}
> >   
> >   	memset(&xfer, 0, sizeof(xfer));
> >   
> >   	xfer.speed_hz = idata->freq * 16;
> > -	xfer.len = len * sizeof(*idata->tx_buf);
> > -	xfer.tx_buf = idata->tx_buf;
> > +	xfer.len = len * sizeof(*tx_buf);
> > +	xfer.tx_buf = tx_buf;
> >   
> >   	ret = regulator_enable(idata->regulator);
> >   	if (ret)
> > -		return ret;
> > +		goto err_free_tx_buf;
> >   
> >   	ret = spi_sync_transfer(idata->spi, &xfer, 1);
> >   	if (ret)
> > @@ -81,6 +84,10 @@ static int ir_spi_tx(struct rc_dev *dev, unsigned int *buffer, unsigned int coun
> >   
> >   	regulator_disable(idata->regulator);
> >   
> > +err_free_tx_buf:
> > +
> > +	kfree(tx_buf);
> > +
> >   	return ret ? ret : count;
> >   }
> >   
> > --
> > 2.49.0