Re: [PATCH v4 4/8] block: implement NVMEM provider

[Loic Poulain]

Hi bartosz,

On Tue, Jun 9, 2026 at 10:52 AM Bartosz Golaszewski <brgl@xxxxxxxxxx> wrote:
>
> On Tue, 9 Jun 2026 09:52:29 +0200, Loic Poulain
> <loic.poulain@xxxxxxxxxxxxxxxx> said:
> > From: Daniel Golle <daniel@xxxxxxxxxxxxxx>
> >
> > On embedded devices using an eMMC it is common that one or more partitions
> > on the eMMC are used to store MAC addresses and Wi-Fi calibration EEPROM
> > data. Allow referencing the partition in device tree for the kernel and
> > Wi-Fi drivers accessing it via the NVMEM layer.
> >
> > Signed-off-by: Daniel Golle <daniel@xxxxxxxxxxxxxx>
> > Co-developed-by: Loic Poulain <loic.poulain@xxxxxxxxxxxxxxxx>
> > Signed-off-by: Loic Poulain <loic.poulain@xxxxxxxxxxxxxxxx>
> > ---
> >  block/Kconfig     |   9 +++++
> >  block/Makefile    |   1 +
> >  block/blk-nvmem.c | 114 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 124 insertions(+)
> >
> > diff --git a/block/Kconfig b/block/Kconfig
> > index 15027963472d7b40e27b9097a5993c457b5b3054..0b33747e16dc33473683706f75c92bdf8b648f7c 100644
> > --- a/block/Kconfig
> > +++ b/block/Kconfig
> > @@ -209,6 +209,15 @@ config BLK_INLINE_ENCRYPTION_FALLBACK
> >         by falling back to the kernel crypto API when inline
> >         encryption hardware is not present.
> >
> > +config BLK_NVMEM
> > +     bool "Block device NVMEM provider"
> > +     depends on OF
> > +     depends on NVMEM
> > +     help
> > +       Allow block devices (or partitions) to act as NVMEM providers,
> > +       typically used with eMMC to store MAC addresses or Wi-Fi
> > +       calibration data on embedded devices.
> > +
> >  source "block/partitions/Kconfig"
> >
> >  config BLK_PM
> > diff --git a/block/Makefile b/block/Makefile
> > index 7dce2e44276c4274c11a0a61121c83d9c43d6e0c..d7ac389e71902bc091a8800ea266190a43b3e63d 100644
> > --- a/block/Makefile
> > +++ b/block/Makefile
> > @@ -36,3 +36,4 @@ obj-$(CONFIG_BLK_INLINE_ENCRYPTION) += blk-crypto.o blk-crypto-profile.o \
> >                                          blk-crypto-sysfs.o
> >  obj-$(CONFIG_BLK_INLINE_ENCRYPTION_FALLBACK) += blk-crypto-fallback.o
> >  obj-$(CONFIG_BLOCK_HOLDER_DEPRECATED)        += holder.o
> > +obj-$(CONFIG_BLK_NVMEM)                += blk-nvmem.o
> > diff --git a/block/blk-nvmem.c b/block/blk-nvmem.c
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..a6e62fa98675ee9bcb9c7035a611b5a573ab9091
> > --- /dev/null
> > +++ b/block/blk-nvmem.c
> > @@ -0,0 +1,114 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * block device NVMEM provider
> > + *
> > + * Copyright (c) 2024 Daniel Golle <daniel@xxxxxxxxxxxxxx>
> > + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
> > + *
> > + * Useful on devices using a partition on an eMMC for MAC addresses or
> > + * Wi-Fi calibration EEPROM data.
> > + */
> > +
> > +#include <linux/file.h>
> > +#include <linux/nvmem-provider.h>
> > +#include <linux/nvmem-consumer.h>
> > +#include <linux/of.h>
> > +#include <linux/pagemap.h>
> > +#include <linux/property.h>
> > +
> > +#include "blk.h"
> > +
> > +static int blk_nvmem_reg_read(void *priv, unsigned int from,
> > +                           void *val, size_t bytes)
> > +{
> > +     blk_mode_t mode = BLK_OPEN_READ | BLK_OPEN_RESTRICT_WRITES;
> > +     dev_t devt = (dev_t)(uintptr_t)priv;
> > +     size_t bytes_left = bytes;
> > +     loff_t pos = from;
> > +     int ret = 0;
> > +
> > +     struct file *bdev_file __free(fput) = bdev_file_open_by_dev(devt, mode, priv, NULL);
> > +     if (IS_ERR(bdev_file))
> > +             return PTR_ERR(bdev_file);
> > +
> > +     while (bytes_left) {
> > +             pgoff_t f_index = pos >> PAGE_SHIFT;
> > +             struct folio *folio;
> > +             size_t folio_off;
> > +             size_t to_read;
> > +
> > +             folio = read_mapping_folio(bdev_file->f_mapping, f_index, NULL);
> > +             if (IS_ERR(folio)) {
> > +                     ret = PTR_ERR(folio);
> > +                     break;
> > +             }
> > +
> > +             folio_off = offset_in_folio(folio, pos);
> > +             to_read = min(bytes_left, folio_size(folio) - folio_off);
> > +             memcpy_from_folio(val, folio, folio_off, to_read);
> > +             pos += to_read;
> > +             bytes_left -= to_read;
> > +             val += to_read;
> > +             folio_put(folio);
> > +     }
> > +
> > +     return ret;
> > +}
> > +
> > +static int blk_nvmem_register(struct device *dev)
> > +{
> > +     struct block_device *bdev = dev_to_bdev(dev);
> > +     struct nvmem_config config = {};
> > +
> > +     /* skip devices which do not have a device tree node */
> > +     if (!dev_of_node(dev))
> > +             return 0;
> > +
> > +     /* skip devices without an nvmem layout defined */
> > +     struct device_node *child __free(device_node) =
> > +             of_get_child_by_name(dev_of_node(dev), "nvmem-layout");
> > +     if (!child)
> > +             return 0;
> > +
> > +     /*
> > +      * skip block device too large to be represented as NVMEM devices,
> > +      * the NVMEM reg_read callback uses an unsigned int offset
> > +      */
> > +     if (bdev_nr_bytes(bdev) > UINT_MAX) {
> > +             dev_warn(dev, "block device too large to be an NVMEM provider\n");
> > +             return -ENODEV;
>
> Wait, I must have suggested -ENODEV here on too little coffee. This callback
> is called from device_add(), not when the device is bound so it's not the same
> thing as returning -ENODEV from probe().
>
> On the other hand, we don't want to not provide the block device just because
> someone added a DT property on one that's too big. I'd say: warn, but return 0.
> Does it make sense?

It’s still technically an error in the sense that we cannot provide
the required nvmem feature. However, it only becomes a real issue if a
consumer actually attempts to use it.
Also, the block device should still be added, since the return code
from add_dev is not checked. In any case, I’m fine with either
approach, as long as we emit a warning message.

>
> > +     }
> > +
> > +     config.id = NVMEM_DEVID_NONE;
> > +     config.dev = dev;
> > +     config.name = dev_name(dev);
> > +     config.owner = THIS_MODULE;
> > +     config.priv = (void *)(uintptr_t)dev->devt;
> > +     config.reg_read = blk_nvmem_reg_read;
> > +     config.size = bdev_nr_bytes(bdev);
> > +     config.word_size = 1;
> > +     config.stride = 1;
> > +     config.read_only = true;
> > +     config.root_only = true;
> > +     config.ignore_wp = true;
> > +     config.of_node = to_of_node(dev->fwnode);
> > +
> > +     return PTR_ERR_OR_ZERO(devm_nvmem_register(dev, &config));
>
> And that was a wrong suggestion on my part too because I was under the
> impression that we're in the probe() path, not device_add(). You can't use
> devres here as the device at this point is not yet bound and may never be.

So I understand The bd_device is purely a class device with no bus, no driver.
For driverless devices, devres_release_all() is called explicitly
within device_del() .

>
> Which leads me to the second point: this is not the moment to add the nvmem
> provider. This should happen at or after probe(). Once nvmem_register()
> returns, you have a visible nvmem resource but nothing backing it in the block
> layer.

There is a short window during which a read attempt will 'properly'
fail, but this does seem somewhat fragile indeed.

> Either do this in block core when registering a new device or schedule
> a notifier here for the BUS_NOTIFY_BOUND_DRIVER event and do it in the notifier
> callback.

So in the end, it seems that the simpler and more robust approach is
probably to move away from the class_interface driver and instead
register/unregister the nvmem directly in add_disk/del_gendisk.
If that's ok I will move to this approach in the next version.

Regards,
Loic