[PATCH 0/2] mtd: spi-nor: support dumping sfdp tables

[PATCH 0/2] mtd: spi-nor: support dumping sfdp tables

[Michael Walle]

Add the possibility to dump the SFDP data of a flash device.

More and more flash devices share the same flash ID and we need per device
fixups. Usually, these fixups differentiate flashes by looking at
differences in the SFDP data. Determining the difference is only possible
if we have the SFDP data for all the flashes which share a flash ID. This
will lay the foundation to dump the whole SFDP data of a flash device.

This is even more important, because some datasheets doesn't even contain
the SFDP data. Fixups for these kind of flashes are nearly impossible to
do.

I envision having a database of all the SFDP data for the flashes we
support and make it a requirement to submit it when a new flash is added.
This might or might not have legal implications. Thus I'd start with having
that database private to the SPI NOR maintainers.

Changes since RFC:
 - Don't read SFDP data after probe. The flash might already be switched to
   8D-8D-8D mode. Instead, cache the SFDP data
 - add two sysfs files: jedec-id and name
 - change the file mode of the sfdp file from 0400 to 0444. There is no
   hardware access anymore.

Michael Walle (2):
  mtd: spi-nor: sfdp: save a copy of the SFDP data
  mtd: spi-nor: add initial sysfs support

 drivers/mtd/spi-nor/Makefile |  2 +-
 drivers/mtd/spi-nor/core.c   |  5 +++
 drivers/mtd/spi-nor/core.h   | 13 ++++++
 drivers/mtd/spi-nor/sfdp.c   | 49 ++++++++++++++++++++
 drivers/mtd/spi-nor/sysfs.c  | 86 ++++++++++++++++++++++++++++++++++++
 include/linux/mtd/spi-nor.h  |  3 ++
 6 files changed, 157 insertions(+), 1 deletion(-)
 create mode 100644 drivers/mtd/spi-nor/sysfs.c

-- 
2.20.1

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

[Michael Walle]

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 = &param_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;
 
-- 
2.20.1

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

[Pratyush Yadav]

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 = &param_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
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

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

[Michael Walle]

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 = &param_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
>> 

-- 
-michael

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

[Michael Walle]

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

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

[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:
[...]
> > > @@ -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));
> > 
> > *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

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

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

[Michael Walle]

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

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.

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

[Tudor.Ambarus]

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 = &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. 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

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@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 = &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

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

[Tudor.Ambarus]

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 = &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?

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

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

[Michael Walle]

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 = &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?
> 
> 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

[PATCH 2/2] mtd: spi-nor: add initial sysfs support

[Michael Walle]

Add support to show the name and JEDEC identifier as well as to dump the
SFDP table. Not all flashes list their SFDP table contents in their
datasheet. So having that is useful. It might also be helpful in bug
reports from users.

The idea behind the sysfs module is also to have raw access to the SPI
NOR flash device registers, which can also be useful for debugging.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/mtd/spi-nor/Makefile |  2 +-
 drivers/mtd/spi-nor/core.c   |  5 +++
 drivers/mtd/spi-nor/core.h   |  3 ++
 drivers/mtd/spi-nor/sysfs.c  | 86 ++++++++++++++++++++++++++++++++++++
 4 files changed, 95 insertions(+), 1 deletion(-)
 create mode 100644 drivers/mtd/spi-nor/sysfs.c

diff --git a/drivers/mtd/spi-nor/Makefile b/drivers/mtd/spi-nor/Makefile
index 653923896205..aff308f75987 100644
--- a/drivers/mtd/spi-nor/Makefile
+++ b/drivers/mtd/spi-nor/Makefile
@@ -1,6 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0
 
-spi-nor-objs			:= core.o sfdp.o
+spi-nor-objs			:= core.o sfdp.o sysfs.o
 spi-nor-objs			+= atmel.o
 spi-nor-objs			+= catalyst.o
 spi-nor-objs			+= eon.o
diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 4a315cb1c4db..2eaf4ba8c0f3 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -3707,6 +3707,10 @@ static int spi_nor_probe(struct spi_mem *spimem)
 	if (ret)
 		return ret;
 
+	ret = spi_nor_sysfs_create(nor);
+	if (ret)
+		return ret;
+
 	return mtd_device_register(&nor->mtd, data ? data->parts : NULL,
 				   data ? data->nr_parts : 0);
 }
@@ -3716,6 +3720,7 @@ static int spi_nor_remove(struct spi_mem *spimem)
 	struct spi_nor *nor = spi_mem_get_drvdata(spimem);
 
 	spi_nor_restore(nor);
+	spi_nor_sysfs_remove(nor);
 
 	/* Clean up MTD stuff. */
 	return mtd_device_unregister(&nor->mtd);
diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index 668f22011b1d..dd592f7b62d1 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -488,4 +488,7 @@ static struct spi_nor __maybe_unused *mtd_to_spi_nor(struct mtd_info *mtd)
 	return mtd->priv;
 }
 
+int spi_nor_sysfs_create(struct spi_nor *nor);
+void spi_nor_sysfs_remove(struct spi_nor *nor);
+
 #endif /* __LINUX_MTD_SPI_NOR_INTERNAL_H */
