Re: [DEBUG] mtd: spi-nor: dump DWORDs of the Basic Flash Parameter Table

[Boris Brezillon]

On Thu, 14 Sep 2017 18:44:31 +0200
Cyrille Pitchen <cyrille.pitchen@xxxxxxxxxx> wrote:

> Le 12/09/2017 Ã 15:12, Boris Brezillon a Ãcrit :
> > Hi Geert,
> > 
> > On Mon, 11 Sep 2017 10:58:36 +0200
> > Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
> >   
> >> Hi Cyrille,
> >>
> >> On Thu, Sep 7, 2017 at 9:28 PM, Cyrille Pitchen
> >> <cyrille.pitchen@xxxxxxxxxx> wrote:  
> >>>> Can you apply this patch on your tree then report me what was printed, please?
> >>>> I have an idea of the root cause of your issue then a potential work-around
> >>>> but I first need to validate my assumption to confirm that the work-around
> >>>> would actually work.    
> >>
> >> +m25p80 spi0.0: DWORD1 = 0xffffffff
> >> +m25p80 spi0.0: DWORD2 = 0xffffffff
> >> +m25p80 spi0.0: DWORD3 = 0xffffffff
> >> +m25p80 spi0.0: DWORD4 = 0xffffffff
> >> +m25p80 spi0.0: DWORD5 = 0xffffffff
> >> +m25p80 spi0.0: DWORD6 = 0xffffffff
> >> +m25p80 spi0.0: DWORD7 = 0xffffffff
> >> +m25p80 spi0.0: DWORD8 = 0xffffffff
> >> +m25p80 spi0.0: DWORD9 = 0xffffffff
> >> +m25p80 spi0.0: DWORD10 = 0x00000000
> >> +m25p80 spi0.0: DWORD11 = 0x00000000
> >> +m25p80 spi0.0: DWORD12 = 0x00000000
> >> +m25p80 spi0.0: DWORD13 = 0x00000000
> >> +m25p80 spi0.0: DWORD14 = 0x00000000
> >> +m25p80 spi0.0: DWORD15 = 0x00000000
> >> +m25p80 spi0.0: DWORD16 = 0x00000000
> >> +m25p80 spi0.0: BFPT version 1.0 (length = 9)
> >>  
> >>> If you could also dump the value of the 'addr' argument of
> >>> spi_nor_read_sfdp_dma_unsafe() just before the for () loop below in the
> >>> very same function. Actually, I suspect the SFDP tables of your SPI NOR    
> >>
> >> +m25p80 spi0.0: addr = 0x448
> >>  
> >>> memory sample to have been programmed with invalid values, neither
> >>> compliant with the JEDEC JESD216 specification nor with the Cypress
> >>> datasheet for this memory part.    
> >>
> >> Sounds plausible.
> >> I get the same values when disabling DMA, so it's not due to bad DMA handling.
> >> All Renesas boards I have local or remote access to have spansion,s25fl512s.  
> > 
> > Can you try with the following patch?
> > 
> > Thanks,
> > 
> > Boris
> >   
> > --->8---  
> > From 000ff63fdb149d87d755483f5edc0aba010da6b4 Mon Sep 17 00:00:00 2001
> > From: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxxxxxxx>
> > Date: Tue, 12 Sep 2017 15:10:35 +0200
> > Subject: [PATCH] mtd: spi-nor: Check consistency of the memory size extracted
> >  from the SFDP
> > 
> > One field of the flash parameter table contains information about the
> > flash device size.
> > Most of the time the data extracted from this field is valid, but
> > sometimes the BFPT section of the SFDP table is corrupted or invalid and
> > this field is set to 0xffffffff, thus resulting in an integer overflow
> > when setting params->size.
> > 
> > Since NOR devices are anayway always smaller than 2^64 bytes, we can
> > easily stop the BFPT parsing if the size reported in this table is
> > invalid.
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxxxxxxx>  
> Acked-by: Cyrille Pitchen <cyrille.pitchen@xxxxxxxxxxx>

Applied after fixing the things you pointed below.

> 
> with few comments below:
> > ---
> >  drivers/mtd/spi-nor/spi-nor.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> > index cf1d4a15e10a..665ccae1d090 100644
> > --- a/drivers/mtd/spi-nor/spi-nor.c
> > +++ b/drivers/mtd/spi-nor/spi-nor.c
> > @@ -2127,6 +2127,15 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
> >  	params->size = bfpt.dwords[BFPT_DWORD(2)];
> >  	if (params->size & BIT(31)) {
> >  		params->size &= ~BIT(31);
> > +
> > +		/*
> > +		 * Prevent overflows on params->size. Anyway, a NOR of 1^64  
> typo: should be 2^64
> > +		 * bytes is unlikely to exist so this error probably means  
> 
> Here the size is still expressed in bits, not yet in byte, the
> conversion is done right after this chunk.
> > +		 * the BFPT we are reading is corrupted/wrong.
> > +		 */
> > +		if (params->size > 63)
> > +			return -EINVAL;
> > +
> >  		params->size = 1ULL << params->size;
> >  	} else {
> >  		params->size++;
> >   
> 
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/