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

[Michael Walle]

Hi,

Am 2021-04-05 15:11, schrieb Tudor.Ambarus@xxxxxxxxxxxxx:
> 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@xxxxxxxx>
> > ---
> >  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 = &param_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