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] [day] [month] [year] [list]

Message-ID: <aEaYLnDejiSXmJgN@gofer.mess.org>
Date: Mon, 9 Jun 2025 09:15:42 +0100
From: Sean Young <sean@...s.org>
To: Cosmin Tanislav <demonsingur@...il.com>
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 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@...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.

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