[PATCH 0/2] mtd/nvmem: don't defer forever on absent NVMEM suppliers

[PATCH 0/2] mtd/nvmem: don't defer forever on absent NVMEM suppliers

[Manikandan Muralidharan]

A consumer that gets its MAC address from an NVMEM cell can be deferred
forever when the cell will never appear, instead of falling back to
another address source. These came up while testing the SFDP EUI-48
series [1], where macb reads its MAC address from a cell exposed by the
QSPI flash, but both are generic:

 - 1/2: when the SPI NOR flash is not populated or not reachable, nothing
   below its node is ever registered, yet fw_devlink keeps waiting on it
   and the NVMEM lookup keeps returning -EPROBE_DEFER. Drop the fw_devlink
   links below a flash that failed to probe, and return -ENOENT for a cell
   on a node fw_devlink has marked as never becoming a device.

 - 2/2: a layout adds its cells from its probe and may add none, e.g. when
   the data the cell describes is not programmed on the device. Only defer
   while the layout is not bound yet, and return -ENOENT once it is.

In both cases the consumer then falls back, e.g. macb uses the address
left by the bootloader or a random one.

This series depends on the SFDP EUI-48 series [1] and applies on top of it.

[1] https://lore.kernel.org/all/cover.1794391660.git.manikandan.m@microchip.com/

Manikandan Muralidharan (2):
  mtd: spi-nor: drop fw_devlink suppliers below a flash that failed to
    probe
  nvmem: core: don't defer for a missing cell once the layout is bound

 drivers/mtd/spi-nor/core.c | 26 ++++++++++++++++++++++++-
 drivers/nvmem/core.c       | 40 ++++++++++++++++++++++++++++++++++++--
 2 files changed, 63 insertions(+), 3 deletions(-)


base-commit: 05441d697f7f9a927264f4058cbbc6a03dfcac9d
prerequisite-patch-id: 76d8b1168f88c3e7912ac2ce01a705f7e69d8a3b
prerequisite-patch-id: b360d0d00ac1a6feb76d4b3f9412b6f1741f6161
prerequisite-patch-id: 659801d6f415ba79ce51f21b3a4f5e132348c150
prerequisite-patch-id: bd1a5a10983aa3cfd99c93ee58fdeb04cdd87b76
prerequisite-patch-id: 94ede38b6c2480cb1e0e102e9bbeb6ffd5b276ef
prerequisite-patch-id: ae3977b6c56c5bd4f352d77bd8a7883a69d390af
prerequisite-patch-id: 58d0ad25e81f16f60864d624eed7ff481607e960
prerequisite-patch-id: ba140b71fdbfc992861ae4c34b6acc737d4e4528
-- 
2.43.0


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

[PATCH 1/2] mtd: spi-nor: drop fw_devlink suppliers below a flash that failed to probe

[Manikandan Muralidharan]

When a SPI NOR flash is not populated or not reachable, spi_nor_scan()
fails and nothing described below the flash node (the SFDP NVMEM device,
its nvmem-layout, partitions) is ever registered. fw_devlink still holds
links from consumers of those nodes, so a consumer such as an Ethernet
MAC whose "nvmem-cells" points at the SFDP EUI-48 cell waits for its
supplier forever and is never probed:

  platform f8008000.ethernet: deferred probe pending: platform: wait for
  supplier /ahb/apb/spi@f0024000/flash@0/sfdp/nvmem-layout/mac-address

On a permanent spi_nor_scan() failure, call
fw_devlink_purge_absent_suppliers() on each child of the flash node. It
marks the nodes that never became devices with FWNODE_FLAG_NOT_DEVICE
and drops their consumer links, so the consumer is probed.

The consumer then probes, but of_nvmem_cell_get() still returns
-EPROBE_DEFER because nvmem_device_match() does not find a device for
the SFDP node. When fw_devlink has marked that node FWNODE_FLAG_NOT_DEVICE,
it will never become a device, so return -ENOENT instead. The consumer
can then fall back, e.g. macb uses a random MAC address.

Signed-off-by: Manikandan Muralidharan <manikandan.m@microchip.com>
---
 drivers/mtd/spi-nor/core.c | 26 +++++++++++++++++++++++++-
 drivers/nvmem/core.c       | 13 ++++++++++++-
 2 files changed, 37 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index a034dce7c839..57faab89f5af 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -12,6 +12,7 @@
 #include <linux/device.h>
 #include <linux/err.h>
 #include <linux/errno.h>
