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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <f6730384-92ca-4f3d-8316-996edc2750fa@gmail.com>
Date: Mon, 9 Jun 2025 00:52:56 +0300
From: Cosmin Tanislav <demonsingur@...il.com>
To: Sean Young <sean@...s.org>
Cc: Mauro Carvalho Chehab <mchehab@...nel.org>, linux-media@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] media: rc: ir-spi: reallocate buffer dynamically



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