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.
