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.
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?
..
> > + 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?
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?
-michael
