Read MAC address from SST vendor specific SFDP region
Subsystems (8)
v9 cover Read MAC address from SST vendor specific SFDP region
Some Microchip/SST QSPI flashes (e.g. the SST26VF064BEUI) are factory programmed with globally unique, write-protected EUI-48 and EUI-64 identifiers stored in a vendor-specific SFDP parameter table. On boards that have no on-board EEPROM (sama5d27_wlsom1, sama5d29 curiosity, sam9x75 curiosity) this is a reliable source for an Ethernet MAC address, instead of relying on a U-Boot-provided or random address.
The series exposes the SFDP as a generic NVMEM framework with no vendor code in the SPI NOR core:
- The SPI NOR core exposes the entire SFDP as a generic read-only NVMEM device, rooted at a new "sfdp" child node of the flash.
- A new NVMEM layout driver (drivers/nvmem/layouts/) locates the Microchip vendor parameter table at runtime and presents the EUI-48 as a "mac-address" cell.
- Arbitrary parameters can be read with a standard fixed-layout (known offset) or with an nvmem-layout parser (location discovered at runtime).
Changes in v9:
- 3/8 - Add Reviewed-by: Miquel Raynal, Reviewed-by: Michael Walle.
- 4/8 - Add Reviewed-by: Miquel Raynal, Reviewed-by: Michael Walle.
Drop the early return after of_get_compatible_child(). Updated the
comments and commit message to match.
- 5/8 - Move the Makefile entry for sst26vf-sfdp-eui.o to the end.
Move the ETH_ALEN check to the top of sfdp_eui_read_post_process()
and bail out early.
Compute nph as hdr.nph+1 and loop for the 0-based parameter-header count.
Cosmetic fixes.
Changes in v8:
- New 3/8 - nvmem: core: take a reference on dev.of_node in
nvmem_register(). v7's 3/8 (now 4/8) called of_node_put() right after devm_nvmem_register(), which left nvmem->dev.of_node backed by no reference at all once the caller's own reference was dropped. Fixing it at the nvmem_register() by taking its own of_node_get() when assigning nvmem->dev.of_node and released in nvmem_release().
- 6/8 - drop the #address-cells/#size-cells from the qspi1_flash node.
Changes in v7:
- 1/7 - Add blank lines in the "sfdp" schema per review;
Add Reviewed-by: Rob Herring.
- 2/7 - Add Reviewed-by: Rob Herring.
- 3/7 - Drop the spi_nor_sfdp_nvmem_put_np() helper and
call of_node_put() right after devm_nvmem_register().
- 6/7 - Note in the commit message that the sama5d27_wlsom1 SoM ships the factory-programmed SST26VF064BEUI, so the EUI-48 "mac-address" cell is always present.
Changes in v6:
- 3/7 - Move the SFDP NVMEM registration from sfdp.c into core.c, since sfdp.c is only for parsing the tables;use of_get_compatible_child() instead of the open-coded child-node lookup.
- 2/7, 6/7, 7/7 - Add Reviewed-by: Linus Walleij.
Changes in v5:
- 1/7 and 5/7 - Add R-b tags
- 3/7- Rework the comments, function name and commit message.Check all the nodes (for_each_available_child) for the right compatible "jedec,sfdp" instead of node name.
Changes in v4:
- Rework per v3 review: remove the vendor-specific SFDP handling from the SPI NOR core; expose the whole SFDP as a generic read-only NVMEM device and move the EUI extraction into an nvmem-layout driver.
- Introduce a new nvmem-layout driver to discover the vendor-table location at runtime; no offset hardcoded in the device tree.
- Describe the SFDP via a dedicated "sfdp" subnode (compatible "jedec,sfdp"), which also resolves the v3 dtbs_check "Unevaluated properties ('nvmem-layout')" warning.
- Reverse the stored EUI bytes into canonical MAC order.
- Enable the layout in sama5_defconfig.
Changes in v3:
- 2/3 - add support to update the QSPI partition into 'fixed-partition' binding in sama5d27_wlsom1
- 3/3 - add nvmem-layout in qspi node for EUI48 MAC Address and nvmem cell properties for macb node in sama5d27_wlsom1
Changes in v2:
- 1/3 - parse the SST vendor table, read and store the addresses into a resource - managed space. Register the addresses into NVMEM framework
- 2/3 - add support to update the QSPI partition into 'fixed-partition' binding
v8: https://lore.kernel.org/all/[email protected]/
v7: https://lore.kernel.org/all/[email protected]/
Manikandan Muralidharan (8):
dt-bindings: mtd: jedec,spi-nor: allow the SFDP to be exposed via
NVMEM
dt-bindings: nvmem: layouts: add Microchip/SST SFDP EUI layout
nvmem: core: take a reference on dev.of_node in nvmem_register()
mtd: spi-nor: sfdp: expose the SFDP as a read-only NVMEM device
nvmem: layouts: add Microchip/SST SFDP EUI layout driver
ARM: dts: microchip: sama5d27_wlsom1: use fixed-partitions for QSPI
flash
ARM: dts: microchip: sama5d27_wlsom1: read MAC address from QSPI SFDP
ARM: configs: sama5: enable Microchip/SST SFDP EUI NVMEM layout
.../bindings/mtd/jedec,spi-nor.yaml | 20 ++ .../layouts/microchip,sst26vf-sfdp-eui.yaml | 60 ++++++ .../bindings/nvmem/layouts/nvmem-layout.yaml | 1 + MAINTAINERS | 6 + .../dts/microchip/at91-sama5d27_wlsom1.dtsi | 63 +++--- .../dts/microchip/at91-sama5d27_wlsom1_ek.dts | 2 + arch/arm/configs/sama5_defconfig | 1 + drivers/mtd/spi-nor/core.c | 75 ++++++++ drivers/nvmem/core.c | 5 +- drivers/nvmem/layouts/Kconfig | 10 + drivers/nvmem/layouts/Makefile | 1 + drivers/nvmem/layouts/sst26vf-sfdp-eui.c | 181 ++++++++++++++++++ 12 files changed, 399 insertions(+), 26 deletions(-) create mode 100644 Documentation/devicetree/bindings/nvmem/layouts/microchip,sst26vf-sfdp-eui.yaml create mode 100644 drivers/nvmem/layouts/sst26vf-sfdp-eui.c
base-commit: 2f2f7eeef8b0a1a9b9ca998231419873ccae8d4e --
2.43.0
PATCH v9 1/8 dt-bindings: mtd: jedec,spi-nor: allow the SFDP to be exposed via NVMEM
Add an optional "sfdp" child node (compatible "jedec,sfdp") that describes the SFDP as a read-only NVMEM provider via nvmem.yaml, so its contents (e.g. a vendor EUI-48/EUI-64) can be read through NVMEM cells.
--- .../bindings/mtd/jedec,spi-nor.yaml | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+)
Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml +20 -0
@@ -103,6 +103,22 @@ properties: spi-cpol: true spi-cpha: true + sfdp: + $ref: /schemas/nvmem/nvmem.yaml# + unevaluatedProperties: false + description: + The Serial Flash Discoverable Parameters (SFDP) tables exposed as a + read-only NVMEM device. This allows standard or vendor-specific SFDP + data (for example a factory-programmed EUI-48/EUI-64 identifier) to be + consumed through NVMEM cells. + + properties: + compatible: + const: jedec,sfdp + + required: + - compatible + dependencies: spi-cpol: [ spi-cpha ] spi-cpha: [ spi-cpol ]
@@ -122,6 +138,10 @@ examples: spi-max-frequency = <40000000>; m25p,fast-read; reset-gpios = <&gpio 12 GPIO_ACTIVE_LOW>; + + sfdp { + compatible = "jedec,sfdp"; + }; }; }; ...
Signature · 1 line
2.43.0
PATCH v9 2/8 dt-bindings: nvmem: layouts: add Microchip/SST SFDP EUI layout
In reply to Manikandan MuralidharanAdd a binding for the NVMEM layout that exposes the factory-programmed EUI-48 identifier from the Microchip/SST vendor-specific SFDP parameter table (e.g. SST26VF064BEUI) as a "mac-address" NVMEM cell, and reference it from nvmem-layout.yaml.
--- .../layouts/microchip,sst26vf-sfdp-eui.yaml | 60 +++++++++++++++++++ .../bindings/nvmem/layouts/nvmem-layout.yaml | 1 + 2 files changed, 61 insertions(+) create mode 100644 Documentation/devicetree/bindings/nvmem/layouts/microchip,sst26vf-sfdp-eui.yaml
Documentation/devicetree/bindings/nvmem/layouts/microchip,sst26vf-sfdp-eui.yaml new file +60 -0
@@ -0,0 +1,60 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/nvmem/layouts/microchip,sst26vf-sfdp-eui.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: NVMEM layout of the Microchip/SST SFDP EUI-48 identifier + +maintainers: + - Manikandan Muralidharan <[email protected]> + +description: + Some Microchip/SST serial flashes (for example the SST26VF064BEUI) are + factory programmed with a globally unique EUI-48 identifier stored in a + vendor-specific SFDP parameter table and permanently write-protected. This + layout locates that table and exposes the EUI-48 as an NVMEM cell so that, + for example, a network driver can use it as a MAC address. The location of + the data is discovered at runtime from the SFDP; no offset is encoded in the + device tree. + +select: false + +properties: + compatible: + const: microchip,sst26vf-sfdp-eui + + mac-address: + type: object + description: + The factory-programmed EUI-48 identifier, usable as a MAC address. + additionalProperties: false + +required: + - compatible + +additionalProperties: false + +examples: + - | + spi { + #address-cells = <1>; + #size-cells = <0>; + + flash@0 { + compatible = "jedec,spi-nor"; + reg = <0>; + + sfdp { + compatible = "jedec,sfdp"; + + nvmem-layout { + compatible = "microchip,sst26vf-sfdp-eui"; + + mac-address { + }; + }; + }; + }; + }; +...
Documentation/devicetree/bindings/nvmem/layouts/nvmem-layout.yaml +1 -0
@@ -20,6 +20,7 @@ description: | oneOf: - $ref: fixed-layout.yaml - $ref: kontron,sl28-vpd.yaml + - $ref: microchip,sst26vf-sfdp-eui.yaml - $ref: onie,tlv-layout.yaml - $ref: u-boot,env.yaml
Signature · 1 line
2.43.0
PATCH v9 3/8 nvmem: core: take a reference on dev.of_node in nvmem_register()
In reply to Manikandan Muralidharannvmem_register() stores config->of_node (or config->dev->of_node) into nvmem->dev.of_node without taking its own reference. Callers that get the node via of_get_compatible_child() etc. and drop their reference right after registering leave nvmem->dev.of_node backed by no reference at all.
Take an of_node_get() when assigning nvmem->dev.of_node and release it in nvmem_release(). Existing callers that hold their own get/put pair around the registration call need no changes, since get/put on a device_node is per-holder and independent of core now also holding one.
--- drivers/nvmem/core.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/nvmem/core.c +3 -2
@@ -541,6 +541,7 @@ static void nvmem_release(struct device *dev)ida_free (&nvmem_ida, nvmem->id);gpiod_put (nvmem->wp_gpio); +of_node_put (nvmem->dev.of_node);kfree (nvmem->ops);kfree (nvmem); }
@@ -950,9 +951,9 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config) nvmem->keepout = config->keepout; nvmem->nkeepout = config->nkeepout;if (config->of_node) - nvmem->dev.of_node = config->of_node; + nvmem->dev.of_node =of_node_get (config->of_node);else - nvmem->dev.of_node = config->dev->of_node; + nvmem->dev.of_node =of_node_get (config->dev->of_node);switch (config->id) {case NVMEM_DEVID_NONE :
Signature · 1 line
2.43.0
PATCH v9 4/8 mtd: spi-nor: sfdp: expose the SFDP as a read-only NVMEM device
In reply to Manikandan MuralidharanThe SPI NOR core already reads the SFDP tables during enumeration and caches them in nor->sfdp->dwords (see spi_nor_parse_sfdp()). Re-expose that cached data as a read-only NVMEM device, in on-flash byte order, rooted at the flash's SFDP child node (compatible "jedec,sfdp"), if any.
This lets NVMEM cells reference any SFDP data: a fixed-layout for parameters at a known offset, or an nvmem-layout parser for vendor data whose location must be discovered at runtime.
--- drivers/mtd/spi-nor/core.c | 75 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 75 insertions(+)
drivers/mtd/spi-nor/core.c +75 -0
@@ -17,6 +17,7 @@#include <linux/mtd/mtd.h> #include <linux/mtd/spi-nor.h> #include <linux/mutex.h> +#include <linux/nvmem-provider.h> #include <linux/of.h> #include <linux/regulator/consumer.h> #include <linux/sched/task_stack.h>
@@ -3001,6 +3002,72 @@ static void spi_nor_init_fixup_flags(struct spi_nor *nor) nor->flags |=SNOR_F_IO_MODE_EN_VOLATILE ; } +static int spi_nor_sfdp_reg_read (void *priv,unsigned int offset, +void *val,size_t bytes) +{ +struct spi_nor *nor = priv; +struct sfdp *sfdp = nor->sfdp; +size_t sfdp_size = sfdp->num_dwords *sizeof (*sfdp->dwords); + +if (offset >= sfdp_size || bytes > sfdp_size - offset) +return -EINVAL ; + +/* The cached SFDP is kept in on-flash (little-endian) byte order. */ +memcpy (val, (u8 *)sfdp->dwords + offset, bytes); + +return 0 ; +} + +/** +* spi_nor_register_sfdp_nvmem() - expose the SFDP as a read-only NVMEM device +* @nor: pointer to a 'struct spi_nor' +* +* Expose the whole SFDP, in on-flash byte order, as a read-only NVMEM device +* rooted at the flash's SFDP child node (compatible "jedec,sfdp"), if any. +* This lets generic (fixed-layout) or vendor (nvmem-layout) cells reference +* any SFDP data. +* +* Return: 0 on success or if there is nothing to do, -errno otherwise. +*/ +static int spi_nor_register_sfdp_nvmem (struct spi_nor *nor) +{ +struct device *dev = nor->dev; +struct nvmem_config config = { }; +struct nvmem_device *nvmem; +struct device_node *np; + +if (!nor->sfdp) +return 0 ; + + np =of_get_compatible_child (dev_of_node (dev),"jedec,sfdp" ); + + config.dev = dev; + config.of_node = np; + config.name ="sfdp" ; + config.id =NVMEM_DEVID_AUTO ; + config.owner =THIS_MODULE ; + config.read_only = true; + config.word_size =1 ; + config.stride =1 ; + config.size = (int )(nor->sfdp->num_dwords *sizeof (*nor->sfdp->dwords)); + config.reg_read = spi_nor_sfdp_reg_read; + config.priv = nor; + + nvmem =devm_nvmem_register (dev, &config); +of_node_put (np); +if (IS_ERR (nvmem)) { +/* NVMEM support is optional. */ +if (PTR_ERR (nvmem) == -EOPNOTSUPP ) +return 0 ; +return dev_err_probe (dev,PTR_ERR (nvmem), +"failed to register SFDP NVMEM device\n" ); + } + +dev_dbg (dev,"exposed %d-byte SFDP as an NVMEM device\n" , config.size); + +return 0 ; +} +/** * spi_nor_late_init_params() - Late initialization of default flash parameters. * @nor: pointer to a 'struct spi_nor'
@@ -3204,6 +3271,14 @@ static int spi_nor_init_params(struct spi_nor *nor)spi_nor_init_params_deprecated (nor); } +/* +* Expose the SFDP table as an NVMEM device only when +* the flash actually provides one +*/ + ret =spi_nor_register_sfdp_nvmem (nor); +if (ret) +return ret; + ret =spi_nor_late_init_params (nor);if (ret)return ret;
Signature · 1 line
2.43.0
[email protected]
AI reviewRe: [PATCH v9 4/8] mtd: spi-nor: sfdp: expose the SFDP as a read-only NVMEM device
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] mtd: spi-nor: sfdp: steals parent's nvmem-layout when 'jedec,sfdp' node is missing --
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] mtd: spi-nor: sfdp: steals parent's nvmem-layout when 'jedec,sfdp' node is missing --
commit e4111fea7a3c70b0bb1f5cd153a259e4fdb62c93
Author: Manikandan Muralidharan <[email protected]>
mtd: spi-nor: sfdp: expose the SFDP as a read-only NVMEM device
This commit exposes the cached SFDP table as a read-only NVMEM device, rooted at the flash's SFDP child node if available. This allows generic and vendor NVMEM cells to reference SFDP data.
Quoted 4 lines
drivers/mtd/spi-nor/core.c +0
-0
[ ... ]
Quoted 1 line
drivers/mtd/spi-nor/core.c +0
-0@@ -3001,6 +3002,72 @@ static void spi_nor_init_fixup_flags(struct spi_nor *nor)
[ ... ]
Quoted 4 lines
drivers/mtd/spi-nor/core.c +4
-0+static int spi_nor_register_sfdp_nvmem (struct spi_nor *nor) +{ +struct device *dev = nor->dev; +struct nvmem_config config = { };
drivers/mtd/spi-nor/core.c static void spi_nor_init_fixup_flags(struct spi_nor *nor) · line 3045
Quoted 10 lines
drivers/mtd/spi-nor/core.c +10
-0+struct nvmem_device *nvmem; +struct device_node *np; + +if (!nor->sfdp) +return 0 ; + + np =of_get_compatible_child (dev_of_node (dev),"jedec,sfdp" ); + + config.dev = dev; + config.of_node = np;
[Severity: High]
If the jedec,sfdp child node is missing from the device tree, np will be NULL and assigned to config.of_node. Does this inadvertently trigger the NVMEM core's fallback behavior?
Looking at nvmem_register() in drivers/nvmem/core.c, there is a fallback mechanism for missing nodes:
if (config->of_node)
nvmem->dev.of_node = of_node_get(config->of_node);
else
nvmem->dev.of_node = of_node_get(config->dev->of_node);If config.of_node is NULL, the SFDP NVMEM device will incorrectly inherit the SPI NOR flash's main device node.
The NVMEM core then searches for an nvmem-layout child node and will find the layout intended for the main flash memory, attaching all NVMEM cells to the small SFDP NVMEM device and marking the layout node populated.
Later, when the MTD subsystem registers the primary NVMEM device for the actual flash contents, it will skip the layout node because it is already marked populated.
Could any consumer attempting to read an NVMEM cell then be misdirected to the small SFDP NVMEM device's read callback instead of the main flash, resulting in -EINVAL bounds check errors or corrupted data?
Quoted 3 lines
drivers/mtd/spi-nor/core.c +3
-0+ config.name ="sfdp" ; + config.id =NVMEM_DEVID_AUTO ; + config.owner =THIS_MODULE ;
[ ... ]
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4
Re: [PATCH v9 4/8] mtd: spi-nor: sfdp: expose the SFDP as a read-only NVMEM device
Quoted 61 lines · On 10/17/26 4:10 PM, [email protected] wrote:
EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] mtd: spi-nor: sfdp: steals parent's nvmem-layout when 'jedec,sfdp' node is missing --commit
e4111fea7a3c70b0bb1f5cd153a259e4fdb62c93
Author: Manikandan Muralidharan <[email protected]>mtd: spi-nor: sfdp: expose the SFDP as a read-only NVMEM device
This commit exposes the cached SFDP table as a read-only NVMEM device, rooted at the flash's SFDP child node if available. This allows generic and vendor NVMEM cells to reference SFDP data.
Quoted 4 lines
drivers/mtd/spi-nor/core.c +0
-0[ ... ]
Quoted 1 line
drivers/mtd/spi-nor/core.c +0
-0@@ -3001,6 +3002,72 @@ static void spi_nor_init_fixup_flags(struct spi_nor *nor)[ ... ]
Quoted 4 lines
drivers/mtd/spi-nor/core.c +4
-0+static int spi_nor_register_sfdp_nvmem (struct spi_nor *nor) +{ +struct device *dev = nor->dev; +struct nvmem_config config = { };drivers/mtd/spi-nor/core.c static void spi_nor_init_fixup_flags(struct spi_nor *nor) · line 3045
Quoted 10 lines
drivers/mtd/spi-nor/core.c +10
-0+struct nvmem_device *nvmem; +struct device_node *np; + +if (!nor->sfdp) +return 0 ; + + np =of_get_compatible_child (dev_of_node (dev),"jedec,sfdp" ); + + config.dev = dev; + config.of_node = np;[Severity: High]
If the jedec,sfdp child node is missing from the device tree, np will be NULL and assigned to config.of_node. Does this inadvertently trigger the NVMEM core's fallback behavior?Looking at
nvmem_register()indrivers/nvmem/core.c, there is a fallback mechanism for missing nodes:if (config->of_node) nvmem->dev.of_node = of_node_get(config->of_node); else nvmem->dev.of_node = of_node_get(config->dev->of_node);If config.of_node is NULL, the SFDP NVMEM device will incorrectly inherit the SPI NOR flash's main device node.
The NVMEM core then searches for an nvmem-layout child node and will find the layout intended for the main flash memory, attaching all NVMEM cells to the small SFDP NVMEM device and marking the layout node populated.
Quoted 7 lines
Later, when the MTD subsystem registers the primary NVMEM device for the actual flash contents, it will skip the layout node because it is already marked populated.
Could any consumer attempting to read an NVMEM cell then be misdirected to the small SFDP NVMEM device's read callback instead of the main flash, resulting in
-EINVALbounds check errors or corrupted data?
Yes, this is a valid issue. With no "jedec,sfdp" node, nvmem_register() falls back to the flash's own node, so the SFDP NVMEM device would claim an "nvmem-layout" meant for the whole flash.
Michael,
this comes from dropping the "if (!np) return 0;" check in v9. I'll bring it back in v10: without a "jedec,sfdp" node there is nowhere to describe SFDP cells, so the SFDP NVMEM device would have no consumers, while registering it can take over the flash's layout.
Do you have any other suggestions?
Quoted 9 lines
drivers/mtd/spi-nor/core.c static void spi_nor_init_fixup_flags(struct spi_nor *nor) · line 3048
Quoted 4 lines
drivers/mtd/spi-nor/core.c +3
-0+ config.name ="sfdp" ; + config.id =NVMEM_DEVID_AUTO ; + config.owner =THIS_MODULE ;[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4
Signature · 2 lines
Thanks and Regards, Manikandan M.
Michael Walle
maintainerRe: [PATCH v9 4/8] mtd: spi-nor: sfdp: expose the SFDP as a read-only NVMEM device
Quoted 43 lines · Manikandan.M
Quoted 35 lines
Quoted 4 lines
drivers/mtd/spi-nor/core.c +4
-0+static int spi_nor_register_sfdp_nvmem (struct spi_nor *nor) +{ +struct device *dev = nor->dev; +struct nvmem_config config = { };drivers/mtd/spi-nor/core.c static void spi_nor_init_fixup_flags(struct spi_nor *nor) · line 3045
Quoted 10 lines
drivers/mtd/spi-nor/core.c +10
-0+struct nvmem_device *nvmem; +struct device_node *np; + +if (!nor->sfdp) +return 0 ; + + np =of_get_compatible_child (dev_of_node (dev),"jedec,sfdp" ); + + config.dev = dev; + config.of_node = np;[Severity: High]
If the jedec,sfdp child node is missing from the device tree, np will be NULL and assigned to config.of_node. Does this inadvertently trigger the NVMEM core's fallback behavior?Looking at
nvmem_register()indrivers/nvmem/core.c, there is a fallback mechanism for missing nodes:if (config->of_node) nvmem->dev.of_node = of_node_get(config->of_node); else nvmem->dev.of_node = of_node_get(config->dev->of_node);If config.of_node is NULL, the SFDP NVMEM device will incorrectly inherit the SPI NOR flash's main device node.
The NVMEM core then searches for an nvmem-layout child node and will find the layout intended for the main flash memory, attaching all NVMEM cells to the small SFDP NVMEM device and marking the layout node populated.
Quoted 7 lines
Later, when the MTD subsystem registers the primary NVMEM device for the actual flash contents, it will skip the layout node because it is already marked populated.
Could any consumer attempting to read an NVMEM cell then be misdirected to the small SFDP NVMEM device's read callback instead of the main flash, resulting in
-EINVALbounds check errors or corrupted data?
Quoted 8 lines
Yes, this is a valid issue. With no "jedec,sfdp" node,
nvmem_register()falls back to the flash's own node, so the SFDP NVMEM device would claim an "nvmem-layout" meant for the whole flash.Michael,
this comes from dropping the "if (!np) return 0;" check in v9. I'll bring it back in v10: without a "jedec,sfdp" node there is nowhere to describe SFDP cells, so the SFDP NVMEM device would have no consumers,
There is no DT user, yes. But there is then a "sfdpN" file in sysfs which can be used from user space. And that's what my first remark was about. Why would that only exist if there is a "jedec,sfdp" child in the device tree.
Quoted 2 lines
while registering it can take over the flash's layout. Do you have any other suggestions?
Probably tell nvmem to not attach it to any of_node?
-michael
Re: [PATCH v9 4/8] mtd: spi-nor: sfdp: expose the SFDP as a read-only NVMEM device
Hi Michael,
Quoted 53 lines · Michael Walle
Quoted 43 lines · Manikandan.M
Quoted 35 lines
Quoted 4 lines
drivers/mtd/spi-nor/core.c +4
-0+static int spi_nor_register_sfdp_nvmem (struct spi_nor *nor) +{ +struct device *dev = nor->dev; +struct nvmem_config config = { };drivers/mtd/spi-nor/core.c static void spi_nor_init_fixup_flags(struct spi_nor *nor) · line 3045
Quoted 10 lines
drivers/mtd/spi-nor/core.c +10
-0+struct nvmem_device *nvmem; +struct device_node *np; + +if (!nor->sfdp) +return 0 ; + + np =of_get_compatible_child (dev_of_node (dev),"jedec,sfdp" ); + + config.dev = dev; + config.of_node = np;[Severity: High]
If the jedec,sfdp child node is missing from the device tree, np will be NULL and assigned to config.of_node. Does this inadvertently trigger the NVMEM core's fallback behavior?Looking at
nvmem_register()indrivers/nvmem/core.c, there is a fallback mechanism for missing nodes:if (config->of_node) nvmem->dev.of_node = of_node_get(config->of_node); else nvmem->dev.of_node = of_node_get(config->dev->of_node);If config.of_node is NULL, the SFDP NVMEM device will incorrectly inherit the SPI NOR flash's main device node.
The NVMEM core then searches for an nvmem-layout child node and will find the layout intended for the main flash memory, attaching all NVMEM cells to the small SFDP NVMEM device and marking the layout node populated.
Quoted 7 lines
Later, when the MTD subsystem registers the primary NVMEM device for the actual flash contents, it will skip the layout node because it is already marked populated.
Could any consumer attempting to read an NVMEM cell then be misdirected to the small SFDP NVMEM device's read callback instead of the main flash, resulting in
-EINVALbounds check errors or corrupted data?Quoted 8 lines
Yes, this is a valid issue. With no "jedec,sfdp" node,
nvmem_register()falls back to the flash's own node, so the SFDP NVMEM device would claim an "nvmem-layout" meant for the whole flash.Michael,
this comes from dropping the "if (!np) return 0;" check in v9. I'll bring it back in v10: without a "jedec,sfdp" node there is nowhere to describe SFDP cells, so the SFDP NVMEM device would have no consumers,
Quoted 4 lines
There is no DT user, yes. But there is then a "sfdpN" file in sysfs which can be used from user space. And that's what my first remark was about. Why would that only exist if there is a "jedec,sfdp" child in the device tree.
Agreed, I missed the sysfs user, I'll keep registering the device without a "jedec,sfdp" node in v10, as in v9.
The case Sashiko reported comes from the of_node fallback in nvmem_register() rather than from this patch, a flag(new field) in struct nvmem_config to skip that fallback would fix, I'll leave it out of this series.
Quoted 7 lines
Quoted 3 lines
while registering it can take over the flash's layout. Do you have any other suggestions?
Probably tell nvmem to not attach it to any
of_node?-michael
Signature · 2 lines
Thanks and Regards, Manikandan M.
PATCH v9 5/8 nvmem: layouts: add Microchip/SST SFDP EUI layout driver
In reply to Manikandan MuralidharanAdd an NVMEM layout that exposes the factory-programmed EUI-48 identifier from the Microchip/SST vendor SFDP parameter table (e.g. SST26VF064BEUI) as a "mac-address" cell, for use as a network MAC address. The vendor table is located at runtime via the SFDP NVMEM device (no offset in DT), and a read_post_process callback reverses the LSB-first bytes into canonical MAC order. Binds to an "nvmem-layout" node with compatible "microchip,sst26vf-sfdp-eui".
--- MAINTAINERS | 6 + drivers/nvmem/layouts/Kconfig | 10 ++ drivers/nvmem/layouts/Makefile | 1 + drivers/nvmem/layouts/sst26vf-sfdp-eui.c | 181 +++++++++++++++++++++++ 4 files changed, 198 insertions(+) create mode 100644 drivers/nvmem/layouts/sst26vf-sfdp-eui.c
MAINTAINERS +6 -0
@@ -17999,6 +17999,12 @@ F: Documentation/devicetree/bindings/sound/atmel,at91-ssc.yaml F: drivers/misc/atmel-ssc.c F: include/linux/atmel-ssc.h +MICROCHIP SST SFDP EUI NVMEM LAYOUT DRIVER +M: Manikandan Muralidharan <[email protected]> +S: Maintained +F: Documentation/devicetree/bindings/nvmem/layouts/microchip,sst26vf-sfdp-eui.yaml +F: drivers/nvmem/layouts/sst26vf-sfdp-eui.c + Microchip Timer Counter Block (TCB) Capture Driver M: Kamel Bouhara <[email protected]> L: [email protected] (moderated for non-subscribers)
drivers/nvmem/layouts/Kconfig +10 -0
@@ -26,6 +26,16 @@ config NVMEM_LAYOUT_ONIE_TLV If unsure, say N. +config NVMEM_LAYOUT_SST26VF_SFDP_EUI + tristate "Microchip/SST SFDP EUI-48 layout support" + help + Say Y here if you want to expose the factory-programmed EUI-48 + identifier stored in the Microchip/SST vendor-specific SFDP parameter + table (e.g. SST26VF064BEUI) as NVMEM cells, so that network drivers + can use them as a MAC address. + + If unsure, say N. + config NVMEM_LAYOUT_U_BOOT_ENV tristate "U-Boot environment variables layout" select CRC32
drivers/nvmem/layouts/Makefile +1 -0
@@ -7,3 +7,4 @@ obj-$(CONFIG_NVMEM_LAYOUTS) += fixed-layout.o obj-$(CONFIG_NVMEM_LAYOUT_SL28_VPD) += sl28vpd.o obj-$(CONFIG_NVMEM_LAYOUT_ONIE_TLV) += onie-tlv.o obj-$(CONFIG_NVMEM_LAYOUT_U_BOOT_ENV) += u-boot-env.o +obj-$(CONFIG_NVMEM_LAYOUT_SST26VF_SFDP_EUI) += sst26vf-sfdp-eui.o
drivers/nvmem/layouts/sst26vf-sfdp-eui.c new file +181 -0
181 lines not on this page · raw message
Signature · 1 line
2.43.0
[email protected]
AI reviewRe: [PATCH v9 5/8] nvmem: layouts: add Microchip/SST SFDP EUI layout driver
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] nvmem: Use-after-free on layout module unload via read_post_process --
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] nvmem: Use-after-free on layout module unload via read_post_process --
commit 38845f9d622725238fc32b8e1eac3c9b7dc8b3b7
Author: Manikandan Muralidharan <[email protected]>
nvmem: layouts: add Microchip/SST SFDP EUI layout driver
Add an NVMEM layout that exposes the factory-programmed EUI-48 identifier from the Microchip/SST vendor SFDP parameter table (e.g. SST26VF064BEUI) as a "mac-address" cell, for use as a network MAC address.
Quoted 5 lines
drivers/nvmem/layouts/sst26vf-sfdp-eui.c new file +0
-0
[ ... ]
Quoted 3 lines
drivers/nvmem/layouts/sst26vf-sfdp-eui.c new file +3
-0+static int sfdp_eui_add_cells (struct nvmem_layout *layout) +{ +struct nvmem_device *nvmem = layout->nvmem;
[ ... ]
Quoted 14 lines
drivers/nvmem/layouts/sst26vf-sfdp-eui.c new file +14
-0+ info.name ="mac-address" ; + info.offset = base +SFDP_MCHP_EUI48_OFFSET ; + info.bytes =ETH_ALEN ; + info.np =of_get_child_by_name (layout_np,"mac-address" ); + info.read_post_process = sfdp_eui_read_post_process; + + ret =nvmem_add_one_cell (nvmem, &info); +if (ret) +of_node_put (info.np); + +of_node_put (layout_np); + +return ret; +}
[ ... ]
drivers/nvmem/layouts/sst26vf-sfdp-eui.c line 161
Quoted 4 lines
drivers/nvmem/layouts/sst26vf-sfdp-eui.c new file +4
-0+static void sfdp_eui_remove (struct nvmem_layout *layout) +{ +nvmem_layout_unregister (layout); +}
[Severity: High]
Does this code leave the module vulnerable to a use-after-free panic on module unload?
The core nvmem function nvmem_layout_unregister() is an empty stub that does not remove the registered cells:
drivers/nvmem/core.c:nvmem_layout_unregister() {
/* Keep the API even with an empty stub in case we need it later */
}When this layout driver is unloaded, sfdp_eui_remove() calls nvmem_layout_unregister(layout) but the cells and sysfs attributes are left dangling.
Because the cell was configured with a read_post_process callback pointing to sfdp_eui_read_post_process in this module's text segment, a subsequent read of the sysfs cell file after the module is unloaded will call the dangling function pointer:
drivers/nvmem/core.c:__nvmem_cell_read() {
...
rc = cell->read_post_process(cell->priv, id, index,
cell->offset, buf, cell->raw_len);
...
}Will this cause a kernel panic by executing freed module memory?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5
PATCH v9 6/8 ARM: dts: microchip: sama5d27_wlsom1: use fixed-partitions for QSPI flash
In reply to Manikandan MuralidharanMove the QSPI flash partitions under a "partitions" node with the "fixed-partitions" compatible, as required by the current MTD partition binding, instead of declaring them as direct children of the flash node. No functional change.
--- .../dts/microchip/at91-sama5d27_wlsom1.dtsi | 54 ++++++++++--------- 1 file changed, 29 insertions(+), 25 deletions(-)
arch/arm/boot/dts/microchip/at91-sama5d27_wlsom1.dtsi +29 -25
@@ -229,8 +229,6 @@ &qspi1 { status = "disabled"; qspi1_flash: flash@0 { - #address-cells = <1>; - #size-cells = <1>; compatible = "jedec,spi-nor"; reg = <0>; spi-max-frequency = <104000000>;
@@ -240,34 +238,40 @@ qspi1_flash: flash@0 { m25p,fast-read; status = "disabled"; - at91bootstrap@0 { - label = "at91bootstrap"; - reg = <0x0 0x40000>; - }; + partitions { + compatible = "fixed-partitions"; + #address-cells = <1>; + #size-cells = <1>; - bootloader@40000 { - label = "bootloader"; - reg = <0x40000 0xc0000>; - }; + at91bootstrap@0 { + label = "at91bootstrap"; + reg = <0x0 0x40000>; + }; - bootloaderenvred@100000 { - label = "bootloader env redundant"; - reg = <0x100000 0x40000>; - }; + bootloader@40000 { + label = "bootloader"; + reg = <0x40000 0xc0000>; + }; - bootloaderenv@140000 { - label = "bootloader env"; - reg = <0x140000 0x40000>; - }; + bootloaderenvred@100000 { + label = "bootloader env redundant"; + reg = <0x100000 0x40000>; + }; - dtb@180000 { - label = "device tree"; - reg = <0x180000 0x80000>; - }; + bootloaderenv@140000 { + label = "bootloader env"; + reg = <0x140000 0x40000>; + }; - kernel@200000 { - label = "kernel"; - reg = <0x200000 0x600000>; + dtb@180000 { + label = "device tree"; + reg = <0x180000 0x80000>; + }; + + kernel@200000 { + label = "kernel"; + reg = <0x200000 0x600000>; + }; }; }; };
Signature · 1 line
2.43.0
PATCH v9 7/8 ARM: dts: microchip: sama5d27_wlsom1: read MAC address from QSPI SFDP
In reply to Manikandan MuralidharanDescribe the QSPI flash SFDP as an NVMEM provider with the microchip,sst26vf-sfdp-eui layout, which exposes the factory-programmed EUI-48 as a "mac-address" cell, and point macb0 at it through nvmem-cells. This yields a stable MAC address on boards where U-Boot does not program one, instead of falling back to a random address.
The sama5d27_wlsom1 SoM is fitted with the factory-programmed SST26VF064BEUI, and the QSPI flash node is only enabled on boards carrying that part, so the EUI-48 marker and its "mac-address" cell are always present. Boards without an EUI-programmed part simply do not reference the cell, so macb0 never defers waiting for it.
--- arch/arm/boot/dts/microchip/at91-sama5d27_wlsom1.dtsi | 11 +++++++++++ .../boot/dts/microchip/at91-sama5d27_wlsom1_ek.dts | 2 ++ 2 files changed, 13 insertions(+)
arch/arm/boot/dts/microchip/at91-sama5d27_wlsom1.dtsi +11 -0
@@ -238,6 +238,17 @@ qspi1_flash: flash@0 { m25p,fast-read; status = "disabled"; + sfdp { + compatible = "jedec,sfdp"; + + nvmem-layout { + compatible = "microchip,sst26vf-sfdp-eui"; + + mac_address_eui48: mac-address { + }; + }; + }; + partitions { compatible = "fixed-partitions"; #address-cells = <1>;
arch/arm/boot/dts/microchip/at91-sama5d27_wlsom1_ek.dts +2 -0
@@ -97,6 +97,8 @@ uart6: serial@200 { &macb0 { status = "okay"; + nvmem-cells = <&mac_address_eui48>; + nvmem-cell-names = "mac-address"; }; &pioA {
Signature · 1 line
2.43.0
[email protected]
AI reviewRe: [PATCH v9 7/8] ARM: dts: microchip: sama5d27_wlsom1: read MAC address from QSPI SFDP
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] nvmem: sst26vf-sfdp-eui: infinite probe deferral on missing EUI-48 --
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] nvmem: sst26vf-sfdp-eui: infinite probe deferral on missing EUI-48 --
commit 2233a3452b4615481340768cbd46d9b4bfaf12ac
Author: Manikandan Muralidharan <[email protected]>
ARM: dts: microchip: sama5d27_wlsom1: read MAC address from QSPI SFDP
This commit configures the QSPI flash SFDP as an NVMEM provider on the sama5d27_wlsom1_ek board to read the factory-programmed MAC address. It adds the nvmem-cells property to the macb0 node so it can retrieve the EUI-48 identifier.
arch/arm/boot/dts/microchip/at91-sama5d27_wlsom1_ek.dts uart6: serial@200 { · line 102
Quoted 11 lines
arch/arm/boot/dts/microchip/at91-sama5d27_wlsom1_ek.dts +2
-0@@ -97,6 +97,8 @@ uart6: serial@200 { &macb0 { status = "okay"; + nvmem-cells = <&mac_address_eui48>; + nvmem-cell-names = "mac-address"; };
[Severity: High]
Will this lead to an infinite probe deferral loop if the QSPI flash EUI-48 marker is unprogrammed, missing, or corrupted?
The commit message notes that boards without an EUI-programmed part simply do not reference the cell. However, if the flash part is present but the marker is missing or corrupted, it appears the macb0 interface will fail to initialize entirely instead of falling back to a random MAC address.
Looking at the nvmem layout driver:
drivers/nvmem/layouts/sst26vf-sfdp-eui.c:sfdp_eui_add_cells() {
if (marker != SFDP_MCHP_EUI48_MARKER) {
dev_dbg(dev, "EUI-48 not programmed (marker 0x%02x)\n", marker);
return 0;
}
}This silently returns 0 without adding the cell, which leaves the layout bound.
When macb_probe() requests the cell, the nvmem core sees the bound layout but missing cell:
drivers/nvmem/core.c:of_nvmem_cell_get() {
ret = nvmem->layout ? -EPROBE_DEFER : -ENOENT;
}This causes of_nvmem_cell_get() to return -EPROBE_DEFER indefinitely.
Then in the ethernet driver:
drivers/net/ethernet/cadence/macb_main.c:macb_probe() {
err = of_get_ethdev_address(np, bp->netdev);
if (err == -EPROBE_DEFER)
goto err_out_free_netdev;
}Because macb_probe() treats -EPROBE_DEFER as a fatal error that requires deferring, it skips the fallback path and never brings up macb0.
Could the layout driver return -ENOENT instead when the marker is missing, or is there another way to ensure the ethernet interface can fall back to a random MAC address in this scenario?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7
PATCH v9 8/8 ARM: configs: sama5: enable Microchip/SST SFDP EUI NVMEM layout
In reply to Manikandan MuralidharanEnable CONFIG_NVMEM_LAYOUT_SST26VF_SFDP_EUI so the factory EUI-48 stored in the SST26VF QSPI flash SFDP can be used as a MAC address on boards such as the sama5d27_wlsom1.
--- arch/arm/configs/sama5_defconfig | 1 + 1 file changed, 1 insertion(+)
arch/arm/configs/sama5_defconfig +1 -0
@@ -219,6 +219,7 @@ CONFIG_PWM=y CONFIG_PWM_ATMEL=y CONFIG_PWM_ATMEL_HLCDC_PWM=y CONFIG_PWM_ATMEL_TCB=y +CONFIG_NVMEM_LAYOUT_SST26VF_SFDP_EUI=y CONFIG_EXT4_FS=y CONFIG_FANOTIFY=y CONFIG_AUTOFS_FS=m
Signature · 1 line
2.43.0