[PATCH v2 0/2] Fix simple-bus issues with fw_devlink

[PATCH v2 0/2] Fix simple-bus issues with fw_devlink

[Saravana Kannan]

Ulf reported an issue[1] with fw_devlink. This series tries to fix that
issue.

I replicated a similar set up on my end and I confirmed these patches work.

Ulf, mind testing this?

Thanks,
Saravana
[1] - https://lore.kernel.org/lkml/CAPDyKFo9Bxremkb1dDrr4OcXSpE0keVze94Cm=zrkOVxHHxBmQ@mail.gmail.com/

Cc: Ulf Hansson <ulf.hansson@linaro.org>
Cc: Geert Uytterhoeven <geert+renesas@glider.be>

Saravana Kannan (2):
  of: platform: Make sure bus only devices get probed
  drivers: bus: simple-pm-bus: Add support for probing simple bus only
    devices

 drivers/bus/simple-pm-bus.c |  7 ++++++
 drivers/of/platform.c       | 45 +++++++++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+)

-- 
2.33.0.153.gba50c8fa24-goog

[PATCH v2 1/2] of: platform: Make sure bus only devices get probed

[Saravana Kannan]

fw_devlink could end up creating device links for bus only devices.
However, bus only devices don't get probed and can block probe() or
sync_state() [1] call backs of other devices. To avoid this, set up
these devices to get probed by the simple-pm-bus.

[1] - https://lore.kernel.org/lkml/CAPDyKFo9Bxremkb1dDrr4OcXSpE0keVze94Cm=zrkOVxHHxBmQ@mail.gmail.com/
Signed-off-by: Saravana Kannan <saravanak@google.com>
Tested-by: Saravana Kannan <saravanak@google.com>
---
 drivers/of/platform.c | 45 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 74afbb7a4f5e..b010e2310131 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -97,6 +97,48 @@ static void of_device_make_bus_id(struct device *dev)
 	}
 }
 
+/**
+ * of_match_only_simple_bus - Check if a device node is only a simple bus
+ * @np: device node to check
+ *
+ * A simple bus in this context is defined as a transparent bus whose child
+ * devices are automatically populated but has no hardware specific
+ * functionality.
+ *
+ * Returns true if the device node is only a simple bus and can never match
+ * with any other specific driver.  Otherwise, returns false.
+ */
+static bool of_match_only_simple_bus(struct device_node *np)
+{
+	/* List of buses that don't have an explicit driver. */
+	static const char * const of_simple_bus_table[] = {
+		"simple-bus",
+		"simple-mfd",
+		"isa",
+		"arm,amba-bus",
+	};
+	const char *cp;
+	struct property *prop;
+	int i;
+
+	prop = of_find_property(np, "compatible", NULL);
+	for (cp = of_prop_next_string(prop, NULL); cp;
+	     cp = of_prop_next_string(prop, cp)) {
+		bool match = false;
+		const char *bus = of_simple_bus_table[i];
+
+		for (i = 0; i < ARRAY_SIZE(of_simple_bus_table); i++)
+			if (!of_compat_cmp(cp, bus, strlen(bus))) {
+				match = true;
+				break;
+			}
+		if (!match)
+			return false;
+	}
+
+	return true;
+}
+
 /**
  * of_device_alloc - Allocate and initialize an of_device
  * @np: device node to assign to device
@@ -143,6 +185,9 @@ struct platform_device *of_device_alloc(struct device_node *np,
 	dev->dev.fwnode = &np->fwnode;
 	dev->dev.parent = parent ? : &platform_bus;
 
+	if (of_match_only_simple_bus(np))
+		dev->driver_override = "simple-pm-bus";
+
 	if (bus_id)
 		dev_set_name(&dev->dev, "%s", bus_id);
 	else
-- 
2.33.0.153.gba50c8fa24-goog

[PATCH v2 2/2] drivers: bus: simple-pm-bus: Add support for probing simple bus only devices

[Saravana Kannan]

The OF platform code sets up simple bus only devices (i.e. devices that
won't match with any other driver) to get probed by the simple-pm-bus to
keep fw_devlink from blocking probe() or sync_state() [1] callbacks of
other devices. There's no need to populate the child devices since the
OF platform code would do that anyway, so return early for these simple
bus only devices.

[1] - https://lore.kernel.org/lkml/CAPDyKFo9Bxremkb1dDrr4OcXSpE0keVze94Cm=zrkOVxHHxBmQ@mail.gmail.com/
Signed-off-by: Saravana Kannan <saravanak@google.com>
Tested-by: Saravana Kannan <saravanak@google.com>
---
 drivers/bus/simple-pm-bus.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/bus/simple-pm-bus.c b/drivers/bus/simple-pm-bus.c
index 01a3d0cd08ed..91d52021b7f9 100644
--- a/drivers/bus/simple-pm-bus.c
+++ b/drivers/bus/simple-pm-bus.c
@@ -19,6 +19,13 @@ static int simple_pm_bus_probe(struct platform_device *pdev)
 	const struct of_dev_auxdata *lookup = dev_get_platdata(&pdev->dev);
 	struct device_node *np = pdev->dev.of_node;
 
+	/*
+	 * These are transparent bus devices (not simple-pm-bus matches) that
+	 * get populated automatically.  So, don't need to do anything more.
+	 */
+	if (pdev->driver_override)
+		return 0;
+
 	dev_dbg(&pdev->dev, "%s\n", __func__);
 
 	pm_runtime_enable(&pdev->dev);
