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

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

[Cosmin Tanislav]

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];
+	}
+
+	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

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

[Sean Young]

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.


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
> 

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@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
>>

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

[Sean Young]

On Tue, Jun 10, 2025 at 01:15:32PM +0300, Cosmin Tanislav wrote:
> 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.

That's perfect. The 16 will need a comment or a #define


Sean