[PATCH 0/5] of: More 'device' vs. 'module' cleanups

[PATCH 0/5] of: More 'device' vs. 'module' cleanups

[Miquel Raynal]

Hello,

As part of a previous series, Rob suggested that keeping too much logic
in of/device.c was backward and would benefit from a gradual cleanup
with the hope some day to move the remaining helpers into inline
functions wrapping the proper of_*() methods.

Link: https://lore.kernel.org/lkml/CAL_JsqJE43qfYzHUuCJsbaPPBTbYX05Q7FFmPTjPFZ3Dmz_mXg@mail.gmail.com/

A few of these "conversions" happened in the series I was originally
working on. At this time I actually wrote a few other changes,
completely unrelated to my original series, but still following Rob's
cleanup idea: here they are.

Link: https://lore.kernel.org/lkml/20230307165359.225361-1-miquel.raynal@bootlin.com/

The last step of this series is to actually remove a copy of one of
these helpers which I think is not needed. This drivers/gpu/ patch
depends on the previous changes.

Thanks, Miquèl

Miquel Raynal (5):
  of: module: Mutate of_device_modalias() into two helpers
  of: module: Mutate of_device_uevent() into two helpers
  of: module: Mutate of_device_uevent_modalias() into two helpers
  of: module: Export of_uevent()
  gpu: host1x: Stop open-coding of_device_uevent()

 drivers/gpu/host1x/bus.c  | 31 +++-----------
 drivers/of/device.c       | 90 ---------------------------------------
 drivers/of/module.c       | 82 +++++++++++++++++++++++++++++++++++
 include/linux/of.h        | 21 +++++++++
 include/linux/of_device.h | 39 ++++++++++++++---
 5 files changed, 141 insertions(+), 122 deletions(-)

-- 
2.34.1

[PATCH 1/5] of: module: Mutate of_device_modalias() into two helpers

[Miquel Raynal]

Move the content of the helper providing printable modaliases in
module.c. Call this new function from an of_device.c inline helper.

There is no functional changes. However, as a side effect, we fix the
return value of the inline helper (in the !CONFIG_OF case) which should
be a ssize_t instead of int.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/of/device.c       | 25 -------------------------
 drivers/of/module.c       | 19 +++++++++++++++++++
 include/linux/of.h        |  8 ++++++++
 include/linux/of_device.h | 13 ++++++++++---
 4 files changed, 37 insertions(+), 28 deletions(-)

diff --git a/drivers/of/device.c b/drivers/of/device.c
index 0f00f1b80708..45ce83a8945f 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -246,31 +246,6 @@ const void *of_device_get_match_data(const struct device *dev)
 }
 EXPORT_SYMBOL(of_device_get_match_data);
 
-/**
- * of_device_modalias - Fill buffer with newline terminated modalias string
- * @dev:	Calling device
- * @str:	Modalias string
- * @len:	Size of @str
- */
-ssize_t of_device_modalias(struct device *dev, char *str, ssize_t len)
-{
-	ssize_t sl;
-
-	if (!dev || !dev->of_node || dev->of_node_reused)
-		return -ENODEV;
-
-	sl = of_modalias(dev->of_node, str, len - 2);
-	if (sl < 0)
-		return sl;
-	if (sl > len - 2)
-		return -ENOMEM;
-
-	str[sl++] = '\n';
-	str[sl] = 0;
-	return sl;
-}
-EXPORT_SYMBOL_GPL(of_device_modalias);
-
 /**
  * of_device_uevent - Display OF related uevent information
  * @dev:	Device to display the uevent information for
diff --git a/drivers/of/module.c b/drivers/of/module.c
index 0e8aa974f0f2..c05fb8ca575c 100644
--- a/drivers/of/module.c
+++ b/drivers/of/module.c
@@ -44,6 +44,25 @@ ssize_t of_modalias(const struct device_node *np, char *str, ssize_t len)
 	return tsize;
 }
 
+ssize_t of_printable_modalias(const struct device_node *np, char *str, ssize_t len)
+{
+	ssize_t sl;
+
+	if (!np)
+		return -ENODEV;
+
+	sl = of_modalias(np, str, len - 2);
+	if (sl < 0)
+		return sl;
+	if (sl > len - 2)
+		return -ENOMEM;
+
+	str[sl++] = '\n';
+	str[sl] = 0;
+	return sl;
+}
+EXPORT_SYMBOL_GPL(of_printable_modalias);
+
 int of_request_module(const struct device_node *np)
 {
 	char *str;
diff --git a/include/linux/of.h b/include/linux/of.h
index 6ecde0515677..dcdd9396ac37 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -385,6 +385,8 @@ extern int of_count_phandle_with_args(const struct device_node *np,
 
 /* module functions */
 extern ssize_t of_modalias(const struct device_node *np, char *str, ssize_t len);
