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

[PATCH v3 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 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                            | 88 +++++++++++++++++++
 block/partitions/core.c                       | 40 +++++++++
 include/linux/blkdev.h                        | 10 +++
 8 files changed, 238 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 v3 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 v3 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 | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/block/partitions/core.c b/block/partitions/core.c
index ab76e64f0f6c..f88829e254e6 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,42 @@ static ssize_t whole_disk_show(struct device *dev,
 }
 static const DEVICE_ATTR(whole_disk, 0444, whole_disk_show, NULL);
 
+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;
+
+	fw_parts = device_get_named_child_node(ddev, "partitions");
+	if (!fw_parts)
+		fw_parts = device_get_named_child_node(ddev->parent, "partitions");
+
+	if (!fw_parts)
+		return NULL;
+
+	fwnode_for_each_child_node(fw_parts, fw_part) {
+		if (!fwnode_property_read_string(fw_part, "uuid", &uuid) &&
+		    (!bdev->bd_meta_info || strlen(uuid) > PARTITION_META_INFO_UUIDLTH ||
+		     strncmp(uuid, bdev->bd_meta_info->uuid, PARTITION_META_INFO_UUIDLTH)))
+			continue;
+
+		if (!fwnode_property_read_string(fw_part, "partname", &partname) &&
+		    (!bdev->bd_meta_info || strlen(uuid) > PARTITION_META_INFO_VOLNAMELTH ||
+		     strncmp(partname, bdev->bd_meta_info->volname,
+			     PARTITION_META_INFO_VOLNAMELTH)))
+			continue;
+
+		if (!fwnode_property_read_u32(fw_part, "partno", &partno) &&
+		    bdev_partno(bdev) != 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 +393,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 v3 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     | 88 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/blkdev.h |  8 ++++
 4 files changed, 103 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..594df7aa7be0
--- /dev/null
+++ b/block/blk-notify.c
@@ -0,0 +1,88 @@
+// 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..8d22ba03e3e1 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -1687,4 +1687,12 @@ static inline bool bdev_can_atomic_write(struct block_device *bdev)
 
 #define DEFINE_IO_COMP_BATCH(name)	struct io_comp_batch name = { }
 
+
+#ifdef CONFIG_BLOCK_NOTIFIERS
+#define BLK_DEVICE_ADD		1
+#define BLK_DEVICE_REMOVE	2
+void blk_register_notify(struct notifier_block *nb);
+void blk_unregister_notify(struct notifier_block *nb);
+#endif
+
 #endif /* _LINUX_BLKDEV_H */
-- 
2.45.2

[PATCH v3 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 8d22ba03e3e1..6c74c69d2ee1 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 v3 2/4] block: partitions: populate fwnode

[Jens Axboe]

On 6/25/24 8:50 PM, Daniel Golle wrote:
> diff --git a/block/partitions/core.c b/block/partitions/core.c
> index ab76e64f0f6c..f88829e254e6 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,42 @@ static ssize_t whole_disk_show(struct device *dev,
>  }
>  static const DEVICE_ATTR(whole_disk, 0444, whole_disk_show, NULL);
>  
> +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;
> +
> +	fw_parts = device_get_named_child_node(ddev, "partitions");
> +	if (!fw_parts)
> +		fw_parts = device_get_named_child_node(ddev->parent, "partitions");
> +
> +	if (!fw_parts)
> +		return NULL;

That last if check should to inside the previous one.

> +	fwnode_for_each_child_node(fw_parts, fw_part) {
> +		if (!fwnode_property_read_string(fw_part, "uuid", &uuid) &&
> +		    (!bdev->bd_meta_info || strlen(uuid) > PARTITION_META_INFO_UUIDLTH ||
> +		     strncmp(uuid, bdev->bd_meta_info->uuid, PARTITION_META_INFO_UUIDLTH)))
> +			continue;
> +
> +		if (!fwnode_property_read_string(fw_part, "partname", &partname) &&
> +		    (!bdev->bd_meta_info || strlen(uuid) > PARTITION_META_INFO_VOLNAMELTH ||
> +		     strncmp(partname, bdev->bd_meta_info->volname,
> +			     PARTITION_META_INFO_VOLNAMELTH)))
> +			continue;

This is pretty hard to make sense of...

-- 
Jens Axboe

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

[Jens Axboe]

On 6/25/24 8:51 PM, Daniel Golle wrote:
> +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;
> +}

Nit: redundant newline.

> +device_initcall(blk_notifications_init);
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index b2f1362c4681..8d22ba03e3e1 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -1687,4 +1687,12 @@ static inline bool bdev_can_atomic_write(struct block_device *bdev)
>  
>  #define DEFINE_IO_COMP_BATCH(name)	struct io_comp_batch name = { }
>  
> +
> +#ifdef CONFIG_BLOCK_NOTIFIERS

#if defined(CONFIG_BLOCK_NOTIFIERS)

> +#define BLK_DEVICE_ADD		1
> +#define BLK_DEVICE_REMOVE	2
> +void blk_register_notify(struct notifier_block *nb);
> +void blk_unregister_notify(struct notifier_block *nb);
> +#endif

Surely these helpers should have a !CONFIG_BLOCK_NOTIFIERS failure case
definition? Either that, or dummies. As it stands, any caller would need
to check if it's enabled or not.

-- 
Jens Axboe

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

[Daniel Golle]

Hi Jens,

thanks a lot for the review!

On Wed, Jun 26, 2024 at 01:46:50PM -0600, Jens Axboe wrote:
> On 6/25/24 8:51 PM, Daniel Golle wrote:
> > +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;
> > +}
> 
> Nit: redundant newline.

I'll remove the newline before the 'return' statement then, right?

> 
> > +device_initcall(blk_notifications_init);
> > diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> > index b2f1362c4681..8d22ba03e3e1 100644
> > --- a/include/linux/blkdev.h
> > +++ b/include/linux/blkdev.h
> > @@ -1687,4 +1687,12 @@ static inline bool bdev_can_atomic_write(struct block_device *bdev)
> >  
> >  #define DEFINE_IO_COMP_BATCH(name)	struct io_comp_batch name = { }
> >  
> > +
> > +#ifdef CONFIG_BLOCK_NOTIFIERS
> 
> #if defined(CONFIG_BLOCK_NOTIFIERS)
> 
> > +#define BLK_DEVICE_ADD		1
> > +#define BLK_DEVICE_REMOVE	2
> > +void blk_register_notify(struct notifier_block *nb);
> > +void blk_unregister_notify(struct notifier_block *nb);
> > +#endif
> 
> Surely these helpers should have a !CONFIG_BLOCK_NOTIFIERS failure case
> definition? Either that, or dummies. As it stands, any caller would need
> to check if it's enabled or not.

Makes sense. I'll add dummies to the header and always define
the macros for notification types.

Note that what I'm planning to do is to have the block nvmem provider
select CONFIG_BLOCK_NOTIFIERS in Kconfig, as without that it simply
won't work at all.


Cheers


Daniel

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

[Daniel Golle]

On Wed, Jun 26, 2024 at 01:43:49PM -0600, Jens Axboe wrote:
> On 6/25/24 8:50 PM, Daniel Golle wrote:
> > diff --git a/block/partitions/core.c b/block/partitions/core.c
> > index ab76e64f0f6c..f88829e254e6 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,42 @@ static ssize_t whole_disk_show(struct device *dev,
> >  }
> >  static const DEVICE_ATTR(whole_disk, 0444, whole_disk_show, NULL);
> >  
> > +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;
> > +
> > +	fw_parts = device_get_named_child_node(ddev, "partitions");
> > +	if (!fw_parts)
> > +		fw_parts = device_get_named_child_node(ddev->parent, "partitions");
> > +
> > +	if (!fw_parts)
> > +		return NULL;
> 
> That last if check should to inside the previous one.

Actually the previous one should have been removed entirely. I somehow
forgot to 'git add' that change.

> 
> > +	fwnode_for_each_child_node(fw_parts, fw_part) {
> > +		if (!fwnode_property_read_string(fw_part, "uuid", &uuid) &&
> > +		    (!bdev->bd_meta_info || strlen(uuid) > PARTITION_META_INFO_UUIDLTH ||
> > +		     strncmp(uuid, bdev->bd_meta_info->uuid, PARTITION_META_INFO_UUIDLTH)))
> > +			continue;
> > +
> > +		if (!fwnode_property_read_string(fw_part, "partname", &partname) &&
> > +		    (!bdev->bd_meta_info || strlen(uuid) > PARTITION_META_INFO_VOLNAMELTH ||
> > +		     strncmp(partname, bdev->bd_meta_info->volname,
> > +			     PARTITION_META_INFO_VOLNAMELTH)))
> > +			continue;
> 
> This is pretty hard to make sense of...

I'll add comments explaining it. Or should I use another syntax, e.g. several
nested if-clauses, instead?

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

[Jens Axboe]

On 6/26/24 1:58 PM, Daniel Golle wrote:
> On Wed, Jun 26, 2024 at 01:43:49PM -0600, Jens Axboe wrote:
>> On 6/25/24 8:50 PM, Daniel Golle wrote:
>>> diff --git a/block/partitions/core.c b/block/partitions/core.c
>>> index ab76e64f0f6c..f88829e254e6 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,42 @@ static ssize_t whole_disk_show(struct device *dev,
>>>  }
>>>  static const DEVICE_ATTR(whole_disk, 0444, whole_disk_show, NULL);
>>>  
>>> +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;
>>> +
>>> +	fw_parts = device_get_named_child_node(ddev, "partitions");
>>> +	if (!fw_parts)
>>> +		fw_parts = device_get_named_child_node(ddev->parent, "partitions");
>>> +
>>> +	if (!fw_parts)
>>> +		return NULL;
>>
>> That last if check should to inside the previous one.
> 
> Actually the previous one should have been removed entirely. I somehow
> forgot to 'git add' that change.
> 
>>
>>> +	fwnode_for_each_child_node(fw_parts, fw_part) {
>>> +		if (!fwnode_property_read_string(fw_part, "uuid", &uuid) &&
>>> +		    (!bdev->bd_meta_info || strlen(uuid) > PARTITION_META_INFO_UUIDLTH ||
>>> +		     strncmp(uuid, bdev->bd_meta_info->uuid, PARTITION_META_INFO_UUIDLTH)))
>>> +			continue;
>>> +
>>> +		if (!fwnode_property_read_string(fw_part, "partname", &partname) &&
>>> +		    (!bdev->bd_meta_info || strlen(uuid) > PARTITION_META_INFO_VOLNAMELTH ||
>>> +		     strncmp(partname, bdev->bd_meta_info->volname,
>>> +			     PARTITION_META_INFO_VOLNAMELTH)))
>>> +			continue;
>>
>> This is pretty hard to make sense of...
> 
> I'll add comments explaining it. Or should I use another syntax, e.g. several
> nested if-clauses, instead?

Maybe some kind of helpers for these instead, with comments? Nobody can
read the above.

-- 
Jens Axboe

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

[Jens Axboe]

On 6/26/24 1:55 PM, Daniel Golle wrote:
> Hi Jens,
> 
> thanks a lot for the review!
> 
> On Wed, Jun 26, 2024 at 01:46:50PM -0600, Jens Axboe wrote:
>> On 6/25/24 8:51 PM, Daniel Golle wrote:
>>> +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;
>>> +}
>>
>> Nit: redundant newline.
> 
> I'll remove the newline before the 'return' statement then, right?

Yup

>>> +device_initcall(blk_notifications_init);
>>> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
>>> index b2f1362c4681..8d22ba03e3e1 100644
>>> --- a/include/linux/blkdev.h
>>> +++ b/include/linux/blkdev.h
>>> @@ -1687,4 +1687,12 @@ static inline bool bdev_can_atomic_write(struct block_device *bdev)
>>>  
>>>  #define DEFINE_IO_COMP_BATCH(name)	struct io_comp_batch name = { }
>>>  
>>> +
>>> +#ifdef CONFIG_BLOCK_NOTIFIERS
>>
>> #if defined(CONFIG_BLOCK_NOTIFIERS)
>>
>>> +#define BLK_DEVICE_ADD		1
>>> +#define BLK_DEVICE_REMOVE	2
>>> +void blk_register_notify(struct notifier_block *nb);
>>> +void blk_unregister_notify(struct notifier_block *nb);
>>> +#endif
>>
>> Surely these helpers should have a !CONFIG_BLOCK_NOTIFIERS failure case
>> definition? Either that, or dummies. As it stands, any caller would need
>> to check if it's enabled or not.
> 
> Makes sense. I'll add dummies to the header and always define
> the macros for notification types.

Exactly

> Note that what I'm planning to do is to have the block nvmem provider
> select CONFIG_BLOCK_NOTIFIERS in Kconfig, as without that it simply
> won't work at all.

Right, but then someone else uses them for something else, and then
we'll need it anyway.

-- 
Jens Axboe