[PATCH 0/8] nvmem: Let layout drivers be modules

[PATCH 0/8] nvmem: Let layout drivers be modules

[Miquel Raynal]

Hello,

Following Greg's opposition to merge the current nvmem layout support
proposal [1], arguing that it would eventually grow the size of the
Linux kernel and asking for some "modularization" support, here is a
proposal to turn layout drivers into regular tristate drivers.

The first three patches are preparation changes in order to extend (and
fix) a little bit the of/device.c support. The fix does not seem to
impact most of the current users so I guess it can live with the rest of
the series in order to avoid future merge conflicts.

The nvmem core is then extended to support the absence of layouts and
possibly lead to probe deferrals when relevant.

Finally, the two existing layout drivers are converted into modules and
their Kconfig symbols changed to tristate.

The base series on which these changes apply is still contained in [1],
I would prefer to keep it as it was and apply this series on top of it.

Tests have been conducted on a Marvell Prestera switch with the mvpp2
Ethernet driver calling for a MAC address stored in the ONIE TLV table
available through a layout driver in an EEPROM/MTD device.

[1] https://github.com/miquelraynal/linux/tree/nvmem-next/layouts

Thanks,
Miquèl

Miquel Raynal (8):
  of: Fix modalias string generation
  of: Change of_device_get_modalias() main argument
  of: Create an of_device_request_module() receiving an OF node
  nvmem: core: Fix error path ordering
  nvmem: core: Handle the absence of expected layouts
  nvmem: core: Request layout modules loading
  nvmem: layouts: sl28vpd: Convert layout driver into a module
  nvmem: layouts: onie-tlv: Convert layout driver into a module

 drivers/nvmem/core.c             | 20 ++++++++++++--
 drivers/nvmem/layouts/Kconfig    |  4 +--
 drivers/nvmem/layouts/onie-tlv.c | 15 ++++++++++-
 drivers/nvmem/layouts/sl28vpd.c  | 14 +++++++++-
 drivers/of/device.c              | 45 ++++++++++++++++++++++----------
 include/linux/of_device.h        |  6 +++++
 6 files changed, 84 insertions(+), 20 deletions(-)

-- 
2.34.1

[PATCH 1/8] of: Fix modalias string generation

[Miquel Raynal]

The helper generating an OF based modalias (of_device_get_modalias())
works fine, but due to the use of snprintf() internally it needs a
buffer one byte longer than what should be needed just for the entire
string (excluding the '\0'). Most users of this helper are sysfs hooks
providing the modalias string to users. They all provide a PAGE_SIZE
buffer which is way above the number of bytes required to fit the
modalias string and hence do not suffer from this issue.

There is another user though, of_device_request_module(), which is only
called by drivers/usb/common/ulpi.c. This request module function is
faulty, but maybe because in most cases there is an alternative, ULPI
driver users have not noticed it.

In this function, of_device_get_modalias() is called twice. The first
time without buffer just to get the number of bytes required by the
modalias string (excluding the null byte), and a second time, after
buffer allocation, to fill the buffer. The allocation asks for an
additional byte, in order to store the trailing '\0'. However, the
buffer *length* provided to of_device_get_modalias() excludes this extra
byte. The internal use of snprintf() with a length that is exactly the
number of bytes to be written has the effect of using the last available
byte to store a '\0', which then smashes the last character of the
modalias string.

Provide the actual size of the buffer to of_device_get_modalias() to fix
this issue.

Note: the "str[size - 1] = '\0';" line is not really needed as snprintf
will anyway end the string with a null byte, but there is a possibility
that this function might be called on a struct device_node without
compatible, in this case snprintf() would not be executed. So we keep it
just to avoid possible unbounded strings.

Cc: Stephen Boyd <stephen.boyd@linaro.org>
Cc: Peter Chen <peter.chen@nxp.com>
Fixes: 9c829c097f2f ("of: device: Support loading a module with OF based modalias")
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/of/device.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/of/device.c b/drivers/of/device.c
index c674a13c3055..877f50379fab 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -297,12 +297,15 @@ int of_device_request_module(struct device *dev)
 	if (size < 0)
 		return size;
 
-	str = kmalloc(size + 1, GFP_KERNEL);
+	/* Reserve an additional byte for the trailing '\0' */
+	size++;
+
+	str = kmalloc(size, GFP_KERNEL);
 	if (!str)
 		return -ENOMEM;
 
 	of_device_get_modalias(dev, str, size);
-	str[size] = '\0';
+	str[size - 1] = '\0';
 	ret = request_module(str);
 	kfree(str);
 
-- 
2.34.1

[PATCH 2/8] of: Change of_device_get_modalias() main argument

[Miquel Raynal]

This function needs "struct device_node" to work, but for convenience
the author and only user of this helper did use a "struct device". As
this helper is a static helper, let's keep the "struct device" for
exported methods and use the OF structure internally.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/of/device.c | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/drivers/of/device.c b/drivers/of/device.c
index 877f50379fab..3efc17de1d57 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -248,7 +248,7 @@ const void *of_device_get_match_data(const struct device *dev)
 }
 EXPORT_SYMBOL(of_device_get_match_data);
 