+#include <linux/fwnode.h>
 #include <linux/math64.h>
 #include <linux/module.h>
 #include <linux/mtd/mtd.h>
@@ -3624,6 +3625,25 @@ static int spi_nor_create_write_dirmap(struct spi_nor *nor)
 	return PTR_ERR_OR_ZERO(nor->dirmap.wdesc);
 }
 
+/**
+ * spi_nor_purge_absent_suppliers() - drop fw_devlink links below a missing
+ * flash
+ * @dev:	pointer to the flash 'struct device'
+ *
+ * When the flash cannot be probed (e.g. it is not populated or not
+ * reachable), nothing described below its node, such as the SFDP NVMEM
+ * device or the partitions, is ever registered. Mark those nodes as never
+ * becoming devices and drop their consumer links, so that consumers such as
+ * an Ethernet MAC using an NVMEM cell are not deferred forever.
+ */
+static void spi_nor_purge_absent_suppliers(struct device *dev)
+{
+	struct fwnode_handle *child;
+
+	device_for_each_child_node(dev, child)
+		fw_devlink_purge_absent_suppliers(child);
+}
+
 static int spi_nor_probe(struct spi_mem *spimem)
 {
 	struct spi_device *spi = spimem->spi;
@@ -3672,8 +3692,12 @@ static int spi_nor_probe(struct spi_mem *spimem)
 		flash_name = spi->modalias;
 
 	ret = spi_nor_scan(nor, flash_name, &hwcaps);
-	if (ret)
+	if (ret) {
+		/* A deferred probe (e.g. on the reset GPIO) may still succeed. */
+		if (ret != -EPROBE_DEFER)
+			spi_nor_purge_absent_suppliers(dev);
 		return ret;
+	}
 
 	spi_nor_debugfs_register(nor);
 
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 2e07f95de7ff..be3a05150085 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -10,6 +10,7 @@
 #include <linux/device.h>
 #include <linux/export.h>
 #include <linux/fs.h>
+#include <linux/fwnode.h>
 #include <linux/idr.h>
 #include <linux/init.h>
 #include <linux/kref.h>
@@ -1458,11 +1459,21 @@ struct nvmem_cell *of_nvmem_cell_get(struct device_node *np, const char *id)
 	}
 
 	nvmem = nvmem_device_match(nvmem_np, device_match_of_node);
-	of_node_put(nvmem_np);
 	if (IS_ERR(nvmem)) {
+		/*
+		 * fw_devlink marks a node that will never become a device,
+		 * either because its parent failed to probe or because the
+		 * parent bound without registering it. Don't defer for it.
+		 */
+		if (PTR_ERR(nvmem) == -EPROBE_DEFER &&
+		    fwnode_test_flag(of_fwnode_handle(nvmem_np),
+				     FWNODE_FLAG_NOT_DEVICE))
+			nvmem = ERR_PTR(-ENOENT);
+		of_node_put(nvmem_np);
 		of_node_put(cell_np);
 		return ERR_CAST(nvmem);
 	}
+	of_node_put(nvmem_np);
 
 	ret = nvmem_layout_module_get_optional(nvmem);
 	if (ret) {
-- 
2.43.0


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

[PATCH 2/2] nvmem: core: don't defer for a missing cell once the layout is bound

[Manikandan Muralidharan]

of_nvmem_cell_get() returns -EPROBE_DEFER whenever a requested cell is
not found and the nvmem device has a layout, assuming the layout has not
added its cells yet. A layout may add none, e.g. when the data the cell
describes is not programmed on the device. The layout is then bound but
the cell is never going to show up, and the consumer is deferred forever
instead of falling back.

Only defer while the layout device is not bound yet, and return -ENOENT
once it is, so that a consumer such as an Ethernet MAC can fall back to a
random address.

Signed-off-by: Manikandan Muralidharan <manikandan.m@microchip.com>
---
 drivers/nvmem/core.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index be3a05150085..f34a35a04e55 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -1403,6 +1403,31 @@ static int nvmem_layout_module_get_optional(struct nvmem_device *nvmem)
 	return 0;
 }
 