diff --git a/drivers/mtd/spi-nor/sysfs.c b/drivers/mtd/spi-nor/sysfs.c
new file mode 100644
index 000000000000..0de031e246c5
--- /dev/null
+++ b/drivers/mtd/spi-nor/sysfs.c
@@ -0,0 +1,86 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/mtd/spi-nor.h>
+#include <linux/spi/spi.h>
+#include <linux/spi/spi-mem.h>
+#include <linux/sysfs.h>
+
+#include "core.h"
+
+static ssize_t name_show(struct device *dev,
+			 struct device_attribute *attr, char *buf)
+{
+	struct spi_device *spi = to_spi_device(dev);
+	struct spi_mem *spimem = spi_get_drvdata(spi);
+	struct spi_nor *nor = spi_mem_get_drvdata(spimem);
+
+	return sprintf(buf, "%s\n", nor->info->name);
+}
+static DEVICE_ATTR_RO(name);
+
+static ssize_t jedec_id_show(struct device *dev,
+			     struct device_attribute *attr, char *buf)
+{
+	struct spi_device *spi = to_spi_device(dev);
+	struct spi_mem *spimem = spi_get_drvdata(spi);
+	struct spi_nor *nor = spi_mem_get_drvdata(spimem);
+
+	return sprintf(buf, "%*phN\n", nor->info->id_len, nor->info->id);
+}
+static DEVICE_ATTR_RO(jedec_id);
+
+static struct attribute *spi_nor_sysfs_entries[] = {
+	&dev_attr_name.attr,
+	&dev_attr_jedec_id.attr,
+	NULL
+};
+
+static ssize_t sfdp_read(struct file *filp, struct kobject *kobj,
+			 struct bin_attribute *bin_attr, char *buf,
+			 loff_t off, size_t count)
+{
+	struct spi_device *spi = to_spi_device(kobj_to_dev(kobj));
+	struct spi_mem *spimem = spi_get_drvdata(spi);
+	struct spi_nor *nor = spi_mem_get_drvdata(spimem);
+	struct sfdp *sfdp = nor->sfdp;
+	size_t sfdp_size = sfdp->num_dwords * sizeof(*sfdp->dwords);
+
+	return memory_read_from_buffer(buf, count, &off, nor->sfdp->dwords,
+				       sfdp_size);
+}
+static BIN_ATTR_RO(sfdp, PAGE_SIZE);
+
+static struct bin_attribute *spi_nor_sysfs_bin_entries[] = {
+	&bin_attr_sfdp,
+	NULL
+};
+
+static umode_t spi_nor_sysfs_is_bin_visible(struct kobject *kobj,
+					    struct bin_attribute *attr, int n)
+{
+	struct spi_device *spi = to_spi_device(kobj_to_dev(kobj));
+	struct spi_mem *spimem = spi_get_drvdata(spi);
+	struct spi_nor *nor = spi_mem_get_drvdata(spimem);
+
+	if (attr == &bin_attr_sfdp && nor->sfdp)
+		return 0444;
+
+	return 0;
+}
+
+static struct attribute_group spi_nor_sysfs_attr_group = {
+	.name		= NULL,
+	.is_bin_visible	= spi_nor_sysfs_is_bin_visible,
+	.attrs		= spi_nor_sysfs_entries,
+	.bin_attrs	= spi_nor_sysfs_bin_entries,
+};
+
+int spi_nor_sysfs_create(struct spi_nor *nor)
+{
+	return sysfs_create_group(&nor->dev->kobj, &spi_nor_sysfs_attr_group);
+}
+
+void spi_nor_sysfs_remove(struct spi_nor *nor)
+{
+	sysfs_remove_group(&nor->dev->kobj, &spi_nor_sysfs_attr_group);
+}
-- 
2.20.1

Re: [PATCH 2/2] mtd: spi-nor: add initial sysfs support

[Yicong Yang]

On 2021/3/18 17:24, Michael Walle wrote:
> Add support to show the name and JEDEC identifier as well as to dump the
> SFDP table. Not all flashes list their SFDP table contents in their
> datasheet. So having that is useful. It might also be helpful in bug
> reports from users.
> 
> The idea behind the sysfs module is also to have raw access to the SPI
> NOR flash device registers, which can also be useful for debugging.

Hi Michael,

I like the idea to dump the sfdp data,it will make debug easier. should it go in debugfs?
we already have debugfs files for partname and partid of the flash.

> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>  drivers/mtd/spi-nor/Makefile |  2 +-
>  drivers/mtd/spi-nor/core.c   |  5 +++
>  drivers/mtd/spi-nor/core.h   |  3 ++
>  drivers/mtd/spi-nor/sysfs.c  | 86 ++++++++++++++++++++++++++++++++++++
>  4 files changed, 95 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/mtd/spi-nor/sysfs.c
> 
> diff --git a/drivers/mtd/spi-nor/Makefile b/drivers/mtd/spi-nor/Makefile
> index 653923896205..aff308f75987 100644
> --- a/drivers/mtd/spi-nor/Makefile
> +++ b/drivers/mtd/spi-nor/Makefile
> @@ -1,6 +1,6 @@
>  # SPDX-License-Identifier: GPL-2.0
>  
> -spi-nor-objs			:= core.o sfdp.o
> +spi-nor-objs			:= core.o sfdp.o sysfs.o
>  spi-nor-objs			+= atmel.o
>  spi-nor-objs			+= catalyst.o
>  spi-nor-objs			+= eon.o
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 4a315cb1c4db..2eaf4ba8c0f3 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -3707,6 +3707,10 @@ static int spi_nor_probe(struct spi_mem *spimem)
>  	if (ret)
>  		return ret;
>  
> +	ret = spi_nor_sysfs_create(nor);
> +	if (ret)
> +		return ret;
> +
>  	return mtd_device_register(&nor->mtd, data ? data->parts : NULL,
>  				   data ? data->nr_parts : 0);
>  }
> @@ -3716,6 +3720,7 @@ static int spi_nor_remove(struct spi_mem *spimem)
>  	struct spi_nor *nor = spi_mem_get_drvdata(spimem);
>  
>  	spi_nor_restore(nor);
> +	spi_nor_sysfs_remove(nor);
>  
>  	/* Clean up MTD stuff. */
>  	return mtd_device_unregister(&nor->mtd);
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index 668f22011b1d..dd592f7b62d1 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -488,4 +488,7 @@ static struct spi_nor __maybe_unused *mtd_to_spi_nor(struct mtd_info *mtd)
>  	return mtd->priv;
>  }
>  
> +int spi_nor_sysfs_create(struct spi_nor *nor);
> +void spi_nor_sysfs_remove(struct spi_nor *nor);
> +
>  #endif /* __LINUX_MTD_SPI_NOR_INTERNAL_H */
> diff --git a/drivers/mtd/spi-nor/sysfs.c b/drivers/mtd/spi-nor/sysfs.c
> new file mode 100644
> index 000000000000..0de031e246c5
> --- /dev/null
> +++ b/drivers/mtd/spi-nor/sysfs.c
> @@ -0,0 +1,86 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/mtd/spi-nor.h>
> +#include <linux/spi/spi.h>
> +#include <linux/spi/spi-mem.h>
> +#include <linux/sysfs.h>
> +
> +#include "core.h"
> +
> +static ssize_t name_show(struct device *dev,
> +			 struct device_attribute *attr, char *buf)
> +{
> +	struct spi_device *spi = to_spi_device(dev);
> +	struct spi_mem *spimem = spi_get_drvdata(spi);
> +	struct spi_nor *nor = spi_mem_get_drvdata(spimem);
> +
> +	return sprintf(buf, "%s\n", nor->info->name);

perhaps sysfs_emit() instead if we go sysfs? as suggested by [1].

[1] Documentation/filesystems/sysfs.rst:line 246

Thanks,
Yicong

> +}
> +static DEVICE_ATTR_RO(name);
> +
> +static ssize_t jedec_id_show(struct device *dev,
> +			     struct device_attribute *attr, char *buf)
> +{
> +	struct spi_device *spi = to_spi_device(dev);
> +	struct spi_mem *spimem = spi_get_drvdata(spi);
> +	struct spi_nor *nor = spi_mem_get_drvdata(spimem);
> +
> +	return sprintf(buf, "%*phN\n", nor->info->id_len, nor->info->id);
> +}
> +static DEVICE_ATTR_RO(jedec_id);
> +
> +static struct attribute *spi_nor_sysfs_entries[] = {
> +	&dev_attr_name.attr,
> +	&dev_attr_jedec_id.attr,
> +	NULL
> +};
> +
> +static ssize_t sfdp_read(struct file *filp, struct kobject *kobj,
> +			 struct bin_attribute *bin_attr, char *buf,
> +			 loff_t off, size_t count)
> +{
> +	struct spi_device *spi = to_spi_device(kobj_to_dev(kobj));
> +	struct spi_mem *spimem = spi_get_drvdata(spi);
> +	struct spi_nor *nor = spi_mem_get_drvdata(spimem);
> +	struct sfdp *sfdp = nor->sfdp;
> +	size_t sfdp_size = sfdp->num_dwords * sizeof(*sfdp->dwords);
> +
> +	return memory_read_from_buffer(buf, count, &off, nor->sfdp->dwords,
> +				       sfdp_size);
> +}
> +static BIN_ATTR_RO(sfdp, PAGE_SIZE);
> +
> +static struct bin_attribute *spi_nor_sysfs_bin_entries[] = {
> +	&bin_attr_sfdp,
> +	NULL
> +};
> +
> +static umode_t spi_nor_sysfs_is_bin_visible(struct kobject *kobj,
> +					    struct bin_attribute *attr, int n)
> +{
> +	struct spi_device *spi = to_spi_device(kobj_to_dev(kobj));
> +	struct spi_mem *spimem = spi_get_drvdata(spi);
> +	struct spi_nor *nor = spi_mem_get_drvdata(spimem);
> +
> +	if (attr == &bin_attr_sfdp && nor->sfdp)
> +		return 0444;
> +
> +	return 0;
> +}
> +
> +static struct attribute_group spi_nor_sysfs_attr_group = {
> +	.name		= NULL,
> +	.is_bin_visible	= spi_nor_sysfs_is_bin_visible,
> +	.attrs		= spi_nor_sysfs_entries,
> +	.bin_attrs	= spi_nor_sysfs_bin_entries,
> +};
> +
> +int spi_nor_sysfs_create(struct spi_nor *nor)
> +{
> +	return sysfs_create_group(&nor->dev->kobj, &spi_nor_sysfs_attr_group);
> +}
> +
> +void spi_nor_sysfs_remove(struct spi_nor *nor)
> +{
> +	sysfs_remove_group(&nor->dev->kobj, &spi_nor_sysfs_attr_group);
> +}
> 