-static ssize_t of_device_get_modalias(struct device *dev, char *str, ssize_t len)
+static ssize_t of_device_get_modalias(struct device_node *np, char *str, ssize_t len)
 {
 	const char *compat;
 	char *c;
@@ -256,19 +256,16 @@ static ssize_t of_device_get_modalias(struct device *dev, char *str, ssize_t len
 	ssize_t csize;
 	ssize_t tsize;
 
-	if ((!dev) || (!dev->of_node))
-		return -ENODEV;
-
 	/* Name & Type */
 	/* %p eats all alphanum characters, so %c must be used here */
-	csize = snprintf(str, len, "of:N%pOFn%c%s", dev->of_node, 'T',
-			 of_node_get_device_type(dev->of_node));
+	csize = snprintf(str, len, "of:N%pOFn%c%s", np, 'T',
+			 of_node_get_device_type(np));
 	tsize = csize;
 	len -= csize;
 	if (str)
 		str += csize;
 
-	of_property_for_each_string(dev->of_node, "compatible", p, compat) {
+	of_property_for_each_string(np, "compatible", p, compat) {
 		csize = strlen(compat) + 1;
 		tsize += csize;
 		if (csize > len)
@@ -293,7 +290,10 @@ int of_device_request_module(struct device *dev)
 	ssize_t size;
 	int ret;
 
-	size = of_device_get_modalias(dev, NULL, 0);
+	if (!dev || !dev->of_node)
+		return -ENODEV;
+
+	size = of_device_get_modalias(dev->of_node, NULL, 0);
 	if (size < 0)
 		return size;
 
@@ -304,7 +304,7 @@ int of_device_request_module(struct device *dev)
 	if (!str)
 		return -ENOMEM;
 
-	of_device_get_modalias(dev, str, size);
+	of_device_get_modalias(dev->of_node, str, size);
 	str[size - 1] = '\0';
 	ret = request_module(str);
 	kfree(str);
@@ -321,7 +321,12 @@ EXPORT_SYMBOL_GPL(of_device_request_module);
  */
 ssize_t of_device_modalias(struct device *dev, char *str, ssize_t len)
 {
-	ssize_t sl = of_device_get_modalias(dev, str, len - 2);
+	ssize_t sl;
+
+	if ((!dev) || (!dev->of_node))
+		return -ENODEV;
+
+	sl = of_device_get_modalias(dev->of_node, str, len - 2);
 	if (sl < 0)
 		return sl;
 	if (sl > len - 2)
@@ -386,7 +391,7 @@ int of_device_uevent_modalias(struct device *dev, struct kobj_uevent_env *env)
 	if (add_uevent_var(env, "MODALIAS="))
 		return -ENOMEM;
 
-	sl = of_device_get_modalias(dev, &env->buf[env->buflen-1],
+	sl = of_device_get_modalias(dev->of_node, &env->buf[env->buflen-1],
 				    sizeof(env->buf) - env->buflen);
 	if (sl >= (sizeof(env->buf) - env->buflen))
 		return -ENOMEM;
-- 
2.34.1

[PATCH 3/8] of: Create an of_device_request_module() receiving an OF node

[Miquel Raynal]

of_device_request_module() currently receives a "struct device" as main
argument, but the only use of this pointer is to access its .of_node
member. In practice, this function only needs a "struct
device_node". Let's move the logic into another helper which would
receive a "struct device_node" instead, and use that new helper from the
ancient of_device_request_module(). Exporting this new function will be
useful to request module loading when the "struct device" is not
available.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/of/device.c       | 17 +++++++++++++----
 include/linux/of_device.h |  6 ++++++
 2 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/drivers/of/device.c b/drivers/of/device.c
index 3efc17de1d57..7cdf252b9526 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -284,16 +284,16 @@ static ssize_t of_device_get_modalias(struct device_node *np, char *str, ssize_t
 	return tsize;
 }
 
-int of_device_request_module(struct device *dev)
+int of_device_node_request_module(struct device_node *np)
 {
 	char *str;
 	ssize_t size;
 	int ret;
 
-	if (!dev || !dev->of_node)
+	if (!np)
 		return -ENODEV;
 
-	size = of_device_get_modalias(dev->of_node, NULL, 0);
+	size = of_device_get_modalias(np, NULL, 0);
 	if (size < 0)
 		return size;
 
@@ -304,13 +304,22 @@ int of_device_request_module(struct device *dev)
 	if (!str)
 		return -ENOMEM;
 
-	of_device_get_modalias(dev->of_node, str, size);
+	of_device_get_modalias(np, str, size);
 	str[size - 1] = '\0';
 	ret = request_module(str);
 	kfree(str);
 
 	return ret;
 }
+EXPORT_SYMBOL_GPL(of_device_node_request_module);
+
+int of_device_request_module(struct device *dev)
+{
+	if (!dev)
+		return -ENODEV;
+
+	return of_device_node_request_module(dev->of_node);
+}
 EXPORT_SYMBOL_GPL(of_device_request_module);
 
 /**
diff --git a/include/linux/of_device.h b/include/linux/of_device.h
index ab7d557d541d..8918f9071ffb 100644
--- a/include/linux/of_device.h
+++ b/include/linux/of_device.h
@@ -33,6 +33,7 @@ extern void of_device_unregister(struct platform_device *ofdev);
 extern const void *of_device_get_match_data(const struct device *dev);
 
 extern ssize_t of_device_modalias(struct device *dev, char *str, ssize_t len);
+extern int of_device_node_request_module(struct device_node *np);
 extern int of_device_request_module(struct device *dev);
 
 extern void of_device_uevent(const struct device *dev, struct kobj_uevent_env *env);
@@ -78,6 +79,11 @@ static inline int of_device_modalias(struct device *dev,
 	return -ENODEV;
 }
 
+static inline int of_device_node_request_module(struct device_node *np)
+{
+	return -ENODEV;
+}
+
 static inline int of_device_request_module(struct device *dev)
 {
 	return -ENODEV;
-- 
2.34.1

[PATCH 4/8] nvmem: core: Fix error path ordering

[Miquel Raynal]

The layout is being retrieved before the addition of nvmem cells and
after the creation of the sysfs entries. Soon the layout operation will
have the possibility to fail, hence adding a new goto label in the error
path. Before doing so, we need to fix the operation order in the error
path.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/nvmem/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 56b4b20a95a9..16044377a41d 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -975,9 +975,9 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
 
 err_remove_cells:
 	nvmem_device_remove_all_cells(nvmem);
+	nvmem_layout_put(nvmem->layout);
 	if (config->compat)
 		nvmem_sysfs_remove_compat(nvmem, config);
-	nvmem_layout_put(nvmem->layout);
 err_put_device:
 	put_device(&nvmem->dev);
 
-- 
2.34.1

[PATCH 5/8] nvmem: core: Handle the absence of expected layouts

[Miquel Raynal]

Make nvmem_layout_get() return -EPROBE_DEFER while the expected layout
is not available. This condition cannot be triggered today as nvmem
layout drivers are initialed as part of an early init call, but soon
these drivers will be converted into modules and be initialized with a
standard priority, so the unavailability of the drivers might become a
reality that must be taken care of.

Let's anticipate this by telling the caller the layout might not yet be
available. A probe deferral is requested in this case.

Please note this does not affect any nvmem device not using layouts,
because an early check against the "nvmem-layout" container presence
will return NULL in this case.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/nvmem/core.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 16044377a41d..a66c37a03a36 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -762,7 +762,7 @@ EXPORT_SYMBOL_GPL(nvmem_layout_unregister);
 static struct nvmem_layout *nvmem_layout_get(struct nvmem_device *nvmem)
 {
 	struct device_node *layout_np, *np = nvmem->dev.of_node;
-	struct nvmem_layout *l, *layout = NULL;
+	struct nvmem_layout *l, *layout = ERR_PTR(-EPROBE_DEFER);
 
 	layout_np = of_get_child_by_name(np, "nvmem-layout");
 	if (!layout_np)
@@ -944,6 +944,13 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
 	 * pointer will be NULL and nvmem_layout_put() will be a noop.
 	 */
 	nvmem->layout = config->layout ?: nvmem_layout_get(nvmem);
+	if (IS_ERR(nvmem->layout)) {
+		rval = PTR_ERR(nvmem->layout);
+		nvmem->layout = NULL;
+
+		if (rval == -EPROBE_DEFER)
+			goto err_teardown_compat;
+	}
 
 	if (config->cells) {
 		rval = nvmem_add_cells(nvmem, config->cells, config->ncells);
@@ -976,6 +983,7 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
 err_remove_cells:
 	nvmem_device_remove_all_cells(nvmem);
 	nvmem_layout_put(nvmem->layout);
+err_teardown_compat:
 	if (config->compat)
 		nvmem_sysfs_remove_compat(nvmem, config);
 err_put_device:
-- 
2.34.1

[PATCH 6/8] nvmem: core: Request layout modules loading

[Miquel Raynal]

When a storage device like an eeprom or an mtd device probes, it
registers an nvmem device if the nvmem subsystem has been enabled (bool
symbol). During nvmem registration, if the device is using layouts to
expose dynamic nvmem cells, the core will first try to get a reference
over the layout driver callbacks. In practice there is not relationship
that can be described between the storage driver and the nvmem
layout. So there is no way we can enforce both drivers will be built-in
or both will be modules. If the storage device driver is built-in but
the layout is built as a module, instead of badly failing with an
endless probe deferral loop, lets just make a modprobe call in case the
driver was made available in an initramfs with
of_device_node_request_module(), and offer a fully functional system to
the user.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/nvmem/core.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index a66c37a03a36..8e07b9df3221 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -17,6 +17,7 @@
 #include <linux/nvmem-provider.h>
 #include <linux/gpio/consumer.h>
 #include <linux/of.h>
+#include <linux/of_device.h>
 #include <linux/slab.h>
 
 struct nvmem_device {
@@ -768,6 +769,13 @@ static struct nvmem_layout *nvmem_layout_get(struct nvmem_device *nvmem)
 	if (!layout_np)
 		return NULL;
 
+	/*
+	 * In case the nvmem device was built-in while the layout was built as a
+	 * module, we shall manually request the layout driver loading otherwise
+	 * we'll never have any match.
+	 */
+	of_device_node_request_module(layout_np);
+
 	spin_lock(&nvmem_layout_lock);
 
 	list_for_each_entry(l, &nvmem_layouts, node) {
-- 
2.34.1

[PATCH 7/8] nvmem: layouts: sl28vpd: Convert layout driver into a module

[Miquel Raynal]

The nvmem core has already been converted to accept layout drivers
compiled as modules. So in order to make this change effective we need
to convert this driver so it can also be compiled as a module. We then
need to expose the match table, provide MODULE_* macros, use
module_init/exit() instead of the early subsys_initcall() and of course
update the Kconfig symbol type to tristate.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/nvmem/layouts/Kconfig   |  2 +-
 drivers/nvmem/layouts/sl28vpd.c | 14 +++++++++++++-
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/nvmem/layouts/Kconfig b/drivers/nvmem/layouts/Kconfig
index 9ad50474cb77..3f2d73282242 100644
--- a/drivers/nvmem/layouts/Kconfig
+++ b/drivers/nvmem/layouts/Kconfig
@@ -3,7 +3,7 @@
 menu "Layout Types"
 
 config NVMEM_LAYOUT_SL28_VPD
-	bool "Kontron sl28 VPD layout support"
+	tristate "Kontron sl28 VPD layout support"
 	select CRC8
 	help
 	  Say Y here if you want to support the VPD layout of the Kontron
diff --git a/drivers/nvmem/layouts/sl28vpd.c b/drivers/nvmem/layouts/sl28vpd.c
index a36800f201a3..9370e41bad73 100644
--- a/drivers/nvmem/layouts/sl28vpd.c
+++ b/drivers/nvmem/layouts/sl28vpd.c
@@ -139,6 +139,7 @@ static const struct of_device_id sl28vpd_of_match_table[] = {
 	{ .compatible = "kontron,sl28-vpd" },
 	{},
 };
+MODULE_DEVICE_TABLE(of, sl28vpd_of_match_table);
 
 struct nvmem_layout sl28vpd_layout = {
 	.name = "sl28-vpd",
@@ -150,4 +151,15 @@ static int __init sl28vpd_init(void)
 {
 	return nvmem_layout_register(&sl28vpd_layout);
 }
-subsys_initcall(sl28vpd_init);
+
+static void __exit sl28vpd_exit(void)
+{
+	nvmem_layout_unregister(&sl28vpd_layout);
+}
+
+module_init(sl28vpd_init);
+module_exit(sl28vpd_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Michael Walle <michael@walle.cc>");
+MODULE_DESCRIPTION("NVMEM layout driver for the VPD of Kontron sl28 boards");
-- 
2.34.1

[PATCH 8/8] nvmem: layouts: onie-tlv: Convert layout driver into a module

[Miquel Raynal]

The nvmem core has already been converted to accept layout drivers
compiled as modules. So in order to make this change effective we need
to convert this driver so it can also be compiled as a module. We then
need to expose the match table, provide MODULE_* macros, use
module_init/exit() instead of the early subsys_initcall() and of course
update the Kconfig symbol type to tristate.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/nvmem/layouts/Kconfig    |  2 +-
 drivers/nvmem/layouts/onie-tlv.c | 15 ++++++++++++++-
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/nvmem/layouts/Kconfig b/drivers/nvmem/layouts/Kconfig
index 3f2d73282242..7ff1ee1c1f05 100644
--- a/drivers/nvmem/layouts/Kconfig
+++ b/drivers/nvmem/layouts/Kconfig
@@ -12,7 +12,7 @@ config NVMEM_LAYOUT_SL28_VPD
 	  If unsure, say N.
 
 config NVMEM_LAYOUT_ONIE_TLV
-	bool "ONIE tlv support"
+	tristate "ONIE tlv support"
 	select CRC32
 	help
 	  Say Y here if you want to support the Open Compute Project ONIE
diff --git a/drivers/nvmem/layouts/onie-tlv.c b/drivers/nvmem/layouts/onie-tlv.c
index 767f39fff717..d45b7301a69d 100644
--- a/drivers/nvmem/layouts/onie-tlv.c
+++ b/drivers/nvmem/layouts/onie-tlv.c
@@ -230,6 +230,7 @@ static const struct of_device_id onie_tlv_of_match_table[] = {
 	{ .compatible = "onie,tlv-layout", },
 	{},
 };
+MODULE_DEVICE_TABLE(of, onie_tlv_of_match_table);
 
 static struct nvmem_layout onie_tlv_layout = {
 	.name = "ONIE tlv layout",
@@ -241,4 +242,16 @@ static int __init onie_tlv_init(void)
 {
 	return nvmem_layout_register(&onie_tlv_layout);
 }
-subsys_initcall(onie_tlv_init);
+
+static void __exit onie_tlv_exit(void)
+{
+	nvmem_layout_unregister(&onie_tlv_layout);
+}
+
+module_init(onie_tlv_init);
+module_exit(onie_tlv_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Miquel Raynal <miquel.raynal@bootlin.com>");
+MODULE_DESCRIPTION("NVMEM layout driver for Onie TLV table parsing");
+MODULE_ALIAS("NVMEM layout driver for Onie TLV table parsing");
-- 
2.34.1

Re: [PATCH 0/8] nvmem: Let layout drivers be modules

[Greg Kroah-Hartman]

On Wed, Mar 01, 2023 at 04:22:31PM +0100, Miquel Raynal wrote:
> Hello,
> 
> Following Greg's opposition to merge the current nvmem layout support
> proposal [1], arguing that it would eventually grow the size of the
> Linux kernel and asking for some "modularization" support, here is a
> proposal to turn layout drivers into regular tristate drivers.
> 
> The first three patches are preparation changes in order to extend (and
> fix) a little bit the of/device.c support. The fix does not seem to
> impact most of the current users so I guess it can live with the rest of
> the series in order to avoid future merge conflicts.
> 
> The nvmem core is then extended to support the absence of layouts and
> possibly lead to probe deferrals when relevant.
> 
> Finally, the two existing layout drivers are converted into modules and
> their Kconfig symbols changed to tristate.
> 
> The base series on which these changes apply is still contained in [1],
> I would prefer to keep it as it was and apply this series on top of it.
> 
> Tests have been conducted on a Marvell Prestera switch with the mvpp2
> Ethernet driver calling for a MAC address stored in the ONIE TLV table
> available through a layout driver in an EEPROM/MTD device.
> 
> [1] https://github.com/miquelraynal/linux/tree/nvmem-next/layouts

These look sane to me, thanks for making the changes.

greg k-h

Re: [PATCH 3/8] of: Create an of_device_request_module() receiving an OF node

[Rob Herring]

On Wed, Mar 1, 2023 at 9:22 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> of_device_request_module() currently receives a "struct device" as main
> argument, but the only use of this pointer is to access its .of_node
> member. In practice, this function only needs a "struct
> device_node". Let's move the logic into another helper which would
> receive a "struct device_node" instead, and use that new helper from the
> ancient of_device_request_module(). Exporting this new function will be
> useful to request module loading when the "struct device" is not
> available.

of_device.h is for things that operate on struct device, so
of_device_request_module() doesn't really belong here.

I wouldn't really care, but we have this ~12 year old line in of_device.h:

#include <linux/of_platform.h> /* temporary until merge */

I started working on a very churny series to fix this only to wonder
if a header split by consumer would be better. Anyways, concrete
suggestions below...

> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/of/device.c       | 17 +++++++++++++----
>  include/linux/of_device.h |  6 ++++++
>  2 files changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/of/device.c b/drivers/of/device.c
> index 3efc17de1d57..7cdf252b9526 100644
> --- a/drivers/of/device.c
> +++ b/drivers/of/device.c
> @@ -284,16 +284,16 @@ static ssize_t of_device_get_modalias(struct device_node *np, char *str, ssize_t
>         return tsize;
>  }
>
> -int of_device_request_module(struct device *dev)
> +int of_device_node_request_module(struct device_node *np)'

We use 'device_node' pretty much nowhere in the DT APIs. Just
of_request_module() and define in of.h. There is only one user of
of_device_request_module, so remove it and update the user to use
of_request_module() (and hopefully drop of_device.h for it).

Rob

Re: [PATCH 1/8] of: Fix modalias string generation

[Rob Herring]

On Wed, Mar 1, 2023 at 9:22 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> The helper generating an OF based modalias (of_device_get_modalias())
> works fine, but due to the use of snprintf() internally it needs a
> buffer one byte longer than what should be needed just for the entire
> string (excluding the '\0'). Most users of this helper are sysfs hooks
> providing the modalias string to users. They all provide a PAGE_SIZE
> buffer which is way above the number of bytes required to fit the
> modalias string and hence do not suffer from this issue.
>
> There is another user though, of_device_request_module(), which is only
> called by drivers/usb/common/ulpi.c. This request module function is
> faulty, but maybe because in most cases there is an alternative, ULPI
> driver users have not noticed it.
>
> In this function, of_device_get_modalias() is called twice. The first
> time without buffer just to get the number of bytes required by the
> modalias string (excluding the null byte), and a second time, after
> buffer allocation, to fill the buffer. The allocation asks for an
> additional byte, in order to store the trailing '\0'. However, the
> buffer *length* provided to of_device_get_modalias() excludes this extra
> byte. The internal use of snprintf() with a length that is exactly the
> number of bytes to be written has the effect of using the last available
> byte to store a '\0', which then smashes the last character of the
> modalias string.
>
> Provide the actual size of the buffer to of_device_get_modalias() to fix
> this issue.
>
> Note: the "str[size - 1] = '\0';" line is not really needed as snprintf
> will anyway end the string with a null byte, but there is a possibility
> that this function might be called on a struct device_node without
> compatible, in this case snprintf() would not be executed. So we keep it
> just to avoid possible unbounded strings.
>
> Cc: Stephen Boyd <stephen.boyd@linaro.org>
> Cc: Peter Chen <peter.chen@nxp.com>
> Fixes: 9c829c097f2f ("of: device: Support loading a module with OF based modalias")
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/of/device.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)

Ahh, an off by 2 error. ;)

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH 2/8] of: Change of_device_get_modalias() main argument

[Rob Herring]

On Wed, Mar 1, 2023 at 9:22 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> This function needs "struct device_node" to work, but for convenience
> the author and only user of this helper did use a "struct device". As
> this helper is a static helper, let's keep the "struct device" for
> exported methods and use the OF structure internally.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/of/device.c | 27 ++++++++++++++++-----------
>  1 file changed, 16 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/of/device.c b/drivers/of/device.c
> index 877f50379fab..3efc17de1d57 100644
> --- a/drivers/of/device.c
> +++ b/drivers/of/device.c
> @@ -248,7 +248,7 @@ const void *of_device_get_match_data(const struct device *dev)
>  }
>  EXPORT_SYMBOL(of_device_get_match_data);
>
> -static ssize_t of_device_get_modalias(struct device *dev, char *str, ssize_t len)
> +static ssize_t of_device_get_modalias(struct device_node *np, char *str, ssize_t len)

Humm, this is static so fine here, but based on my comments on patch
3, it would need to move too as base.c having a dependency on device.c
is backwards.

How about moving everything module related to drivers/of/module.c. Put
the new functions in of.h (rather than an of_module.h). Then maybe the
rest of device.c could move to inline wrappers or elsewhere.

Rob

Re: [PATCH 0/8] nvmem: Let layout drivers be modules

[Michael Walle]

> Miquel Raynal (8):
>   of: Fix modalias string generation
>   of: Change of_device_get_modalias() main argument
>   of: Create an of_device_request_module() receiving an OF node
>   nvmem: core: Fix error path ordering
>   nvmem: core: Handle the absence of expected layouts
>   nvmem: core: Request layout modules loading
>   nvmem: layouts: sl28vpd: Convert layout driver into a module
>   nvmem: layouts: onie-tlv: Convert layout driver into a module

With the fixes series [1] applied:

Tested-by: Michael Walle <michael@walle.cc>

I didn't test module autoloading, but I presume you did.

Thanks for working on this!
-michael

[1] https://lore.kernel.org/r/20230306125805.678668-1-michael@walle.cc/

Re: [PATCH 0/8] nvmem: Let layout drivers be modules

[Miquel Raynal]

Hi Michael,

michael@walle.cc wrote on Mon, 06 Mar 2023 14:01:34 +0100:

> > Miquel Raynal (8):
> >   of: Fix modalias string generation
> >   of: Change of_device_get_modalias() main argument
> >   of: Create an of_device_request_module() receiving an OF node
> >   nvmem: core: Fix error path ordering
> >   nvmem: core: Handle the absence of expected layouts
> >   nvmem: core: Request layout modules loading
> >   nvmem: layouts: sl28vpd: Convert layout driver into a module
> >   nvmem: layouts: onie-tlv: Convert layout driver into a module  
> 
> With the fixes series [1] applied:

Thanks for the series! Looks good to me. I believe both series can live
in separate tress, any reason why we would like to avoid this? I am keen
to apply [1] into the mtd tree rather soon.

I will handle the remaining deferral errors in the regular mtd path as
discussed on IRC.

> Tested-by: Michael Walle <michael@walle.cc>
> 
> I didn't test module autoloading, but I presume you did.

Yes, I generated an initramfs with Buildroot, in which an overlay
containing the result of modules_install got merged (storage device =y
and nvmem layout to =m). I could observe the modprobe call being
successful and the layout driver being loaded early.

> Thanks for working on this!

👍

> -michael
> 
> [1] https://lore.kernel.org/r/20230306125805.678668-1-michael@walle.cc/

Thanks,
Miquèl

Re: [PATCH 0/8] nvmem: Let layout drivers be modules

[Michael Walle]

Hi Miquel,

Am 2023-03-06 14:35, schrieb Miquel Raynal:
> michael@walle.cc wrote on Mon, 06 Mar 2023 14:01:34 +0100:
> 
>> > Miquel Raynal (8):
>> >   of: Fix modalias string generation
>> >   of: Change of_device_get_modalias() main argument
>> >   of: Create an of_device_request_module() receiving an OF node
>> >   nvmem: core: Fix error path ordering
>> >   nvmem: core: Handle the absence of expected layouts
>> >   nvmem: core: Request layout modules loading
>> >   nvmem: layouts: sl28vpd: Convert layout driver into a module
>> >   nvmem: layouts: onie-tlv: Convert layout driver into a module
>> 
>> With the fixes series [1] applied:
> 
> Thanks for the series! Looks good to me. I believe both series can live
> in separate tress, any reason why we would like to avoid this? I am 
> keen
> to apply [1] into the mtd tree rather soon.

I'm fine with that.

-michael

[1] https://lore.kernel.org/r/20230306125805.678668-1-michael@walle.cc/

Re: [PATCH 0/8] nvmem: Let layout drivers be modules

[Rafał Miłecki]

On 2023-03-01 16:22, Miquel Raynal wrote:
> The base series on which these changes apply is still contained in [1],
> I would prefer to keep it as it was and apply this series on top of it.
> 
> (...)
> 
> [1] https://github.com/miquelraynal/linux/tree/nvmem-next/layouts

My experience with kernel development over all subsystems I touched is
that patches should be improved until being clean & acceptable. I never
sent a series with more recent patches fixing issues in earlier patches
of the same seriee.

So my preference would be to get a new, clean & complete set of patches.

Re: [PATCH 0/8] nvmem: Let layout drivers be modules

[Greg Kroah-Hartman]

On Mon, Mar 06, 2023 at 02:54:10PM +0100, Rafał Miłecki wrote:
> On 2023-03-01 16:22, Miquel Raynal wrote:
> > The base series on which these changes apply is still contained in [1],
> > I would prefer to keep it as it was and apply this series on top of it.
> > 
> > (...)
> > 
> > [1] https://github.com/miquelraynal/linux/tree/nvmem-next/layouts
> 
> My experience with kernel development over all subsystems I touched is
> that patches should be improved until being clean & acceptable. I never
> sent a series with more recent patches fixing issues in earlier patches
> of the same seriee.
> 
> So my preference would be to get a new, clean & complete set of patches.

I agree, don't break something and then fix it up in a later patch, that
makes bisection impossible.

thanks,

greg k-h

Re: [PATCH 0/8] nvmem: Let layout drivers be modules

[Rafał Miłecki]

On 2023-03-06 14:35, Miquel Raynal wrote:
> Hi Michael,
> 
> michael@walle.cc wrote on Mon, 06 Mar 2023 14:01:34 +0100:
> 
>> > Miquel Raynal (8):
>> >   of: Fix modalias string generation
>> >   of: Change of_device_get_modalias() main argument
>> >   of: Create an of_device_request_module() receiving an OF node
>> >   nvmem: core: Fix error path ordering
>> >   nvmem: core: Handle the absence of expected layouts
>> >   nvmem: core: Request layout modules loading
>> >   nvmem: layouts: sl28vpd: Convert layout driver into a module
>> >   nvmem: layouts: onie-tlv: Convert layout driver into a module
>> 
>> With the fixes series [1] applied:
> 
> Thanks for the series! Looks good to me. I believe both series can live
> in separate tress, any reason why we would like to avoid this? I am 
> keen
> to apply [1] into the mtd tree rather soon.

Given past events with nvmem patches I'm against that.

Let's wait for Srinivas to collect pending patches, let them spend a
moment in linux-next maybe, ask Srinivas to send them to Greg early if
he can. That way maybe you can merge Greg's branch (assuming he doesn't
rebase).

Re: [PATCH 0/8] nvmem: Let layout drivers be modules

[Michael Walle]

Am 2023-03-06 14:57, schrieb Rafał Miłecki:
> On 2023-03-06 14:35, Miquel Raynal wrote:
>> Hi Michael,
>> 
>> michael@walle.cc wrote on Mon, 06 Mar 2023 14:01:34 +0100:
>> 
>>> > Miquel Raynal (8):
>>> >   of: Fix modalias string generation
>>> >   of: Change of_device_get_modalias() main argument
>>> >   of: Create an of_device_request_module() receiving an OF node
>>> >   nvmem: core: Fix error path ordering
>>> >   nvmem: core: Handle the absence of expected layouts
>>> >   nvmem: core: Request layout modules loading
>>> >   nvmem: layouts: sl28vpd: Convert layout driver into a module
>>> >   nvmem: layouts: onie-tlv: Convert layout driver into a module
>>> 
>>> With the fixes series [1] applied:
>> 
>> Thanks for the series! Looks good to me. I believe both series can 
>> live
>> in separate tress, any reason why we would like to avoid this? I am 
>> keen
>> to apply [1] into the mtd tree rather soon.
> 
> Given past events with nvmem patches I'm against that.
> 
> Let's wait for Srinivas to collect pending patches, let them spend a
> moment in linux-next maybe, ask Srinivas to send them to Greg early if
> he can. That way maybe you can merge Greg's branch (assuming he doesn't
> rebase).

Mh? None of these fixes have anything to do with nvmem (except maybe 
patch
4/4). The bugs were just discovered while I was testing this series. But
OTOH they are kind of a prerequisite for this series. So what are you
suggesting here?

-michael

Re: [PATCH 0/8] nvmem: Let layout drivers be modules

[Rafał Miłecki]

On 2023-03-06 15:03, Michael Walle wrote:
> Am 2023-03-06 14:57, schrieb Rafał Miłecki:
>> On 2023-03-06 14:35, Miquel Raynal wrote:
>>> Hi Michael,
>>> 
>>> michael@walle.cc wrote on Mon, 06 Mar 2023 14:01:34 +0100:
>>> 
>>>> > Miquel Raynal (8):
>>>> >   of: Fix modalias string generation
>>>> >   of: Change of_device_get_modalias() main argument
>>>> >   of: Create an of_device_request_module() receiving an OF node
>>>> >   nvmem: core: Fix error path ordering
>>>> >   nvmem: core: Handle the absence of expected layouts
>>>> >   nvmem: core: Request layout modules loading
>>>> >   nvmem: layouts: sl28vpd: Convert layout driver into a module
>>>> >   nvmem: layouts: onie-tlv: Convert layout driver into a module
>>>> 
>>>> With the fixes series [1] applied:
>>> 
>>> Thanks for the series! Looks good to me. I believe both series can 
>>> live
>>> in separate tress, any reason why we would like to avoid this? I am 
>>> keen
>>> to apply [1] into the mtd tree rather soon.
>> 
>> Given past events with nvmem patches I'm against that.
>> 
>> Let's wait for Srinivas to collect pending patches, let them spend a
>> moment in linux-next maybe, ask Srinivas to send them to Greg early if
>> he can. That way maybe you can merge Greg's branch (assuming he 
>> doesn't
>> rebase).
> 
> Mh? None of these fixes have anything to do with nvmem (except maybe 
> patch
> 4/4). The bugs were just discovered while I was testing this series. 
> But
> OTOH they are kind of a prerequisite for this series. So what are you
> suggesting here?

I'm sorry, I didn't realize you are commenting on linked mtd series.
I thought you want to take nvmem patches series over mtd tree ;) My
bad.

Re: [PATCH 0/8] nvmem: Let layout drivers be modules

[Michael Walle]

Am 2023-03-06 15:06, schrieb Rafał Miłecki:
> On 2023-03-06 15:03, Michael Walle wrote:
>> Am 2023-03-06 14:57, schrieb Rafał Miłecki:
>>> On 2023-03-06 14:35, Miquel Raynal wrote:
>>>> Hi Michael,
>>>> 
>>>> michael@walle.cc wrote on Mon, 06 Mar 2023 14:01:34 +0100:
>>>> 
>>>>> > Miquel Raynal (8):
>>>>> >   of: Fix modalias string generation
>>>>> >   of: Change of_device_get_modalias() main argument
>>>>> >   of: Create an of_device_request_module() receiving an OF node
>>>>> >   nvmem: core: Fix error path ordering
>>>>> >   nvmem: core: Handle the absence of expected layouts
>>>>> >   nvmem: core: Request layout modules loading
>>>>> >   nvmem: layouts: sl28vpd: Convert layout driver into a module
>>>>> >   nvmem: layouts: onie-tlv: Convert layout driver into a module
>>>>> 
>>>>> With the fixes series [1] applied:
>>>> 
>>>> Thanks for the series! Looks good to me. I believe both series can 
>>>> live
>>>> in separate tress, any reason why we would like to avoid this? I am 
>>>> keen
>>>> to apply [1] into the mtd tree rather soon.
>>> 
>>> Given past events with nvmem patches I'm against that.
>>> 
>>> Let's wait for Srinivas to collect pending patches, let them spend a
>>> moment in linux-next maybe, ask Srinivas to send them to Greg early 
>>> if
>>> he can. That way maybe you can merge Greg's branch (assuming he 
>>> doesn't
>>> rebase).
>> 
>> Mh? None of these fixes have anything to do with nvmem (except maybe 
>> patch
>> 4/4). The bugs were just discovered while I was testing this series. 
>> But
>> OTOH they are kind of a prerequisite for this series. So what are you
>> suggesting here?
> 
> I'm sorry, I didn't realize you are commenting on linked mtd series.
> I thought you want to take nvmem patches series over mtd tree ;) My
> bad.

So was Miquel I think ;). But maybe it will make sense to provide a 
stable
tag/branch to srinivas so if someone is using the nvmem-next branch it 
will
work. Although I doubt there are many users of the spi-nor otp stuff.

-michael

Re: [PATCH 0/8] nvmem: Let layout drivers be modules

[Miquel Raynal]

Hi Rafał,

rafal@milecki.pl wrote on Mon, 06 Mar 2023 14:57:03 +0100:

> On 2023-03-06 14:35, Miquel Raynal wrote:
> > Hi Michael,
> > 
> > michael@walle.cc wrote on Mon, 06 Mar 2023 14:01:34 +0100:
> >   
> >> > Miquel Raynal (8):
> >> >   of: Fix modalias string generation
> >> >   of: Change of_device_get_modalias() main argument
> >> >   of: Create an of_device_request_module() receiving an OF node
> >> >   nvmem: core: Fix error path ordering
> >> >   nvmem: core: Handle the absence of expected layouts
> >> >   nvmem: core: Request layout modules loading
> >> >   nvmem: layouts: sl28vpd: Convert layout driver into a module
> >> >   nvmem: layouts: onie-tlv: Convert layout driver into a module  
> >> >> With the fixes series [1] applied:  
> > 
> > Thanks for the series! Looks good to me. I believe both series can live
> > in separate tress, any reason why we would like to avoid this? I am > keen
> > to apply [1] into the mtd tree rather soon.  
> 
> Given past events with nvmem patches I'm against that.
> 
> Let's wait for Srinivas to collect pending patches, let them spend a
> moment in linux-next maybe, ask Srinivas to send them to Greg early if
> he can. That way maybe you can merge Greg's branch (assuming he doesn't
> rebase).

Just to be on the same page, we're talking about the mtd core fixups to
handle correctly probe deferrals in the nvmem side.

Applying mtd patches then nvmem patches is totally fine in this order.
Applying nvmem patches and then mtd patches creates a range of commits
where some otp devices might have troubles probing if:
- a layout driver is used
- the driver is compiled as a module
- the driver is also not installed in an initramfs

I was actually asking out loud whether we should care about this
commit range given the unlikelihood that someone would have troubles
with this while bisecting a linux-next kernel.

So getting an immutable tag from Greg would not help. The opposite
might make sense though, and involves that I apply [1] to mtd/next
rather soon anyway, I guess?

Thanks,
Miquèl

Re: [PATCH 0/8] nvmem: Let layout drivers be modules

[Rafał Miłecki]

On 2023-03-06 15:18, Miquel Raynal wrote:
> Hi Rafał,
> 
> rafal@milecki.pl wrote on Mon, 06 Mar 2023 14:57:03 +0100:
> 
>> On 2023-03-06 14:35, Miquel Raynal wrote:
>> > Hi Michael,
>> >
>> > michael@walle.cc wrote on Mon, 06 Mar 2023 14:01:34 +0100:
>> >
>> >> > Miquel Raynal (8):
>> >> >   of: Fix modalias string generation
>> >> >   of: Change of_device_get_modalias() main argument
>> >> >   of: Create an of_device_request_module() receiving an OF node
>> >> >   nvmem: core: Fix error path ordering
>> >> >   nvmem: core: Handle the absence of expected layouts
>> >> >   nvmem: core: Request layout modules loading
>> >> >   nvmem: layouts: sl28vpd: Convert layout driver into a module
>> >> >   nvmem: layouts: onie-tlv: Convert layout driver into a module
>> >> >> With the fixes series [1] applied:
>> >
>> > Thanks for the series! Looks good to me. I believe both series can live
>> > in separate tress, any reason why we would like to avoid this? I am > keen
>> > to apply [1] into the mtd tree rather soon.
>> 
>> Given past events with nvmem patches I'm against that.
>> 
>> Let's wait for Srinivas to collect pending patches, let them spend a
>> moment in linux-next maybe, ask Srinivas to send them to Greg early if
>> he can. That way maybe you can merge Greg's branch (assuming he 
>> doesn't
>> rebase).
> 
> Just to be on the same page, we're talking about the mtd core fixups to
> handle correctly probe deferrals in the nvmem side.
> 
> Applying mtd patches then nvmem patches is totally fine in this order.
> Applying nvmem patches and then mtd patches creates a range of commits
> where some otp devices might have troubles probing if:
> - a layout driver is used
> - the driver is compiled as a module
> - the driver is also not installed in an initramfs
> 
> I was actually asking out loud whether we should care about this
> commit range given the unlikelihood that someone would have troubles
> with this while bisecting a linux-next kernel.
> 
> So getting an immutable tag from Greg would not help. The opposite
> might make sense though, and involves that I apply [1] to mtd/next
> rather soon anyway, I guess?