-- 
2.33.0.153.gba50c8fa24-goog

Re: [PATCH v2 2/2] drivers: bus: simple-pm-bus: Add support for probing simple bus only devices

[Ulf Hansson]

On Fri, 3 Sept 2021 at 01:04, Saravana Kannan <saravanak@google.com> wrote:
>
> The OF platform code sets up simple bus only devices (i.e. devices that
> won't match with any other driver) to get probed by the simple-pm-bus to
> keep fw_devlink from blocking probe() or sync_state() [1] callbacks of
> other devices. There's no need to populate the child devices since the
> OF platform code would do that anyway, so return early for these simple
> bus only devices.

This looks like a neat solution to our problem. Although, a few comments below.

>
> [1] - https://lore.kernel.org/lkml/CAPDyKFo9Bxremkb1dDrr4OcXSpE0keVze94Cm=zrkOVxHHxBmQ@mail.gmail.com/
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> Tested-by: Saravana Kannan <saravanak@google.com>
> ---
>  drivers/bus/simple-pm-bus.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/drivers/bus/simple-pm-bus.c b/drivers/bus/simple-pm-bus.c
> index 01a3d0cd08ed..91d52021b7f9 100644
> --- a/drivers/bus/simple-pm-bus.c
> +++ b/drivers/bus/simple-pm-bus.c
> @@ -19,6 +19,13 @@ static int simple_pm_bus_probe(struct platform_device *pdev)
>         const struct of_dev_auxdata *lookup = dev_get_platdata(&pdev->dev);
>         struct device_node *np = pdev->dev.of_node;
>
> +       /*
> +        * These are transparent bus devices (not simple-pm-bus matches) that
> +        * get populated automatically.  So, don't need to do anything more.
> +        */
> +       if (pdev->driver_override)
> +               return 0;

You need the same kind of check in simple_pm_bus_remove(), to avoid
pm_runtime_disable(). At least for consistency.

> +
>         dev_dbg(&pdev->dev, "%s\n", __func__);
>
>         pm_runtime_enable(&pdev->dev);
> --
> 2.33.0.153.gba50c8fa24-goog
>

It also looks like we should flip the order of the patches in the
series, to keep things bisectable.

Kind regards
Uffe

Re: [PATCH v2 1/2] of: platform: Make sure bus only devices get probed

[Ulf Hansson]

On Fri, 3 Sept 2021 at 01:04, Saravana Kannan <saravanak@google.com> wrote:
>
> fw_devlink could end up creating device links for bus only devices.
> However, bus only devices don't get probed and can block probe() or
> sync_state() [1] call backs of other devices. To avoid this, set up
> these devices to get probed by the simple-pm-bus.
>
> [1] - https://lore.kernel.org/lkml/CAPDyKFo9Bxremkb1dDrr4OcXSpE0keVze94Cm=zrkOVxHHxBmQ@mail.gmail.com/
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> Tested-by: Saravana Kannan <saravanak@google.com>

Again, this looks like a nice solution to the problem.

One question though. The Kconfig SIMPLE_PM_BUS, should probably be
"default y" - or something along those lines to make sure fw_devlink
works as expected.

Kind regards
Uffe


> ---
>  drivers/of/platform.c | 45 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 45 insertions(+)
>
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index 74afbb7a4f5e..b010e2310131 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -97,6 +97,48 @@ static void of_device_make_bus_id(struct device *dev)
>         }
>  }
>
> +/**
> + * of_match_only_simple_bus - Check if a device node is only a simple bus
> + * @np: device node to check
> + *
> + * A simple bus in this context is defined as a transparent bus whose child
> + * devices are automatically populated but has no hardware specific
> + * functionality.
> + *
> + * Returns true if the device node is only a simple bus and can never match
> + * with any other specific driver.  Otherwise, returns false.
> + */
> +static bool of_match_only_simple_bus(struct device_node *np)
> +{
> +       /* List of buses that don't have an explicit driver. */
> +       static const char * const of_simple_bus_table[] = {
> +               "simple-bus",
> +               "simple-mfd",
> +               "isa",
> +               "arm,amba-bus",
> +       };
> +       const char *cp;
> +       struct property *prop;
> +       int i;
> +
> +       prop = of_find_property(np, "compatible", NULL);
> +       for (cp = of_prop_next_string(prop, NULL); cp;
> +            cp = of_prop_next_string(prop, cp)) {
> +               bool match = false;
> +               const char *bus = of_simple_bus_table[i];
> +
> +               for (i = 0; i < ARRAY_SIZE(of_simple_bus_table); i++)
> +                       if (!of_compat_cmp(cp, bus, strlen(bus))) {
> +                               match = true;
> +                               break;
> +                       }
> +               if (!match)
> +                       return false;
> +       }
> +
> +       return true;
> +}
> +
>  /**
>   * of_device_alloc - Allocate and initialize an of_device
>   * @np: device node to assign to device
> @@ -143,6 +185,9 @@ struct platform_device *of_device_alloc(struct device_node *np,
>         dev->dev.fwnode = &np->fwnode;
>         dev->dev.parent = parent ? : &platform_bus;
>
> +       if (of_match_only_simple_bus(np))
> +               dev->driver_override = "simple-pm-bus";
> +
>         if (bus_id)
>                 dev_set_name(&dev->dev, "%s", bus_id);
>         else
> --
> 2.33.0.153.gba50c8fa24-goog
>

Re: [PATCH v2 1/2] of: platform: Make sure bus only devices get probed

[Geert Uytterhoeven]

Hi Ulf,

On Fri, Sep 3, 2021 at 11:19 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On Fri, 3 Sept 2021 at 01:04, Saravana Kannan <saravanak@google.com> wrote:
> > fw_devlink could end up creating device links for bus only devices.
> > However, bus only devices don't get probed and can block probe() or
> > sync_state() [1] call backs of other devices. To avoid this, set up
> > these devices to get probed by the simple-pm-bus.
> >
> > [1] - https://lore.kernel.org/lkml/CAPDyKFo9Bxremkb1dDrr4OcXSpE0keVze94Cm=zrkOVxHHxBmQ@mail.gmail.com/
> > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > Tested-by: Saravana Kannan <saravanak@google.com>
>
> Again, this looks like a nice solution to the problem.
>
> One question though. The Kconfig SIMPLE_PM_BUS, should probably be
> "default y" - or something along those lines to make sure fw_devlink
> works as expected.

I would love for SIMPLE_PM_BUS to go away, and all of its functionality
to be usurped by the standard simple-bus handling.

In the modern world, everything uses power management and Runtime
PM, and the distinction between "simple-bus" and "simple-pm-bus"
is purely artificial.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

Re: [PATCH v2 1/2] of: platform: Make sure bus only devices get probed

[Saravana Kannan]

On Fri, Sep 3, 2021 at 7:29 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Ulf,
>
> On Fri, Sep 3, 2021 at 11:19 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > On Fri, 3 Sept 2021 at 01:04, Saravana Kannan <saravanak@google.com> wrote:
> > > fw_devlink could end up creating device links for bus only devices.
> > > However, bus only devices don't get probed and can block probe() or
> > > sync_state() [1] call backs of other devices. To avoid this, set up
> > > these devices to get probed by the simple-pm-bus.
> > >
> > > [1] - https://lore.kernel.org/lkml/CAPDyKFo9Bxremkb1dDrr4OcXSpE0keVze94Cm=zrkOVxHHxBmQ@mail.gmail.com/
> > > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > > Tested-by: Saravana Kannan <saravanak@google.com>
> >
> > Again, this looks like a nice solution to the problem.
> >
> > One question though. The Kconfig SIMPLE_PM_BUS, should probably be
> > "default y" - or something along those lines to make sure fw_devlink
> > works as expected.
>
> I would love for SIMPLE_PM_BUS to go away, and all of its functionality
> to be usurped by the standard simple-bus handling.

What if SIMPLE_PM_BUS *is* the standard simple-bus handling?
Insert "I'm the captain now" meme.

I think SIMPLE_PM_BUS config should just go away and we should compile
the driver in if CONFIG_OF is selected.

-Saravana

Re: [PATCH v2 2/2] drivers: bus: simple-pm-bus: Add support for probing simple bus only devices

[Saravana Kannan]

On Fri, Sep 3, 2021 at 2:15 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Fri, 3 Sept 2021 at 01:04, Saravana Kannan <saravanak@google.com> wrote:
> >
> > The OF platform code sets up simple bus only devices (i.e. devices that
> > won't match with any other driver) to get probed by the simple-pm-bus to
> > keep fw_devlink from blocking probe() or sync_state() [1] callbacks of
> > other devices. There's no need to populate the child devices since the
> > OF platform code would do that anyway, so return early for these simple
> > bus only devices.
>
> This looks like a neat solution to our problem. Although, a few comments below.
>
> >
> > [1] - https://lore.kernel.org/lkml/CAPDyKFo9Bxremkb1dDrr4OcXSpE0keVze94Cm=zrkOVxHHxBmQ@mail.gmail.com/
> > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > Tested-by: Saravana Kannan <saravanak@google.com>
> > ---
> >  drivers/bus/simple-pm-bus.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/drivers/bus/simple-pm-bus.c b/drivers/bus/simple-pm-bus.c
> > index 01a3d0cd08ed..91d52021b7f9 100644
> > --- a/drivers/bus/simple-pm-bus.c
> > +++ b/drivers/bus/simple-pm-bus.c
> > @@ -19,6 +19,13 @@ static int simple_pm_bus_probe(struct platform_device *pdev)
> >         const struct of_dev_auxdata *lookup = dev_get_platdata(&pdev->dev);
> >         struct device_node *np = pdev->dev.of_node;
> >
> > +       /*
> > +        * These are transparent bus devices (not simple-pm-bus matches) that
> > +        * get populated automatically.  So, don't need to do anything more.
> > +        */
> > +       if (pdev->driver_override)
> > +               return 0;
>
> You need the same kind of check in simple_pm_bus_remove(), to avoid
> pm_runtime_disable(). At least for consistency.