+extern ssize_t of_printable_modalias(const struct device_node *np, char *str,
+				     ssize_t len);
 extern int of_request_module(const struct device_node *np);
 
 /* phandle iterator functions */
@@ -758,6 +760,12 @@ static inline ssize_t of_modalias(const struct device_node *np, char *str,
 	return -ENODEV;
 }
 
+static inline ssize_t of_printable_modalias(const struct device_node *np,
+					    char *str, ssize_t len)
+{
+	return -ENODEV;
+}
+
 static inline int of_request_module(const struct device_node *np)
 {
 	return -ENODEV;
diff --git a/include/linux/of_device.h b/include/linux/of_device.h
index 2c7a3d4bc775..bca66f59814a 100644
--- a/include/linux/of_device.h
+++ b/include/linux/of_device.h
@@ -26,7 +26,14 @@ static inline int of_driver_match_device(struct device *dev,
 	return of_match_device(drv->of_match_table, dev) != NULL;
 }
 
-extern ssize_t of_device_modalias(struct device *dev, char *str, ssize_t len);
+static inline ssize_t of_device_modalias(struct device *dev, char *str,
+					 ssize_t len)
+{
+	if (!dev || !dev->of_node || dev->of_node_reused)
+		return -ENODEV;
+
+	return of_printable_modalias(dev->of_node, str, len);
+}
 
 extern void of_device_uevent(const struct device *dev, struct kobj_uevent_env *env);
 extern int of_device_uevent_modalias(const struct device *dev, struct kobj_uevent_env *env);
@@ -51,8 +58,8 @@ static inline int of_driver_match_device(struct device *dev,
 static inline void of_device_uevent(const struct device *dev,
 			struct kobj_uevent_env *env) { }
 
-static inline int of_device_modalias(struct device *dev,
-				     char *str, ssize_t len)
+static inline ssize_t of_device_modalias(struct device *dev,
+					 char *str, ssize_t len)
 {
 	return -ENODEV;
 }
-- 
2.34.1

[PATCH 2/5] of: module: Mutate of_device_uevent() into two helpers

[Miquel Raynal]

Move the OF related logic inside of/module.c and use it from of_device.h
with an inline helper so there is no visible change from the users point
of view.

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

diff --git a/drivers/of/device.c b/drivers/of/device.c
index 45ce83a8945f..5e538e1ed623 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -246,48 +246,6 @@ const void *of_device_get_match_data(const struct device *dev)
 }
 EXPORT_SYMBOL(of_device_get_match_data);
 
-/**
- * of_device_uevent - Display OF related uevent information
- * @dev:	Device to display the uevent information for
- * @env:	Kernel object's userspace event reference to fill up
- */
-void of_device_uevent(const struct device *dev, struct kobj_uevent_env *env)
-{
-	const char *compat, *type;
-	struct alias_prop *app;
-	struct property *p;
-	int seen = 0;
-
-	if ((!dev) || (!dev->of_node))
-		return;
-
-	add_uevent_var(env, "OF_NAME=%pOFn", dev->of_node);
-	add_uevent_var(env, "OF_FULLNAME=%pOF", dev->of_node);
-	type = of_node_get_device_type(dev->of_node);
-	if (type)
-		add_uevent_var(env, "OF_TYPE=%s", type);
-
-	/* Since the compatible field can contain pretty much anything
-	 * it's not really legal to split it out with commas. We split it
-	 * up using a number of environment variables instead. */
-	of_property_for_each_string(dev->of_node, "compatible", p, compat) {
-		add_uevent_var(env, "OF_COMPATIBLE_%d=%s", seen, compat);
-		seen++;
-	}
-	add_uevent_var(env, "OF_COMPATIBLE_N=%d", seen);
-
-	seen = 0;
-	mutex_lock(&of_mutex);
-	list_for_each_entry(app, &aliases_lookup, link) {
-		if (dev->of_node == app->np) {
-			add_uevent_var(env, "OF_ALIAS_%d=%s", seen,
-				       app->alias);
-			seen++;
-		}
-	}
-	mutex_unlock(&of_mutex);
-}
-
 int of_device_uevent_modalias(const struct device *dev, struct kobj_uevent_env *env)
 {
 	int sl;
diff --git a/drivers/of/module.c b/drivers/of/module.c
index c05fb8ca575c..c729675fdd04 100644
--- a/drivers/of/module.c
+++ b/drivers/of/module.c
@@ -8,6 +8,8 @@
 #include <linux/slab.h>
 #include <linux/string.h>
 
+#include "of_private.h"
+
 ssize_t of_modalias(const struct device_node *np, char *str, ssize_t len)
 {
 	const char *compat;
@@ -91,3 +93,42 @@ int of_request_module(const struct device_node *np)
 	return ret;
 }
 EXPORT_SYMBOL_GPL(of_request_module);
+
+int of_uevent(struct device_node *np, struct kobj_uevent_env *env)
+{
+	const char *compat, *type;
+	struct alias_prop *app;
+	struct property *p;
+	int seen = 0;
+
+	if (!np)
+		return -ENODEV;
+
+	add_uevent_var(env, "OF_NAME=%pOFn", np);
+	add_uevent_var(env, "OF_FULLNAME=%pOF", np);
+	type = of_node_get_device_type(np);
+	if (type)
+		add_uevent_var(env, "OF_TYPE=%s", type);
+
+	/* Since the compatible field can contain pretty much anything
+	 * it's not really legal to split it out with commas. We split it
+	 * up using a number of environment variables instead. */
+	of_property_for_each_string(np, "compatible", p, compat) {
+		add_uevent_var(env, "OF_COMPATIBLE_%d=%s", seen, compat);
+		seen++;
+	}
+	add_uevent_var(env, "OF_COMPATIBLE_N=%d", seen);
+
+	seen = 0;
+	mutex_lock(&of_mutex);
+	list_for_each_entry(app, &aliases_lookup, link) {
+		if (np == app->np) {
+			add_uevent_var(env, "OF_ALIAS_%d=%s", seen,
+				       app->alias);
+			seen++;
+		}
+	}
+	mutex_unlock(&of_mutex);
+
+	return 0;
+}
diff --git a/include/linux/of.h b/include/linux/of.h
index dcdd9396ac37..d99f33fc25d3 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -388,6 +388,7 @@ extern ssize_t of_modalias(const struct device_node *np, char *str, ssize_t len)
 extern ssize_t of_printable_modalias(const struct device_node *np, char *str,
 				     ssize_t len);
 extern int of_request_module(const struct device_node *np);
+extern int of_uevent(struct device_node *np, struct kobj_uevent_env *env);
 
 /* phandle iterator functions */
 extern int of_phandle_iterator_init(struct of_phandle_iterator *it,
@@ -771,6 +772,11 @@ static inline int of_request_module(const struct device_node *np)
 	return -ENODEV;
 }
 
+static inline int of_uevent(struct device_node *np, struct kobj_uevent_env *env)
+{
+	return -ENODEV;
+}
+
 static inline int of_phandle_iterator_init(struct of_phandle_iterator *it,
 					   const struct device_node *np,
 					   const char *list_name,
diff --git a/include/linux/of_device.h b/include/linux/of_device.h
index bca66f59814a..af5ee78e0c05 100644
--- a/include/linux/of_device.h
+++ b/include/linux/of_device.h
@@ -35,7 +35,15 @@ static inline ssize_t of_device_modalias(struct device *dev, char *str,
 	return of_printable_modalias(dev->of_node, str, len);
 }
 
-extern void of_device_uevent(const struct device *dev, struct kobj_uevent_env *env);
+static inline int of_device_uevent(const struct device *dev,
+				   struct kobj_uevent_env *env)
+{
+	if (!dev || !dev->of_node)
+		return -ENODEV;
+
+	return of_uevent(dev->of_node, env);
+}
+
 extern int of_device_uevent_modalias(const struct device *dev, struct kobj_uevent_env *env);
 
 int of_dma_configure_id(struct device *dev,
@@ -55,8 +63,11 @@ static inline int of_driver_match_device(struct device *dev,
 	return 0;
 }
 
-static inline void of_device_uevent(const struct device *dev,
-			struct kobj_uevent_env *env) { }
+static inline int of_device_uevent(const struct device *dev,
+				   struct kobj_uevent_env *env)
+{
+	return -ENODEV;
+}
 
 static inline ssize_t of_device_modalias(struct device *dev,
 					 char *str, ssize_t len)
-- 
2.34.1

[PATCH 3/5] of: module: Mutate of_device_uevent_modalias() into two helpers

[Miquel Raynal]

Let's move the logic of the former helper into module.c and use it from
an inline helper located under of_device.c. This way there is no change
for users while the logic gets moved to an OF-only file.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/of/device.c       | 23 -----------------------
 drivers/of/module.c       | 21 +++++++++++++++++++++
 include/linux/of.h        |  7 +++++++
 include/linux/of_device.h |  9 ++++++++-
 4 files changed, 36 insertions(+), 24 deletions(-)

diff --git a/drivers/of/device.c b/drivers/of/device.c
index 5e538e1ed623..7909eefc650e 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -245,26 +245,3 @@ const void *of_device_get_match_data(const struct device *dev)
 	return match->data;
 }
 EXPORT_SYMBOL(of_device_get_match_data);
-
-int of_device_uevent_modalias(const struct device *dev, struct kobj_uevent_env *env)
-{
-	int sl;
-
-	if ((!dev) || (!dev->of_node) || dev->of_node_reused)
-		return -ENODEV;
-
-	/* Devicetree modalias is tricky, we add it in 2 steps */
-	if (add_uevent_var(env, "MODALIAS="))
-		return -ENOMEM;
-
-	sl = of_modalias(dev->of_node, &env->buf[env->buflen-1],
-			 sizeof(env->buf) - env->buflen);
-	if (sl < 0)
-		return sl;
-	if (sl >= (sizeof(env->buf) - env->buflen))
-		return -ENOMEM;
-	env->buflen += sl;
-
-	return 0;
-}
-EXPORT_SYMBOL_GPL(of_device_uevent_modalias);
diff --git a/drivers/of/module.c b/drivers/of/module.c
index c729675fdd04..874f3fb8220f 100644
--- a/drivers/of/module.c
+++ b/drivers/of/module.c
@@ -132,3 +132,24 @@ int of_uevent(struct device_node *np, struct kobj_uevent_env *env)
 
 	return 0;
 }
+
+int of_uevent_modalias(const struct device_node *np, struct kobj_uevent_env *env)
+{
+	int sl;
+
+	if (!np)
+		return -ENODEV;
+
+	/* Devicetree modalias is tricky, we add it in 2 steps */
+	if (add_uevent_var(env, "MODALIAS="))
+		return -ENOMEM;
+
+	sl = of_modalias(np, &env->buf[env->buflen-1],
+			 sizeof(env->buf) - env->buflen);
+	if (sl >= (sizeof(env->buf) - env->buflen))
+		return -ENOMEM;
+	env->buflen += sl;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(of_uevent_modalias);
diff --git a/include/linux/of.h b/include/linux/of.h
index d99f33fc25d3..203bd2895d94 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -389,6 +389,7 @@ extern ssize_t of_printable_modalias(const struct device_node *np, char *str,
 				     ssize_t len);
 extern int of_request_module(const struct device_node *np);
 extern int of_uevent(struct device_node *np, struct kobj_uevent_env *env);
+extern int of_uevent_modalias(const struct device_node *np, struct kobj_uevent_env *env);
 
 /* phandle iterator functions */
 extern int of_phandle_iterator_init(struct of_phandle_iterator *it,
@@ -777,6 +778,12 @@ static inline int of_uevent(struct device_node *np, struct kobj_uevent_env *env)
 	return -ENODEV;
 }
 
+static inline int of_uevent_modalias(const struct device_node *np,
+				     struct kobj_uevent_env *env)
+{
+	return -ENODEV;
+}
+
 static inline int of_phandle_iterator_init(struct of_phandle_iterator *it,
 					   const struct device_node *np,
 					   const char *list_name,
diff --git a/include/linux/of_device.h b/include/linux/of_device.h
index af5ee78e0c05..5e428bcf3d63 100644
--- a/include/linux/of_device.h
+++ b/include/linux/of_device.h
@@ -44,7 +44,14 @@ static inline int of_device_uevent(const struct device *dev,
 	return of_uevent(dev->of_node, env);
 }
 
-extern int of_device_uevent_modalias(const struct device *dev, struct kobj_uevent_env *env);
+static inline int of_device_uevent_modalias(const struct device *dev,
+					    struct kobj_uevent_env *env)
+{
+	if (!dev || !dev->of_node || dev->of_node_reused)
+		return -ENODEV;
+
+	return of_uevent_modalias(dev->of_node, env);
+}
 
 int of_dma_configure_id(struct device *dev,
 		     struct device_node *np,
-- 
2.34.1

[PATCH 4/5] of: module: Export of_uevent()

[Miquel Raynal]

The content of of_uevent() is currently hardcoded in a driver that can
be compiled as a module. Nothing prevents of_uevent() to be exported to
modules, most of the other helpers in of_device.c actually are.The
reason why this helper was not exported is because it has been so far
only useful in drivers/base, which is built-in anyway.

With the idea of getting rid of the hardcoded implementation of
of_uevent() in other places in the kernel, let's export it to GPL
modules (very much like its cousins in the same file).

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

diff --git a/drivers/of/module.c b/drivers/of/module.c
index 874f3fb8220f..8b402a716951 100644
--- a/drivers/of/module.c
+++ b/drivers/of/module.c
@@ -132,6 +132,7 @@ int of_uevent(struct device_node *np, struct kobj_uevent_env *env)
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(of_uevent);
 
 int of_uevent_modalias(const struct device_node *np, struct kobj_uevent_env *env)
 {
-- 
2.34.1

[PATCH 5/5] gpu: host1x: Stop open-coding of_device_uevent()

[Miquel Raynal]

There is apparently no reasons to open-code of_device_uevent() besides:
- The helper receives a struct device while we want to use the of_node
  member of the struct device *parent*.
- of_device_uevent() could not be called by modules because of a missing
  EXPORT_SYMBOL*().

In practice, the former point is not very constraining, just calling
of_device_uevent(dev->parent, ...) would have made the trick.

The latter point is more an observation rather than a real blocking
point because nothing prevented of_uevent() (called by the inline
function of_device_uevent()) to be exported to modules. In practice,
this helper is now exported, so nothing prevent us from using
of_device_uevent() anymore.

Let's use the core helper directly instead of open-coding it.

Cc: Thierry Reding <thierry.reding@gmail.com>
Cc: David Airlie <airlied@gmail.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Mikko Perttunen <mperttunen@nvidia.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Frank Rowand <frowand.list@gmail.com>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>

---

This patch depends on the changes performed earlier in the series under
the drivers/of/ folder.
---
 drivers/gpu/host1x/bus.c | 31 ++++++-------------------------
 1 file changed, 6 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/host1x/bus.c b/drivers/gpu/host1x/bus.c
index 4d16a3396c4a..6434a183fb72 100644
--- a/drivers/gpu/host1x/bus.c
+++ b/drivers/gpu/host1x/bus.c
@@ -338,34 +338,15 @@ static int host1x_device_match(struct device *dev, struct device_driver *drv)
 	return strcmp(dev_name(dev), drv->name) == 0;
 }
 
+/*
+ * Note that this is really only needed for backwards compatibility
+ * with libdrm, which parses this information from sysfs and will
+ * fail if it can't find the OF_FULLNAME, specifically.
+ */
 static int host1x_device_uevent(const struct device *dev,
 				struct kobj_uevent_env *env)
 {
-	struct device_node *np = dev->parent->of_node;
-	unsigned int count = 0;
-	struct property *p;
-	const char *compat;
-
-	/*
-	 * This duplicates most of of_device_uevent(), but the latter cannot
-	 * be called from modules and operates on dev->of_node, which is not
-	 * available in this case.
-	 *
-	 * Note that this is really only needed for backwards compatibility
-	 * with libdrm, which parses this information from sysfs and will
-	 * fail if it can't find the OF_FULLNAME, specifically.
-	 */
-	add_uevent_var(env, "OF_NAME=%pOFn", np);
-	add_uevent_var(env, "OF_FULLNAME=%pOF", np);
-
-	of_property_for_each_string(np, "compatible", p, compat) {
-		add_uevent_var(env, "OF_COMPATIBLE_%u=%s", count, compat);
-		count++;
-	}
-
-	add_uevent_var(env, "OF_COMPATIBLE_N=%u", count);
-
-	return 0;
+	return of_device_uevent((const struct device *)&dev->parent, env);
 }
 
 static int host1x_dma_configure(struct device *dev)
-- 
2.34.1

Re: [PATCH 0/5] of: More 'device' vs. 'module' cleanups

[Rob Herring]

On Wed, May 10, 2023 at 05:47:58PM +0200, Miquel Raynal wrote:
> Hello,
> 
> As part of a previous series, Rob suggested that keeping too much logic
> in of/device.c was backward and would benefit from a gradual cleanup
> with the hope some day to move the remaining helpers into inline
> functions wrapping the proper of_*() methods.

Where I'm at on device.c is it should be functions that bus 
implementations need. I have a ton of tree wide changes to disentangle 
of_device.h and of_platform.h. Some of that landed in 6.4.

uevents are pretty much tied to struct device and the bus, so I don't 
think moving these parts to module.c makes sense unless there is a need 
for the functionality without a struct device. 

Also, perhaps we want to make module.c configurable?:

obj-$(CONFIG_MODULE) += module.o

The uevent stuff is certainly independent of module support.

> Link: https://lore.kernel.org/lkml/CAL_JsqJE43qfYzHUuCJsbaPPBTbYX05Q7FFmPTjPFZ3Dmz_mXg@mail.gmail.com/
> 
> A few of these "conversions" happened in the series I was originally
> working on. At this time I actually wrote a few other changes,
> completely unrelated to my original series, but still following Rob's
> cleanup idea: here they are.
> 
> Link: https://lore.kernel.org/lkml/20230307165359.225361-1-miquel.raynal@bootlin.com/
> 
> The last step of this series is to actually remove a copy of one of
> these helpers which I think is not needed. This drivers/gpu/ patch
> depends on the previous changes.
> 
> Thanks, Miquèl
> 
> Miquel Raynal (5):
>   of: module: Mutate of_device_modalias() into two helpers
>   of: module: Mutate of_device_uevent() into two helpers
>   of: module: Mutate of_device_uevent_modalias() into two helpers
>   of: module: Export of_uevent()
>   gpu: host1x: Stop open-coding of_device_uevent()

This last patch is certainly worthwhile doing.

Rob