Re: [PATCH 1/2] mtd: spi-nor: sfdp: save a copy of the SFDP data

[Pratyush Yadav]

On 22/03/21 11:31PM, Michael Walle wrote:
> Am 2021-03-22 19:42, schrieb Pratyush Yadav:
> > On 22/03/21 04:32PM, Michael Walle wrote:
> > > Am 2021-03-22 15:21, schrieb Pratyush Yadav:
> > > > On 18/03/21 10:24AM, Michael Walle wrote:
> > > > > +
> > > > > +	sfdp->num_dwords = DIV_ROUND_UP(sfdp_size, sizeof(*sfdp->dwords));
> > > >
> > > > The SFDP spec says that Parameter Table Pointer should be DWORD aligned
> > > > and Parameter Table length is specified in number of DWORDs. So,
> > > > sfdp_size should always be a multiple of 4. Any SFDP table where this is
> > > > not true is an invalid one.
> > > >
> > > > Also, the spec says "Device behavior when the Read SFDP command crosses
> > > > the SFDP structure boundary is not defined".
> > > >
> > > > So I think this should be a check for alignment instead of a round-up.
> > > 
> > > Well, that woundn't help for debugging. I.e. you also want the SFDP
> > > data
> > > in cases like this. IMHO we should try hard enough to actually get a
> > > reasonable dump.
> > > 
> > > OTOH we also rely on the header and the pointers in the header. Any
> > > other ideas, but just to chicken out?
> > 
> > Honestly, I don't think reading past the SFDP boundary would be too bad.
> > It probably will just be some garbage data. But if you want to avoid
> > that, you can always round down instead of up.
> 
> Like I said, while the storage will be rounded up to a multiple of
> DWORDs, only sfdp_size is transferred. Thus it case a pointer is not
> DWORD aligned, we end up with zeros at the end.
> 
> I'll add a comment.

Right.
 
> > This way you will only
> > miss the last DWORD at most. In either case, a warning should be printed
> > so this problem can be brought to the user's attention.
> 
> I was about to add a warning/debug message. But its the wrong place.
> It should really be checked in the for loop which iterates over the
> headers before parsing them. You could check sfdp_size but then two
> unaligned param pointers might cancel each other out.
> 
> This can be a seperate patch, besides adding a warning, should there
> be any other things to do, e.g. stop parsing and error out?

Just removing the bad table from the "tables to parse" list should be 
the most conservative option.

> 
> ..
> 
> > > > > +		goto exit;
> > > > > +	}
> > > > > +
> > > > > +	err = spi_nor_read_sfdp_dma_unsafe(nor, 0, sfdp_size, sfdp->dwords);
> 
> Btw, this can be spi_nor_read_sfdp(). But I'm not sure, what this
> whole dma capable buffer should be. Is kmalloc(GFP_KERNEL)
> considered DMA safe?

I think spi_nor_read_sfdp_dma_unsafe() is meant for buffers that are 
allocated on stack. Both its current users pass in buffers on the stack. 
Also see bfa4133795e5 (mtd: spi-nor: fix DMA unsafe buffer issue in 
spi_nor_read_sfdp(), 2017-09-06).

sfdp->dwords is a DMA safe buffer, so you should directly use 
spi_nor_read_sfdp() here. All spi_nor_read_sfdp_dma_unsafe() does is 
allocate a buffer using kmalloc(GFP_KERNEL), pass it to 
spi_nor_read_sfdp() and copy the contents back.

> 
> The buffer ends in spi_nor_read_data(), which is also called from
> mtdcore:
> 
> spi_nor_read_sfdp()
>   spi_nor_read_raw()
>     spi_nor_read_data()
> 
> mtd_read()
>   mtd_read_oob()
>     mtd_read_oob_std()
>       spi_nor_read()
>         spi_nor_read_data()
> 
> Is the buffer passed from mtd_read() also DMA-safe? Doesn't the SPI
> drivers allocate DMA safe buffers if they need them?

SPI MEM at least requires the buffers to be DMA-safe. The comment for 
data.buf.in says "input buffer (must be DMA-able)".

Dunno if mtd_read() always passes a DMA-safe buffer though.

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.