Ack.

>
> > +
> >         dev_dbg(&pdev->dev, "%s\n", __func__);
> >
> >         pm_runtime_enable(&pdev->dev);
> > --
> > 2.33.0.153.gba50c8fa24-goog
> >
>
> It also looks like we should flip the order of the patches in the
> series, to keep things bisectable.

Sorry I didn't get this. I'm not causing any compilation issues. Why
does this matter for bisecting?

-Saravana

Re: [PATCH v2 1/2] of: platform: Make sure bus only devices get probed

[Geert Uytterhoeven]

Hi Saravana,

On Fri, Sep 3, 2021 at 7:09 PM Saravana Kannan <saravanak@google.com> wrote:
> On Fri, Sep 3, 2021 at 7:29 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Fri, Sep 3, 2021 at 11:19 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > On Fri, 3 Sept 2021 at 01:04, Saravana Kannan <saravanak@google.com> wrote:
> > > > fw_devlink could end up creating device links for bus only devices.
> > > > However, bus only devices don't get probed and can block probe() or
> > > > sync_state() [1] call backs of other devices. To avoid this, set up
> > > > these devices to get probed by the simple-pm-bus.
> > > >
> > > > [1] - https://lore.kernel.org/lkml/CAPDyKFo9Bxremkb1dDrr4OcXSpE0keVze94Cm=zrkOVxHHxBmQ@mail.gmail.com/
> > > > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > > > Tested-by: Saravana Kannan <saravanak@google.com>
> > >
> > > Again, this looks like a nice solution to the problem.
> > >
> > > One question though. The Kconfig SIMPLE_PM_BUS, should probably be
> > > "default y" - or something along those lines to make sure fw_devlink
> > > works as expected.
> >
> > I would love for SIMPLE_PM_BUS to go away, and all of its functionality
> > to be usurped by the standard simple-bus handling.
>
> What if SIMPLE_PM_BUS *is* the standard simple-bus handling?
> Insert "I'm the captain now" meme.