+/**
+ * nvmem_layout_pending() - Check whether a layout may still add cells
+ * @nvmem: nvmem device the layout belongs to
+ *
+ * A layout adds its cells from its probe. Once the layout device is bound,
+ * a cell that is still missing is never going to show up.
+ *
+ * Return: true if @nvmem has a layout that is not bound yet, false otherwise.
+ */
+static bool nvmem_layout_pending(struct nvmem_device *nvmem)
+{
+	struct device *dev;
+	bool bound;
+
+	if (!nvmem->layout)
+		return false;
+
+	dev = &nvmem->layout->dev;
+	device_lock(dev);
+	bound = device_is_bound(dev);
+	device_unlock(dev);
+
+	return !bound;
+}
+
 /**
  * of_nvmem_cell_get() - Get a nvmem cell from given device node and cell id
  *
@@ -1486,7 +1511,7 @@ struct nvmem_cell *of_nvmem_cell_get(struct device_node *np, const char *id)
 	of_node_put(cell_np);
 	if (!cell_entry) {
 		nvmem_layout_module_put(nvmem);
-		ret = nvmem->layout ? -EPROBE_DEFER : -ENOENT;
+		ret = nvmem_layout_pending(nvmem) ? -EPROBE_DEFER : -ENOENT;
 		nvmem_device_put(nvmem);
 		return ERR_PTR(ret);
 	}
-- 
2.43.0


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

Re: [PATCH 1/2] mtd: spi-nor: drop fw_devlink suppliers below a flash that failed to probe

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] mtd: spi-nor: missing deferred probe trigger in spi_nor_probe()
  error path
- [High] nvmem: core: premature -ENOENT in of_nvmem_cell_get() for child
  NVMEM nodes
- [Medium] mtd: spi-nor: incomplete supplier purging in spi_nor_probe()
  on error paths
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [Medium] nvmem: core: OF node reference leak in of_nvmem_cell_get()
  [1]
--

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

mtd: spi-nor: drop fw_devlink suppliers below a flash that failed to probe

This commit purges fw_devlink consumer links below a SPI NOR flash node when
spi_nor_scan() fails with a non-deferral error, and updates
of_nvmem_cell_get() to return -ENOENT instead of -EPROBE_DEFER when the
NVMEM node is marked FWNODE_FLAG_NOT_DEVICE.

> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index a034dce7c839e..57faab89f5af4 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
[ ... ]
> @@ -3624,6 +3625,25 @@ static int spi_nor_create_write_dirmap(struct spi_nor *nor)
>  	return PTR_ERR_OR_ZERO(nor->dirmap.wdesc);
>  }
>  
> +/**
> + * spi_nor_purge_absent_suppliers() - drop fw_devlink links below a missing
> + * flash
> + * @dev:	pointer to the flash 'struct device'
> + *
> + * When the flash cannot be probed (e.g. it is not populated or not
> + * reachable), nothing described below its node, such as the SFDP NVMEM
> + * device or the partitions, is ever registered. Mark those nodes as never
> + * becoming devices and drop their consumer links, so that consumers such as
> + * an Ethernet MAC using an NVMEM cell are not deferred forever.
> + */
> +static void spi_nor_purge_absent_suppliers(struct device *dev)
> +{
> +	struct fwnode_handle *child;
> +
> +	device_for_each_child_node(dev, child)
> +		fw_devlink_purge_absent_suppliers(child);
> +}
> +
>  static int spi_nor_probe(struct spi_mem *spimem)
>  {
>  	struct spi_device *spi = spimem->spi;
> @@ -3672,8 +3692,12 @@ static int spi_nor_probe(struct spi_mem *spimem)
>  		flash_name = spi->modalias;
>  
>  	ret = spi_nor_scan(nor, flash_name, &hwcaps);
> -	if (ret)
> +	if (ret) {
> +		/* A deferred probe (e.g. on the reset GPIO) may still succeed. */
> +		if (ret != -EPROBE_DEFER)
> +			spi_nor_purge_absent_suppliers(dev);
>  		return ret;
> +	}

[Severity: High]
Can a consumer on deferred_probe_pending_list remain unprobed when
spi_nor_probe() fails here?

fw_devlink_purge_absent_suppliers() drops the fwnode consumer links, but
neither it nor the probe_failed path in really_probe() calls
driver_deferred_probe_trigger().  If the consumer already deferred before
spi_nor_probe() ran (for example when spi-nor is loaded as a module, or when
spi_nor_probe() initially deferred on vcc-supply or reset-gpios), nothing
re-triggers deferred probing after the links are purged unless an unrelated
device happens to bind later.

[Severity: Medium]
Does spi_nor_purge_absent_suppliers() miss the nvmem-layout child nodes when
spi_nor_scan() fails after spi_nor_register_sfdp_nvmem()?

spi_nor_init_params() calls spi_nor_register_sfdp_nvmem() ->
devm_nvmem_register() -> nvmem_layout_create_device(), which attaches
layout->dev to the nvmem-layout fwnode before spi_nor_late_init_params(),
spi_nor_setup(), and spi_nor_init() run.  If one of those later steps fails,
spi_nor_purge_absent_suppliers() runs before device_unbind_cleanup() ->
devres_release_all() unregisters layout->dev, so
fw_devlink_purge_absent_suppliers() sees fwnode->dev != NULL on nvmem-layout
and returns early without purging the cell nodes underneath it.

In addition, permanent failures in spi_nor_probe() before or after
spi_nor_scan() (such as devm_regulator_get_enable(), dirmap creation, or
mtd_device_register()) do not call spi_nor_purge_absent_suppliers(), and
NVMEM cells placed directly under the flash node resolve nvmem_np to the
flash node itself (where fwnode->dev is &spi->dev), which is never marked
FWNODE_FLAG_NOT_DEVICE.

> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index 2e07f95de7ff2..be3a051500850 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
[ ... ]
> @@ -1458,11 +1459,21 @@ struct nvmem_cell *of_nvmem_cell_get(struct device_node *np, const char *id)
>  	}
>  
>  	nvmem = nvmem_device_match(nvmem_np, device_match_of_node);
> -	of_node_put(nvmem_np);
>  	if (IS_ERR(nvmem)) {
> +		/*
> +		 * fw_devlink marks a node that will never become a device,
> +		 * either because its parent failed to probe or because the
> +		 * parent bound without registering it. Don't defer for it.
> +		 */
> +		if (PTR_ERR(nvmem) == -EPROBE_DEFER &&
> +		    fwnode_test_flag(of_fwnode_handle(nvmem_np),
> +				     FWNODE_FLAG_NOT_DEVICE))
> +			nvmem = ERR_PTR(-ENOENT);
> +		of_node_put(nvmem_np);
>  		of_node_put(cell_np);
>  		return ERR_CAST(nvmem);
>  	}
> +	of_node_put(nvmem_np);

