[PATCH] drm/repaper: combine allocs in repaper_spi_transfer()

[PATCH] drm/repaper: combine allocs in repaper_spi_transfer()

[trix]

From: Tom Rix <trix@redhat.com>

repaper_spi_transfer() allocates a single byte
for the spi header and then another buffer for
the payload.  Combine the allocs into a single
buffer with offsets.  To simplify the offsets
put the header after the payload.

Signed-off-by: Tom Rix <trix@redhat.com>
---
 drivers/gpu/drm/tiny/repaper.c | 40 ++++++++++------------------------
 1 file changed, 12 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/tiny/repaper.c b/drivers/gpu/drm/tiny/repaper.c
index 37b6bb90e46e1..22a6732f35a01 100644
--- a/drivers/gpu/drm/tiny/repaper.c
+++ b/drivers/gpu/drm/tiny/repaper.c
@@ -100,50 +100,34 @@ static inline struct repaper_epd *drm_to_epd(struct drm_device *drm)
 static int repaper_spi_transfer(struct spi_device *spi, u8 header,
 				const void *tx, void *rx, size_t len)
 {
-	void *txbuf = NULL, *rxbuf = NULL;
 	struct spi_transfer tr[2] = {};
-	u8 *headerbuf;
+	u8 *buf;
 	int ret;
 
-	headerbuf = kmalloc(1, GFP_KERNEL);
-	if (!headerbuf)
+	buf = kmalloc(1 + len, GFP_KERNEL);
+	if (!buf)
 		return -ENOMEM;
 
-	headerbuf[0] = header;
-	tr[0].tx_buf = headerbuf;
+	buf[len] = header;
+	tr[0].tx_buf = &buf[len];
 	tr[0].len = 1;
 
-	/* Stack allocated tx? */
-	if (tx && len <= 32) {
-		txbuf = kmemdup(tx, len, GFP_KERNEL);
-		if (!txbuf) {
-			ret = -ENOMEM;
-			goto out_free;
-		}
+	if (tx) {
+		memcpy(buf, tx, len);
+		tr[1].tx_buf = buf;
 	}
 
-	if (rx) {
-		rxbuf = kmalloc(len, GFP_KERNEL);
-		if (!rxbuf) {
-			ret = -ENOMEM;
-			goto out_free;
-		}
-	}
+	if (rx)
+		tr[1].rx_buf = buf;
 
-	tr[1].tx_buf = txbuf ? txbuf : tx;
-	tr[1].rx_buf = rxbuf;
 	tr[1].len = len;
 
 	ndelay(80);
 	ret = spi_sync_transfer(spi, tr, 2);
 	if (rx && !ret)
-		memcpy(rx, rxbuf, len);
-
-out_free:
-	kfree(headerbuf);
-	kfree(txbuf);
-	kfree(rxbuf);
+		memcpy(rx, buf, len);
 
+	kfree(buf);
 	return ret;
 }
 
-- 
2.26.3

Re: [PATCH] drm/repaper: combine allocs in repaper_spi_transfer()

[Noralf Trønnes]

Den 13.03.2022 15.10, skrev trix@redhat.com:
> From: Tom Rix <trix@redhat.com>
> 
> repaper_spi_transfer() allocates a single byte
> for the spi header and then another buffer for
> the payload.  Combine the allocs into a single
> buffer with offsets.  To simplify the offsets
> put the header after the payload.
> 
> Signed-off-by: Tom Rix <trix@redhat.com>
> ---
>  drivers/gpu/drm/tiny/repaper.c | 40 ++++++++++------------------------
>  1 file changed, 12 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tiny/repaper.c b/drivers/gpu/drm/tiny/repaper.c
> index 37b6bb90e46e1..22a6732f35a01 100644
> --- a/drivers/gpu/drm/tiny/repaper.c
> +++ b/drivers/gpu/drm/tiny/repaper.c
> @@ -100,50 +100,34 @@ static inline struct repaper_epd *drm_to_epd(struct drm_device *drm)
>  static int repaper_spi_transfer(struct spi_device *spi, u8 header,
>  				const void *tx, void *rx, size_t len)
>  {
> -	void *txbuf = NULL, *rxbuf = NULL;
>  	struct spi_transfer tr[2] = {};
> -	u8 *headerbuf;
> +	u8 *buf;
>  	int ret;
>  
> -	headerbuf = kmalloc(1, GFP_KERNEL);
> -	if (!headerbuf)
> +	buf = kmalloc(1 + len, GFP_KERNEL);
> +	if (!buf)
>  		return -ENOMEM;
>  
> -	headerbuf[0] = header;
> -	tr[0].tx_buf = headerbuf;
> +	buf[len] = header;
> +	tr[0].tx_buf = &buf[len];

I don't think this will work since the buffer is used directly for DMA
on some platforms[1] so the buffers need to be at the correct alignment
for that to work. For this reason I think it's better to leave this
as-is since we know the kmalloc buffers will always be useable for DMA
and the code is also easy to read and understand instead of calculating
offsets.

[1] tinydrm/mipi-dbi: Use dma-safe buffers for all SPI transfers
(a89bfc5d9a07)

Noralf.

>  	tr[0].len = 1;
>  
> -	/* Stack allocated tx? */
> -	if (tx && len <= 32) {
> -		txbuf = kmemdup(tx, len, GFP_KERNEL);
> -		if (!txbuf) {
> -			ret = -ENOMEM;
> -			goto out_free;
> -		}
> +	if (tx) {
> +		memcpy(buf, tx, len);
> +		tr[1].tx_buf = buf;
>  	}
>  
> -	if (rx) {
> -		rxbuf = kmalloc(len, GFP_KERNEL);
> -		if (!rxbuf) {
> -			ret = -ENOMEM;
> -			goto out_free;
> -		}
> -	}
> +	if (rx)
> +		tr[1].rx_buf = buf;
>  
> -	tr[1].tx_buf = txbuf ? txbuf : tx;
> -	tr[1].rx_buf = rxbuf;
>  	tr[1].len = len;
>  
>  	ndelay(80);
>  	ret = spi_sync_transfer(spi, tr, 2);
>  	if (rx && !ret)
> -		memcpy(rx, rxbuf, len);
> -
> -out_free:
> -	kfree(headerbuf);
> -	kfree(txbuf);
> -	kfree(rxbuf);
> +		memcpy(rx, buf, len);
>  
> +	kfree(buf);
>  	return ret;
>  }
>  

Re: [PATCH] drm/repaper: combine allocs in repaper_spi_transfer()

[Tom Rix]

On 3/13/22 8:41 AM, Noralf Trønnes wrote:
>
> Den 13.03.2022 15.10, skrev trix@redhat.com:
>> From: Tom Rix <trix@redhat.com>
>>
>> repaper_spi_transfer() allocates a single byte
>> for the spi header and then another buffer for
>> the payload.  Combine the allocs into a single
>> buffer with offsets.  To simplify the offsets
>> put the header after the payload.
>>
>> Signed-off-by: Tom Rix <trix@redhat.com>
>> ---
>>   drivers/gpu/drm/tiny/repaper.c | 40 ++++++++++------------------------
>>   1 file changed, 12 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/tiny/repaper.c b/drivers/gpu/drm/tiny/repaper.c
>> index 37b6bb90e46e1..22a6732f35a01 100644
>> --- a/drivers/gpu/drm/tiny/repaper.c
>> +++ b/drivers/gpu/drm/tiny/repaper.c
>> @@ -100,50 +100,34 @@ static inline struct repaper_epd *drm_to_epd(struct drm_device *drm)
>>   static int repaper_spi_transfer(struct spi_device *spi, u8 header,
>>   				const void *tx, void *rx, size_t len)
>>   {
>> -	void *txbuf = NULL, *rxbuf = NULL;
>>   	struct spi_transfer tr[2] = {};
>> -	u8 *headerbuf;
>> +	u8 *buf;
>>   	int ret;
>>   
>> -	headerbuf = kmalloc(1, GFP_KERNEL);
>> -	if (!headerbuf)
>> +	buf = kmalloc(1 + len, GFP_KERNEL);
>> +	if (!buf)
>>   		return -ENOMEM;
>>   
>> -	headerbuf[0] = header;
>> -	tr[0].tx_buf = headerbuf;
>> +	buf[len] = header;
>> +	tr[0].tx_buf = &buf[len];
> I don't think this will work since the buffer is used directly for DMA
> on some platforms[1] so the buffers need to be at the correct alignment
> for that to work. For this reason I think it's better to leave this
> as-is since we know the kmalloc buffers will always be useable for DMA
> and the code is also easy to read and understand instead of calculating
> offsets.
>
> [1] tinydrm/mipi-dbi: Use dma-safe buffers for all SPI transfers
> (a89bfc5d9a07)
>
> Noralf.
>
>>   	tr[0].len = 1;
>>   
>> -	/* Stack allocated tx? */
>> -	if (tx && len <= 32) {

How about a change to remove this ?

It seems like you are getting lucky.

reduce the to single txrx_buf

Tom

>> -		txbuf = kmemdup(tx, len, GFP_KERNEL);
>> -		if (!txbuf) {
>> -			ret = -ENOMEM;
>> -			goto out_free;
>> -		}
>> +	if (tx) {
>> +		memcpy(buf, tx, len);
>> +		tr[1].tx_buf = buf;
>>   	}
>>   
>> -	if (rx) {
>> -		rxbuf = kmalloc(len, GFP_KERNEL);
>> -		if (!rxbuf) {
>> -			ret = -ENOMEM;
>> -			goto out_free;
>> -		}
>> -	}
>> +	if (rx)
>> +		tr[1].rx_buf = buf;
>>   
>> -	tr[1].tx_buf = txbuf ? txbuf : tx;
>> -	tr[1].rx_buf = rxbuf;
>>   	tr[1].len = len;
>>   
>>   	ndelay(80);
>>   	ret = spi_sync_transfer(spi, tr, 2);
>>   	if (rx && !ret)
>> -		memcpy(rx, rxbuf, len);
>> -
>> -out_free:
>> -	kfree(headerbuf);
>> -	kfree(txbuf);
>> -	kfree(rxbuf);
>> +		memcpy(rx, buf, len);
>>   
>> +	kfree(buf);
>>   	return ret;
>>   }
>>   

Re: [PATCH] drm/repaper: combine allocs in repaper_spi_transfer()

[Noralf Trønnes]

Den 13.03.2022 17.07, skrev Tom Rix:
> 
> On 3/13/22 8:41 AM, Noralf Trønnes wrote:
>>
>> Den 13.03.2022 15.10, skrev trix@redhat.com:
>>> From: Tom Rix <trix@redhat.com>
>>>
>>> repaper_spi_transfer() allocates a single byte
>>> for the spi header and then another buffer for
>>> the payload.  Combine the allocs into a single
>>> buffer with offsets.  To simplify the offsets
>>> put the header after the payload.
>>>
>>> Signed-off-by: Tom Rix <trix@redhat.com>
>>> ---
>>>   drivers/gpu/drm/tiny/repaper.c | 40 ++++++++++------------------------
>>>   1 file changed, 12 insertions(+), 28 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/tiny/repaper.c
>>> b/drivers/gpu/drm/tiny/repaper.c
>>> index 37b6bb90e46e1..22a6732f35a01 100644
>>> --- a/drivers/gpu/drm/tiny/repaper.c
>>> +++ b/drivers/gpu/drm/tiny/repaper.c
>>> @@ -100,50 +100,34 @@ static inline struct repaper_epd
>>> *drm_to_epd(struct drm_device *drm)
>>>   static int repaper_spi_transfer(struct spi_device *spi, u8 header,
>>>                   const void *tx, void *rx, size_t len)
>>>   {
>>> -    void *txbuf = NULL, *rxbuf = NULL;
>>>       struct spi_transfer tr[2] = {};
>>> -    u8 *headerbuf;
>>> +    u8 *buf;
>>>       int ret;
>>>   -    headerbuf = kmalloc(1, GFP_KERNEL);
>>> -    if (!headerbuf)
>>> +    buf = kmalloc(1 + len, GFP_KERNEL);
>>> +    if (!buf)
>>>           return -ENOMEM;
>>>   -    headerbuf[0] = header;
>>> -    tr[0].tx_buf = headerbuf;
>>> +    buf[len] = header;
>>> +    tr[0].tx_buf = &buf[len];
>> I don't think this will work since the buffer is used directly for DMA
>> on some platforms[1] so the buffers need to be at the correct alignment
>> for that to work. For this reason I think it's better to leave this
>> as-is since we know the kmalloc buffers will always be useable for DMA
>> and the code is also easy to read and understand instead of calculating
>> offsets.
>>
>> [1] tinydrm/mipi-dbi: Use dma-safe buffers for all SPI transfers
>> (a89bfc5d9a07)
>>
>> Noralf.
>>
>>>       tr[0].len = 1;
>>>   -    /* Stack allocated tx? */
>>> -    if (tx && len <= 32) {
> 
> How about a change to remove this ?
> 
> It seems like you are getting lucky.
> 

Lucky how?

If tx len is 32 bytes or less it's config data which lives on the stack
so it's put in a kmalloc buffer, if it's more it's pixel data which
already comes in a kmalloc buffer.

Ideally we should have had a spi_sync_transfer_safe() that would check
if the buffer comes from the stack and allocates buffers for us so the
driver didn't have to do this. Alternatively the spi core could have
done this, maybe spi_map_buf() could have done it since it already has
special handling for vmalloc buffers.

> reduce the to single txrx_buf
> 

I see that repaper_spi_transfer() is written so tx and rx can be both
set, but the driver never does tx and rx in the same transfer so it's
possible to use one txrx buffer. I'm not entirely convinced that it will
be more readable.

Noralf.

> Tom
> 
>>> -        txbuf = kmemdup(tx, len, GFP_KERNEL);
>>> -        if (!txbuf) {
>>> -            ret = -ENOMEM;
>>> -            goto out_free;
>>> -        }
>>> +    if (tx) {
>>> +        memcpy(buf, tx, len);
>>> +        tr[1].tx_buf = buf;
>>>       }
>>>   -    if (rx) {
>>> -        rxbuf = kmalloc(len, GFP_KERNEL);
>>> -        if (!rxbuf) {
>>> -            ret = -ENOMEM;
>>> -            goto out_free;
>>> -        }
>>> -    }
>>> +    if (rx)
>>> +        tr[1].rx_buf = buf;
>>>   -    tr[1].tx_buf = txbuf ? txbuf : tx;
>>> -    tr[1].rx_buf = rxbuf;
>>>       tr[1].len = len;
>>>         ndelay(80);
>>>       ret = spi_sync_transfer(spi, tr, 2);
>>>       if (rx && !ret)
>>> -        memcpy(rx, rxbuf, len);
>>> -
>>> -out_free:
>>> -    kfree(headerbuf);
>>> -    kfree(txbuf);
>>> -    kfree(rxbuf);
>>> +        memcpy(rx, buf, len);
>>>   +    kfree(buf);
>>>       return ret;
>>>   }
>>>   
>