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

[Daniel Golle]

Hi Bart,

thank you for looking at the patches!

On Thu, Mar 21, 2024 at 12:44:19PM -0700, Bart Van Assche wrote:
> On 3/21/24 12:34, Daniel Golle wrote:
> > 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.
> 
> Why to store calibration data in a partition instead of in a file on a
> filesystem?

First of all, it's just how it is already in the practical world out
there. The same methods for mass-production are used independently of
the type of flash memory, so vendors don't care if in Linux the flash
ends up as MMC/block (in case of an eMMC) device or MTD device (in
case of SPI-NOR, for example). I can name countless devices of
numerous vendors following this generally very common practise (and
then ending up extracting that using ugly custom drivers, or poking
around in the block devices in early userland, ... none of it is nice,
which is the motivation for this series).
Adtran, GL-iNet, Netgear, ... to name just a few very popular vendors.

The devices are already out there, and the way they store those
details is considered part of the low level firmware which will never
change. Yet it would be nice to run vanilla Linux on them (or
OpenWrt), and make sure things like NFS root can work, and for that
the MAC address needs to be in place already, ie. extracting it in
userland would be too late.

However, I also believe there is nothing wrong with that and using a
filesystem comes with many additional pitfalls, such as being possibly
not cleanly unmounted, the file could be renamed or deleted by the
user, .... All that should not result in a device not having it's
proper MAC address any more.

Why have all the complexity for something as simple as storing 6 bytes
of MAC address?

I will not re-iterate over all that discussion now, you may look at
list archives where this has been explained and discussed also for the
first run of the RFC series last year.

> 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 8c88f362feb55..242a0a139c00a 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -3662,6 +3662,11 @@ L:	linux-mtd@xxxxxxxxxxxxxxxxxxx
> >   S:	Maintained
> >   F:	drivers/mtd/devices/block2mtd.c
> > +BLOCK NVMEM DRIVER
> > +M:	Daniel Golle <daniel@xxxxxxxxxxxxxx>
> > +S:	Maintained
> > +F:	block/blk-nvmem.c
> 
> Why to add this functionality to the block layer instead of somewhere
> in the drivers/ directory?

Simply because we need notifications about appearing and disappearing
block devices, or a way to iterate over all block devices in a system.
For both there isn't currently any other interface than using a
class_interface for that, and that requires access to &block_class
which is considered a block subsystem internal.

Also note that the same is true for the MTD NVMEM provider (in
drivers/mtd/mtdcore.c) as well as the UBI NVMEM provider (in
drivers/mtd/ubi/nvmem.c), both are considered an integral part of
their corresponding subsystems -- despite the fact that in those cases
this wouldn't even be stricktly needed as for MTD we got
register_mtd_user() and for UBI we'd have
ubi_register_volume_notifier().

Doing it differently for block devices would hence not only complicate
things unnessesarily, it would also be inconsistent.