[PATCH v4 0/4] block: preparations for NVMEM provider

[PATCH v4 0/4] block: preparations for NVMEM provider

[Daniel Golle]

On embedded devices using an eMMC it is common that one or more (hw/sw)
partitions on the eMMC are used to store MAC addresses and Wi-Fi
calibration EEPROM data.

Typically the NVMEM framework is used to have kernel drivers read and
use binary data from EEPROMs, efuses, flash memory (MTD), ...

Using references to NVMEM bits in Device Tree allows the kernel to
access and apply e.g. the Ethernet MAC address, which can be a requirement
for userland to come up (e.g. for nfsroot).

The goal of this series is to prepare the block subsystem to allow for
the implementation of an NVMEM provider similar to other types of
non-volatile storage, so the same approach already used for EEPROMs, MTD
(raw flashes) and UBI-managed NAND can also be used for devices storing
those bits on an eMMC.

Define a device tree schema for block devices and partitions on them,
which (similar to how it now works also for UBI volumes) can be matched
by one or more properties.

Also add a simple notification API for other subsystems to be notified
about additions and removals of block devices, which is going to be used
by the block-backed NVMEM provider.

Overall, this enables uniform handling across practially all flash
storage types used for this purpose (MTD, UBI, and soon also MMC or and
in future maybe also other block devices).
---
Changes since v3 sent on Jun 26th, addressing comments from Jens Axboe:
 - improve readability and error-handling in fwnode-matching code
 - remove forgotten code from earlier development accessing ddev->parent
 - use '#if defined' instead of '#ifdef' in header
 - provide inline dummies in case of CONFIG_BLOCK_NOTIFIERS not being set

