Re: [PATCH v10 4/8] mtd: spi-nor: sfdp: expose the SFDP as a read-only NVMEM device

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] mtd: spi-nor: sfdp: fallback to flash of_node when jedec,sfdp
  node is absent
- [Medium] mtd: spi-nor: sfdp: wrong DT node lookup in
  spi_nor_register_sfdp_nvmem()
- [Medium] mtd: spi-nor: sfdp: missing ignore_wp in
  spi_nor_register_sfdp_nvmem()
--

commit d6b1f5d68968819cd02f25dd18526725661f7542
Author: Manikandan Muralidharan <manikandan.m@microchip.com>

mtd: spi-nor: sfdp: expose the SFDP as a read-only NVMEM device

This commit exposes the cached SFDP table of a SPI NOR flash as a read-only
NVMEM device rooted at the flash's "jedec,sfdp" child node.

> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 90524c58602e1..a034dce7c839e 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c

[ ... ]

> +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");

[Severity: Medium]
Should this look up the child node under spi_nor_get_flash_node(nor) instead
of dev_of_node(dev)?

In SPI NOR controller drivers such as hisi-sfc and nxp-spifi, nor->dev is the
controller device while the flash node is a child DT node stored via
spi_nor_set_flash_node().  Using dev_of_node(dev) searches the controller's
DT node and misses the flash's "jedec,sfdp" child node.

> +
> +	config.dev = dev;
> +	config.of_node = np;

[Severity: High]
When the flash DT node has no "jedec,sfdp" child node, np is NULL, but
spi_nor_register_sfdp_nvmem() still registers the NVMEM device with
config.of_node = NULL and config.dev = dev.  In nvmem_register(), a NULL
config->of_node falls back to config->dev->of_node:

drivers/nvmem/core.c:nvmem_register() {
    ...
	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);
    ...
}

Because spi_nor_register_sfdp_nvmem() runs during spi_nor_scan() before
mtd_device_parse_register() registers the main MTD and OTP NVMEM devices, does
this bind the SFDP NVMEM device to the parent flash DT node first?

On any SPI NOR flash with SFDP whose DT node has no "jedec,sfdp" subnode, the
SFDP NVMEM device claims any nvmem-layout on the flash node (setting
OF_POPULATED_BUS so mtd_nvmem_add() skips it) and shadows the flash MTD and
OTP NVMEM providers in of_nvmem_cell_get() and of_nvmem_device_get(), causing
cell lookups on the flash node to fail or return SFDP bytes instead of flash
data.

If registering without a "jedec,sfdp" subnode is intended, setting
config.no_of_node = !np prevents nvmem_register() from falling back to
dev->of_node.

> +	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;

[Severity: Medium]
Does config also need ignore_wp = true, like mtd_nvmem_add() and
mtd_otp_nvmem_register()?

When config.ignore_wp is false, nvmem_register() calls
gpiod_get_optional(config->dev, "wp", GPIOD_OUT_HIGH) even for read-only
providers.  Since the SFDP provider only reads from the cached
nor->sfdp->dwords buffer, leaving ignore_wp unset claims any wp-gpios
property on nor->dev and drives the write-protect line high, or fails probe
with -EBUSY if the GPIO is already claimed.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1794391660.git.manikandan.m@microchip.com?part=4

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/