No objections from my side.  In fact that's how I wanted it to be
from the beginning, but the DT people wanted a separate compatible
value (and a separate driver).  The only difference is the presence
of pm_runtime_*() calls.

> I think SIMPLE_PM_BUS config should just go away and we should compile
> the driver in if CONFIG_OF is selected.

I think there would be no need for a separate driver, if the standard
simple-bus handling would take care of it.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

Re: [PATCH v2 1/2] of: platform: Make sure bus only devices get probed

[Saravana Kannan]

On Fri, Sep 3, 2021 at 12:06 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Saravana,
>
> On Fri, Sep 3, 2021 at 7:09 PM Saravana Kannan <saravanak@google.com> wrote:
> > On Fri, Sep 3, 2021 at 7:29 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > On Fri, Sep 3, 2021 at 11:19 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > > On Fri, 3 Sept 2021 at 01:04, Saravana Kannan <saravanak@google.com> wrote:
> > > > > fw_devlink could end up creating device links for bus only devices.
> > > > > However, bus only devices don't get probed and can block probe() or
> > > > > sync_state() [1] call backs of other devices. To avoid this, set up
> > > > > these devices to get probed by the simple-pm-bus.
> > > > >
> > > > > [1] - https://lore.kernel.org/lkml/CAPDyKFo9Bxremkb1dDrr4OcXSpE0keVze94Cm=zrkOVxHHxBmQ@mail.gmail.com/
> > > > > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > > > > Tested-by: Saravana Kannan <saravanak@google.com>
> > > >
> > > > Again, this looks like a nice solution to the problem.
> > > >
> > > > One question though. The Kconfig SIMPLE_PM_BUS, should probably be
> > > > "default y" - or something along those lines to make sure fw_devlink
> > > > works as expected.
> > >
> > > I would love for SIMPLE_PM_BUS to go away, and all of its functionality
> > > to be usurped by the standard simple-bus handling.
> >
> > What if SIMPLE_PM_BUS *is* the standard simple-bus handling?
> > Insert "I'm the captain now" meme.
>
> No objections from my side.  In fact that's how I wanted it to be
> from the beginning, but the DT people wanted a separate compatible
> value (and a separate driver).  The only difference is the presence
> of pm_runtime_*() calls.
>
> > I think SIMPLE_PM_BUS config should just go away and we should compile
> > the driver in if CONFIG_OF is selected.
>
> I think there would be no need for a separate driver, if the standard
> simple-bus handling would take care of it.