Re: [PATCH 2/2] mtd: spi-nor: add initial sysfs support

[Michael Walle]

Hi Yicong,

Am 2021-03-20 05:16, schrieb Yicong Yang:
> On 2021/3/18 17:24, Michael Walle wrote:
>> Add support to show the name and JEDEC identifier as well as to dump 
>> the
>> SFDP table. Not all flashes list their SFDP table contents in their
>> datasheet. So having that is useful. It might also be helpful in bug
>> reports from users.
>> 
>> The idea behind the sysfs module is also to have raw access to the SPI
>> NOR flash device registers, which can also be useful for debugging.
> 
> I like the idea to dump the sfdp data,it will make debug easier.
> should it go in debugfs?
> we already have debugfs files for partname and partid of the flash.

I've seen that, but thats for the MTD subsystem and is per MTD. Well,
one could add an own debugfs for spi-nor, but I still fear it might
not be available just when its needed. Of course a developer can
easily enable it for its debugging kernel. But I'm not sure if thats
the only use case.

I'd guess it boils down to, whether there could also be tooling
around this. Eg. think of something like "lssfdp".

I'd prefer sysfs, but lets hear what the maintainers think.

[..]

>> +static ssize_t name_show(struct device *dev,
>> +			 struct device_attribute *attr, char *buf)
>> +{
>> +	struct spi_device *spi = to_spi_device(dev);
>> +	struct spi_mem *spimem = spi_get_drvdata(spi);
>> +	struct spi_nor *nor = spi_mem_get_drvdata(spimem);
>> +
>> +	return sprintf(buf, "%s\n", nor->info->name);
> 
> perhaps sysfs_emit() instead if we go sysfs? as suggested by [1].

Thanks, didn't know about that new helper.

-michael

Re: [PATCH 2/2] mtd: spi-nor: add initial sysfs support

[Pratyush Yadav]

Hi Michael,

I have not worked with sysfs much so only giving this patch a quick 
overview.

On 18/03/21 10:24AM, Michael Walle wrote:
> Add support to show the name and JEDEC identifier as well as to dump the
> SFDP table. Not all flashes list their SFDP table contents in their
> datasheet. So having that is useful. It might also be helpful in bug
> reports from users.
> 
> The idea behind the sysfs module is also to have raw access to the SPI
> NOR flash device registers, which can also be useful for debugging.

Your current patch does not add this feature. Why do you think it is 
important enough to mention it?

> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>  drivers/mtd/spi-nor/Makefile |  2 +-
>  drivers/mtd/spi-nor/core.c   |  5 +++
>  drivers/mtd/spi-nor/core.h   |  3 ++
>  drivers/mtd/spi-nor/sysfs.c  | 86 ++++++++++++++++++++++++++++++++++++
>  4 files changed, 95 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/mtd/spi-nor/sysfs.c
> 
> diff --git a/drivers/mtd/spi-nor/Makefile b/drivers/mtd/spi-nor/Makefile
> index 653923896205..aff308f75987 100644
> --- a/drivers/mtd/spi-nor/Makefile
> +++ b/drivers/mtd/spi-nor/Makefile
> @@ -1,6 +1,6 @@
>  # SPDX-License-Identifier: GPL-2.0
>  
> -spi-nor-objs			:= core.o sfdp.o
> +spi-nor-objs			:= core.o sfdp.o sysfs.o
>  spi-nor-objs			+= atmel.o
>  spi-nor-objs			+= catalyst.o
>  spi-nor-objs			+= eon.o
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 4a315cb1c4db..2eaf4ba8c0f3 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -3707,6 +3707,10 @@ static int spi_nor_probe(struct spi_mem *spimem)
>  	if (ret)
>  		return ret;
>  
> +	ret = spi_nor_sysfs_create(nor);
> +	if (ret)
> +		return ret;
> +
>  	return mtd_device_register(&nor->mtd, data ? data->parts : NULL,
>  				   data ? data->nr_parts : 0);
>  }
> @@ -3716,6 +3720,7 @@ static int spi_nor_remove(struct spi_mem *spimem)
>  	struct spi_nor *nor = spi_mem_get_drvdata(spimem);
>  
>  	spi_nor_restore(nor);
> +	spi_nor_sysfs_remove(nor);
>  
>  	/* Clean up MTD stuff. */
>  	return mtd_device_unregister(&nor->mtd);
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index 668f22011b1d..dd592f7b62d1 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -488,4 +488,7 @@ static struct spi_nor __maybe_unused *mtd_to_spi_nor(struct mtd_info *mtd)
>  	return mtd->priv;
>  }
>  
> +int spi_nor_sysfs_create(struct spi_nor *nor);
> +void spi_nor_sysfs_remove(struct spi_nor *nor);
> +
>  #endif /* __LINUX_MTD_SPI_NOR_INTERNAL_H */
> diff --git a/drivers/mtd/spi-nor/sysfs.c b/drivers/mtd/spi-nor/sysfs.c
> new file mode 100644
> index 000000000000..0de031e246c5
> --- /dev/null
> +++ b/drivers/mtd/spi-nor/sysfs.c
> @@ -0,0 +1,86 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/mtd/spi-nor.h>
> +#include <linux/spi/spi.h>
> +#include <linux/spi/spi-mem.h>
> +#include <linux/sysfs.h>
> +
> +#include "core.h"
> +
> +static ssize_t name_show(struct device *dev,
> +			 struct device_attribute *attr, char *buf)
> +{
> +	struct spi_device *spi = to_spi_device(dev);
> +	struct spi_mem *spimem = spi_get_drvdata(spi);
> +	struct spi_nor *nor = spi_mem_get_drvdata(spimem);
> +
> +	return sprintf(buf, "%s\n", nor->info->name);
> +}
> +static DEVICE_ATTR_RO(name);
> +
> +static ssize_t jedec_id_show(struct device *dev,
> +			     struct device_attribute *attr, char *buf)
> +{
> +	struct spi_device *spi = to_spi_device(dev);
> +	struct spi_mem *spimem = spi_get_drvdata(spi);
> +	struct spi_nor *nor = spi_mem_get_drvdata(spimem);
> +
> +	return sprintf(buf, "%*phN\n", nor->info->id_len, nor->info->id);
> +}
> +static DEVICE_ATTR_RO(jedec_id);
> +
> +static struct attribute *spi_nor_sysfs_entries[] = {
> +	&dev_attr_name.attr,
> +	&dev_attr_jedec_id.attr,
> +	NULL
> +};
> +
> +static ssize_t sfdp_read(struct file *filp, struct kobject *kobj,
> +			 struct bin_attribute *bin_attr, char *buf,
> +			 loff_t off, size_t count)
> +{
> +	struct spi_device *spi = to_spi_device(kobj_to_dev(kobj));
> +	struct spi_mem *spimem = spi_get_drvdata(spi);
> +	struct spi_nor *nor = spi_mem_get_drvdata(spimem);
> +	struct sfdp *sfdp = nor->sfdp;
> +	size_t sfdp_size = sfdp->num_dwords * sizeof(*sfdp->dwords);

Is a NULL check needed here? If the flash does not have a SFDP table 
nor->sfdp will be NULL.

> +
> +	return memory_read_from_buffer(buf, count, &off, nor->sfdp->dwords,
> +				       sfdp_size);
> +}
> +static BIN_ATTR_RO(sfdp, PAGE_SIZE);
> +
> +static struct bin_attribute *spi_nor_sysfs_bin_entries[] = {
> +	&bin_attr_sfdp,
> +	NULL
> +};
> +
> +static umode_t spi_nor_sysfs_is_bin_visible(struct kobject *kobj,
> +					    struct bin_attribute *attr, int n)
> +{
> +	struct spi_device *spi = to_spi_device(kobj_to_dev(kobj));
> +	struct spi_mem *spimem = spi_get_drvdata(spi);
> +	struct spi_nor *nor = spi_mem_get_drvdata(spimem);
> +
> +	if (attr == &bin_attr_sfdp && nor->sfdp)
> +		return 0444;
> +
> +	return 0;
> +}
> +
> +static struct attribute_group spi_nor_sysfs_attr_group = {
> +	.name		= NULL,
> +	.is_bin_visible	= spi_nor_sysfs_is_bin_visible,
> +	.attrs		= spi_nor_sysfs_entries,
> +	.bin_attrs	= spi_nor_sysfs_bin_entries,
> +};
> +
> +int spi_nor_sysfs_create(struct spi_nor *nor)
> +{
> +	return sysfs_create_group(&nor->dev->kobj, &spi_nor_sysfs_attr_group);
> +}
> +
> +void spi_nor_sysfs_remove(struct spi_nor *nor)
> +{
> +	sysfs_remove_group(&nor->dev->kobj, &spi_nor_sysfs_attr_group);
> +}
> -- 
> 2.20.1
> 

