Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <e9be8d81-786f-4b13-468e-d910a16e2e61@tronnes.org>
Date:   Sun, 13 Mar 2022 22:02:05 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.6.2
Subject: Re: [PATCH] drm/repaper: combine allocs in repaper_spi_transfer()
To:     Tom Rix <trix@redhat.com>, airlied@linux.ie, daniel@ffwll.ch
Cc:     dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org
References: <20220313141008.1503638-1-trix@redhat.com>
 <569b42fb-5d85-645a-2947-77216c44696a@tronnes.org>
 <52a1c253-0206-182e-5e41-03f94e7e7275@redhat.com>
From:   =?UTF-8?Q?Noralf_Tr=c3=b8nnes?= <noralf@tronnes.org>
In-Reply-To: <52a1c253-0206-182e-5e41-03f94e7e7275@redhat.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk


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;
>>>   }
>>>   
>