I'm not sure about the history there. For now, I'm just extending this
driver to take care of simple-bus too.

-Saravana

Re: [PATCH v2 1/2] of: platform: Make sure bus only devices get probed

[Ulf Hansson]

On Fri, 3 Sept 2021 at 16:29, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Ulf,
>
> On Fri, Sep 3, 2021 at 11:19 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > On Fri, 3 Sept 2021 at 01:04, Saravana Kannan <saravanak@google.com> wrote:
> > > fw_devlink could end up creating device links for bus only devices.
> > > However, bus only devices don't get probed and can block probe() or
> > > sync_state() [1] call backs of other devices. To avoid this, set up
> > > these devices to get probed by the simple-pm-bus.
> > >
> > > [1] - https://lore.kernel.org/lkml/CAPDyKFo9Bxremkb1dDrr4OcXSpE0keVze94Cm=zrkOVxHHxBmQ@mail.gmail.com/
> > > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > > Tested-by: Saravana Kannan <saravanak@google.com>
> >
> > Again, this looks like a nice solution to the problem.
> >
> > One question though. The Kconfig SIMPLE_PM_BUS, should probably be
> > "default y" - or something along those lines to make sure fw_devlink
> > works as expected.
>
> I would love for SIMPLE_PM_BUS to go away, and all of its functionality
> to be usurped by the standard simple-bus handling.
>
> In the modern world, everything uses power management and Runtime
> PM, and the distinction between "simple-bus" and "simple-pm-bus"
> is purely artificial.

I think it's not that easy, but maybe I am wrong.

Today we have an opt-in way of supporting runtime PM (and power
management). In most cases it's up to drivers or subsystem level code
to decide if runtime PM should be enabled for the device.