Rest of it looks good to me.

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

Re: [PATCH 2/2] mtd: spi-nor: add initial sysfs support

[Michael Walle]

Hi Pratyush,

Am 2021-03-22 15:43, schrieb Pratyush Yadav:
> I have not worked with sysfs much so only giving this patch a quick
> overview.
> 
> On 18/03/21 10:24AM, Michael Walle wrote:
>> Add support to show the name and JEDEC identifier as well as to dump 
>> the
>> SFDP table. Not all flashes list their SFDP table contents in their
>> datasheet. So having that is useful. It might also be helpful in bug
>> reports from users.
>> 
>> The idea behind the sysfs module is also to have raw access to the SPI
>> NOR flash device registers, which can also be useful for debugging.
> 
> Your current patch does not add this feature. Why do you think it is
> important enough to mention it?

It was part of my first (not submitted) version. I.e. you could also
read the status register(s). Comes in handy for debugging the locking
code, for example.

I can also remove that paragraph ;)

>> Signed-off-by: Michael Walle <michael@walle.cc>
>> ---
>>  drivers/mtd/spi-nor/Makefile |  2 +-
>>  drivers/mtd/spi-nor/core.c   |  5 +++
>>  drivers/mtd/spi-nor/core.h   |  3 ++
>>  drivers/mtd/spi-nor/sysfs.c  | 86 
>> ++++++++++++++++++++++++++++++++++++
>>  4 files changed, 95 insertions(+), 1 deletion(-)
>>  create mode 100644 drivers/mtd/spi-nor/sysfs.c
>> 
>> diff --git a/drivers/mtd/spi-nor/Makefile 
>> b/drivers/mtd/spi-nor/Makefile
>> index 653923896205..aff308f75987 100644
>> --- a/drivers/mtd/spi-nor/Makefile
>> +++ b/drivers/mtd/spi-nor/Makefile
>> @@ -1,6 +1,6 @@
>>  # SPDX-License-Identifier: GPL-2.0
>> 
>> -spi-nor-objs			:= core.o sfdp.o
>> +spi-nor-objs			:= core.o sfdp.o sysfs.o
>>  spi-nor-objs			+= atmel.o
>>  spi-nor-objs			+= catalyst.o
>>  spi-nor-objs			+= eon.o
>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>> index 4a315cb1c4db..2eaf4ba8c0f3 100644
>> --- a/drivers/mtd/spi-nor/core.c
>> +++ b/drivers/mtd/spi-nor/core.c
>> @@ -3707,6 +3707,10 @@ static int spi_nor_probe(struct spi_mem 
>> *spimem)
>>  	if (ret)
>>  		return ret;
>> 
>> +	ret = spi_nor_sysfs_create(nor);
>> +	if (ret)
>> +		return ret;
>> +
>>  	return mtd_device_register(&nor->mtd, data ? data->parts : NULL,
>>  				   data ? data->nr_parts : 0);
>>  }
>> @@ -3716,6 +3720,7 @@ static int spi_nor_remove(struct spi_mem 
>> *spimem)
>>  	struct spi_nor *nor = spi_mem_get_drvdata(spimem);
>> 
>>  	spi_nor_restore(nor);
>> +	spi_nor_sysfs_remove(nor);
>> 
>>  	/* Clean up MTD stuff. */
>>  	return mtd_device_unregister(&nor->mtd);
>> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
>> index 668f22011b1d..dd592f7b62d1 100644
>> --- a/drivers/mtd/spi-nor/core.h
>> +++ b/drivers/mtd/spi-nor/core.h
>> @@ -488,4 +488,7 @@ static struct spi_nor __maybe_unused 
>> *mtd_to_spi_nor(struct mtd_info *mtd)
>>  	return mtd->priv;
>>  }
>> 
>> +int spi_nor_sysfs_create(struct spi_nor *nor);
>> +void spi_nor_sysfs_remove(struct spi_nor *nor);
>> +
>>  #endif /* __LINUX_MTD_SPI_NOR_INTERNAL_H */
>> diff --git a/drivers/mtd/spi-nor/sysfs.c b/drivers/mtd/spi-nor/sysfs.c
>> new file mode 100644
>> index 000000000000..0de031e246c5
>> --- /dev/null
>> +++ b/drivers/mtd/spi-nor/sysfs.c
>> @@ -0,0 +1,86 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +#include <linux/mtd/spi-nor.h>
>> +#include <linux/spi/spi.h>
>> +#include <linux/spi/spi-mem.h>
>> +#include <linux/sysfs.h>
>> +
>> +#include "core.h"
>> +
>> +static ssize_t name_show(struct device *dev,
>> +			 struct device_attribute *attr, char *buf)
>> +{
>> +	struct spi_device *spi = to_spi_device(dev);
>> +	struct spi_mem *spimem = spi_get_drvdata(spi);
>> +	struct spi_nor *nor = spi_mem_get_drvdata(spimem);
>> +
>> +	return sprintf(buf, "%s\n", nor->info->name);
>> +}
>> +static DEVICE_ATTR_RO(name);
>> +
>> +static ssize_t jedec_id_show(struct device *dev,
>> +			     struct device_attribute *attr, char *buf)
>> +{
>> +	struct spi_device *spi = to_spi_device(dev);
>> +	struct spi_mem *spimem = spi_get_drvdata(spi);
>> +	struct spi_nor *nor = spi_mem_get_drvdata(spimem);
>> +
>> +	return sprintf(buf, "%*phN\n", nor->info->id_len, nor->info->id);
>> +}
>> +static DEVICE_ATTR_RO(jedec_id);
>> +
>> +static struct attribute *spi_nor_sysfs_entries[] = {
>> +	&dev_attr_name.attr,
>> +	&dev_attr_jedec_id.attr,
>> +	NULL
>> +};
>> +
>> +static ssize_t sfdp_read(struct file *filp, struct kobject *kobj,
>> +			 struct bin_attribute *bin_attr, char *buf,
>> +			 loff_t off, size_t count)
>> +{
>> +	struct spi_device *spi = to_spi_device(kobj_to_dev(kobj));
>> +	struct spi_mem *spimem = spi_get_drvdata(spi);
>> +	struct spi_nor *nor = spi_mem_get_drvdata(spimem);
>> +	struct sfdp *sfdp = nor->sfdp;
>> +	size_t sfdp_size = sfdp->num_dwords * sizeof(*sfdp->dwords);
> 
> Is a NULL check needed here? If the flash does not have a SFDP table
> nor->sfdp will be NULL.

That can't happen because..

>> +
>> +	return memory_read_from_buffer(buf, count, &off, nor->sfdp->dwords,
>> +				       sfdp_size);
>> +}
>> +static BIN_ATTR_RO(sfdp, PAGE_SIZE);
>> +
>> +static struct bin_attribute *spi_nor_sysfs_bin_entries[] = {
>> +	&bin_attr_sfdp,
>> +	NULL
>> +};
>> +
>> +static umode_t spi_nor_sysfs_is_bin_visible(struct kobject *kobj,
>> +					    struct bin_attribute *attr, int n)
>> +{
>> +	struct spi_device *spi = to_spi_device(kobj_to_dev(kobj));
>> +	struct spi_mem *spimem = spi_get_drvdata(spi);
>> +	struct spi_nor *nor = spi_mem_get_drvdata(spimem);
>> +
>> +	if (attr == &bin_attr_sfdp && nor->sfdp)
>> +		return 0444;

.. it is not visible. Maybe I should use

	if (attr == &bin_attr_sfdp && !nor->sfdp)
		return 0;

	return 0444;

>> +
>> +	return 0;
>> +}
>> +
>> +static struct attribute_group spi_nor_sysfs_attr_group = {
>> +	.name		= NULL,
>> +	.is_bin_visible	= spi_nor_sysfs_is_bin_visible,
>> +	.attrs		= spi_nor_sysfs_entries,
>> +	.bin_attrs	= spi_nor_sysfs_bin_entries,
>> +};
>> +
>> +int spi_nor_sysfs_create(struct spi_nor *nor)
>> +{
>> +	return sysfs_create_group(&nor->dev->kobj, 
>> &spi_nor_sysfs_attr_group);
>> +}
>> +
>> +void spi_nor_sysfs_remove(struct spi_nor *nor)
>> +{
>> +	sysfs_remove_group(&nor->dev->kobj, &spi_nor_sysfs_attr_group);
>> +}
>> --
>> 2.20.1
>> 
> 
> Rest of it looks good to me.