The problem IIUC is nvmem.git / for-next containing broken code after
adding nvmem stuff. That is unless Srinivas takes your patches in some
way. Hopefully not by waiting for 6.4-rc1.

Re: [PATCH 0/8] nvmem: Let layout drivers be modules

[Miquel Raynal]

Hi Rafał,

rafal@milecki.pl wrote on Mon, 06 Mar 2023 15:23:50 +0100:

> On 2023-03-06 15:18, Miquel Raynal wrote:
> > Hi Rafał,
> > 
> > rafal@milecki.pl wrote on Mon, 06 Mar 2023 14:57:03 +0100:
> >   
> >> On 2023-03-06 14:35, Miquel Raynal wrote:  
> >> > Hi Michael,
> >> >
> >> > michael@walle.cc wrote on Mon, 06 Mar 2023 14:01:34 +0100:
> >> >  
> >> >> > Miquel Raynal (8):
> >> >> >   of: Fix modalias string generation
> >> >> >   of: Change of_device_get_modalias() main argument
> >> >> >   of: Create an of_device_request_module() receiving an OF node
> >> >> >   nvmem: core: Fix error path ordering
> >> >> >   nvmem: core: Handle the absence of expected layouts
> >> >> >   nvmem: core: Request layout modules loading
> >> >> >   nvmem: layouts: sl28vpd: Convert layout driver into a module
> >> >> >   nvmem: layouts: onie-tlv: Convert layout driver into a module  
> >> >> >> With the fixes series [1] applied:  
> >> >
> >> > Thanks for the series! Looks good to me. I believe both series can live
> >> > in separate tress, any reason why we would like to avoid this? I am > keen
> >> > to apply [1] into the mtd tree rather soon.  
> >> >> Given past events with nvmem patches I'm against that.
> >> >> Let's wait for Srinivas to collect pending patches, let them spend a  
> >> moment in linux-next maybe, ask Srinivas to send them to Greg early if
> >> he can. That way maybe you can merge Greg's branch (assuming he >> doesn't
> >> rebase).  
> > 
> > Just to be on the same page, we're talking about the mtd core fixups to
> > handle correctly probe deferrals in the nvmem side.
> > 
> > Applying mtd patches then nvmem patches is totally fine in this order.
> > Applying nvmem patches and then mtd patches creates a range of commits
> > where some otp devices might have troubles probing if:
> > - a layout driver is used
> > - the driver is compiled as a module
> > - the driver is also not installed in an initramfs
> > 
> > I was actually asking out loud whether we should care about this
> > commit range given the unlikelihood that someone would have troubles
> > with this while bisecting a linux-next kernel.
> > 
> > So getting an immutable tag from Greg would not help. The opposite
> > might make sense though, and involves that I apply [1] to mtd/next
> > rather soon anyway, I guess?  
> 
> The problem IIUC is nvmem.git / for-next containing broken code after
> adding nvmem stuff. That is unless Srinivas takes your patches in some
> way. Hopefully not by waiting for 6.4-rc1.

