Date: Fri, 28 Jun 2024 15:00:05 +0100
From: Daniel Golle <daniel@...rotopia.org>
To: Christoph Hellwig <hch@...radead.org>
Cc: Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Ulf Hansson <ulf.hansson@...aro.org>, Jens Axboe <axboe@...nel.dk>,
	Hauke Mehrtens <hauke@...ke-m.de>, Felix Fietkau <nbd@....name>,
	Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
	Dave Chinner <dchinner@...hat.com>, Jan Kara <jack@...e.cz>,
	Christian Brauner <brauner@...nel.org>,
	Thomas Weißschuh <linux@...ssschuh.net>,
	Al Viro <viro@...iv.linux.org.uk>,
	Li Lingfeng <lilingfeng3@...wei.com>,
	Christian Heusel <christian@...sel.eu>,
	Min Li <min15.li@...sung.com>, Avri Altman <avri.altman@....com>,
	Adrian Hunter <adrian.hunter@...el.com>,
	Hannes Reinecke <hare@...e.de>,
	Mikko Rapeli <mikko.rapeli@...aro.org>, Yeqi Fu <asuk4.q@...il.com>,
	Victor Shih <victor.shih@...esyslogic.com.tw>,
	Christophe JAILLET <christophe.jaillet@...adoo.fr>,
	Li Zhijian <lizhijian@...itsu.com>,
	"Ricardo B. Marliere" <ricardo@...liere.net>,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-mmc@...r.kernel.org, linux-block@...r.kernel.org
Subject: Re: [PATCH v4 3/4] block: add support for notifications

Hi Christoph,

On Fri, Jun 28, 2024 at 05:50:20AM -0700, Christoph Hellwig wrote:
> On Fri, Jun 28, 2024 at 01:23:47PM +0100, Daniel Golle wrote:
> > So that's what I did consequently. Using the notification interface
> > the NVMEM driver can live in drivers/nvmem/ and doesn't need to be
> > using block internals.
> > 
> > > And not actually having a user for it is a complete no-go.
> > > 
> > 
> > The user will be the nvmem provider, you can see the code in earlier
> > versions of the patchset where I had included it:
> > 
> > https://patchwork.kernel.org/project/linux-block/patch/96554d6b4d9fa72f936c2c476eb0b023cdd60a64.1717031992.git.daniel@makrotopia.org/
> > 
> > Being another subsystem I thought it'd be better to deal with the
> > block related things first, and once that has been sorted out I will
> > move on to add the NVMEM driver and make the necessary changes for
> > using it on eMMC.
> 
> It is rather hard to review an interface without the users.

I understand that, please take a look at previous iterations of this very
series or use the link to the patch which I have sent before (see above).

> 
> I still dislike the idea of notifications from bdev discovery /
> partition scanning into the users of them.  We have one such users
> in the MD legacy autodetect code, but that has largely been considered
> at bad idea and distros tend to use mdadm based assembly from initramfs
> instead.  Which IMHO feels like the right thing for nvmem as well,
> just have an nvmem provider that opens a file for a user provided
> path and use kernel_read on it.  This can covered block devices,
> character devices and even regular files.  It will require initramfs
> support, but that is pretty much used everywhere for storage discovery
> and setup anyway.

The problem there is that then we cannot use Device Tree to device the
NVMEM layouts, and reference NVMEM bits to the dirvers which need them.
Hence also the definition of the NVMEM layout would have to happen in
userspace, inside an initramfs. I know that having an initramfs is
common for classic desktop or server distributions, but the same is not
true on more simple embedded devices such as router/firewall or WiFi
access point appliances running OpenWrt.

Carrying all that board-specific knowledge in the form of scripts
identifying the board is not exactly nice, nor is creating an individual
initramfs for each of the 1000+ boards supported by OpenWrt, for
example. Extracting the layout information from /sys/firmware/devicetree
in userspace just to then somehow use libblkid to identify the block
device and throw that information back at the kernel which requested it
e.g. using a firmware hotplug call is an option, but imho an unnecessary
complication. And obviously it would still prevent things like nfsroot
(which requires the MAC address and potentially a WiFi calibration data
to be setup) from working out of the box.

Doing the detour via userspace only for devices with an eMMC would be
different from how it is done for any other type of backing storage such
as simple I2C EEPROMs, (SPI-)flashes or UBI volumes.
Having a unified approach for all of them would make things much more
convenient, as typically the actual layouts are even the same and can be
reused accross devices of the same vendor. GL-iNet or ASUS router
devices are a good example for that: The more high-end ones come with an
eMMC and use the same NVMEM layout inside a GPT partition than mid-field
devices on SPI-NAND or UBI, and older and/or lower-end devices on an
SPI-NOR flash MTD partition.

To better understand the situation, maybe look at a few examples for
which we are currently already using this patchset downstream at
OpenWrt:

Inside a GPT partition on an eMMC:
https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob;f=target/linux/mediatek/dts/mt7981b-unielec-u7981-01-emmc.dts;h=abd4d4e59d74cc0d24e8c9e10e16c0859844e003;hb=HEAD#l39

On raw SPI-NAND (already possible with vanilla Linux):
https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob;f=target/linux/mediatek/dts/mt7981b-unielec-u7981-01-nand.dts;h=230a612a34f7d370eb09e92284950d9949bf10cd;hb=HEAD#l45

Inside a UBI volume (also already possible with vanilla Linux):
https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob;f=target/linux/mediatek/dts/mt7986a-asus-tuf-ax4200.dts;h=e40602fa215e1a677b534c85b767e824af041518;hb=HEAD#l255

Inside the boot hwpartition of the eMMC:
https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob;f=target/linux/mediatek/dts/mt7981b-glinet-gl-mt2500.dts;h=15818a90fca02955879d1bcca55ba2153e390621;hb=HEAD#l156

Inside a GPT partition on an eMMC:
https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob;f=target/linux/mediatek/dts/mt7986a-glinet-gl-mt6000.dts;h=fd0e1a69157ed0f27c32376aabab0fcc85ce23b9;hb=HEAD#l317

https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob;f=target/linux/mediatek/dts/mt7988a-smartrg-mt-stuart.dtsi;h=2b468f9bb311141d083e51ca5edaa7dce253c91c;hb=HEAD#l504

Those are just the examples I have been working on myself, so it was
easy to come up with them. There are also ASUS devices with Qualcomm
SoC using the same layout as the MediaTek-based devices inside a UBI
volume.

Once a unified way to describe the loaction of the NVMEM bits is also
present in upstream Linux, all those downstream device trees could be
submitted for inclusion in Linux, and one could install a
general-purpose OS like Debian **which wouldn't need to know anything
about the details of where to read MAC addresses or calibration data
from**, all hardware-specific knowledge would reside in DT.

The failure of defining this in a nice way results in very ugly
situations such as distributions carrying the (board-specific!)
calibration data for very few but very common single-board computers
like the RaspberryPi in their rootfs or even in initramfs. Each
distribution with a different level of hacks to give them individual MAC
addresses and to load the correct file... And while this doesn't look so
bad for systems which anyway come only with removable microSD storage,
it **does* get ugly when it comes to systems which do store that
information on their eMMC (typically "appliances" rather than SBCs meant
for tinkerers).

Please take all that into consiration and also note that obviously, on
systems not making use of any of that, you may simple not enable
CONFIG_BLOCK_NOTIFIERS -- other than in previous iterations of the
patchset this is all completely optional now.


Cheers


Daniel

Powered by blists - more mailing lists