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

[<Manikandan.M@microchip.com>]

On 10/17/26 4:10 PM, sashiko-bot@kernel.org 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 <manikandan.m@microchip.com>
> 
> 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.
> 
>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>> index 8bc117b46e028..ed478e48ca116 100644
>> --- a/drivers/mtd/spi-nor/core.c
>> +++ b/drivers/mtd/spi-nor/core.c
> 
> [ ... ]
> 
>> @@ -3001,6 +3002,72 @@ static void spi_nor_init_fixup_flags(struct spi_nor *nor)
> 
> [ ... ]
> 
>> +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;
> 
> [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?

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?

> 
>> +     config.name = "sfdp";
>> +     config.id = NVMEM_DEVID_AUTO;
>> +     config.owner = THIS_MODULE;
> 
> [ ... ]
> 
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/cover.1792145382.git.manikandan.m@microchip.com?part=4


-- 
Thanks and Regards,
Manikandan M.