I don't follow. There will be nothing broken after applying the nvmem
patches, at least nothing more than today. I will apply the patches
provided by Michael, they fix existing issues, nothing related to the
nvmem changes. Just, it is easier to trigger these issues with the
nvmem series thanks to the probe deferral situations.

Both series can live on their own. If required I will produce an
immutable tag to Greg.

Thanks,
Miquèl

Re: [PATCH 0/8] nvmem: Let layout drivers be modules

[Rafał Miłecki]

On 2023-03-06 15:29, Miquel Raynal wrote:
> Hi Rafał,
> 
> rafal@milecki.pl wrote on Mon, 06 Mar 2023 15:23:50 +0100:
> 
>> On 2023-03-06 15:18, Miquel Raynal wrote:
>> > Hi Rafał,
>> >
>> > rafal@milecki.pl wrote on Mon, 06 Mar 2023 14:57:03 +0100:
>> >
>> >> On 2023-03-06 14:35, Miquel Raynal wrote:
>> >> > Hi Michael,
>> >> >
>> >> > michael@walle.cc wrote on Mon, 06 Mar 2023 14:01:34 +0100:
>> >> >
>> >> >> > Miquel Raynal (8):
>> >> >> >   of: Fix modalias string generation
>> >> >> >   of: Change of_device_get_modalias() main argument
>> >> >> >   of: Create an of_device_request_module() receiving an OF node
>> >> >> >   nvmem: core: Fix error path ordering
>> >> >> >   nvmem: core: Handle the absence of expected layouts
>> >> >> >   nvmem: core: Request layout modules loading
>> >> >> >   nvmem: layouts: sl28vpd: Convert layout driver into a module
>> >> >> >   nvmem: layouts: onie-tlv: Convert layout driver into a module
>> >> >> >> With the fixes series [1] applied:
>> >> >
>> >> > Thanks for the series! Looks good to me. I believe both series can live
>> >> > in separate tress, any reason why we would like to avoid this? I am > keen
>> >> > to apply [1] into the mtd tree rather soon.
>> >> >> Given past events with nvmem patches I'm against that.
>> >> >> Let's wait for Srinivas to collect pending patches, let them spend a
>> >> moment in linux-next maybe, ask Srinivas to send them to Greg early if
>> >> he can. That way maybe you can merge Greg's branch (assuming he >> doesn't
>> >> rebase).
>> >
>> > Just to be on the same page, we're talking about the mtd core fixups to
>> > handle correctly probe deferrals in the nvmem side.
>> >
>> > Applying mtd patches then nvmem patches is totally fine in this order.
>> > Applying nvmem patches and then mtd patches creates a range of commits
>> > where some otp devices might have troubles probing if:
>> > - a layout driver is used
>> > - the driver is compiled as a module
>> > - the driver is also not installed in an initramfs
>> >
>> > I was actually asking out loud whether we should care about this
>> > commit range given the unlikelihood that someone would have troubles
>> > with this while bisecting a linux-next kernel.
>> >
>> > So getting an immutable tag from Greg would not help. The opposite
>> > might make sense though, and involves that I apply [1] to mtd/next
>> > rather soon anyway, I guess?
>> 
>> The problem IIUC is nvmem.git / for-next containing broken code after
>> adding nvmem stuff. That is unless Srinivas takes your patches in some
>> way. Hopefully not by waiting for 6.4-rc1.
> 
> I don't follow. There will be nothing broken after applying the nvmem
> patches, at least nothing more than today. I will apply the patches
> provided by Michael, they fix existing issues, nothing related to the
> nvmem changes. Just, it is easier to trigger these issues with the
> nvmem series thanks to the probe deferral situations.
> 
> Both series can live on their own. If required I will produce an
> immutable tag to Greg.

