Skip to contentSkip to conversation

pmdomain: core: switch to dynamic root device

PATCH In review Johan Hovold 3 participants 5 messages Apr 24 – Jun 9 Generic PM domains

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 pmdomain: core: switch to dynamic root device

Driver core expects devices to be dynamically allocated and will, for example, complain loudly if a device that lacks a release function is ever freed.

Use root_device_register() to allocate and register the root device instead of open coding using a static device.

---
 drivers/pmdomain/core.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)
drivers/pmdomain/core.c +6 -10
index 4d32fc676aaf..e01884f2d7b7 100644
@@ -33,9 +33,7 @@ static const struct bus_type genpd_provider_bus_type = {
 };
 
 /* The parent for genpd_provider devices. */
-static struct device genpd_provider_bus = {
-	.init_name = "genpd_provider",
-};
+static struct device *genpd_provider_bus;
 
 #define GENPD_RETRY_MAX_MS	250		/* Approximate */
 
@@ -2325,7 +2323,7 @@ static int genpd_alloc_data(struct generic_pm_domain *genpd)
 	device_initialize(&genpd->dev);
 	genpd->dev.release = genpd_provider_release;
 	genpd->dev.bus = &genpd_provider_bus_type;
-	genpd->dev.parent = &genpd_provider_bus;
+	genpd->dev.parent = genpd_provider_bus;
 
 	if (!genpd_is_dev_name_fw(genpd)) {
 		dev_set_name(&genpd->dev, "%s", genpd->name);
@@ -3567,11 +3565,9 @@ static int __init genpd_bus_init(void)
 {
 	int ret;
 
-	ret = device_register(&genpd_provider_bus);
-	if (ret) {
-		put_device(&genpd_provider_bus);
-		return ret;
-	}
+	genpd_provider_bus = root_device_register("genpd_provider");
+	if (IS_ERR(genpd_provider_bus))
+		return PTR_ERR(genpd_provider_bus);
 
 	ret = bus_register(&genpd_provider_bus_type);
 	if (ret)
@@ -3593,7 +3589,7 @@ static int __init genpd_bus_init(void)
 err_prov_bus:
 	bus_unregister(&genpd_provider_bus_type);
 err_dev:
-	device_unregister(&genpd_provider_bus);
+	root_device_unregister(genpd_provider_bus);
 	return ret;
 }
 core_initcall(genpd_bus_init);
Signature · 1 line
2.53.0
#1 lore raw
Quoted 9 lines · Johan Hovold

Driver core expects devices to be dynamically allocated and will, for example, complain loudly if a device that lacks a release function is ever freed.

Use root_device_register() to allocate and register the root device instead of open coding using a static device.

Applied for next, thanks!

Kind regards
Uffe

Quoted 54 lines
---
 drivers/pmdomain/core.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)
drivers/pmdomain/core.c +6 -11
index 4d32fc676aaf..e01884f2d7b7 100644
@@ -33,9 +33,7 @@ static const struct bus_type genpd_provider_bus_type = {
 };
 
 /* The parent for genpd_provider devices. */
-static struct device genpd_provider_bus = {
-       .init_name = "genpd_provider",
-};
+static struct device *genpd_provider_bus;
 
 #define GENPD_RETRY_MAX_MS     250             /* Approximate */
 
@@ -2325,7 +2323,7 @@ static int genpd_alloc_data(struct generic_pm_domain *genpd)
        device_initialize(&genpd->dev);
        genpd->dev.release = genpd_provider_release;
        genpd->dev.bus = &genpd_provider_bus_type;
