linux-kernel - Re: [PATCH RFC] spi-nor: provide a range for poll

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Mon, 13 Feb 2017 07:39:21 +0000
From:   Nicholas Mc Guire <der.herr@...r.at>
To:     Boris Brezillon <boris.brezillon@...e-electrons.com>
Cc:     Mika Westerberg <mika.westerberg@...ux.intel.com>,
        Cyrille Pitchen <cyrille.pitchen@...el.com>,
        Marek Vasut <marek.vasut@...il.com>,
        David Woodhouse <dwmw2@...radead.org>,
        Brian Norris <computersforpeace@...il.com>,
        Richard Weinberger <richard@....at>,
        linux-mtd@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH RFC] spi-nor: provide a range for poll_timout

On Sun, Feb 12, 2017 at 10:59:23PM +0100, Boris Brezillon wrote:
> +Mika
> 
> On Sun, 12 Feb 2017 17:42:57 +0100
> Nicholas Mc Guire <der.herr@...r.at> wrote:
> 
> > The overall poll time here is INTEL_SPI_TIMEOUT * 1000 which is 
> > 5000 * 1000 - so 5seconds and it is coded as a tight loop here delay_us
> > to readl_poll_timeout() is set to 0. As this is never called in an atomic
> > context sleeping should be no issue and there is no reasons for the
> > tight-loop here.
> 
> Hm, let's wait for Mika's feedback on this one. BTW, can you please Cc
> him on you other spi-nor/intel patches and prefix your patches with the
> driver name ('mtd: spi-nor: intel: ') so that we know where the changes
> are made (without this prefix it looks like you're touching core files)?
>

will do - just saw that his email shows up in git plame which I normally also include along with what scripts/get_maintainer.pl -f shows - sorry for the ommision.

thx!
hofrat
 
> 
> > 
> > Signed-off-by: Nicholas Mc Guire <der.herr@...r.at>
> > ---
> > 
> > Problem located by experimental coccinelle script:
> > ./drivers/mtd/spi-nor/intel-spi.c:265:8-26: WARNING: usleep_range min=0 for delay INTEL_SPI_TIMEOUT * 1000
> > ./drivers/mtd/spi-nor/intel-spi.c:274:8-26: WARNING: usleep_range min=0 for delay INTEL_SPI_TIMEOUT * 1000
> > 
> > The rational for setting the delay_us here to 40 is that readx_poll_timeout()
> > will take delay_us >> 2 + 1 as min value and that should be at least 10us (see
> > Documentation/timers/timers-howto.txt). Ideally the delay would be made
> > even larger to keep the load on the hrtimer subsystem low as these delays
> > here do not seem to be critical. Someone that knows the details of this device
> > would need to check if a larger delay would be ok here.
> > 
> > Patch was compile tested with: multi_v7_defconfig (implies CONFIG_MTD_SPI_NOR=y)
> > one coccicheck finding reported and one spars finding (in separate patches)
> > 
> > Patch is against 4.10-rc6 (localversion-next is next-20170210)
> > 
> >  drivers/mtd/spi-nor/intel-spi.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/mtd/spi-nor/intel-spi.c b/drivers/mtd/spi-nor/intel-spi.c
> > index a10f602..371bcf9 100644
> > --- a/drivers/mtd/spi-nor/intel-spi.c
> > +++ b/drivers/mtd/spi-nor/intel-spi.c
> > @@ -263,7 +263,7 @@ static int intel_spi_wait_hw_busy(struct intel_spi *ispi)
> >  	u32 val;
> >  
> >  	return readl_poll_timeout(ispi->base + HSFSTS_CTL, val,
> > -				  !(val & HSFSTS_CTL_SCIP), 0,
> > +				  !(val & HSFSTS_CTL_SCIP), 40,
> >  				  INTEL_SPI_TIMEOUT * 1000);
> >  }
> >  
> > @@ -272,7 +272,7 @@ static int intel_spi_wait_sw_busy(struct intel_spi *ispi)
> >  	u32 val;
> >  
> >  	return readl_poll_timeout(ispi->sregs + SSFSTS_CTL, val,
> > -				  !(val & SSFSTS_CTL_SCIP), 0,
> > +				  !(val & SSFSTS_CTL_SCIP), 40,
> >  				  INTEL_SPI_TIMEOUT * 1000);
> >  }
> >  
>