Would it really be okay to enable runtime PM for all of them?

Kind regards
Uffe

Re: [PATCH v2 1/2] of: platform: Make sure bus only devices get probed

[Geert Uytterhoeven]

Hi Ulf,

On Tue, Sep 7, 2021 at 12:36 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On Fri, 3 Sept 2021 at 16:29, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Fri, Sep 3, 2021 at 11:19 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > On Fri, 3 Sept 2021 at 01:04, Saravana Kannan <saravanak@google.com> wrote:
> > > > fw_devlink could end up creating device links for bus only devices.
> > > > However, bus only devices don't get probed and can block probe() or
> > > > sync_state() [1] call backs of other devices. To avoid this, set up
> > > > these devices to get probed by the simple-pm-bus.
> > > >
> > > > [1] - https://lore.kernel.org/lkml/CAPDyKFo9Bxremkb1dDrr4OcXSpE0keVze94Cm=zrkOVxHHxBmQ@mail.gmail.com/
> > > > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > > > Tested-by: Saravana Kannan <saravanak@google.com>
> > >
> > > Again, this looks like a nice solution to the problem.
> > >
> > > One question though. The Kconfig SIMPLE_PM_BUS, should probably be
> > > "default y" - or something along those lines to make sure fw_devlink
> > > works as expected.
> >
> > I would love for SIMPLE_PM_BUS to go away, and all of its functionality
> > to be usurped by the standard simple-bus handling.
> >
> > In the modern world, everything uses power management and Runtime
> > PM, and the distinction between "simple-bus" and "simple-pm-bus"
> > is purely artificial.
>
> I think it's not that easy, but maybe I am wrong.
>
> Today we have an opt-in way of supporting runtime PM (and power
> management). In most cases it's up to drivers or subsystem level code
> to decide if runtime PM should be enabled for the device.
>
> Would it really be okay to enable runtime PM for all of them?

You're talking about the software policy side.

From my PoV, the issue is that this decision is leaked into DT, through
the different compatible values ("simple-pm-bus" vs. "simple-bus").

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

Re: [PATCH v2 1/2] of: platform: Make sure bus only devices get probed

[Ulf Hansson]

On Tue, 7 Sept 2021 at 12:52, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Ulf,
>
> On Tue, Sep 7, 2021 at 12:36 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > On Fri, 3 Sept 2021 at 16:29, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > On Fri, Sep 3, 2021 at 11:19 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > > On Fri, 3 Sept 2021 at 01:04, Saravana Kannan <saravanak@google.com> wrote:
> > > > > fw_devlink could end up creating device links for bus only devices.
> > > > > However, bus only devices don't get probed and can block probe() or
> > > > > sync_state() [1] call backs of other devices. To avoid this, set up
> > > > > these devices to get probed by the simple-pm-bus.
> > > > >
> > > > > [1] - https://lore.kernel.org/lkml/CAPDyKFo9Bxremkb1dDrr4OcXSpE0keVze94Cm=zrkOVxHHxBmQ@mail.gmail.com/
> > > > > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > > > > Tested-by: Saravana Kannan <saravanak@google.com>
> > > >
> > > > Again, this looks like a nice solution to the problem.
> > > >
> > > > One question though. The Kconfig SIMPLE_PM_BUS, should probably be
> > > > "default y" - or something along those lines to make sure fw_devlink
> > > > works as expected.
> > >
> > > I would love for SIMPLE_PM_BUS to go away, and all of its functionality
> > > to be usurped by the standard simple-bus handling.
> > >
> > > In the modern world, everything uses power management and Runtime
> > > PM, and the distinction between "simple-bus" and "simple-pm-bus"
> > > is purely artificial.
> >
> > I think it's not that easy, but maybe I am wrong.
> >
> > Today we have an opt-in way of supporting runtime PM (and power
> > management). In most cases it's up to drivers or subsystem level code
> > to decide if runtime PM should be enabled for the device.
> >
> > Would it really be okay to enable runtime PM for all of them?
>
> You're talking about the software policy side.
>
> From my PoV, the issue is that this decision is leaked into DT, through
> the different compatible values ("simple-pm-bus" vs. "simple-bus").

Yes, I do agree with you there.

On the other hand, it's probably not the only place where it's used as
"software configuration", so I don't have a big issue with it.

Kind regards
Uffe