OK, it's me how didn't follow then.

I thought your mtd fixes are needed before applying nvmem stuff.

It sounds OK then.

Re: [PATCH 0/8] nvmem: Let layout drivers be modules

[Miquel Raynal]

Hi Rafał,

rafal@milecki.pl wrote on Mon, 06 Mar 2023 15:34:50 +0100:

> On 2023-03-06 15:29, Miquel Raynal wrote:
> > Hi Rafał,
> > 
> > rafal@milecki.pl wrote on Mon, 06 Mar 2023 15:23:50 +0100:
> >   
> >> On 2023-03-06 15:18, Miquel Raynal wrote:  
> >> > Hi Rafał,
> >> >
> >> > rafal@milecki.pl wrote on Mon, 06 Mar 2023 14:57:03 +0100:
> >> >  
> >> >> On 2023-03-06 14:35, Miquel Raynal wrote:  
> >> >> > Hi Michael,
> >> >> >
> >> >> > michael@walle.cc wrote on Mon, 06 Mar 2023 14:01:34 +0100:
> >> >> >  
> >> >> >> > Miquel Raynal (8):
> >> >> >> >   of: Fix modalias string generation
> >> >> >> >   of: Change of_device_get_modalias() main argument
> >> >> >> >   of: Create an of_device_request_module() receiving an OF node
> >> >> >> >   nvmem: core: Fix error path ordering
> >> >> >> >   nvmem: core: Handle the absence of expected layouts
> >> >> >> >   nvmem: core: Request layout modules loading
> >> >> >> >   nvmem: layouts: sl28vpd: Convert layout driver into a module
> >> >> >> >   nvmem: layouts: onie-tlv: Convert layout driver into a module  
> >> >> >> >> With the fixes series [1] applied:  
> >> >> >
> >> >> > Thanks for the series! Looks good to me. I believe both series can live
> >> >> > in separate tress, any reason why we would like to avoid this? I am > keen
> >> >> > to apply [1] into the mtd tree rather soon.  
> >> >> >> Given past events with nvmem patches I'm against that.
> >> >> >> Let's wait for Srinivas to collect pending patches, let them spend a  
> >> >> moment in linux-next maybe, ask Srinivas to send them to Greg early if
> >> >> he can. That way maybe you can merge Greg's branch (assuming he >> doesn't
> >> >> rebase).  
> >> >
> >> > Just to be on the same page, we're talking about the mtd core fixups to
> >> > handle correctly probe deferrals in the nvmem side.
> >> >
> >> > Applying mtd patches then nvmem patches is totally fine in this order.
> >> > Applying nvmem patches and then mtd patches creates a range of commits
> >> > where some otp devices might have troubles probing if:
> >> > - a layout driver is used
> >> > - the driver is compiled as a module
> >> > - the driver is also not installed in an initramfs
> >> >
> >> > I was actually asking out loud whether we should care about this
> >> > commit range given the unlikelihood that someone would have troubles
> >> > with this while bisecting a linux-next kernel.
> >> >
> >> > So getting an immutable tag from Greg would not help. The opposite
> >> > might make sense though, and involves that I apply [1] to mtd/next
> >> > rather soon anyway, I guess?  
> >> >> The problem IIUC is nvmem.git / for-next containing broken code after  
> >> adding nvmem stuff. That is unless Srinivas takes your patches in some
> >> way. Hopefully not by waiting for 6.4-rc1.  
> > 
> > I don't follow. There will be nothing broken after applying the nvmem
> > patches, at least nothing more than today. I will apply the patches
> > provided by Michael, they fix existing issues, nothing related to the
> > nvmem changes. Just, it is easier to trigger these issues with the
> > nvmem series thanks to the probe deferral situations.
> > 
> > Both series can live on their own. If required I will produce an
> > immutable tag to Greg.  
> 
> OK, it's me how didn't follow then.
> 
> I thought your mtd fixes are needed before applying nvmem stuff.