Thanks for reviewing!

-michael

Re: [PATCH 2/2] mtd: spi-nor: add initial sysfs support

[Vignesh Raghavendra]

Hi,

On 3/18/21 2:54 PM, Michael Walle wrote:
> Add support to show the name and JEDEC identifier as well as to dump the
> SFDP table. Not all flashes list their SFDP table contents in their
> datasheet. So having that is useful. It might also be helpful in bug
> reports from users.
> 

Sorry for the delay..

There is already debugfs support for dumping JEDEC ID [1]. Any reason to
add sysfs entry as well?

That brings up another question. Since SFDP dumps are more of a debug
aid, should this be a debugfs entry rather than sysfs entry?

Note that sysfs entries are userspace ABIs just like syscalls and thus
need to be documented in Documentation/ABI/testing/ or
Documentation/ABI/stable. Thus need to be carefully designed compared to
debugfs which are much more flexible.

[1]drivers/mtd/spi-nor/core.c 3380

Regards
Vignesh

> The idea behind the sysfs module is also to have raw access to the SPI
> NOR flash device registers, which can also be useful for debugging.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>  drivers/mtd/spi-nor/Makefile |  2 +-
>  drivers/mtd/spi-nor/core.c   |  5 +++
>  drivers/mtd/spi-nor/core.h   |  3 ++
>  drivers/mtd/spi-nor/sysfs.c  | 86 ++++++++++++++++++++++++++++++++++++
>  4 files changed, 95 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/mtd/spi-nor/sysfs.c
> 
> diff --git a/drivers/mtd/spi-nor/Makefile b/drivers/mtd/spi-nor/Makefile
> index 653923896205..aff308f75987 100644
> --- a/drivers/mtd/spi-nor/Makefile
> +++ b/drivers/mtd/spi-nor/Makefile
> @@ -1,6 +1,6 @@
>  # SPDX-License-Identifier: GPL-2.0
>  
> -spi-nor-objs			:= core.o sfdp.o
> +spi-nor-objs			:= core.o sfdp.o sysfs.o
>  spi-nor-objs			+= atmel.o
>  spi-nor-objs			+= catalyst.o
>  spi-nor-objs			+= eon.o
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 4a315cb1c4db..2eaf4ba8c0f3 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -3707,6 +3707,10 @@ static int spi_nor_probe(struct spi_mem *spimem)
>  	if (ret)
>  		return ret;
>  
> +	ret = spi_nor_sysfs_create(nor);
> +	if (ret)
> +		return ret;
> +
>  	return mtd_device_register(&nor->mtd, data ? data->parts : NULL,
>  				   data ? data->nr_parts : 0);
>  }
> @@ -3716,6 +3720,7 @@ static int spi_nor_remove(struct spi_mem *spimem)
>  	struct spi_nor *nor = spi_mem_get_drvdata(spimem);
>  
>  	spi_nor_restore(nor);
> +	spi_nor_sysfs_remove(nor);
>  
>  	/* Clean up MTD stuff. */
>  	return mtd_device_unregister(&nor->mtd);
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index 668f22011b1d..dd592f7b62d1 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -488,4 +488,7 @@ static struct spi_nor __maybe_unused *mtd_to_spi_nor(struct mtd_info *mtd)
>  	return mtd->priv;
>  }
>  
> +int spi_nor_sysfs_create(struct spi_nor *nor);
> +void spi_nor_sysfs_remove(struct spi_nor *nor);
> +
>  #endif /* __LINUX_MTD_SPI_NOR_INTERNAL_H */
> diff --git a/drivers/mtd/spi-nor/sysfs.c b/drivers/mtd/spi-nor/sysfs.c
> new file mode 100644
> index 000000000000..0de031e246c5
> --- /dev/null
> +++ b/drivers/mtd/spi-nor/sysfs.c
> @@ -0,0 +1,86 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/mtd/spi-nor.h>
> +#include <linux/spi/spi.h>
> +#include <linux/spi/spi-mem.h>
> +#include <linux/sysfs.h>
> +
> +#include "core.h"
> +
> +static ssize_t name_show(struct device *dev,
> +			 struct device_attribute *attr, char *buf)
> +{
> +	struct spi_device *spi = to_spi_device(dev);
> +	struct spi_mem *spimem = spi_get_drvdata(spi);
> +	struct spi_nor *nor = spi_mem_get_drvdata(spimem);
> +
> +	return sprintf(buf, "%s\n", nor->info->name);
> +}
> +static DEVICE_ATTR_RO(name);
> +
> +static ssize_t jedec_id_show(struct device *dev,
> +			     struct device_attribute *attr, char *buf)
> +{
> +	struct spi_device *spi = to_spi_device(dev);
> +	struct spi_mem *spimem = spi_get_drvdata(spi);
> +	struct spi_nor *nor = spi_mem_get_drvdata(spimem);
> +
> +	return sprintf(buf, "%*phN\n", nor->info->id_len, nor->info->id);
> +}
> +static DEVICE_ATTR_RO(jedec_id);
> +
> +static struct attribute *spi_nor_sysfs_entries[] = {
> +	&dev_attr_name.attr,
> +	&dev_attr_jedec_id.attr,
> +	NULL
> +};
> +
> +static ssize_t sfdp_read(struct file *filp, struct kobject *kobj,
> +			 struct bin_attribute *bin_attr, char *buf,
> +			 loff_t off, size_t count)
> +{
> +	struct spi_device *spi = to_spi_device(kobj_to_dev(kobj));
> +	struct spi_mem *spimem = spi_get_drvdata(spi);
> +	struct spi_nor *nor = spi_mem_get_drvdata(spimem);
> +	struct sfdp *sfdp = nor->sfdp;
> +	size_t sfdp_size = sfdp->num_dwords * sizeof(*sfdp->dwords);
> +
> +	return memory_read_from_buffer(buf, count, &off, nor->sfdp->dwords,
> +				       sfdp_size);
> +}
> +static BIN_ATTR_RO(sfdp, PAGE_SIZE);
> +
> +static struct bin_attribute *spi_nor_sysfs_bin_entries[] = {
> +	&bin_attr_sfdp,
> +	NULL
> +};
> +
> +static umode_t spi_nor_sysfs_is_bin_visible(struct kobject *kobj,
> +					    struct bin_attribute *attr, int n)
> +{
> +	struct spi_device *spi = to_spi_device(kobj_to_dev(kobj));
> +	struct spi_mem *spimem = spi_get_drvdata(spi);
> +	struct spi_nor *nor = spi_mem_get_drvdata(spimem);
> +
> +	if (attr == &bin_attr_sfdp && nor->sfdp)
> +		return 0444;
> +
> +	return 0;
> +}
> +
> +static struct attribute_group spi_nor_sysfs_attr_group = {
> +	.name		= NULL,
> +	.is_bin_visible	= spi_nor_sysfs_is_bin_visible,
> +	.attrs		= spi_nor_sysfs_entries,
> +	.bin_attrs	= spi_nor_sysfs_bin_entries,
> +};
> +
> +int spi_nor_sysfs_create(struct spi_nor *nor)
> +{
> +	return sysfs_create_group(&nor->dev->kobj, &spi_nor_sysfs_attr_group);
> +}
> +
> +void spi_nor_sysfs_remove(struct spi_nor *nor)
> +{
> +	sysfs_remove_group(&nor->dev->kobj, &spi_nor_sysfs_attr_group);
> +}
> 

