From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 629DAC433F5 for ; Sun, 13 Mar 2022 21:02:24 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S235540AbiCMVDV (ORCPT ); Sun, 13 Mar 2022 17:03:21 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:36264 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233003AbiCMVDU (ORCPT ); Sun, 13 Mar 2022 17:03:20 -0400 Received: from smtp.domeneshop.no (smtp.domeneshop.no [IPv6:2a01:5b40:0:3005::1]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 7C1F179C4E for ; Sun, 13 Mar 2022 14:02:11 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=tronnes.org ; s=ds202112; h=Content-Transfer-Encoding:Content-Type:In-Reply-To:From: References:Cc:To:Subject:MIME-Version:Date:Message-ID:Sender:Reply-To: Content-ID:Content-Description:Resent-Date:Resent-From:Resent-Sender: Resent-To:Resent-Cc:Resent-Message-ID:List-Id:List-Help:List-Unsubscribe: List-Subscribe:List-Post:List-Owner:List-Archive; bh=36t4lSZYEjksqRQj48h4XUvzBGkfiIIMwPuRKua96P0=; b=rut1essUtcLuqpbMOJ2ptbk3VB go+2GYHG5SA3M4ZeQid7WkfXjOKSDIBslHzPcmNdQZ00Akt9YIHU9lPjsZH7aHKLJpZa8TzrUTbYi liBCrJw70INaJNsRdaIeSf+RLOzmJ5OjPeNjXB9dYuBlQaxHKyx5VbPr0SDmIqdTK9/X91EdP+ldJ GGis0Wrh9JygNyXI9Pmh4vECfQJHLqWjArgitgRhmLt7bVgR6pUYR7a2uoChHSl5Y7PwktVwC9qOg 8DDp3BEd727hEhxd8sppJ1F4mPmjlbBUT7HUdJyVxVJKfxrCVU5BfDEWb5k7HpsjmBB5EbGG2Wvpp oKZe9NbA==; Received: from 211.81-166-168.customer.lyse.net ([81.166.168.211]:51607 helo=[192.168.10.61]) by smtp.domeneshop.no with esmtpsa (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1nTVLs-0004dx-9H; Sun, 13 Mar 2022 22:02:08 +0100 Message-ID: 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 , 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?= In-Reply-To: <52a1c253-0206-182e-5e41-03f94e7e7275@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >>> >>> 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 >>> --- >>>   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; >>>   } >>>   >