[Severity: High]
Does checking FWNODE_FLAG_NOT_DEVICE on nvmem_np cause of_nvmem_cell_get()
to return -ENOENT prematurely for valid descendant NVMEM providers such as
UBI volumes?

When an ancestor bus device (such as the flash controller) finishes probing,
driver_bound() -> device_links_driver_bound() ->
__fw_devlink_pickup_dangling_consumers() marks every descendant fwnode
whose struct device is not on a bus with FWNODE_FLAG_NOT_DEVICE and moves
its consumer links to the ancestor:

drivers/base/core.c:__fw_devlink_pickup_dangling_consumers() {
	if (fwnode->dev && fwnode->dev->bus)
		return;

	fwnode_set_flag(fwnode, FWNODE_FLAG_NOT_DEVICE);
	__fwnode_links_move_consumers(fwnode, new_sup);
    ...
}

Because MTD, UBI volume, and NVMEM devices are class devices with
dev->bus == NULL, binding the flash device sets FWNODE_FLAG_NOT_DEVICE on
all descendant partition and UBI volume nodes before ubi_nvmem_add()
registers the volume's nvmem_device.  A consumer probing in that window will
now get -ENOENT instead of -EPROBE_DEFER and permanently fail or fall back
to a random MAC address.

[Severity: Medium]
This problem wasn't introduced by this patch, but does of_nvmem_cell_get()
leak cell_spec.np [1] when cell_spec.args_count > 1?

Earlier in of_nvmem_cell_get(), of_parse_phandle_with_optional_args()
returns with a reference held on cell_spec.np, and the
cell_spec.args_count > 1 error path returns ERR_PTR(-EINVAL) without calling
of_node_put(cell_spec.np).

[1] https://sashiko.dev/#/bug/linux-16ae4391-b371-4c5a-be3b-f853ba4fc2ab

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

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

Re: [PATCH 2/2] nvmem: core: don't defer for a missing cell once the layout is bound

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] nvmem: core: race in of_nvmem_cell_get() with concurrent
  layout probe
- [Medium] nvmem: core: race in nvmem_layout_pending() with layout init,
  probe, and free
- [Low] nvmem: core: missing Fixes trailer for commit fc29fd821d9a
--

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