Re: [PATCH 2/2] mtd: spi-nor: add initial sysfs support

[Michael Walle]

Hi,

Am 2021-04-06 09:56, schrieb Vignesh Raghavendra:
> Hi,
> 
> On 3/18/21 2:54 PM, Michael Walle wrote:
>> Add support to show the name and JEDEC identifier as well as to dump 
>> the
>> SFDP table. Not all flashes list their SFDP table contents in their
>> datasheet. So having that is useful. It might also be helpful in bug
>> reports from users.
>> 
> 
> Sorry for the delay..
> 
> There is already debugfs support for dumping JEDEC ID [1]. Any reason 
> to
> add sysfs entry as well?

This is per mtd while the sfdp is per flash device. IMHO both should
be at the same place.

> That brings up another question. Since SFDP dumps are more of a debug
> aid, should this be a debugfs entry rather than sysfs entry?

And you're not the first one asking that. My argument was that the
debugfs might not be available just when you need it. A developer
could easily rebuild a kernel, but imagine some user with a COTS
distro and some problems, then it is not that easy anymore. But
thats your call to make.

> Note that sysfs entries are userspace ABIs just like syscalls and thus
> need to be documented in Documentation/ABI/testing/ or
> Documentation/ABI/stable. Thus need to be carefully designed compared 
> to
> debugfs which are much more flexible.

