| 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
| ||
|
Message-ID: <e546de4ea3b1e29da6f9cdf56c8bf1582b70944e.camel@mediatek.com>
Date: Sat, 12 Nov 2022 06:06:49 +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 Fri, 2022-11-11 at 10:16 +0100, AngeloGioacchino Del Regno wrote:
> Il 11/11/22 05:16, Bayi Cheng (程八意) ha scritto:
> > On Fri, 2022-11-04 at 07:53 +0000, Bayi Cheng (程八意) wrote:
> > > 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, 10U)).
> > >
> > > If you think these changes are not enough, please let me know,
> > > Thanks！
> > >
> > > Best Regards,
> > > Bayi
> > >
> >
> > Hi Angelo, Hi David,
> >
> > Just a gentle ping on this.
> > Could you please review this patch and give us some suggestion?
> >
> > PS: With your permission, I will make the following changes in the
> > next
> > version:
> >
> > Change in v2:
> > -Change the type of "timeout" from ulong to u32.
> > -Replace max_t with max.
> >
>
> I still recommend to use usecs_to_jiffies() when appropriate, instead
> of
> converting usecs to msecs and using msecs_to_jiffies().
>
> As for the rest in your list: yes, please.
>
> Regards,
> Angelo
>
Hi Angelo,
Thanks for your comments!
I will change it to be usecs_to_jiffies(max(timeout, 10000U)) in the
next patch.
BRs,
Bayi Cheng
> >
> > Thanks.
> >
> > BRs,
> > Bayi Cheng
> >
> > > > >
> > > > > 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)
>
>
Powered by blists - more mailing lists