linux-kernel - Re: [PATCH v1] spi: spi-mtk-nor: Optimize timeout for dma read

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <617c0b563a2602668fde7d96e1bc98648870d30c.camel@mediatek.com>
Date:   Fri, 4 Nov 2022 07:53:21 +0000
From:   Bayi Cheng (程八意) 
        <bayi.cheng@...iatek.com>
To:     "angelogioacchino.delregno@...labora.com" 
        <angelogioacchino.delregno@...labora.com>,
        "David.Laight@...LAB.COM" <David.Laight@...LAB.COM>,
        "broonie@...nel.org" <broonie@...nel.org>
CC:     "linux-spi@...r.kernel.org" <linux-spi@...r.kernel.org>,
        "matthias.bgg@...il.com" <matthias.bgg@...il.com>,
        "linux-mediatek@...ts.infradead.org" 
        <linux-mediatek@...ts.infradead.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "gch981213@...il.com" <gch981213@...il.com>,
        Project_Global_Chrome_Upstream_Group 
        <Project_Global_Chrome_Upstream_Group@...iatek.com>
Subject: Re: [PATCH v1] spi: spi-mtk-nor: Optimize timeout for dma read

On Thu, 2022-11-03 at 22:35 +0000, David Laight wrote:
> From: AngeloGioacchino Del Regno
> > Sent: 03 November 2022 09:44
> > 
> > Il 03/11/22 06:28, Bayi Cheng ha scritto:
> > > From: bayi cheng <bayi.cheng@...iatek.com>
> > > 
> > > The timeout value of the current dma read is unreasonable. For
> > > example,
> > > If the spi flash clock is 26Mhz, It will takes about 1.3ms to
> > > read a
> > > 4KB data in spi mode. But the actual measurement exceeds 50s when
> > > a
> > > dma read timeout is encountered.
> > > 
> > > In order to be more accurately, It is necessary to use
> > > msecs_to_jiffies,
> > > After modification, the measured timeout value is about 130ms.
> > > 
> > > Signed-off-by: bayi cheng <bayi.cheng@...iatek.com>
> > > ---
> > >   drivers/spi/spi-mtk-nor.c | 7 ++++---
> > >   1 file changed, 4 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/spi/spi-mtk-nor.c b/drivers/spi/spi-mtk-
> > > nor.c
> > > index d167699a1a96..3d989db80ee9 100644
> > > --- a/drivers/spi/spi-mtk-nor.c
> > > +++ b/drivers/spi/spi-mtk-nor.c
> > > @@ -354,7 +354,7 @@ static int mtk_nor_dma_exec(struct mtk_nor
> > > *sp, u32 from, unsigned int length,
> > >   			    dma_addr_t dma_addr)
> > >   {
> > >   	int ret = 0;
> > > -	ulong delay;
> > > +	ulong delay, timeout;
> > >   	u32 reg;
> > > 
> > >   	writel(from, sp->base + MTK_NOR_REG_DMA_FADR);
> > > @@ -376,15 +376,16 @@ static int mtk_nor_dma_exec(struct mtk_nor
> > > *sp, u32 from, unsigned int length,
> > >   	mtk_nor_rmw(sp, MTK_NOR_REG_DMA_CTL, MTK_NOR_DMA_START,
> > > 0);
> > > 
> > >   	delay = CLK_TO_US(sp, (length + 5) * BITS_PER_BYTE);
> > > +	timeout = (delay + 1) * 100;
> > > 
> > >   	if (sp->has_irq) {
> > >   		if (!wait_for_completion_timeout(&sp->op_done,
> > > -						 (delay + 1) * 100))
> > > +		    msecs_to_jiffies(max_t(size_t, timeout / 1000,
> > > 10))))
> > 
> > You're giving a `size_t` variable to msecs_to_jiffies(), but
> > checking `jiffies.h`,
> > this function takes a `const unsigned int` param.
> > Please change the type to match that.
> 
> The type shouldn't matter at all.
> What matters is the domain of the value.
> 
> Quite why you need to use max_t(size_t, ...) is another matter.
> timeout is ulong so max(timeout/1000, 10ul) should be fine.
> 
> But is ulong even right?
> The domain of the value is almost certainly the same on 32bit and
> 64bit.
> So you almost certainly need u32 or u64.
> 
> 	David
> 
Hi David & Angelo

Thank you for your comments!
To sum up, I think the next version will make the following two
changes:
1, The timeout value will not exceed u32, so the type of timeout will
be changed from ulong to u32.
2, Change msecs_to_jiffies(max_t(size_t, timeout / 1000, 10)) to be
msecs_to_jiffies(max(timeout/1000, 10ul)).

If you think these changes are not enough, please let me know, Thanks！

Best Regards,
Bayi

> > 
> > Aside from that, your `timeout` variable contains a timeout in
> > microseconds and
> > this means that actually using msecs_to_jiffies() is suboptimal
> > here.
> > 
> > Please use usecs_to_jiffies() instead.
> > 
> > Regards,
> > Angelo
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes,
> MK1 1PT, UK
> Registration No: 1397386 (Wales)