Ok. But I don't see a problem adding these read-only files
  /sfdp
  /name
  /jedec-id

Do you?

-michael

Re: [PATCH 2/2] mtd: spi-nor: add initial sysfs support

[Vignesh Raghavendra]

On 4/6/21 2:17 PM, Michael Walle wrote:
> Hi,
> 
> Am 2021-04-06 09:56, schrieb Vignesh Raghavendra:
>> Hi,
>>
>> On 3/18/21 2:54 PM, Michael Walle wrote:
>>> Add support to show the name and JEDEC identifier as well as to dump the
>>> SFDP table. Not all flashes list their SFDP table contents in their
>>> datasheet. So having that is useful. It might also be helpful in bug
>>> reports from users.
>>>
>>
>> Sorry for the delay..
>>
>> There is already debugfs support for dumping JEDEC ID [1]. Any reason to
>> add sysfs entry as well?
> 
> This is per mtd while the sfdp is per flash device. IMHO both should
> be at the same place.
> 
>> That brings up another question. Since SFDP dumps are more of a debug
>> aid, should this be a debugfs entry rather than sysfs entry?
> 
> And you're not the first one asking that. My argument was that the
> debugfs might not be available just when you need it. A developer
> could easily rebuild a kernel, but imagine some user with a COTS
> distro and some problems, then it is not that easy anymore. But
> thats your call to make.
> 
>> Note that sysfs entries are userspace ABIs just like syscalls and thus
>> need to be documented in Documentation/ABI/testing/ or
>> Documentation/ABI/stable. Thus need to be carefully designed compared to
>> debugfs which are much more flexible.
> 
> Ok. But I don't see a problem adding these read-only files
>  /sfdp
>  /name
>  /jedec-id
> 

Hmm, ok. but do add documentation please.

Regards
Vignesh

Re: [PATCH 2/2] mtd: spi-nor: add initial sysfs support

[Alexander Williams]

Hi Michael,

> On Thu, Mar 18, 2021 at 10:24 AM Michael Walle <michael@walle.cc>
> wrote:
> Add support to show the name and JEDEC identifier as well as to dump the
> SFDP table. Not all flashes list their SFDP table contents in their
> datasheet. So having that is useful. It might also be helpful in bug
> reports from users.
> 
> The idea behind the sysfs module is also to have raw access to the SPI
> NOR flash device registers, which can also be useful for debugging.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>  drivers/mtd/spi-nor/Makefile |  2 +-
>  drivers/mtd/spi-nor/core.c   |  5 +++
>  drivers/mtd/spi-nor/core.h   |  3 ++
>  drivers/mtd/spi-nor/sysfs.c  | 86 ++++++++++++++++++++++++++++++++++++
>  4 files changed, 95 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/mtd/spi-nor/sysfs.c
> 
> diff --git a/drivers/mtd/spi-nor/Makefile b/drivers/mtd/spi-nor/Makefile
> index 653923896205..aff308f75987 100644
> --- a/drivers/mtd/spi-nor/Makefile
> +++ b/drivers/mtd/spi-nor/Makefile
> @@ -1,6 +1,6 @@
>  # SPDX-License-Identifier: GPL-2.0
>  
> -spi-nor-objs			:= core.o sfdp.o
> +spi-nor-objs			:= core.o sfdp.o sysfs.o
>  spi-nor-objs			+= atmel.o
>  spi-nor-objs			+= catalyst.o
>  spi-nor-objs			+= eon.o
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 4a315cb1c4db..2eaf4ba8c0f3 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -3707,6 +3707,10 @@ static int spi_nor_probe(struct spi_mem *spimem)
>  	if (ret)
>  		return ret;
>  
> +	ret = spi_nor_sysfs_create(nor);

This appears to be racing with user space. By the time we reach probe(), the
device embedded in the spi_device has already been registered, with the uevent
sent out, right? udev may not see the new attributes created here.

Since we reuse a preexisting device throughout spi-nor, it seems awfully
challenging to be able to safely add sysfs attributes. Would it make sense to
create a spi-nor-specific device/class? Or perhaps attach these attributes to
the device in mtd_info like I've done in
https://lore.kernel.org/linux-mtd/20210428052725.530939-1-awill@google.com/ ?

- Alex