-       genpd->dev.parent = &genpd_provider_bus;
+       genpd->dev.parent = genpd_provider_bus;
 
        if (!genpd_is_dev_name_fw(genpd)) {
                dev_set_name(&genpd->dev, "%s", genpd->name);
@@ -3567,11 +3565,9 @@ static int __init genpd_bus_init(void)
 {
        int ret;
 
-       ret = device_register(&genpd_provider_bus);
-       if (ret) {
-               put_device(&genpd_provider_bus);
-               return ret;
-       }
+       genpd_provider_bus = root_device_register("genpd_provider");
+       if (IS_ERR(genpd_provider_bus))
+               return PTR_ERR(genpd_provider_bus);
 
        ret = bus_register(&genpd_provider_bus_type);
        if (ret)
@@ -3593,7 +3589,7 @@ static int __init genpd_bus_init(void)
 err_prov_bus:
        bus_unregister(&genpd_provider_bus_type);
 err_dev:
-       device_unregister(&genpd_provider_bus);
+       root_device_unregister(genpd_provider_bus);
        return ret;
 }
 core_initcall(genpd_bus_init);
--

2.53.0

#2 lore raw
In reply to Johan Hovold

Hi Johan,

Quoted 8 lines · Johan Hovold

Driver core expects devices to be dynamically allocated and will, for example, complain loudly if a device that lacks a release function is ever freed.

Use root_device_register() to allocate and register the root device instead of open coding using a static device.

Thanks for your patch, which is now commit a96e40f4afdcb52a ("pmdomain: core: switch to dynamic root device") in pmdomain/next. On e.g. R-Car H1, R-Car M2-W, and R-Car H3, this causes scary messages when systemd-journald.service is started:

synth uevent: /always-on: failed to send uevent genpd_provider always-on: uevent: failed to send synthetic uevent: -22 synth uevent: /ca15-cpu0: failed to send uevent genpd_provider ca15-cpu0: uevent: failed to send synthetic uevent: -22 [...]

Reverting the commit fixes the issue.

Quoted 38 lines
drivers/pmdomain/core.c +5 -9
@@ -33,9 +33,7 @@ static const struct bus_type genpd_provider_bus_type = {
 };
 
 /* The parent for genpd_provider devices. */
-static struct device genpd_provider_bus = {
-       .init_name = "genpd_provider",
-};
+static struct device *genpd_provider_bus;
 
 #define GENPD_RETRY_MAX_MS     250             /* Approximate */
 
@@ -2325,7 +2323,7 @@ static int genpd_alloc_data(struct generic_pm_domain *genpd)
        device_initialize(&genpd->dev);
        genpd->dev.release = genpd_provider_release;
        genpd->dev.bus = &genpd_provider_bus_type;
-       genpd->dev.parent = &genpd_provider_bus;
+       genpd->dev.parent = genpd_provider_bus;
 
        if (!genpd_is_dev_name_fw(genpd)) {
                dev_set_name(&genpd->dev, "%s", genpd->name);
@@ -3567,11 +3565,9 @@ static int __init genpd_bus_init(void)
 {
        int ret;
 
-       ret = device_register(&genpd_provider_bus);
-       if (ret) {
-               put_device(&genpd_provider_bus);
-               return ret;
-       }
+       genpd_provider_bus = root_device_register("genpd_provider");
+       if (IS_ERR(genpd_provider_bus))
+               return PTR_ERR(genpd_provider_bus);
 
        ret = bus_register(&genpd_provider_bus_type);
        if (ret)

drivers/pmdomain/core.c static int __init genpd_bus_init(void) · line 3595

Quoted 8 lines
drivers/pmdomain/core.c +1 -1
@@ -3593,7 +3589,7 @@ static int __init genpd_bus_init(void)
 err_prov_bus:
        bus_unregister(&genpd_provider_bus_type);
 err_dev:
-       device_unregister(&genpd_provider_bus);
+       root_device_unregister(genpd_provider_bus);
        return ret;
 }
 core_initcall(genpd_bus_init);

Apparently this is done too late for drivers/pmdomain/renesas/rcar-sysc.c, which registers PM domains from an early_initcall(). Doing all work in rcar-sysc.c from the postcore_initcall() is not an option, as the CPU power domains are needed for secondary CPU startup on R-Car H1.

Note that R-Car Gen4 is not affected, as it uses rcar-gen4-sysc.c instead.

Gr{oetje,eeting}s,

Geert

Signature · 5 lines
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
#3 lore raw
Quoted 15 lines · Geert Uytterhoeven
Quoted 8 lines · Johan Hovold

Driver core expects devices to be dynamically allocated and will, for example, complain loudly if a device that lacks a release function is ever freed.

Use root_device_register() to allocate and register the root device instead of open coding using a static device.

Thanks for your patch, which is now commit a96e40f4afdcb52a ("pmdomain: core: switch to dynamic root device") in pmdomain/next. On e.g. R-Car H1, R-Car M2-W, and R-Car H3, this causes scary messages when systemd-journald.service is started:

Quoted 5 lines

synth uevent: /always-on: failed to send uevent genpd_provider always-on: uevent: failed to send synthetic uevent: -22 synth uevent: /ca15-cpu0: failed to send uevent genpd_provider ca15-cpu0: uevent: failed to send synthetic uevent: -22 [...]

I missed that there is another set on e.g. R-Car H1 for the clock domain:

synth uevent: /clocks: failed to send uevent genpd_provider clocks: uevent: failed to send synthetic uevent: -22

Quoted 38 lines

Reverting the commit fixes the issue.

Quoted 35 lines
drivers/pmdomain/core.c +5 -9
@@ -33,9 +33,7 @@ static const struct bus_type genpd_provider_bus_type = {
 };
 
 /* The parent for genpd_provider devices. */
-static struct device genpd_provider_bus = {
-       .init_name = "genpd_provider",
-};
+static struct device *genpd_provider_bus;
 
 #define GENPD_RETRY_MAX_MS     250             /* Approximate */
 
@@ -2325,7 +2323,7 @@ static int genpd_alloc_data(struct generic_pm_domain *genpd)
        device_initialize(&genpd->dev);
        genpd->dev.release = genpd_provider_release;
        genpd->dev.bus = &genpd_provider_bus_type;
-       genpd->dev.parent = &genpd_provider_bus;
+       genpd->dev.parent = genpd_provider_bus;
 
        if (!genpd_is_dev_name_fw(genpd)) {
                dev_set_name(&genpd->dev, "%s", genpd->name);
@@ -3567,11 +3565,9 @@ static int __init genpd_bus_init(void)
 {
        int ret;
 
-       ret = device_register(&genpd_provider_bus);
-       if (ret) {
-               put_device(&genpd_provider_bus);
-               return ret;
-       }
+       genpd_provider_bus = root_device_register("genpd_provider");
+       if (IS_ERR(genpd_provider_bus))
+               return PTR_ERR(genpd_provider_bus);
 
Quoted 16 lines

drivers/pmdomain/core.c static int __init genpd_bus_init(void) · line 3595

Quoted 11 lines
drivers/pmdomain/core.c +1 -1
        ret = bus_register(&genpd_provider_bus_type);
        if (ret)
@@ -3593,7 +3589,7 @@ static int __init genpd_bus_init(void)
 err_prov_bus:
        bus_unregister(&genpd_provider_bus_type);
 err_dev:
-       device_unregister(&genpd_provider_bus);
+       root_device_unregister(genpd_provider_bus);
        return ret;
 }
 core_initcall(genpd_bus_init);

Apparently this is done too late for drivers/pmdomain/renesas/rcar-sysc.c, which registers PM domains from an early_initcall(). Doing all work in rcar-sysc.c from the postcore_initcall() is not an option, as the CPU power domains are needed for secondary CPU startup on R-Car H1.

Same story for cpg_mstp_add_clk_domain(), which is called from

    CLK_OF_DECLARE(r8a7778_cpg_clks, "renesas,r8a7778-cpg-clocks",
                   r8a7778_cpg_clocks_init);
    CLK_OF_DECLARE(r8a7779_cpg_clks, "renesas,r8a7779-cpg-clocks",
                   r8a7779_cpg_clocks_init);
    CLK_OF_DECLARE(rz_cpg_clks, "renesas,rz-cpg-clocks", rz_cpg_clocks_init);

Gr{oetje,eeting}s,

Geert

Signature · 5 lines
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
#4 lore raw
In reply to Geert Uytterhoeven

Hi Geert,

Quoted 15 lines · Geert Uytterhoeven
Quoted 8 lines · Johan Hovold

Driver core expects devices to be dynamically allocated and will, for example, complain loudly if a device that lacks a release function is ever freed.

Use root_device_register() to allocate and register the root device instead of open coding using a static device.

Thanks for your patch, which is now commit a96e40f4afdcb52a ("pmdomain: core: switch to dynamic root device") in pmdomain/next. On e.g. R-Car H1, R-Car M2-W, and R-Car H3, this causes scary messages when systemd-journald.service is started:

Quoted 7 lines

synth uevent: /always-on: failed to send uevent genpd_provider always-on: uevent: failed to send synthetic uevent: -22 synth uevent: /ca15-cpu0: failed to send uevent genpd_provider ca15-cpu0: uevent: failed to send synthetic uevent: -22 [...]

Reverting the commit fixes the issue.

Thanks for the report and sorry about the breakage.

I mistakenly convinced myself that this would not be an issue due to the pmdomain code making sure that the bus has been registered but missed the two-step initialisation.

I just sent a fix here if you want to give it a spin:

	https://lore.kernel.org/lkml/[email protected]/

Johan