[PATCH v3 0/2] Small of/device cleanup

[PATCH v3 0/2] Small of/device cleanup

[Miquel Raynal]

My previous attempt to slightly clean the OF core wrt device structures
was rather unsuccessful as the idea behind the discussed cleanup was
more impacting than what I thought, leading to most of the previous
series to be dropped. However, aside, two patches seemed actually
relevant, so here they are, alone.

Link: https://lore.kernel.org/all/20230608184903.GA3200973-robh@kernel.org/

Thanks,
Miquèl

Changes in v3:
* Fixed the dev->parent referencing in the host1x driver.
* Collected Rob's Acked-by.

Changes in v2:
* Dropped all the of_device.h/of_module.h changes
* Directly used of_device_uevent() from the host1x driver


Miquel Raynal (2):
  of: module: Export of_device_uevent()
  gpu: host1x: Stop open-coding of_device_uevent()

 drivers/gpu/host1x/bus.c | 29 ++++++-----------------------
 drivers/of/device.c      |  1 +
 2 files changed, 7 insertions(+), 23 deletions(-)

-- 
2.34.1

[PATCH v3 1/2] of: module: Export of_device_uevent()

[Miquel Raynal]

The content of of_device_uevent() is currently hardcoded in a driver
that can be compiled as a module. Nothing prevents of_device_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_device_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>
Acked-by: Rob Herring <robh@kernel.org>
---
 drivers/of/device.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/of/device.c b/drivers/of/device.c
index 0f00f1b80708..90131de6d75b 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -312,6 +312,7 @@ void of_device_uevent(const struct device *dev, struct kobj_uevent_env *env)
 	}
 	mutex_unlock(&of_mutex);
 }
+EXPORT_SYMBOL_GPL(of_device_uevent);
 
 int of_device_uevent_modalias(const struct device *dev, struct kobj_uevent_env *env)
 {
-- 
2.34.1

[PATCH v3 2/2] 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>
---
 drivers/gpu/host1x/bus.c | 29 ++++++-----------------------
 1 file changed, 6 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/host1x/bus.c b/drivers/gpu/host1x/bus.c
index 4d16a3396c4a..84d042796d2e 100644
--- a/drivers/gpu/host1x/bus.c
+++ b/drivers/gpu/host1x/bus.c
@@ -338,32 +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);
+	of_device_uevent(dev->parent, env);
 
 	return 0;
 }
-- 
2.34.1

Re: [PATCH v3 0/2] Small of/device cleanup

[Miquel Raynal]

Hello,

miquel.raynal@bootlin.com wrote on Thu, 22 Jun 2023 23:32:12 +0200:

> My previous attempt to slightly clean the OF core wrt device structures
> was rather unsuccessful as the idea behind the discussed cleanup was
> more impacting than what I thought, leading to most of the previous
> series to be dropped. However, aside, two patches seemed actually
> relevant, so here they are, alone.
> 
> Link: https://lore.kernel.org/all/20230608184903.GA3200973-robh@kernel.org/

I expect this small series to go through the drm tree, but as I actually
sent it right before the beginning of the merge window and because I am
not an experienced drm contributor, I would like to know if I am
required to resend the patches or if they are fine as-is (I don't
expect any conflicts with v6.5-rc1).

Just let me know if a re-send is expected.

Cheers,
Miquèl

> 
> Thanks,
> Miquèl
> 
> Changes in v3:
> * Fixed the dev->parent referencing in the host1x driver.
> * Collected Rob's Acked-by.
> 
> Changes in v2:
> * Dropped all the of_device.h/of_module.h changes
> * Directly used of_device_uevent() from the host1x driver
> 
> 
> Miquel Raynal (2):
>   of: module: Export of_device_uevent()
>   gpu: host1x: Stop open-coding of_device_uevent()
> 
>  drivers/gpu/host1x/bus.c | 29 ++++++-----------------------
>  drivers/of/device.c      |  1 +
>  2 files changed, 7 insertions(+), 23 deletions(-)
> 

Re: [PATCH v3 2/2] gpu: host1x: Stop open-coding of_device_uevent()

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1083 bytes --]

On Thu, Jun 22, 2023 at 11:32:14PM +0200, Miquel Raynal wrote:
> 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.

Yeah, looks like that's correct. I guess I always thought this
information would get added to the sysfs node of the struct device *
that was passed in, while it actually gets passed to the environment
created for the struct device passed into the caller. In other words
what we pass to of_device_uevent() here is only ever used as a source of
information, so passing dev->parent works.

I've also verified this on Tegra30 Beaver just to make sure and it looks
like the generated uevent file is identical before and after this patch.

Applied to drm-misc, thanks.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]