> +	if (ret)
> +		return ret;
> +
>  	return mtd_device_register(&nor->mtd, data ? data->parts : NULL,
>  				   data ? data->nr_parts : 0);
>  }
> @@ -3716,6 +3720,7 @@ static int spi_nor_remove(struct spi_mem *spimem)
>  	struct spi_nor *nor = spi_mem_get_drvdata(spimem);
>  
>  	spi_nor_restore(nor);
> +	spi_nor_sysfs_remove(nor);
>  
>  	/* Clean up MTD stuff. */
>  	return mtd_device_unregister(&nor->mtd);
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index 668f22011b1d..dd592f7b62d1 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -488,4 +488,7 @@ static struct spi_nor __maybe_unused *mtd_to_spi_nor(struct mtd_info *mtd)
>  	return mtd->priv;
>  }
>  
> +int spi_nor_sysfs_create(struct spi_nor *nor);
> +void spi_nor_sysfs_remove(struct spi_nor *nor);
> +
>  #endif /* __LINUX_MTD_SPI_NOR_INTERNAL_H */
> diff --git a/drivers/mtd/spi-nor/sysfs.c b/drivers/mtd/spi-nor/sysfs.c
> new file mode 100644
> index 000000000000..0de031e246c5
> --- /dev/null
> +++ b/drivers/mtd/spi-nor/sysfs.c
> @@ -0,0 +1,86 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/mtd/spi-nor.h>
> +#include <linux/spi/spi.h>
> +#include <linux/spi/spi-mem.h>
> +#include <linux/sysfs.h>
> +
> +#include "core.h"
> +
> +static ssize_t name_show(struct device *dev,
> +			 struct device_attribute *attr, char *buf)
> +{
> +	struct spi_device *spi = to_spi_device(dev);
> +	struct spi_mem *spimem = spi_get_drvdata(spi);
> +	struct spi_nor *nor = spi_mem_get_drvdata(spimem);
> +
> +	return sprintf(buf, "%s\n", nor->info->name);
> +}
> +static DEVICE_ATTR_RO(name);
> +
> +static ssize_t jedec_id_show(struct device *dev,
> +			     struct device_attribute *attr, char *buf)
> +{
> +	struct spi_device *spi = to_spi_device(dev);
> +	struct spi_mem *spimem = spi_get_drvdata(spi);
> +	struct spi_nor *nor = spi_mem_get_drvdata(spimem);
> +
> +	return sprintf(buf, "%*phN\n", nor->info->id_len, nor->info->id);
> +}
> +static DEVICE_ATTR_RO(jedec_id);
> +
> +static struct attribute *spi_nor_sysfs_entries[] = {
> +	&dev_attr_name.attr,
> +	&dev_attr_jedec_id.attr,
> +	NULL
> +};
> +
> +static ssize_t sfdp_read(struct file *filp, struct kobject *kobj,
> +			 struct bin_attribute *bin_attr, char *buf,
> +			 loff_t off, size_t count)
> +{
> +	struct spi_device *spi = to_spi_device(kobj_to_dev(kobj));
> +	struct spi_mem *spimem = spi_get_drvdata(spi);
> +	struct spi_nor *nor = spi_mem_get_drvdata(spimem);
> +	struct sfdp *sfdp = nor->sfdp;
> +	size_t sfdp_size = sfdp->num_dwords * sizeof(*sfdp->dwords);
> +
> +	return memory_read_from_buffer(buf, count, &off, nor->sfdp->dwords,
> +				       sfdp_size);
> +}
> +static BIN_ATTR_RO(sfdp, PAGE_SIZE);
> +
> +static struct bin_attribute *spi_nor_sysfs_bin_entries[] = {
> +	&bin_attr_sfdp,
> +	NULL
> +};
> +
> +static umode_t spi_nor_sysfs_is_bin_visible(struct kobject *kobj,
> +					    struct bin_attribute *attr, int n)
> +{
> +	struct spi_device *spi = to_spi_device(kobj_to_dev(kobj));
> +	struct spi_mem *spimem = spi_get_drvdata(spi);
> +	struct spi_nor *nor = spi_mem_get_drvdata(spimem);
> +
> +	if (attr == &bin_attr_sfdp && nor->sfdp)
> +		return 0444;
> +
> +	return 0;
> +}
> +
> +static struct attribute_group spi_nor_sysfs_attr_group = {
> +	.name		= NULL,
> +	.is_bin_visible	= spi_nor_sysfs_is_bin_visible,
> +	.attrs		= spi_nor_sysfs_entries,
> +	.bin_attrs	= spi_nor_sysfs_bin_entries,
> +};
> +
> +int spi_nor_sysfs_create(struct spi_nor *nor)
> +{
> +	return sysfs_create_group(&nor->dev->kobj, &spi_nor_sysfs_attr_group);
> +}
> +
> +void spi_nor_sysfs_remove(struct spi_nor *nor)
> +{
> +	sysfs_remove_group(&nor->dev->kobj, &spi_nor_sysfs_attr_group);
> +}
> -- 
> 2.20.1

Re: [PATCH 2/2] mtd: spi-nor: add initial sysfs support

[Michael Walle]

Hi Alex,

Am 2021-04-29 17:37, schrieb Alexander Williams:
> Hi Michael,
> 
>> On Thu, Mar 18, 2021 at 10:24 AM Michael Walle <michael@walle.cc>
>> wrote:
>> Add support to show the name and JEDEC identifier as well as to dump 
>> the
>> SFDP table. Not all flashes list their SFDP table contents in their
>> datasheet. So having that is useful. It might also be helpful in bug
>> reports from users.
>> 
>> The idea behind the sysfs module is also to have raw access to the SPI
>> NOR flash device registers, which can also be useful for debugging.
>> 
>> Signed-off-by: Michael Walle <michael@walle.cc>
>> ---
>>  drivers/mtd/spi-nor/Makefile |  2 +-
>>  drivers/mtd/spi-nor/core.c   |  5 +++
>>  drivers/mtd/spi-nor/core.h   |  3 ++
>>  drivers/mtd/spi-nor/sysfs.c  | 86 
>> ++++++++++++++++++++++++++++++++++++
>>  4 files changed, 95 insertions(+), 1 deletion(-)
>>  create mode 100644 drivers/mtd/spi-nor/sysfs.c
>> 
>> diff --git a/drivers/mtd/spi-nor/Makefile 
>> b/drivers/mtd/spi-nor/Makefile
>> index 653923896205..aff308f75987 100644
>> --- a/drivers/mtd/spi-nor/Makefile
>> +++ b/drivers/mtd/spi-nor/Makefile
>> @@ -1,6 +1,6 @@
>>  # SPDX-License-Identifier: GPL-2.0
>> 
>> -spi-nor-objs			:= core.o sfdp.o
>> +spi-nor-objs			:= core.o sfdp.o sysfs.o
>>  spi-nor-objs			+= atmel.o
>>  spi-nor-objs			+= catalyst.o
>>  spi-nor-objs			+= eon.o
>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>> index 4a315cb1c4db..2eaf4ba8c0f3 100644
>> --- a/drivers/mtd/spi-nor/core.c
>> +++ b/drivers/mtd/spi-nor/core.c
>> @@ -3707,6 +3707,10 @@ static int spi_nor_probe(struct spi_mem 
>> *spimem)
>>  	if (ret)
>>  		return ret;
>> 
>> +	ret = spi_nor_sysfs_create(nor);
> 
> This appears to be racing with user space. By the time we reach 
> probe(), the
> device embedded in the spi_device has already been registered, with the 
> uevent
> sent out, right? udev may not see the new attributes created here.
> 
> Since we reuse a preexisting device throughout spi-nor, it seems 
> awfully
> challenging to be able to safely add sysfs attributes. Would it make 
> sense to
> create a spi-nor-specific device/class? Or perhaps attach these 
> attributes to
> the device in mtd_info like I've done in
> https://lore.kernel.org/linux-mtd/20210428052725.530939-1-awill@google.com/ 
> ?

Do you encounter this problem? I'm currently working on this and dropped
the sysfs_create() and use dev_groups of the driver spi nor driver.

But I'm not sure how it will be handled anyway. Because we know the
content/size SFDP only after the probe and in any case the probe could
also fail. So I don't really understand how that is handled.

I've looked at your patch and it seems that the surpress_uevent() is
rarely used in the kernel.

I don't want to attach it to an MTD device because you might have
multiple ones which has the same SPI flash device as parent. The
SFDP is really a property of the flash device and not one of the
MTD partition.

-michael