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

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

[Cosmin Tanislav]

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);
+		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;
+
 	return devm_rc_register_device(dev, idata->rc);
 }
 
-- 
2.49.0

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

[Sean Young]

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

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

[Cosmin Tanislav]

On 6/8/25 11:25 PM, Sean Young wrote:
> 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.
> 

I'm allocating in a loop indeed, but I add at least IR_SPI_MAX_BUFSIZE
each time the new length exceeds the existing length. This should cut
down on the amount of allocations.

In my testing, controlling various devices around the house using IR,
I haven't exceeded two allocations, aka 8192. But having this static
size in place prevents bigger transfers from being done.

>> +		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.
> 

My reasoning was not to save memory, but to allow transfers bigger than
IR_SPI_MAX_BUFSIZE, which currently is 4096. Note that that's periods,
not number of pulses/spaces.

If you want an allocation per transfer then we should compute the total
period size and allocate a big enough buffer to fit all the periods.

Each pulse/space value translates to a different period (depending on
the carrier frequency) so we'd have to compute it once ahead of time
to allocate the periods buffer, and once again afterwards.

To avoid doing the period finding two times we could reuse the passed in
buffer to hold the calculated period, but we'd still be iterating two
times over the passed in buffer.

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

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

[Sean Young]

On Mon, Jun 09, 2025 at 12:52:56AM +0300, Cosmin Tanislav wrote:
> On 6/8/25 11:25 PM, Sean Young wrote:
> > 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.
> > 
> 
> I'm allocating in a loop indeed, but I add at least IR_SPI_MAX_BUFSIZE
> each time the new length exceeds the existing length. This should cut
> down on the amount of allocations.

Yes, you're right.

> In my testing, controlling various devices around the house using IR,
> I haven't exceeded two allocations, aka 8192. But having this static
> size in place prevents bigger transfers from being done.

The name IR_SPI_MAX_BUFSIZE is no longer correct, the buffer can get larger
than that. It should have a new name.
> 
> > > +		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.
> > 
> 
> My reasoning was not to save memory, but to allow transfers bigger than
> IR_SPI_MAX_BUFSIZE, which currently is 4096. Note that that's periods,
> not number of pulses/spaces.
> 
> If you want an allocation per transfer then we should compute the total
> period size and allocate a big enough buffer to fit all the periods.
> 
> Each pulse/space value translates to a different period (depending on
> the carrier frequency) so we'd have to compute it once ahead of time
> to allocate the periods buffer, and once again afterwards.
> 
> To avoid doing the period finding two times we could reuse the passed in
> buffer to hold the calculated period, but we'd still be iterating two
> times over the passed in buffer.

This all makes sense.

There is no reason to do an allocation in the probe function. We should free
the buffer after the transmit too.


Sean