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

[Cosmin Tanislav <demonsingur@gmail.com>]

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@gmail.com>
>> ---
>> 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
>>