| Message ID | 20210318092406.5340-2-michael@walle.cc |
|---|---|
| State | Superseded |
| Delegated to: | Ambarus Tudor |
| Headers | show |
| Series | mtd: spi-nor: support dumping sfdp tables | expand |
Hi Michael, On 18/03/21 10:24AM, Michael Walle wrote: > Due to possible mode switching to 8D-8D-8D, it might not be possible to > read the SFDP after the initial probe. To be able to dump the SFDP via > sysfs afterwards, make a complete copy of it. > > Signed-off-by: Michael Walle <michael@walle.cc> > --- > drivers/mtd/spi-nor/core.h | 10 ++++++++ > drivers/mtd/spi-nor/sfdp.c | 49 +++++++++++++++++++++++++++++++++++++ > include/linux/mtd/spi-nor.h | 3 +++ > 3 files changed, 62 insertions(+) > > diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h > index 4a3f7f150b5d..668f22011b1d 100644 > --- a/drivers/mtd/spi-nor/core.h > +++ b/drivers/mtd/spi-nor/core.h > @@ -407,6 +407,16 @@ struct spi_nor_manufacturer { > const struct spi_nor_fixups *fixups; > }; > > +/** > + * struct sfdp - SFDP data > + * @num_dwords: number of entries in the dwords array > + * @dwords: array of double words of the SFDP data > + */ > +struct sfdp { > + size_t num_dwords; > + u32 *dwords; > +}; > + > /* Manufacturer drivers. */ > extern const struct spi_nor_manufacturer spi_nor_atmel; > extern const struct spi_nor_manufacturer spi_nor_catalyst; > diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c > index 25142ec4737b..2b6c96e02532 100644 > --- a/drivers/mtd/spi-nor/sfdp.c > +++ b/drivers/mtd/spi-nor/sfdp.c > @@ -16,6 +16,7 @@ > (((p)->parameter_table_pointer[2] << 16) | \ > ((p)->parameter_table_pointer[1] << 8) | \ > ((p)->parameter_table_pointer[0] << 0)) > +#define SFDP_PARAM_HEADER_PARAM_LEN(p) ((p)->length * 4) > > #define SFDP_BFPT_ID 0xff00 /* Basic Flash Parameter Table */ > #define SFDP_SECTOR_MAP_ID 0xff81 /* Sector Map Table */ > @@ -1263,6 +1264,8 @@ int spi_nor_parse_sfdp(struct spi_nor *nor, > struct sfdp_parameter_header *param_headers = NULL; > struct sfdp_header header; > struct device *dev = nor->dev; > + struct sfdp *sfdp; > + size_t sfdp_size; > size_t psize; > int i, err; > > @@ -1285,6 +1288,9 @@ int spi_nor_parse_sfdp(struct spi_nor *nor, > bfpt_header->major != SFDP_JESD216_MAJOR) > return -EINVAL; > > + sfdp_size = SFDP_PARAM_HEADER_PTP(bfpt_header) + > + SFDP_PARAM_HEADER_PARAM_LEN(bfpt_header); > + > /* > * Allocate memory then read all parameter headers with a single > * Read SFDP command. These parameter headers will actually be parsed > @@ -1311,6 +1317,49 @@ int spi_nor_parse_sfdp(struct spi_nor *nor, > } > } > > + /* > + * Cache the complete SFDP data. It is not (easily) possible to fetch > + * SFDP after probe time and we need it for the sysfs access. > + */ > + for (i = 0; i < header.nph; i++) { > + param_header = ¶m_headers[i]; > + sfdp_size = max_t(size_t, sfdp_size, > + SFDP_PARAM_HEADER_PTP(param_header) + > + SFDP_PARAM_HEADER_PARAM_LEN(param_header)); *sigh* If BFPT header was not made a part of the main SFDP header, two "sfdp_size = ..." would not be needed, and we could do it all in one place. You can do that refactor if you're feeling like it, but I won't insist on it. > + } > + > + /* > + * Limit the total size to a reasonable value to avoid allocating too > + * much memory just of because the flash returned some insane values. > + */ > + if (sfdp_size > PAGE_SIZE) { > + dev_dbg(dev, "SFDP data (%zu) too big, truncating\n", > + sfdp_size); > + sfdp_size = PAGE_SIZE; Ok. 4K should be large enough for any reasonable SFDP table. > + } > + > + sfdp = devm_kzalloc(dev, sizeof(*sfdp), GFP_KERNEL); > + if (!sfdp) { > + err = -ENOMEM; > + goto exit; > + } I assume you made nor->sfdp a pointer and not an embedded struct so it can easily indicate if SFDP was found or not, correct? > + > + 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. > + sfdp->dwords = devm_kcalloc(dev, sfdp->num_dwords, > + sizeof(*sfdp->dwords), GFP_KERNEL); > + if (!sfdp->dwords) { > + err = -ENOMEM; Free sfdp here since it won't be used any longer. > + goto exit; > + } > + > + err = spi_nor_read_sfdp_dma_unsafe(nor, 0, sfdp_size, sfdp->dwords); > + if (err < 0) { > + dev_dbg(dev, "failed to read SFDP data\n"); > + goto exit; Free sfdp and sfdp->dwords here since they won't be used any longer. > + } > + > + nor->sfdp = sfdp; > + > /* > * Check other parameter headers to get the latest revision of > * the basic flash parameter table. > diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h > index a0d572855444..55c550208949 100644 > --- a/include/linux/mtd/spi-nor.h > +++ b/include/linux/mtd/spi-nor.h > @@ -351,6 +351,7 @@ enum spi_nor_cmd_ext { > struct flash_info; > struct spi_nor_manufacturer; > struct spi_nor_flash_parameter; > +struct sfdp; nor->sfdp is a pointer. This should not be needed. > > /** > * struct spi_nor - Structure for defining the SPI NOR layer > @@ -375,6 +376,7 @@ struct spi_nor_flash_parameter; > * @read_proto: the SPI protocol for read operations > * @write_proto: the SPI protocol for write operations > * @reg_proto: the SPI protocol for read_reg/write_reg/erase operations > + * @sfdp: the SFDP data of the flash > * @controller_ops: SPI NOR controller driver specific operations. > * @params: [FLASH-SPECIFIC] SPI NOR flash parameters and settings. > * The structure includes legacy flash parameters and > @@ -404,6 +406,7 @@ struct spi_nor { > bool sst_write_second; > u32 flags; > enum spi_nor_cmd_ext cmd_ext_type; > + struct sfdp *sfdp; > > const struct spi_nor_controller_ops *controller_ops; > > -- > 2.20.1 >
Am 2021-03-22 15:21, schrieb Pratyush Yadav: > On 18/03/21 10:24AM, Michael Walle wrote: >> Due to possible mode switching to 8D-8D-8D, it might not be possible >> to >> read the SFDP after the initial probe. To be able to dump the SFDP via >> sysfs afterwards, make a complete copy of it. >> >> Signed-off-by: Michael Walle <michael@walle.cc> >> --- >> drivers/mtd/spi-nor/core.h | 10 ++++++++ >> drivers/mtd/spi-nor/sfdp.c | 49 >> +++++++++++++++++++++++++++++++++++++ >> include/linux/mtd/spi-nor.h | 3 +++ >> 3 files changed, 62 insertions(+) >> >> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h >> index 4a3f7f150b5d..668f22011b1d 100644 >> --- a/drivers/mtd/spi-nor/core.h >> +++ b/drivers/mtd/spi-nor/core.h >> @@ -407,6 +407,16 @@ struct spi_nor_manufacturer { >> const struct spi_nor_fixups *fixups; >> }; >> >> +/** >> + * struct sfdp - SFDP data >> + * @num_dwords: number of entries in the dwords array >> + * @dwords: array of double words of the SFDP data >> + */ >> +struct sfdp { >> + size_t num_dwords; >> + u32 *dwords; >> +}; >> + >> /* Manufacturer drivers. */ >> extern const struct spi_nor_manufacturer spi_nor_atmel; >> extern const struct spi_nor_manufacturer spi_nor_catalyst; >> diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c >> index 25142ec4737b..2b6c96e02532 100644 >> --- a/drivers/mtd/spi-nor/sfdp.c >> +++ b/drivers/mtd/spi-nor/sfdp.c >> @@ -16,6 +16,7 @@ >> (((p)->parameter_table_pointer[2] << 16) | \ >> ((p)->parameter_table_pointer[1] << 8) | \ >> ((p)->parameter_table_pointer[0] << 0)) >> +#define SFDP_PARAM_HEADER_PARAM_LEN(p) ((p)->length * 4) >> >> #define SFDP_BFPT_ID 0xff00 /* Basic Flash Parameter Table */ >> #define SFDP_SECTOR_MAP_ID 0xff81 /* Sector Map Table */ >> @@ -1263,6 +1264,8 @@ int spi_nor_parse_sfdp(struct spi_nor *nor, >> struct sfdp_parameter_header *param_headers = NULL; >> struct sfdp_header header; >> struct device *dev = nor->dev; >> + struct sfdp *sfdp; >> + size_t sfdp_size; >> size_t psize; >> int i, err; >> >> @@ -1285,6 +1288,9 @@ int spi_nor_parse_sfdp(struct spi_nor *nor, >> bfpt_header->major != SFDP_JESD216_MAJOR) >> return -EINVAL; >> >> + sfdp_size = SFDP_PARAM_HEADER_PTP(bfpt_header) + >> + SFDP_PARAM_HEADER_PARAM_LEN(bfpt_header); >> + >> /* >> * Allocate memory then read all parameter headers with a single >> * Read SFDP command. These parameter headers will actually be >> parsed >> @@ -1311,6 +1317,49 @@ int spi_nor_parse_sfdp(struct spi_nor *nor, >> } >> } >> >> + /* >> + * Cache the complete SFDP data. It is not (easily) possible to >> fetch >> + * SFDP after probe time and we need it for the sysfs access. >> + */ >> + for (i = 0; i < header.nph; i++) { >> + param_header = ¶m_headers[i]; >> + sfdp_size = max_t(size_t, sfdp_size, >> + SFDP_PARAM_HEADER_PTP(param_header) + >> + SFDP_PARAM_HEADER_PARAM_LEN(param_header)); > > *sigh* If BFPT header was not made a part of the main SFDP header, two > "sfdp_size = ..." would not be needed, and we could do it all in one > place. > > You can do that refactor if you're feeling like it, but I won't insist > on it. I think that could be refactored when we also use the SFDP cache for the remaining parsing. > >> + } >> + >> + /* >> + * Limit the total size to a reasonable value to avoid allocating >> too >> + * much memory just of because the flash returned some insane >> values. >> + */ >> + if (sfdp_size > PAGE_SIZE) { >> + dev_dbg(dev, "SFDP data (%zu) too big, truncating\n", >> + sfdp_size); >> + sfdp_size = PAGE_SIZE; > > Ok. 4K should be large enough for any reasonable SFDP table. > >> + } >> + >> + sfdp = devm_kzalloc(dev, sizeof(*sfdp), GFP_KERNEL); >> + if (!sfdp) { >> + err = -ENOMEM; >> + goto exit; >> + } > > I assume you made nor->sfdp a pointer and not an embedded struct so it > can easily indicate if SFDP was found or not, correct? Correct. > >> + >> + 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? >> + sfdp->dwords = devm_kcalloc(dev, sfdp->num_dwords, >> + sizeof(*sfdp->dwords), GFP_KERNEL); >> + if (!sfdp->dwords) { >> + err = -ENOMEM; > > Free sfdp here since it won't be used any longer. I see, spi_nor_parse_sfdp() is optional and won't fail the probe. Nice catch. > >> + goto exit; >> + } >> + >> + err = spi_nor_read_sfdp_dma_unsafe(nor, 0, sfdp_size, sfdp->dwords); >> + if (err < 0) { >> + dev_dbg(dev, "failed to read SFDP data\n"); >> + goto exit; > > Free sfdp and sfdp->dwords here since they won't be used any longer. > >> + } >> + >> + nor->sfdp = sfdp; >> + >> /* >> * Check other parameter headers to get the latest revision of >> * the basic flash parameter table. >> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h >> index a0d572855444..55c550208949 100644 >> --- a/include/linux/mtd/spi-nor.h >> +++ b/include/linux/mtd/spi-nor.h >> @@ -351,6 +351,7 @@ enum spi_nor_cmd_ext { >> struct flash_info; >> struct spi_nor_manufacturer; >> struct spi_nor_flash_parameter; >> +struct sfdp; > > nor->sfdp is a pointer. This should not be needed. > >> >> /** >> * struct spi_nor - Structure for defining the SPI NOR layer >> @@ -375,6 +376,7 @@ struct spi_nor_flash_parameter; >> * @read_proto: the SPI protocol for read operations >> * @write_proto: the SPI protocol for write operations >> * @reg_proto: the SPI protocol for read_reg/write_reg/erase >> operations >> + * @sfdp: the SFDP data of the flash >> * @controller_ops: SPI NOR controller driver specific operations. >> * @params: [FLASH-SPECIFIC] SPI NOR flash parameters and settings. >> * The structure includes legacy flash >> parameters and >> @@ -404,6 +406,7 @@ struct spi_nor { >> bool sst_write_second; >> u32 flags; >> enum spi_nor_cmd_ext cmd_ext_type; >> + struct sfdp *sfdp; >> >> const struct spi_nor_controller_ops *controller_ops; >> >> -- >> 2.20.1 >>
Am 2021-03-22 16:32, schrieb Michael Walle: >>> + >>> + 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? Oh, forgot to mention, sfdp_size is used to read the data. I just want to make sure, the allocated area is large enough. We shouldn't hit the undefined behavior by reading past the SFDP. Maybe that check should be part of the parsing code. -michael
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: [...] > > > @@ -1311,6 +1317,49 @@ int spi_nor_parse_sfdp(struct spi_nor *nor, > > > } > > > } > > > > > > + /* > > > + * Cache the complete SFDP data. It is not (easily) possible to > > > fetch > > > + * SFDP after probe time and we need it for the sysfs access. > > > + */ > > > + for (i = 0; i < header.nph; i++) { > > > + param_header = ¶m_headers[i]; > > > + sfdp_size = max_t(size_t, sfdp_size, > > > + SFDP_PARAM_HEADER_PTP(param_header) + > > > + SFDP_PARAM_HEADER_PARAM_LEN(param_header)); > > > > *sigh* If BFPT header was not made a part of the main SFDP header, two > > "sfdp_size = ..." would not be needed, and we could do it all in one > > place. > > > > You can do that refactor if you're feeling like it, but I won't insist > > on it. > > I think that could be refactored when we also use the SFDP cache for > the remaining parsing. Ok. [...] > > > > > + > > > + 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. 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. > > > > + sfdp->dwords = devm_kcalloc(dev, sfdp->num_dwords, > > > + sizeof(*sfdp->dwords), GFP_KERNEL); > > > + if (!sfdp->dwords) { > > > + err = -ENOMEM; > > > > Free sfdp here since it won't be used any longer. > > I see, spi_nor_parse_sfdp() is optional and won't fail the probe. > Nice catch. > > > > > > + goto exit; > > > + } > > > + > > > + err = spi_nor_read_sfdp_dma_unsafe(nor, 0, sfdp_size, sfdp->dwords); > > > + if (err < 0) { > > > + dev_dbg(dev, "failed to read SFDP data\n"); > > > + goto exit; > > > > Free sfdp and sfdp->dwords here since they won't be used any longer. > > > > > + } > > > + > > > + nor->sfdp = sfdp; > > > + > > > /* > > > * Check other parameter headers to get the latest revision of > > > * the basic flash parameter table. > > > diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h > > > index a0d572855444..55c550208949 100644 > > > --- a/include/linux/mtd/spi-nor.h > > > +++ b/include/linux/mtd/spi-nor.h > > > @@ -351,6 +351,7 @@ enum spi_nor_cmd_ext { > > > struct flash_info; > > > struct spi_nor_manufacturer; > > > struct spi_nor_flash_parameter; > > > +struct sfdp; > > > > nor->sfdp is a pointer. This should not be needed. > > > > > > > > /** > > > * struct spi_nor - Structure for defining the SPI NOR layer > > > @@ -375,6 +376,7 @@ struct spi_nor_flash_parameter; > > > * @read_proto: the SPI protocol for read operations > > > * @write_proto: the SPI protocol for write operations > > > * @reg_proto: the SPI protocol for read_reg/write_reg/erase > > > operations > > > + * @sfdp: the SFDP data of the flash > > > * @controller_ops: SPI NOR controller driver specific operations. > > > * @params: [FLASH-SPECIFIC] SPI NOR flash parameters and settings. > > > * The structure includes legacy flash > > > parameters and > > > @@ -404,6 +406,7 @@ struct spi_nor { > > > bool sst_write_second; > > > u32 flags; > > > enum spi_nor_cmd_ext cmd_ext_type; > > > + struct sfdp *sfdp; > > > > > > const struct spi_nor_controller_ops *controller_ops; > > > > > > -- > > > 2.20.1 > > > > > -- > -michael
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
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.
Hi, On 3/18/21 11:24 AM, Michael Walle wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > Due to possible mode switching to 8D-8D-8D, it might not be possible to > read the SFDP after the initial probe. To be able to dump the SFDP via > sysfs afterwards, make a complete copy of it. > > Signed-off-by: Michael Walle <michael@walle.cc> > --- > drivers/mtd/spi-nor/core.h | 10 ++++++++ > drivers/mtd/spi-nor/sfdp.c | 49 +++++++++++++++++++++++++++++++++++++ > include/linux/mtd/spi-nor.h | 3 +++ > 3 files changed, 62 insertions(+) > > diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h > index 4a3f7f150b5d..668f22011b1d 100644 > --- a/drivers/mtd/spi-nor/core.h > +++ b/drivers/mtd/spi-nor/core.h > @@ -407,6 +407,16 @@ struct spi_nor_manufacturer { > const struct spi_nor_fixups *fixups; > }; > > +/** > + * struct sfdp - SFDP data > + * @num_dwords: number of entries in the dwords array > + * @dwords: array of double words of the SFDP data > + */ > +struct sfdp { > + size_t num_dwords; > + u32 *dwords; > +}; > + > /* Manufacturer drivers. */ > extern const struct spi_nor_manufacturer spi_nor_atmel; > extern const struct spi_nor_manufacturer spi_nor_catalyst; > diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c > index 25142ec4737b..2b6c96e02532 100644 > --- a/drivers/mtd/spi-nor/sfdp.c > +++ b/drivers/mtd/spi-nor/sfdp.c > @@ -16,6 +16,7 @@ > (((p)->parameter_table_pointer[2] << 16) | \ > ((p)->parameter_table_pointer[1] << 8) | \ > ((p)->parameter_table_pointer[0] << 0)) > +#define SFDP_PARAM_HEADER_PARAM_LEN(p) ((p)->length * 4) > > #define SFDP_BFPT_ID 0xff00 /* Basic Flash Parameter Table */ > #define SFDP_SECTOR_MAP_ID 0xff81 /* Sector Map Table */ > @@ -1263,6 +1264,8 @@ int spi_nor_parse_sfdp(struct spi_nor *nor, > struct sfdp_parameter_header *param_headers = NULL; > struct sfdp_header header; > struct device *dev = nor->dev; > + struct sfdp *sfdp; > + size_t sfdp_size; > size_t psize; > int i, err; > > @@ -1285,6 +1288,9 @@ int spi_nor_parse_sfdp(struct spi_nor *nor, > bfpt_header->major != SFDP_JESD216_MAJOR) > return -EINVAL; > > + sfdp_size = SFDP_PARAM_HEADER_PTP(bfpt_header) + > + SFDP_PARAM_HEADER_PARAM_LEN(bfpt_header); > + > /* > * Allocate memory then read all parameter headers with a single > * Read SFDP command. These parameter headers will actually be parsed > @@ -1311,6 +1317,49 @@ int spi_nor_parse_sfdp(struct spi_nor *nor, > } > } > > + /* > + * Cache the complete SFDP data. It is not (easily) possible to fetch > + * SFDP after probe time and we need it for the sysfs access. > + */ > + for (i = 0; i < header.nph; i++) { > + param_header = ¶m_headers[i]; > + sfdp_size = max_t(size_t, sfdp_size, > + SFDP_PARAM_HEADER_PTP(param_header) + > + SFDP_PARAM_HEADER_PARAM_LEN(param_header)); > + } Michael, I like the idea of saving the SFDP data, but I think this can be improved a little. For example, it is not mandatory for the tables to be continuous in memory, there can be some gaps between BFPT and SMPT for example, thus we can improve the memory allocation logic. Also, we can make the saved sfdp data table-agnostic so that we don't duplicate the reads in parse_bfpt/smpt/4bait. Are you willing to respin? Cheers, ta
Hi, Am 2021-04-05 15:11, schrieb Tudor.Ambarus@microchip.com: > On 3/18/21 11:24 AM, Michael Walle wrote: >> EXTERNAL EMAIL: Do not click links or open attachments unless you know >> the content is safe >> >> Due to possible mode switching to 8D-8D-8D, it might not be possible >> to >> read the SFDP after the initial probe. To be able to dump the SFDP via >> sysfs afterwards, make a complete copy of it. >> >> Signed-off-by: Michael Walle <michael@walle.cc> >> --- >> drivers/mtd/spi-nor/core.h | 10 ++++++++ >> drivers/mtd/spi-nor/sfdp.c | 49 >> +++++++++++++++++++++++++++++++++++++ >> include/linux/mtd/spi-nor.h | 3 +++ >> 3 files changed, 62 insertions(+) >> >> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h >> index 4a3f7f150b5d..668f22011b1d 100644 >> --- a/drivers/mtd/spi-nor/core.h >> +++ b/drivers/mtd/spi-nor/core.h >> @@ -407,6 +407,16 @@ struct spi_nor_manufacturer { >> const struct spi_nor_fixups *fixups; >> }; >> >> +/** >> + * struct sfdp - SFDP data >> + * @num_dwords: number of entries in the dwords array >> + * @dwords: array of double words of the SFDP data >> + */ >> +struct sfdp { >> + size_t num_dwords; >> + u32 *dwords; >> +}; >> + >> /* Manufacturer drivers. */ >> extern const struct spi_nor_manufacturer spi_nor_atmel; >> extern const struct spi_nor_manufacturer spi_nor_catalyst; >> diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c >> index 25142ec4737b..2b6c96e02532 100644 >> --- a/drivers/mtd/spi-nor/sfdp.c >> +++ b/drivers/mtd/spi-nor/sfdp.c >> @@ -16,6 +16,7 @@ >> (((p)->parameter_table_pointer[2] << 16) | \ >> ((p)->parameter_table_pointer[1] << 8) | \ >> ((p)->parameter_table_pointer[0] << 0)) >> +#define SFDP_PARAM_HEADER_PARAM_LEN(p) ((p)->length * 4) >> >> #define SFDP_BFPT_ID 0xff00 /* Basic Flash Parameter Table >> */ >> #define SFDP_SECTOR_MAP_ID 0xff81 /* Sector Map Table */ >> @@ -1263,6 +1264,8 @@ int spi_nor_parse_sfdp(struct spi_nor *nor, >> struct sfdp_parameter_header *param_headers = NULL; >> struct sfdp_header header; >> struct device *dev = nor->dev; >> + struct sfdp *sfdp; >> + size_t sfdp_size; >> size_t psize; >> int i, err; >> >> @@ -1285,6 +1288,9 @@ int spi_nor_parse_sfdp(struct spi_nor *nor, >> bfpt_header->major != SFDP_JESD216_MAJOR) >> return -EINVAL; >> >> + sfdp_size = SFDP_PARAM_HEADER_PTP(bfpt_header) + >> + SFDP_PARAM_HEADER_PARAM_LEN(bfpt_header); >> + >> /* >> * Allocate memory then read all parameter headers with a >> single >> * Read SFDP command. These parameter headers will actually be >> parsed >> @@ -1311,6 +1317,49 @@ int spi_nor_parse_sfdp(struct spi_nor *nor, >> } >> } >> >> + /* >> + * Cache the complete SFDP data. It is not (easily) possible >> to fetch >> + * SFDP after probe time and we need it for the sysfs access. >> + */ >> + for (i = 0; i < header.nph; i++) { >> + param_header = ¶m_headers[i]; >> + sfdp_size = max_t(size_t, sfdp_size, >> + SFDP_PARAM_HEADER_PTP(param_header) >> + >> + >> SFDP_PARAM_HEADER_PARAM_LEN(param_header)); >> + } > > Michael, I like the idea of saving the SFDP data, but I think this can > be > improved a little. For example, it is not mandatory for the tables to > be > continuous in memory, there can be some gaps between BFPT and SMPT for > example, > thus we can improve the memory allocation logic. I want to parse the SFDP as little as possible. Keep in mind, that this should help to debug SFDP (errors). Therefore, I don't want to rely on the SFDP saying "hey there is a hole, please skip it". Who knows if there is some useful data? > Also, we can make the saved sfdp > data table-agnostic so that we don't duplicate the reads in > parse_bfpt/smpt/4bait. This falls into the same category as above. While it might be reused, the primary use case is to have the SFDP data available to a developer/user. Eg. what will you do with some holes in the sysfs read()? Return zeros? FWIW I'm planning to refactor the read code so the cached values are used. -michael
On 4/5/21 6:07 PM, Michael Walle wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > Hi, > > Am 2021-04-05 15:11, schrieb Tudor.Ambarus@microchip.com: >> On 3/18/21 11:24 AM, Michael Walle wrote: >>> EXTERNAL EMAIL: Do not click links or open attachments unless you know >>> the content is safe >>> >>> Due to possible mode switching to 8D-8D-8D, it might not be possible >>> to >>> read the SFDP after the initial probe. To be able to dump the SFDP via >>> sysfs afterwards, make a complete copy of it. >>> >>> Signed-off-by: Michael Walle <michael@walle.cc> >>> --- >>> drivers/mtd/spi-nor/core.h | 10 ++++++++ >>> drivers/mtd/spi-nor/sfdp.c | 49 >>> +++++++++++++++++++++++++++++++++++++ >>> include/linux/mtd/spi-nor.h | 3 +++ >>> 3 files changed, 62 insertions(+) >>> >>> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h >>> index 4a3f7f150b5d..668f22011b1d 100644 >>> --- a/drivers/mtd/spi-nor/core.h >>> +++ b/drivers/mtd/spi-nor/core.h >>> @@ -407,6 +407,16 @@ struct spi_nor_manufacturer { >>> const struct spi_nor_fixups *fixups; >>> }; >>> >>> +/** >>> + * struct sfdp - SFDP data >>> + * @num_dwords: number of entries in the dwords array >>> + * @dwords: array of double words of the SFDP data >>> + */ >>> +struct sfdp { >>> + size_t num_dwords; >>> + u32 *dwords; >>> +}; >>> + >>> /* Manufacturer drivers. */ >>> extern const struct spi_nor_manufacturer spi_nor_atmel; >>> extern const struct spi_nor_manufacturer spi_nor_catalyst; >>> diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c >>> index 25142ec4737b..2b6c96e02532 100644 >>> --- a/drivers/mtd/spi-nor/sfdp.c >>> +++ b/drivers/mtd/spi-nor/sfdp.c >>> @@ -16,6 +16,7 @@ >>> (((p)->parameter_table_pointer[2] << 16) | \ >>> ((p)->parameter_table_pointer[1] << 8) | \ >>> ((p)->parameter_table_pointer[0] << 0)) >>> +#define SFDP_PARAM_HEADER_PARAM_LEN(p) ((p)->length * 4) >>> >>> #define SFDP_BFPT_ID 0xff00 /* Basic Flash Parameter Table >>> */ >>> #define SFDP_SECTOR_MAP_ID 0xff81 /* Sector Map Table */ >>> @@ -1263,6 +1264,8 @@ int spi_nor_parse_sfdp(struct spi_nor *nor, >>> struct sfdp_parameter_header *param_headers = NULL; >>> struct sfdp_header header; >>> struct device *dev = nor->dev; >>> + struct sfdp *sfdp; >>> + size_t sfdp_size; >>> size_t psize; >>> int i, err; >>> >>> @@ -1285,6 +1288,9 @@ int spi_nor_parse_sfdp(struct spi_nor *nor, >>> bfpt_header->major != SFDP_JESD216_MAJOR) >>> return -EINVAL; >>> >>> + sfdp_size = SFDP_PARAM_HEADER_PTP(bfpt_header) + >>> + SFDP_PARAM_HEADER_PARAM_LEN(bfpt_header); >>> + >>> /* >>> * Allocate memory then read all parameter headers with a >>> single >>> * Read SFDP command. These parameter headers will actually be >>> parsed >>> @@ -1311,6 +1317,49 @@ int spi_nor_parse_sfdp(struct spi_nor *nor, >>> } >>> } >>> >>> + /* >>> + * Cache the complete SFDP data. It is not (easily) possible >>> to fetch >>> + * SFDP after probe time and we need it for the sysfs access. >>> + */ >>> + for (i = 0; i < header.nph; i++) { >>> + param_header = ¶m_headers[i]; >>> + sfdp_size = max_t(size_t, sfdp_size, >>> + SFDP_PARAM_HEADER_PTP(param_header) >>> + >>> + >>> SFDP_PARAM_HEADER_PARAM_LEN(param_header)); >>> + } >> >> Michael, I like the idea of saving the SFDP data, but I think this can >> be >> improved a little. For example, it is not mandatory for the tables to >> be >> continuous in memory, there can be some gaps between BFPT and SMPT for >> example, >> thus we can improve the memory allocation logic. > > I want to parse the SFDP as little as possible. Keep in mind, that this > should > help to debug SFDP (errors). Therefore, I don't want to rely on the SFDP > saying > "hey there is a hole, please skip it". Who knows if there is some useful > data? What kind of useful data? Do we care about data that doesn't follow the jesd216 standard? > >> Also, we can make the saved sfdp >> data table-agnostic so that we don't duplicate the reads in >> parse_bfpt/smpt/4bait. > > This falls into the same category as above. While it might be reused, > the > primary use case is to have the SFDP data available to a developer/user. > Eg. > what will you do with some holes in the sysfs read()? Return zeros? We don't have to have gaps in our internal buffer, we just allocate as much as we need and we write into our internal buffer just the sfdp tables, without the gaps. > > FWIW I'm planning to refactor the read code so the cached values are > used. > > -michael
Am 2021-04-05 17:42, schrieb Tudor.Ambarus@microchip.com: > On 4/5/21 6:07 PM, Michael Walle wrote: >> EXTERNAL EMAIL: Do not click links or open attachments unless you know >> the content is safe >> >> Hi, >> >> Am 2021-04-05 15:11, schrieb Tudor.Ambarus@microchip.com: >>> On 3/18/21 11:24 AM, Michael Walle wrote: >>>> EXTERNAL EMAIL: Do not click links or open attachments unless you >>>> know >>>> the content is safe >>>> >>>> Due to possible mode switching to 8D-8D-8D, it might not be possible >>>> to >>>> read the SFDP after the initial probe. To be able to dump the SFDP >>>> via >>>> sysfs afterwards, make a complete copy of it. >>>> >>>> Signed-off-by: Michael Walle <michael@walle.cc> >>>> --- >>>> drivers/mtd/spi-nor/core.h | 10 ++++++++ >>>> drivers/mtd/spi-nor/sfdp.c | 49 >>>> +++++++++++++++++++++++++++++++++++++ >>>> include/linux/mtd/spi-nor.h | 3 +++ >>>> 3 files changed, 62 insertions(+) >>>> >>>> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h >>>> index 4a3f7f150b5d..668f22011b1d 100644 >>>> --- a/drivers/mtd/spi-nor/core.h >>>> +++ b/drivers/mtd/spi-nor/core.h >>>> @@ -407,6 +407,16 @@ struct spi_nor_manufacturer { >>>> const struct spi_nor_fixups *fixups; >>>> }; >>>> >>>> +/** >>>> + * struct sfdp - SFDP data >>>> + * @num_dwords: number of entries in the dwords array >>>> + * @dwords: array of double words of the SFDP data >>>> + */ >>>> +struct sfdp { >>>> + size_t num_dwords; >>>> + u32 *dwords; >>>> +}; >>>> + >>>> /* Manufacturer drivers. */ >>>> extern const struct spi_nor_manufacturer spi_nor_atmel; >>>> extern const struct spi_nor_manufacturer spi_nor_catalyst; >>>> diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c >>>> index 25142ec4737b..2b6c96e02532 100644 >>>> --- a/drivers/mtd/spi-nor/sfdp.c >>>> +++ b/drivers/mtd/spi-nor/sfdp.c >>>> @@ -16,6 +16,7 @@ >>>> (((p)->parameter_table_pointer[2] << 16) | \ >>>> ((p)->parameter_table_pointer[1] << 8) | \ >>>> ((p)->parameter_table_pointer[0] << 0)) >>>> +#define SFDP_PARAM_HEADER_PARAM_LEN(p) ((p)->length * 4) >>>> >>>> #define SFDP_BFPT_ID 0xff00 /* Basic Flash Parameter >>>> Table >>>> */ >>>> #define SFDP_SECTOR_MAP_ID 0xff81 /* Sector Map Table */ >>>> @@ -1263,6 +1264,8 @@ int spi_nor_parse_sfdp(struct spi_nor *nor, >>>> struct sfdp_parameter_header *param_headers = NULL; >>>> struct sfdp_header header; >>>> struct device *dev = nor->dev; >>>> + struct sfdp *sfdp; >>>> + size_t sfdp_size; >>>> size_t psize; >>>> int i, err; >>>> >>>> @@ -1285,6 +1288,9 @@ int spi_nor_parse_sfdp(struct spi_nor *nor, >>>> bfpt_header->major != SFDP_JESD216_MAJOR) >>>> return -EINVAL; >>>> >>>> + sfdp_size = SFDP_PARAM_HEADER_PTP(bfpt_header) + >>>> + SFDP_PARAM_HEADER_PARAM_LEN(bfpt_header); >>>> + >>>> /* >>>> * Allocate memory then read all parameter headers with a >>>> single >>>> * Read SFDP command. These parameter headers will actually >>>> be >>>> parsed >>>> @@ -1311,6 +1317,49 @@ int spi_nor_parse_sfdp(struct spi_nor *nor, >>>> } >>>> } >>>> >>>> + /* >>>> + * Cache the complete SFDP data. It is not (easily) possible >>>> to fetch >>>> + * SFDP after probe time and we need it for the sysfs >>>> access. >>>> + */ >>>> + for (i = 0; i < header.nph; i++) { >>>> + param_header = ¶m_headers[i]; >>>> + sfdp_size = max_t(size_t, sfdp_size, >>>> + >>>> SFDP_PARAM_HEADER_PTP(param_header) >>>> + >>>> + >>>> SFDP_PARAM_HEADER_PARAM_LEN(param_header)); >>>> + } >>> >>> Michael, I like the idea of saving the SFDP data, but I think this >>> can >>> be >>> improved a little. For example, it is not mandatory for the tables to >>> be >>> continuous in memory, there can be some gaps between BFPT and SMPT >>> for >>> example, >>> thus we can improve the memory allocation logic. >> >> I want to parse the SFDP as little as possible. Keep in mind, that >> this >> should >> help to debug SFDP (errors). Therefore, I don't want to rely on the >> SFDP >> saying >> "hey there is a hole, please skip it". Who knows if there is some >> useful >> data? > > What kind of useful data? Do we care about data that doesn't follow the > jesd216 > standard? Yes because, it should be a raw dump of the SFDP data (of whatever the flash vendor thinks is valid). You want to be able to debug non-compliant SFDP data. Otherwise, this doesn't make any sense to have it in the first place. >>> Also, we can make the saved sfdp >>> data table-agnostic so that we don't duplicate the reads in >>> parse_bfpt/smpt/4bait. >> >> This falls into the same category as above. While it might be reused, >> the >> primary use case is to have the SFDP data available to a >> developer/user. >> Eg. >> what will you do with some holes in the sysfs read()? Return zeros? > > We don't have to have gaps in our internal buffer, we just allocate as > much > as we need and we write into our internal buffer just the sfdp tables, > without > the gaps. There are two use cases: (1) cache the data for the SFDP table parsing (2) provide a raw dump of the SFDP This patch targets (2). So first, you'd need to allocate multiple buffers, then you'd have to combine them again for the raw SFDP dump and finally you'd need to fill the gaps for the dump again. Because what I expect is to have a contiguous "sfdp" sysfs file. -michael
diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h index 4a3f7f150b5d..668f22011b1d 100644 --- a/drivers/mtd/spi-nor/core.h +++ b/drivers/mtd/spi-nor/core.h @@ -407,6 +407,16 @@ struct spi_nor_manufacturer { const struct spi_nor_fixups *fixups; }; +/** + * struct sfdp - SFDP data + * @num_dwords: number of entries in the dwords array + * @dwords: array of double words of the SFDP data + */ +struct sfdp { + size_t num_dwords; + u32 *dwords; +}; + /* Manufacturer drivers. */ extern const struct spi_nor_manufacturer spi_nor_atmel; extern const struct spi_nor_manufacturer spi_nor_catalyst; diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c index 25142ec4737b..2b6c96e02532 100644 --- a/drivers/mtd/spi-nor/sfdp.c +++ b/drivers/mtd/spi-nor/sfdp.c @@ -16,6 +16,7 @@ (((p)->parameter_table_pointer[2] << 16) | \ ((p)->parameter_table_pointer[1] << 8) | \ ((p)->parameter_table_pointer[0] << 0)) +#define SFDP_PARAM_HEADER_PARAM_LEN(p) ((p)->length * 4) #define SFDP_BFPT_ID 0xff00 /* Basic Flash Parameter Table */ #define SFDP_SECTOR_MAP_ID 0xff81 /* Sector Map Table */ @@ -1263,6 +1264,8 @@ int spi_nor_parse_sfdp(struct spi_nor *nor, struct sfdp_parameter_header *param_headers = NULL; struct sfdp_header header; struct device *dev = nor->dev; + struct sfdp *sfdp; + size_t sfdp_size; size_t psize; int i, err; @@ -1285,6 +1288,9 @@ int spi_nor_parse_sfdp(struct spi_nor *nor, bfpt_header->major != SFDP_JESD216_MAJOR) return -EINVAL; + sfdp_size = SFDP_PARAM_HEADER_PTP(bfpt_header) + + SFDP_PARAM_HEADER_PARAM_LEN(bfpt_header); + /* * Allocate memory then read all parameter headers with a single * Read SFDP command. These parameter headers will actually be parsed @@ -1311,6 +1317,49 @@ int spi_nor_parse_sfdp(struct spi_nor *nor, } } + /* + * Cache the complete SFDP data. It is not (easily) possible to fetch + * SFDP after probe time and we need it for the sysfs access. + */ + for (i = 0; i < header.nph; i++) { + param_header = ¶m_headers[i]; + sfdp_size = max_t(size_t, sfdp_size, + SFDP_PARAM_HEADER_PTP(param_header) + + SFDP_PARAM_HEADER_PARAM_LEN(param_header)); + } + + /* + * Limit the total size to a reasonable value to avoid allocating too + * much memory just of because the flash returned some insane values. + */ + if (sfdp_size > PAGE_SIZE) { + dev_dbg(dev, "SFDP data (%zu) too big, truncating\n", + sfdp_size); + sfdp_size = PAGE_SIZE; + } + + sfdp = devm_kzalloc(dev, sizeof(*sfdp), GFP_KERNEL); + if (!sfdp) { + err = -ENOMEM; + goto exit; + } + + sfdp->num_dwords = DIV_ROUND_UP(sfdp_size, sizeof(*sfdp->dwords)); + sfdp->dwords = devm_kcalloc(dev, sfdp->num_dwords, + sizeof(*sfdp->dwords), GFP_KERNEL); + if (!sfdp->dwords) { + err = -ENOMEM; + goto exit; + } + + err = spi_nor_read_sfdp_dma_unsafe(nor, 0, sfdp_size, sfdp->dwords); + if (err < 0) { + dev_dbg(dev, "failed to read SFDP data\n"); + goto exit; + } + + nor->sfdp = sfdp; + /* * Check other parameter headers to get the latest revision of * the basic flash parameter table. diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h index a0d572855444..55c550208949 100644 --- a/include/linux/mtd/spi-nor.h +++ b/include/linux/mtd/spi-nor.h @@ -351,6 +351,7 @@ enum spi_nor_cmd_ext { struct flash_info; struct spi_nor_manufacturer; struct spi_nor_flash_parameter; +struct sfdp; /** * struct spi_nor - Structure for defining the SPI NOR layer @@ -375,6 +376,7 @@ struct spi_nor_flash_parameter; * @read_proto: the SPI protocol for read operations * @write_proto: the SPI protocol for write operations * @reg_proto: the SPI protocol for read_reg/write_reg/erase operations + * @sfdp: the SFDP data of the flash * @controller_ops: SPI NOR controller driver specific operations. * @params: [FLASH-SPECIFIC] SPI NOR flash parameters and settings. * The structure includes legacy flash parameters and @@ -404,6 +406,7 @@ struct spi_nor { bool sst_write_second; u32 flags; enum spi_nor_cmd_ext cmd_ext_type; + struct sfdp *sfdp; const struct spi_nor_controller_ops *controller_ops;
Due to possible mode switching to 8D-8D-8D, it might not be possible to read the SFDP after the initial probe. To be able to dump the SFDP via sysfs afterwards, make a complete copy of it. Signed-off-by: Michael Walle <michael@walle.cc> --- drivers/mtd/spi-nor/core.h | 10 ++++++++ drivers/mtd/spi-nor/sfdp.c | 49 +++++++++++++++++++++++++++++++++++++ include/linux/mtd/spi-nor.h | 3 +++ 3 files changed, 62 insertions(+)