Re: [PATCH v7 9/9] iio: ssp_sensors: reuse embedded RX buffer for SPI transfers

[Sanjay Chitroda <sanjayembeddedse@gmail.com>]

On 26 April 2026 8:01:54 pm IST, Jonathan Cameron <jic23@kernel.org> wrote:
>On Sun, 26 Apr 2026 14:47:10 +0530
>Sanjay Chitroda <sanjayembeddedse@gmail.com> wrote:
>
>> From: Sanjay Chitroda <sanjayembeddedse@gmail.com>
>> 
>> Avoid allocating a temporary DMA buffer in the interrupt context when
>> handling hub-to-AP and AP-to-hub SPI write messages.
>> 
>> Replace the dynamically allocated RX buffer with a fixed-size,
>> preallocated buffer embedded in the driver structure and reused for all
>> SPI receive operations. This removes memory allocation from the
>> IRQ path, simplifies lifetime management.
>> 
>> Signed-off-by: Sanjay Chitroda <sanjayembeddedse@gmail.com>
>> ---
>> Changes in v6:
>> - Replace dynamically allocated RX buffer with embedded fixed-size buffer
>> - Fix struct layout to satisfy DMA alignment constraints comment from David
>> - v5 change: https://lore.kernel.org/all/20260406080852.2727453-6-sanjayembedded@gmail.com/
>> Changes in v5:
>> - Rebase change on top of latest v5 patch series.
>> Changes in v4:
>> - Use preallocated buffer and stash a buffer that gets reused each time instead of a fresh allocation.
>> - v3 change: https://lore.kernel.org/all/20260315125509.857195-3-sanjayembedded@gmail.com/
>> Changes in v3:
>> - prepare series to have all respective cleanup API support for the ssp_sensors following input from Andy Shevchenko
>> - v2 change: https://lore.kernel.org/all/20260311174151.3441429-1-sanjayembedded@gmail.com/
>> Changes in v2:
>> - split series to individual patch
>> - address review comment from Andy Shevchenko
>> - v1 change: https://lore.kernel.org/all/20260310200513.2162018-3-sanjayembedded@gmail.com/
>> ---
>>  drivers/iio/common/ssp_sensors/ssp.h     |  3 +++
>>  drivers/iio/common/ssp_sensors/ssp_spi.c | 20 ++------------------
>>  2 files changed, 5 insertions(+), 18 deletions(-)
>> 
>> diff --git a/drivers/iio/common/ssp_sensors/ssp.h b/drivers/iio/common/ssp_sensors/ssp.h
>> index f649cdecc277..8295bb7062a3 100644
>> --- a/drivers/iio/common/ssp_sensors/ssp.h
>> +++ b/drivers/iio/common/ssp_sensors/ssp.h
>> @@ -174,6 +174,7 @@ struct ssp_sensorhub_info {
>>   * @pending_list:	pending list for messages queued to be sent/read
>>   * @sensor_devs:	registered IIO devices table
>>   * @enable_refcount:	enable reference count for wdt (watchdog timer)
>> + * @rx_buf:		buffer to receive SPI data
>>   * @header_buffer:	cache aligned buffer for packet header
>>   */
>>  struct ssp_data {
>> @@ -221,6 +222,8 @@ struct ssp_data {
>>  	struct iio_dev *sensor_devs[SSP_SENSOR_MAX];
>>  	atomic_t enable_refcount;
>>  
>> +	u8 rx_buf[SSP_DATA_PACKET_SIZE];
>
>Whilst the define exists for this size, do we have anything to confirm it is right?
>It wasn't previously used for anything so is a bit high risk.
>
Nothing specific to confirm, I'm assuming this macro and value is added during development so must be specificing as per technical or reference manual for the SSP firmware protocol.

Although, we can plan to add check as pre-change like below:

if (length -> sizeof(data->rx_buf)
     return -EINVAL;


>Also, why doesn't it need to be DMA safe?  I'd expect it either after the following buffer
>or with a __aligned(IIO_DMA_MINALIGN) marking in which case the one below isn't used.
>
I will make it 
u8 rx_buf[SSP_DATA_PACKET_SIZE] __aligned(IIO_DMA_MINALIGN);
>
>> +
>>  	__le16 header_buffer[SSP_HEADER_BUFFER_SIZE / sizeof(__le16)] __aligned(IIO_DMA_MINALIGN);
>>  };
>>  
>> diff --git a/drivers/iio/common/ssp_sensors/ssp_spi.c b/drivers/iio/common/ssp_sensors/ssp_spi.c
>> index 870214551f0b..e41da88bf96d 100644
>> --- a/drivers/iio/common/ssp_sensors/ssp_spi.c
>> +++ b/drivers/iio/common/ssp_sensors/ssp_spi.c
>> @@ -339,7 +339,7 @@ static int ssp_parse_dataframe(struct ssp_data *data, char *dataframe, int len)
>>  /* threaded irq */
>>  int ssp_irq_msg(struct ssp_data *data)
>>  {
>> -	char *buffer;
>> +	char *buffer = data->rx_buf;
>
>Put this in inline. The local variable may reduce code churn but
>it also hides the important info that this is not locally allocated.
>
Agree, will update.

>>  	u8 msg_type;
>>  	int ret;
>>  	u16 length, msg_options;
>> @@ -383,19 +383,12 @@ int ssp_irq_msg(struct ssp_data *data)
>>  			 * but the slave should not send such ones - it is to
>>  			 * check but let's handle this
>>  			 */
>> -			buffer = kmalloc(length, GFP_KERNEL | GFP_DMA);
>> -			if (!buffer) {
>> -				ret = -ENOMEM;
>> -				goto _unlock;
>> -			}
>>  
>>  			/* got dead packet so it is always an error */
>>  			ret = spi_read(data->spi, buffer, length);
>>  			if (ret >= 0)
>>  				ret = -EPROTO;
>>  
>> -			kfree(buffer);
>> -
>>  			dev_err(SSP_DEV, "No match error %x\n",
>>  				msg_options);
>>  
>> @@ -428,22 +421,13 @@ int ssp_irq_msg(struct ssp_data *data)
>>  		mutex_unlock(&data->pending_lock);
>>  		break;
>>  	case SSP_HUB2AP_WRITE:
>> -		buffer = kzalloc(length, GFP_KERNEL | GFP_DMA);
>> -		if (!buffer)
>> -			return -ENOMEM;
>> -
>>  		ret = spi_read(data->spi, buffer, length);
>>  		if (ret < 0) {
>>  			dev_err(SSP_DEV, "spi read fail\n");
>> -			kfree(buffer);
>>  			break;
>>  		}
>>  
>> -		ret = ssp_parse_dataframe(data, buffer, length);
>> -
>> -		kfree(buffer);
>> -		break;
>> -
>> +		return ssp_parse_dataframe(data, buffer, length);
>
>This now makes this path different from the other switch legs.
>I would precede this patch with one doing direct returns from
>all the legs in here.  Then this just becomes an obvious update
>in this patch.
>
I will precede a patch to update all the legs in switch.

>>  	default:
>>  		dev_err(SSP_DEV, "unknown msg type\n");
>>  		return -EPROTO;
>