Changes since v2 sent on May 30th 2024 [1] addressing comments from
Hauke Mehrtens (https://patchwork.kernel.org/comment/25892133/)
 - Check length of UUID and PARTNAME.
 - Remove forgotten fallback to get 'partitions' subnode from parent.
   It is no longer needed and was a left over from earlier development.
 - Split series into 3 parts, one for each affected subsystem. This is
   the first part covering only the changes needed in the block
   subsystem. The second part adds the actual nvmem provider to
   drivers/nvmem/, the third part is going to make use of it for MMC
   block devices and cover changes in drivers/mmc.

Changes since v1 sent on March 21st 2024 [2]:
 - introduce notifications for block device addition and removal for
   in-kernel users. This allows the nvmem driver to be built as a module
   and avoids using class_interface and block subsystem internals as
   suggested in https://patchwork.kernel.org/comment/25771998/ and
   https://patchwork.kernel.org/comment/25770441/

This series has previously been submitted as RFC on July 19th 2023[3]
and most of the basic idea did not change since. Another round of RFC
was submitted on March 5th 2024[4].

[1]: https://patchwork.kernel.org/project/linux-block/list/?series=857192
[2]: https://patchwork.kernel.org/project/linux-block/list/?series=837150&archive=both
[3]: https://patchwork.kernel.org/project/linux-block/list/?series=767565
[4]: https://patchwork.kernel.org/project/linux-block/list/?series=832705


Daniel Golle (4):
  dt-bindings: block: add basic bindings for block devices
  block: partitions: populate fwnode
  block: add support for notifications
  block: add new genhd flag GENHD_FL_NVMEM

 .../bindings/block/block-device.yaml          | 22 +++++
 .../devicetree/bindings/block/partition.yaml  | 51 +++++++++++
 .../devicetree/bindings/block/partitions.yaml | 20 +++++
 block/Kconfig                                 |  6 ++
 block/Makefile                                |  1 +
 block/blk-notify.c                            | 87 +++++++++++++++++++
 block/partitions/core.c                       | 70 +++++++++++++++
 include/linux/blkdev.h                        | 13 +++
 8 files changed, 270 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/block/block-device.yaml
 create mode 100644 Documentation/devicetree/bindings/block/partition.yaml
 create mode 100644 Documentation/devicetree/bindings/block/partitions.yaml
 create mode 100644 block/blk-notify.c

-- 
2.45.2

[PATCH v4 1/4] dt-bindings: block: add basic bindings for block devices

[Daniel Golle]

Add bindings for block devices which are used to allow referencing
nvmem bits on them.

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 .../bindings/block/block-device.yaml          | 22 ++++++++
 .../devicetree/bindings/block/partition.yaml  | 51 +++++++++++++++++++
 .../devicetree/bindings/block/partitions.yaml | 20 ++++++++
 3 files changed, 93 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/block/block-device.yaml
 create mode 100644 Documentation/devicetree/bindings/block/partition.yaml
 create mode 100644 Documentation/devicetree/bindings/block/partitions.yaml

diff --git a/Documentation/devicetree/bindings/block/block-device.yaml b/Documentation/devicetree/bindings/block/block-device.yaml
new file mode 100644
index 000000000000..c83ea525650b
--- /dev/null
+++ b/Documentation/devicetree/bindings/block/block-device.yaml
@@ -0,0 +1,22 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/block/block-device.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: block storage device
+
+description: |
+  This binding is generic and describes a block-oriented storage device.
+
+maintainers:
+  - Daniel Golle <daniel@makrotopia.org>
+
+properties:
+  partitions:
+    $ref: /schemas/block/partitions.yaml
+
+  nvmem-layout:
+    $ref: /schemas/nvmem/layouts/nvmem-layout.yaml#
+
+unevaluatedProperties: false
diff --git a/Documentation/devicetree/bindings/block/partition.yaml b/Documentation/devicetree/bindings/block/partition.yaml
new file mode 100644
index 000000000000..86b61e30f9a4
--- /dev/null
+++ b/Documentation/devicetree/bindings/block/partition.yaml
@@ -0,0 +1,51 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/block/partition.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Partition on a block device
+
+description: |
+  This binding describes a partition on a block device.
+  Partitions may be matched by a combination of partition number, name,
+  and UUID.
+
+maintainers:
+  - Daniel Golle <daniel@makrotopia.org>
+
+properties:
+  $nodename:
+    pattern: '^block-partition-.+$'
+
+  partnum:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      Matches partition by number if present.
+
+  partname:
+    $ref: /schemas/types.yaml#/definitions/string
+    description:
+      Matches partition by PARTNAME if present.
+
+  partuuid:
+    $ref: /schemas/types.yaml#/definitions/string
+    description:
+      Matches partition by PARTUUID if present.
+
+  nvmem-layout:
+    $ref: /schemas/nvmem/layouts/nvmem-layout.yaml#
+    description:
+      This container may reference an NVMEM layout parser.
+
+anyOf:
+  - required:
+      - partnum
+
+  - required:
+      - partname
+
+  - required:
+      - partuuid
+
+unevaluatedProperties: false
diff --git a/Documentation/devicetree/bindings/block/partitions.yaml b/Documentation/devicetree/bindings/block/partitions.yaml
new file mode 100644
index 000000000000..fd84c3ba8493
--- /dev/null
+++ b/Documentation/devicetree/bindings/block/partitions.yaml
@@ -0,0 +1,20 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/block/partitions.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Partitions on block devices
+
+description: |
+  This binding is generic and describes the content of the partitions container
+  node.
+
+maintainers:
+  - Daniel Golle <daniel@makrotopia.org>
+
+patternProperties:
+  "^block-partition-.+$":
+    $ref: partition.yaml
+
+unevaluatedProperties: false
-- 
2.45.2

[PATCH v4 2/4] block: partitions: populate fwnode

[Daniel Golle]

Let block partitions to be represented by a firmware node and hence
allow them to being referenced e.g. for use with blk-nvmem.

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 block/partitions/core.c | 70 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 70 insertions(+)

diff --git a/block/partitions/core.c b/block/partitions/core.c
index ab76e64f0f6c..edf582de741f 100644
--- a/block/partitions/core.c
+++ b/block/partitions/core.c
@@ -10,6 +10,8 @@
 #include <linux/ctype.h>
 #include <linux/vmalloc.h>
 #include <linux/raid/detect.h>
+#include <linux/property.h>
+
 #include "check.h"
 
 static int (*const check_part[])(struct parsed_partitions *) = {
@@ -281,6 +283,72 @@ static ssize_t whole_disk_show(struct device *dev,
 }
 static const DEVICE_ATTR(whole_disk, 0444, whole_disk_show, NULL);
 
+static bool part_meta_match(const char *attr, const char *member, size_t length)
+{
+	/* check if length of attr exceeds specified maximum length */
+	if (strnlen(attr, length) == length)
+		return false;
+
+	/* return true if strings match */
+	return !strncmp(attr, member, length);
+}
+
+static struct fwnode_handle *find_partition_fwnode(struct block_device *bdev)
+{
+	struct fwnode_handle *fw_parts, *fw_part;
+	struct device *ddev = disk_to_dev(bdev->bd_disk);
+	const char *partname, *uuid;
+	u32 partno;
+	bool got_uuid, got_partname, got_partno;
+
+	fw_parts = device_get_named_child_node(ddev, "partitions");
+	if (!fw_parts)
+		return NULL;
+
+	fwnode_for_each_child_node(fw_parts, fw_part) {
+		got_uuid = false;
+		got_partname = false;
+		got_partno = false;
+		/*
+		 * In case 'uuid' is defined in the partitions firmware node require
+		 * partition meta info being present and the specified uuid to match.
+		 */
+		got_uuid = !fwnode_property_read_string(fw_part, "uuid", &uuid);
+		if (got_uuid && (!bdev->bd_meta_info ||
+				 !part_meta_match(uuid, bdev->bd_meta_info->uuid,
+						  PARTITION_META_INFO_UUIDLTH)))
+			continue;
+
+		/*
+		 * In case 'partname' is defined in the partitions firmware node require
+		 * partition meta info being present and the specified volname to match.
+		 */
+		got_partname = !fwnode_property_read_string(fw_part, "partname",
+							    &partname);
+		if (got_partname && (!bdev->bd_meta_info ||
+				     !part_meta_match(partname,
+						      bdev->bd_meta_info->volname,
+						      PARTITION_META_INFO_VOLNAMELTH)))
+			continue;
+
+		/*
+		 * In case 'partno' is defined in the partitions firmware node the
+		 * specified partno needs to match.
+		 */
+		got_partno = !fwnode_property_read_u32(fw_part, "partno", &partno);
+		if (got_partno && bdev_partno(bdev) != partno)
+			continue;
+
+		/* Skip if no matching criteria is present in firmware node */
+		if (!got_uuid && !got_partname && !got_partno)
+			continue;
+
+		return fw_part;
+	}
+
+	return NULL;
+}
+
 /*
  * Must be called either with open_mutex held, before a disk can be opened or
  * after all disk users are gone.
@@ -355,6 +423,8 @@ static struct block_device *add_partition(struct gendisk *disk, int partno,
 			goto out_put;
 	}
 
+	device_set_node(pdev, find_partition_fwnode(bdev));
+
 	/* delay uevent until 'holders' subdir is created */
 	dev_set_uevent_suppress(pdev, 1);
 	err = device_add(pdev);
-- 
2.45.2

[PATCH v4 3/4] block: add support for notifications

[Daniel Golle]

Add notifier block to notify other subsystems about the addition or
removal of block devices.

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 block/Kconfig          |  6 +++
 block/Makefile         |  1 +
 block/blk-notify.c     | 87 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/blkdev.h | 11 ++++++
 4 files changed, 105 insertions(+)
 create mode 100644 block/blk-notify.c

diff --git a/block/Kconfig b/block/Kconfig
index 5b623b876d3b..67cd4f92378a 100644
--- a/block/Kconfig
+++ b/block/Kconfig
@@ -209,6 +209,12 @@ config BLK_INLINE_ENCRYPTION_FALLBACK
 	  by falling back to the kernel crypto API when inline
 	  encryption hardware is not present.
 
+config BLOCK_NOTIFIERS
+	bool "Enable support for notifications in block layer"
+	help
+	  Enable this option to provide notifiers for other subsystems
+	  upon addition or removal of block devices.
+
 source "block/partitions/Kconfig"
 
 config BLK_MQ_PCI
diff --git a/block/Makefile b/block/Makefile
index ddfd21c1a9ff..a131fa7d6b26 100644
--- a/block/Makefile
+++ b/block/Makefile
@@ -38,3 +38,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_BLOCK_NOTIFIERS) += blk-notify.o
diff --git a/block/blk-notify.c b/block/blk-notify.c
new file mode 100644
index 000000000000..fd727288ea19
--- /dev/null
+++ b/block/blk-notify.c
@@ -0,0 +1,87 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Notifiers for addition and removal of block devices
+ *
+ * Copyright (c) 2024 Daniel Golle <daniel@makrotopia.org>
+ */
+
+#include <linux/list.h>
+#include <linux/mutex.h>
+#include <linux/notifier.h>
+
+#include "blk.h"
+
+struct blk_device_list {
+	struct device *dev;
+	struct list_head list;
+};
+
+static RAW_NOTIFIER_HEAD(blk_notifier_list);
+static DEFINE_MUTEX(blk_notifier_lock);
+static LIST_HEAD(blk_devices);
+
+void blk_register_notify(struct notifier_block *nb)
+{
+	struct blk_device_list *existing_blkdev;
+
+	mutex_lock(&blk_notifier_lock);
+	raw_notifier_chain_register(&blk_notifier_list, nb);
+
+	list_for_each_entry(existing_blkdev, &blk_devices, list)
+		nb->notifier_call(nb, BLK_DEVICE_ADD, existing_blkdev->dev);
+
+	mutex_unlock(&blk_notifier_lock);
+}
+EXPORT_SYMBOL_GPL(blk_register_notify);
+
+void blk_unregister_notify(struct notifier_block *nb)
+{
+	mutex_lock(&blk_notifier_lock);
+	raw_notifier_chain_unregister(&blk_notifier_list, nb);
+	mutex_unlock(&blk_notifier_lock);
+}
+EXPORT_SYMBOL_GPL(blk_unregister_notify);
+
+static int blk_call_notifier_add(struct device *dev)
+{
+	struct blk_device_list *new_blkdev;
+
+	new_blkdev = kmalloc(sizeof(*new_blkdev), GFP_KERNEL);
+	if (!new_blkdev)
+		return -ENOMEM;
+
+	new_blkdev->dev = dev;
+	mutex_lock(&blk_notifier_lock);
+	list_add_tail(&new_blkdev->list, &blk_devices);
+	raw_notifier_call_chain(&blk_notifier_list, BLK_DEVICE_ADD, dev);
+	mutex_unlock(&blk_notifier_lock);
+	return 0;
+}
+
+static void blk_call_notifier_remove(struct device *dev)
+{
+	struct blk_device_list *old_blkdev, *tmp;
+
+	mutex_lock(&blk_notifier_lock);
+	list_for_each_entry_safe(old_blkdev, tmp, &blk_devices, list) {
+		if (old_blkdev->dev != dev)
+			continue;
+
+		list_del(&old_blkdev->list);
+		kfree(old_blkdev);
+	}
+	raw_notifier_call_chain(&blk_notifier_list, BLK_DEVICE_REMOVE, dev);
+	mutex_unlock(&blk_notifier_lock);
+}
+
+static struct class_interface blk_notifications_bus_interface __refdata = {
+	.class = &block_class,
+	.add_dev = &blk_call_notifier_add,
+	.remove_dev = &blk_call_notifier_remove,
+};
+
+static int __init blk_notifications_init(void)
+{
+	return class_interface_register(&blk_notifications_bus_interface);
+}
+device_initcall(blk_notifications_init);
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index b2f1362c4681..c6e9fb16f76e 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -1687,4 +1687,15 @@ static inline bool bdev_can_atomic_write(struct block_device *bdev)
 
 #define DEFINE_IO_COMP_BATCH(name)	struct io_comp_batch name = { }
 
+
+#define BLK_DEVICE_ADD		1
+#define BLK_DEVICE_REMOVE	2
+#if defined(CONFIG_BLOCK_NOTIFIERS)
+void blk_register_notify(struct notifier_block *nb);
+void blk_unregister_notify(struct notifier_block *nb);
+#else
+static inline void blk_register_notify(struct notifier_block *nb) { };
+static inline void blk_unregister_notify(struct notifier_block *nb) { };
+#endif
+
 #endif /* _LINUX_BLKDEV_H */
-- 
2.45.2

[PATCH v4 4/4] block: add new genhd flag GENHD_FL_NVMEM

[Daniel Golle]

Add new flag to destinguish block devices which may act as an NVMEM
provider.

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 include/linux/blkdev.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index c6e9fb16f76e..c6c6153dbede 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -81,11 +81,13 @@ struct partition_meta_info {
  * ``GENHD_FL_NO_PART``: partition support is disabled.  The kernel will not
  * scan for partitions from add_disk, and users can't add partitions manually.
  *
+ * ``GENHD_FL_NVMEM``: the block device should be considered as NVMEM provider.
  */
 enum {
 	GENHD_FL_REMOVABLE			= 1 << 0,
 	GENHD_FL_HIDDEN				= 1 << 1,
 	GENHD_FL_NO_PART			= 1 << 2,
+	GENHD_FL_NVMEM				= 1 << 3,
 };
 
 enum {
-- 
2.45.2

Re: [PATCH v4 0/4] block: preparations for NVMEM provider

[Jens Axboe]

On 6/27/24 2:49 PM, Daniel Golle wrote:
> On embedded devices using an eMMC it is common that one or more (hw/sw)
> partitions on the eMMC are used to store MAC addresses and Wi-Fi
> calibration EEPROM data.
> 
> Typically the NVMEM framework is used to have kernel drivers read and
> use binary data from EEPROMs, efuses, flash memory (MTD), ...
> 
> Using references to NVMEM bits in Device Tree allows the kernel to
> access and apply e.g. the Ethernet MAC address, which can be a requirement
> for userland to come up (e.g. for nfsroot).
> 
> The goal of this series is to prepare the block subsystem to allow for
> the implementation of an NVMEM provider similar to other types of
> non-volatile storage, so the same approach already used for EEPROMs, MTD
> (raw flashes) and UBI-managed NAND can also be used for devices storing
> those bits on an eMMC.
> 
> Define a device tree schema for block devices and partitions on them,
> which (similar to how it now works also for UBI volumes) can be matched
> by one or more properties.
> 
> Also add a simple notification API for other subsystems to be notified
> about additions and removals of block devices, which is going to be used
> by the block-backed NVMEM provider.
> 
> Overall, this enables uniform handling across practially all flash
> storage types used for this purpose (MTD, UBI, and soon also MMC or and
> in future maybe also other block devices).

2-4 look fine to me now, but I don't know anything about the device
bindings so someone qualified should review that before it gets queued
up.
 
-- 
Jens Axboe

Re: [PATCH v4 2/4] block: partitions: populate fwnode

[Christoph Hellwig]

On Thu, Jun 27, 2024 at 09:50:39PM +0100, Daniel Golle wrote:
> +		/*
> +		 * In case 'uuid' is defined in the partitions firmware node require
> +		 * partition meta info being present and the specified uuid to match.
> +		 */

Overly long lines, which is really annyoing for block comments.

> +		got_uuid = !fwnode_property_read_string(fw_part, "uuid", &uuid);
> +		if (got_uuid && (!bdev->bd_meta_info ||
> +				 !part_meta_match(uuid, bdev->bd_meta_info->uuid,
> +						  PARTITION_META_INFO_UUIDLTH)))

Can we please not use the crazy part_meta stuff for anything new?
We should never have merge it, and right now it is at least isolated
to the boot time root dev_t partsing, and I'd really prefer to keep it
in that corner.

Re: [PATCH v4 3/4] block: add support for notifications

[Christoph Hellwig]

On Thu, Jun 27, 2024 at 09:50:50PM +0100, Daniel Golle wrote:
> Add notifier block to notify other subsystems about the addition or
> removal of block devices.

Notification for what?  I really hate the concept of random modular
code being able to hook into device discovery / partition scanning.
And not actually having a user for it is a complete no-go.

Re: [PATCH v4 2/4] block: partitions: populate fwnode

[Daniel Golle]

Hi Christoph,

thank you for reviewing.

On Thu, Jun 27, 2024 at 09:44:28PM -0700, Christoph Hellwig wrote:
> On Thu, Jun 27, 2024 at 09:50:39PM +0100, Daniel Golle wrote:
> > +		/*
> > +		 * In case 'uuid' is defined in the partitions firmware node require
> > +		 * partition meta info being present and the specified uuid to match.
> > +		 */
> 
> Overly long lines, which is really annyoing for block comments.

Should I use 80 chars as limit everywhere?

> 
> > +		got_uuid = !fwnode_property_read_string(fw_part, "uuid", &uuid);
> > +		if (got_uuid && (!bdev->bd_meta_info ||
> > +				 !part_meta_match(uuid, bdev->bd_meta_info->uuid,
> > +						  PARTITION_META_INFO_UUIDLTH)))
> 
> Can we please not use the crazy part_meta stuff for anything new?
> We should never have merge it, and right now it is at least isolated
> to the boot time root dev_t partsing, and I'd really prefer to keep it
> in that corner.
> 

At least up to my understanding there isn't any other to know a
partitions UUID or volume name.

If there is another way to access this information I'd happily make
use of it, but I couldn't find any.

Re: [PATCH v4 3/4] block: add support for notifications

[Daniel Golle]

On Thu, Jun 27, 2024 at 09:45:45PM -0700, Christoph Hellwig wrote:
> On Thu, Jun 27, 2024 at 09:50:50PM +0100, Daniel Golle wrote:
> > Add notifier block to notify other subsystems about the addition or
> > removal of block devices.
> 
> Notification for what?  I really hate the concept of random modular
> code being able to hook into device discovery / partition scanning.

Adding a dedicated notification interface (instead of using block
internals in the nvmem driver) has been requested in a previous review
by Bart Van Assche:

https://patchwork.kernel.org/comment/25771998/

Quote from that previous review comment:
>>> 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.
>
> That's an argument for adding an interface to the block layer that
> implements this functionality but not for adding this code in the block
> layer.
---

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.


Thank you for taking a look at this. Let me know if anything else
is not clear or needs to be changed.


Cheers


Daniel

Re: [PATCH v4 2/4] block: partitions: populate fwnode

[Christoph Hellwig]

On Fri, Jun 28, 2024 at 01:16:13PM +0100, Daniel Golle wrote:
> > Overly long lines, which is really annyoing for block comments.
> 
> Should I use 80 chars as limit everywhere?

In my opinion that makes things easier.  The coding style allows to
exceed it for individual lines where it improves readability, which
is a bit of an odd case.

> > Can we please not use the crazy part_meta stuff for anything new?
> > We should never have merge it, and right now it is at least isolated
> > to the boot time root dev_t partsing, and I'd really prefer to keep it
> > in that corner.
> > 
> 
> At least up to my understanding there isn't any other to know a
> partitions UUID or volume name.
> 
> If there is another way to access this information I'd happily make
> use of it, but I couldn't find any.

That is true, but except for the early dev_t parsing we actually never
use these partition uuids in the kernel at all.  Also in all the
normal file system references either use the file system uuid,
or the device provided one for devices that support them.  Most of
that is handled entirely in userspace, though - the kernel largely
operates just on the dev_t.

Re: [PATCH v4 3/4] block: add support for notifications

[Christoph Hellwig]

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 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.

Re: [PATCH v4 3/4] block: add support for notifications

[Daniel Golle]

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

Re: [PATCH v4 3/4] block: add support for notifications

[Christoph Hellwig]

On Fri, Jun 28, 2024 at 03:00:05PM +0100, Daniel Golle wrote:
> 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.

Maybe it needs to become more common so that we don't need crazy
kernel workarounds for something that can be trivially done with
a few lines of userspace code?