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

[sashiko-bot@kernel.org]

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/