> nvmem: core: don't defer for a missing cell once the layout is bound
>
> of_nvmem_cell_get() returns -EPROBE_DEFER whenever a requested cell is
> not found and the nvmem device has a layout, assuming the layout has not
> added its cells yet. A layout may add none, e.g. when the data the cell
> describes is not programmed on the device. The layout is then bound but
> the cell is never going to show up, and the consumer is deferred forever
> instead of falling back.
>
> Only defer while the layout device is not bound yet, and return -ENOENT
> once it is, so that a consumer such as an Ethernet MAC can fall back to a
> random address.
>
> Signed-off-by: Manikandan Muralidharan <manikandan.m@microchip.com>

[Severity: Low]
This looks like a bug fix for layout device handling.  Should this
include:

  Fixes: fc29fd821d9a ("nvmem: core: Rework layouts to become regular devices")

> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index be3a051500850..f34a35a04e553 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -1403,6 +1403,31 @@ static int nvmem_layout_module_get_optional(struct nvmem_device *nvmem)
>  	return 0;
>  }
>  
> +/**
> + * nvmem_layout_pending() - Check whether a layout may still add cells
> + * @nvmem: nvmem device the layout belongs to
> + *
> + * A layout adds its cells from its probe. Once the layout device is bound,
> + * a cell that is still missing is never going to show up.
> + *
> + * Return: true if @nvmem has a layout that is not bound yet, false otherwise.
> + */
> +static bool nvmem_layout_pending(struct nvmem_device *nvmem)
> +{
> +	struct device *dev;
> +	bool bound;
> +
> +	if (!nvmem->layout)
> +		return false;
> +
> +	dev = &nvmem->layout->dev;
> +	device_lock(dev);
> +	bound = device_is_bound(dev);
> +	device_unlock(dev);
> +
> +	return !bound;
> +}

[Severity: Medium]
Can nvmem_layout_pending() lock an uninitialized or freed mutex on
nvmem->layout->dev?

In nvmem_register(), device_add(&nvmem->dev) publishes the nvmem device on
nvmem_bus_type before nvmem_populate_layout() runs, and
nvmem_layout_create_device() sets nvmem->layout before device_initialize()
initializes layout->dev.mutex:

CPU0                                CPU1
nvmem_register()
  device_add(&nvmem->dev)
  nvmem_populate_layout()
    nvmem_layout_create_device()
      nvmem->layout = layout;
                                    of_nvmem_cell_get()
                                      ...
                                      nvmem_layout_pending()
                                        device_lock(&nvmem->layout->dev)
                                          <-- uninitialized mutex
      device_initialize(dev)

Similarly, nvmem_destroy_layout() unregisters and frees nvmem->layout
before nvmem->dev is removed from nvmem_bus_type and without clearing
nvmem->layout, so a concurrent nvmem_layout_pending() can dereference and
lock a freed layout device.

> @@ -1486,7 +1511,7 @@ struct nvmem_cell *of_nvmem_cell_get(struct device_node *np, const char *id)
>  	of_node_put(cell_np);
>  	if (!cell_entry) {
>  		nvmem_layout_module_put(nvmem);
> -		ret = nvmem->layout ? -EPROBE_DEFER : -ENOENT;
> +		ret = nvmem_layout_pending(nvmem) ? -EPROBE_DEFER : -ENOENT;
>  		nvmem_device_put(nvmem);
>  		return ERR_PTR(ret);
>  	}

[Severity: Medium]
Can of_nvmem_cell_get() falsely return -ENOENT when racing with a layout
driver probe?

During really_probe(), device_set_driver() sets layout->dev.driver under
device_lock(&layout->dev) before the layout's probe callback adds cells to
nvmem->cells:

CPU0                                CPU1
really_probe(&layout->dev)
  device_lock(&layout->dev)
  device_set_driver(dev, drv)
                                    of_nvmem_cell_get()
                                      nvmem_layout_module_get_optional()
                                        sees dev.driver != NULL
                                      nvmem_find_cell_entry_by_node()
                                        returns NULL
                                      nvmem_layout_pending()
                                        device_lock(&layout->dev)
  call_driver_probe()
    adds cell to nvmem->cells
  driver_bound(dev)
  device_unlock(&layout->dev)
                                        device_is_bound(dev) == true
                                      returns ERR_PTR(-ENOENT)

Consumer drivers then treat a valid cell that was just added as missing.
In addition, if really_probe() fails, device_unbind_cleanup() clears
layout->dev.driver under device_lock(&layout->dev), so
nvmem_layout_module_put() can see dev.driver == NULL and leak the module
reference acquired by nvmem_layout_module_get_optional().

Checking device_is_bound() and taking the module reference under
device_lock(&nvmem->layout->dev) in nvmem_layout_module_get_optional()
before searching nvmem->cells would avoid both issues.

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

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