Yes, that would be ideal. I will produce an immutable branch.

Thanks,
Miquèl

Re: [PATCH 0/8] nvmem: Let layout drivers be modules

[Miquel Raynal]

Hello,

gregkh@linuxfoundation.org wrote on Mon, 6 Mar 2023 14:55:44 +0100:

> On Mon, Mar 06, 2023 at 02:54:10PM +0100, Rafał Miłecki wrote:
> > On 2023-03-01 16:22, Miquel Raynal wrote:  
> > > The base series on which these changes apply is still contained in [1],
> > > I would prefer to keep it as it was and apply this series on top of it.
> > > 
> > > (...)
> > > 
> > > [1] https://github.com/miquelraynal/linux/tree/nvmem-next/layouts  
> > 
> > My experience with kernel development over all subsystems I touched is
> > that patches should be improved until being clean & acceptable. I never
> > sent a series with more recent patches fixing issues in earlier patches
> > of the same seriee.
> > 
> > So my preference would be to get a new, clean & complete set of patches.  
> 
> I agree, don't break something and then fix it up in a later patch, that
> makes bisection impossible.

Apart from two rather small fixes which I can squash if that's what you
are requesting, most of the series is already fine on its own, fully
working and bisectable. On top of that initial series from Michael I am
adding support for compiling additional code as modules, which is
arguably another feature. I don't see the point in merging them both
besides mixing two different works. Looking at the code shows that every
step is pretty clean, there is nothing going back and forth.

I will anyway try to make it look like a single series with the changes
requested by Rob in v2.

Thanks,
Miquèl