PM / Domains: Don't leave unused PM domains powered after ->probe()

From that patchset, there is a "scenario 5" described, which actually is a more

There may be more than one device in a PM domain which then will be probed at different points in time.

Depending on timing and runtime PM support, in for the device related driver/subsystem, a PM domain may be advised to power off after a successful probe sequence.

A general requirement for a device within a PM domain, is that the PM domain must stay powered during the probe sequence. To cope with such requirement, let's add the dev_pm_domain_get|put() APIs.

These APIs are intended to be invoked from subsystem-level code and the calls between get/put needs to be balanced.

dev_pm_domain_get(), tells the PM domain that it needs to increase a usage count and to keep supplying power. dev_pm_domain_put(), does the opposite.

For a PM domain to support this feature, it must implement the optional ->get|put() callbacks.

Signed-off-by Ulf Hansson <[email protected]>

---
 drivers/base/power/common.c | 40 ++++++++++++++++++++++++++++++++++++++++
 include/linux/pm.h          |  2 ++
 include/linux/pm_domain.h   |  4 ++++
 3 files changed, 46 insertions(+)

drivers/base/power/common.c +40 -0

index f32b802..99225af 100644
@@ -128,3 +128,43 @@ void dev_pm_domain_detach(struct device *dev, bool power_off)
 		dev->pm_domain->detach(dev, power_off);
 }
 EXPORT_SYMBOL_GPL(dev_pm_domain_detach);
+
+/**
+ * dev_pm_domain_get - Increase usage count to keep a PM domain powered.
+ * @domain: The PM domain to operate on.
+ *
+ * This function will not by itself increase the usage count, that's up to each
+ * PM domain implementation to support. Typically it should be invoked from
+ * subsystem level code prior drivers starts probing.
+ *
+ * Do note, it's optional to implement the ->get() callback for a PM domain.
+ *
+ * Returns 0 on successfully increased usage count or negative error code.
+ */
+int dev_pm_domain_get(struct dev_pm_domain *domain)
+{
+	int ret = 0;
+
+	if (domain && domain->get)
+		ret = domain->get(domain);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(dev_pm_domain_get);
+
+/**
+ * dev_pm_domain_put - Decrease usage count to allow a PM domain to power off.
+ * @domain: The PM domain to operate on.
+ *
+ * This function will not by itself decrease the usage count, that's up to each
+ * PM domain implementation to support. Typically it should be invoked from
+ * subsystem level code after drivers has finished probing.
+ *
+ * Do note, it's optional to implement the ->put() callback for a PM domain.
+ */
+void dev_pm_domain_put(struct dev_pm_domain *domain)
+{
+	if (domain && domain->put)
+		domain->put(domain);
+}
+EXPORT_SYMBOL_GPL(dev_pm_domain_put);

include/linux/pm.h +0 -0

index e2f1be6..e62330b 100644

Quoted 81 lines · Ulf Hansson

There may be more than one device in a PM domain which then will be probed at different points in time.

Depending on timing and runtime PM support, in for the device related driver/subsystem, a PM domain may be advised to power off after a successful probe sequence.

A general requirement for a device within a PM domain, is that the PM domain must stay powered during the probe sequence. To cope with such requirement, let's add the dev_pm_domain_get|put() APIs.

These APIs are intended to be invoked from subsystem-level code and the calls between get/put needs to be balanced.

dev_pm_domain_get(), tells the PM domain that it needs to increase a usage count and to keep supplying power. dev_pm_domain_put(), does the opposite.

For a PM domain to support this feature, it must implement the optional ->get|put() callbacks.

Signed-off-by Ulf Hansson <[email protected]>

---
 drivers/base/power/common.c | 40 ++++++++++++++++++++++++++++++++++++++++
 include/linux/pm.h          |  2 ++
 include/linux/pm_domain.h   |  4 ++++
 3 files changed, 46 insertions(+)

drivers/base/power/common.c +40 -0

index f32b802..99225af 100644
@@ -128,3 +128,43 @@ void dev_pm_domain_detach(struct device *dev, bool power_off)
              dev->pm_domain->detach(dev, power_off);
 }
 EXPORT_SYMBOL_GPL(dev_pm_domain_detach);
+
+/**
+ * dev_pm_domain_get - Increase usage count to keep a PM domain powered.
+ * @domain: The PM domain to operate on.
+ *
+ * This function will not by itself increase the usage count, that's up to each
+ * PM domain implementation to support. Typically it should be invoked from
+ * subsystem level code prior drivers starts probing.
+ *
+ * Do note, it's optional to implement the ->get() callback for a PM domain.
+ *
+ * Returns 0 on successfully increased usage count or negative error code.
+ */
+int dev_pm_domain_get(struct dev_pm_domain *domain)
+{
+     int ret = 0;
+
+     if (domain && domain->get)
+             ret = domain->get(domain);
+
+     return ret;
+}
+EXPORT_SYMBOL_GPL(dev_pm_domain_get);
+
+/**
+ * dev_pm_domain_put - Decrease usage count to allow a PM domain to power off.
+ * @domain: The PM domain to operate on.
+ *
+ * This function will not by itself decrease the usage count, that's up to each
+ * PM domain implementation to support. Typically it should be invoked from
+ * subsystem level code after drivers has finished probing.
+ *
+ * Do note, it's optional to implement the ->put() callback for a PM domain.
+ */
+void dev_pm_domain_put(struct dev_pm_domain *domain)
+{
+     if (domain && domain->put)
+             domain->put(domain);
+}
+EXPORT_SYMBOL_GPL(dev_pm_domain_put);

include/linux/pm.h +0 -0

index e2f1be6..e62330b 100644
@@ -607,6 +607,8 @@ extern void dev_pm_put_subsys_data(struct device *dev);

include/linux/pm.h extern void dev_pm_put_subsys_data(struct device *dev); · line 611

Quoted 5 lines

include/linux/pm.h +2 -0

 struct dev_pm_domain {
      struct dev_pm_ops       ops;
      void (*detach)(struct device *dev, bool power_off);
+     int (*get)(struct dev_pm_domain *domain);
+     void (*put)(struct dev_pm_domain *domain);

I don't like these names. They suggest that it's always going to be about reference counting which doesn't have to be the case in principle.

Also what about calling these from the driver core, ie. really_probe()?

Quoted 82 lines · Rafael J. Wysocki

Quoted 81 lines · Ulf Hansson

There may be more than one device in a PM domain which then will be probed at different points in time.

Depending on timing and runtime PM support, in for the device related driver/subsystem, a PM domain may be advised to power off after a successful probe sequence.

A general requirement for a device within a PM domain, is that the PM domain must stay powered during the probe sequence. To cope with such requirement, let's add the dev_pm_domain_get|put() APIs.

These APIs are intended to be invoked from subsystem-level code and the calls between get/put needs to be balanced.

dev_pm_domain_get(), tells the PM domain that it needs to increase a usage count and to keep supplying power. dev_pm_domain_put(), does the opposite.

For a PM domain to support this feature, it must implement the optional ->get|put() callbacks.

Signed-off-by Ulf Hansson <[email protected]>

---
 drivers/base/power/common.c | 40 ++++++++++++++++++++++++++++++++++++++++
 include/linux/pm.h          |  2 ++
 include/linux/pm_domain.h   |  4 ++++
 3 files changed, 46 insertions(+)

drivers/base/power/common.c +40 -0

index f32b802..99225af 100644
@@ -128,3 +128,43 @@ void dev_pm_domain_detach(struct device *dev, bool power_off)
              dev->pm_domain->detach(dev, power_off);
 }
 EXPORT_SYMBOL_GPL(dev_pm_domain_detach);
+
+/**
+ * dev_pm_domain_get - Increase usage count to keep a PM domain powered.
+ * @domain: The PM domain to operate on.
+ *
+ * This function will not by itself increase the usage count, that's up to each
+ * PM domain implementation to support. Typically it should be invoked from
+ * subsystem level code prior drivers starts probing.
+ *
+ * Do note, it's optional to implement the ->get() callback for a PM domain.
+ *
+ * Returns 0 on successfully increased usage count or negative error code.
+ */
+int dev_pm_domain_get(struct dev_pm_domain *domain)
+{
+     int ret = 0;
+
+     if (domain && domain->get)
+             ret = domain->get(domain);
+
+     return ret;
+}
+EXPORT_SYMBOL_GPL(dev_pm_domain_get);
+
+/**
+ * dev_pm_domain_put - Decrease usage count to allow a PM domain to power off.
+ * @domain: The PM domain to operate on.
+ *
+ * This function will not by itself decrease the usage count, that's up to each
+ * PM domain implementation to support. Typically it should be invoked from
+ * subsystem level code after drivers has finished probing.
+ *
+ * Do note, it's optional to implement the ->put() callback for a PM domain.
+ */
+void dev_pm_domain_put(struct dev_pm_domain *domain)
+{
+     if (domain && domain->put)
+             domain->put(domain);
+}
+EXPORT_SYMBOL_GPL(dev_pm_domain_put);

include/linux/pm.h +0 -0

index e2f1be6..e62330b 100644
@@ -607,6 +607,8 @@ extern void dev_pm_put_subsys_data(struct device *dev);

Quoted 8 lines

include/linux/pm.h extern void dev_pm_put_subsys_data(struct device *dev); · line 611

Quoted 5 lines

include/linux/pm.h +2 -0

 struct dev_pm_domain {
      struct dev_pm_ops       ops;
      void (*detach)(struct device *dev, bool power_off);
+     int (*get)(struct dev_pm_domain *domain);
+     void (*put)(struct dev_pm_domain *domain);

I don't like these names. They suggest that it's always going to be about reference counting which doesn't have to be the case in principle.

I am happy to change, you don't happen to have a proposal? :-)

For genpd we already have these related APIs:

pm_genpd_poweron()
pm_genpd_name_poweron()
pm_genpd_poweroff_unused()

Theoretically we should be able to replace these with dev_pm_domain_get|put() or whatever we decide to name them to.

Quoted 1 line

Quoted 1 line

Also what about calling these from the driver core, ie. really_probe()?

I like that!

That also implies moving the calls to dev_pm_domain_attach|detach() out of the buses and into the drivercore, since we need the PM domain to be attached before calling dev_pm_domain_get|put(). I assume that's what you also propose me to change, right?

Quoted 104 lines · Ulf Hansson

Quoted 82 lines · Rafael J. Wysocki

Quoted 81 lines · Ulf Hansson

There may be more than one device in a PM domain which then will be probed at different points in time.

Depending on timing and runtime PM support, in for the device related driver/subsystem, a PM domain may be advised to power off after a successful probe sequence.

A general requirement for a device within a PM domain, is that the PM domain must stay powered during the probe sequence. To cope with such requirement, let's add the dev_pm_domain_get|put() APIs.

These APIs are intended to be invoked from subsystem-level code and the calls between get/put needs to be balanced.

dev_pm_domain_get(), tells the PM domain that it needs to increase a usage count and to keep supplying power. dev_pm_domain_put(), does the opposite.

For a PM domain to support this feature, it must implement the optional ->get|put() callbacks.

Signed-off-by Ulf Hansson <[email protected]>

---
 drivers/base/power/common.c | 40 ++++++++++++++++++++++++++++++++++++++++
 include/linux/pm.h          |  2 ++
 include/linux/pm_domain.h   |  4 ++++
 3 files changed, 46 insertions(+)

drivers/base/power/common.c +40 -0

index f32b802..99225af 100644
@@ -128,3 +128,43 @@ void dev_pm_domain_detach(struct device *dev, bool power_off)
              dev->pm_domain->detach(dev, power_off);
 }
 EXPORT_SYMBOL_GPL(dev_pm_domain_detach);
+
+/**
+ * dev_pm_domain_get - Increase usage count to keep a PM domain powered.
+ * @domain: The PM domain to operate on.
+ *
+ * This function will not by itself increase the usage count, that's up to each
+ * PM domain implementation to support. Typically it should be invoked from
+ * subsystem level code prior drivers starts probing.
+ *
+ * Do note, it's optional to implement the ->get() callback for a PM domain.
+ *
+ * Returns 0 on successfully increased usage count or negative error code.
+ */
+int dev_pm_domain_get(struct dev_pm_domain *domain)
+{
+     int ret = 0;
+
+     if (domain && domain->get)
+             ret = domain->get(domain);
+
+     return ret;
+}
+EXPORT_SYMBOL_GPL(dev_pm_domain_get);
+
+/**
+ * dev_pm_domain_put - Decrease usage count to allow a PM domain to power off.
+ * @domain: The PM domain to operate on.
+ *
+ * This function will not by itself decrease the usage count, that's up to each
+ * PM domain implementation to support. Typically it should be invoked from
+ * subsystem level code after drivers has finished probing.
+ *
+ * Do note, it's optional to implement the ->put() callback for a PM domain.
+ */
+void dev_pm_domain_put(struct dev_pm_domain *domain)
+{
+     if (domain && domain->put)
+             domain->put(domain);
+}
+EXPORT_SYMBOL_GPL(dev_pm_domain_put);

include/linux/pm.h +0 -0

index e2f1be6..e62330b 100644
@@ -607,6 +607,8 @@ extern void dev_pm_put_subsys_data(struct device *dev);

Quoted 8 lines

include/linux/pm.h extern void dev_pm_put_subsys_data(struc

Quoted 5 lines

include/linux/pm.h +2 -0

 struct dev_pm_domain {
      struct dev_pm_ops       ops;
      void (*detach)(struct device *dev, bool power_off);
+     int (*get)(struct dev_pm_domain *domain);
+     void (*put)(struct dev_pm_domain *domain);

I don't like these names. They suggest that it's always going to be about reference counting which doesn't have to be the case in principle.

I am happy to change, you don't happen to have a proposal? :-)

For genpd we already have these related APIs:

pm_genpd_poweron()
pm_genpd_name_poweron()
pm_genpd_poweroff_unused()

Theoretically we should be able to replace these with dev_pm_domain_get|put() or whatever we decide to name them to.

Quoted 1 line

Quoted 8 lines

Quoted 1 line

Also what about calling these from the driver core, ie. really_probe()?

I like that!

That also implies moving the calls to dev_pm_domain_attach|detach() out of the buses and into the drivercore, since we need the PM domain to be attached before calling dev_pm_domain_get|put(). I assume that's what you also propose me to change, right?

Not really. I don't want to inflict the ACPI PM domain on every bus type that may not be prepared for using it or even may not want to use it (like PCI or PNP). That applies to genpd too to some extent.

So bus types need to be able to opt in for using "default" PM domains, but at the same time if they do, the core is a better place to invoke the callbacks.

The patch below shows how that can be done. For the bus types using genpd or the ACPI PM domain today ->pm_domain_init and ->pm_domain_exit may point to the routines initializing/detaching those (which also will help to reduce code duplication somewhat) and that guarantees that the domains will be attached to devices before probing and the core can do the ->pre_probe and ->post_probe things for everybody.

Rafael

---
 drivers/base/bus.c     |   12 +++++++++++-
 drivers/base/dd.c      |    9 +++++++++
 include/linux/device.h |    3 +++
 include/linux/pm.h     |    2 ++
 4 files changed, 25 insertions(+), 1 deletion(-)

Index: linux-pm/drivers/base/bus.c
===================================================================

drivers/base/bus.c +6 -1

@@ -509,10 +509,15 @@ int bus_add_device(struct device *dev)
        int error = 0;
 
        if (bus) {
+               if (bus->pm_domain_init) {
+                       error = bus->pm_domain_init(dev);
+                       if (error)
+                               goto out_put;
+               }
                pr_debug("bus: '%s': add device %s\n", bus->name, dev_name(dev));
                error = device_add_attrs(bus, dev);
                if (error)
-                       goto out_put;
+                       goto out_pm_domain;
                error = device_add_groups(dev, bus->dev_groups);
                if (error)
                        goto out_groups;

drivers/base/bus.c +2 -0

@@ -597,6 +605,8 @@ void bus_remove_device(struct device *de
        device_remove_groups(dev, dev->bus->dev_groups);
        if (klist_node_attached(&dev->p->knode_bus))
                klist_del(&dev->p->knode_bus);
+       if (bus->pm_domain_exit)
+               bus->pm_domain_exit(dev);
 
        pr_debug("bus: '%s': remove device %s\n",
                 dev->bus->name, dev_name(dev));

Index: linux-pm/include/linux/device.h
===================================================================

include/linux/device.h +3 -0

@@ -123,6 +123,9 @@ struct bus_type {
        int (*suspend)(struct device *dev, pm_message_t state);
        int (*resume)(struct device *dev);
 
+       int (*pm_domain_init)(struct device *dev);
+       void (*pm_domain_exit)(struct device *dev);
+
        const struct dev_pm_ops *pm;
 
        const struct iommu_ops *iommu_ops;

Index: linux-pm/include/linux/pm.h
===================================================================

include/linux/pm.h +2 -0

@@ -607,6 +607,8 @@ extern void dev_pm_put_subsys_data(struc
 struct dev_pm_domain {
        struct dev_pm_ops       ops;
        void (*detach)(struct device *dev, bool power_off);
+       int (*pre_probe)(struct device *dev);
+       void (*post_probe)(struct device *dev);
 };
 
 /*

Index: linux-pm/drivers/base/dd.c
===================================================================

drivers/base/dd.c +6 -0

@@ -298,6 +298,12 @@ static int really_probe(struct device *d
                goto probe_failed;
        }
 
+       if (dev->pm_domain && dev->pm_domain->pre_probe) {
+               ret = dev->pm_domain->pre_probe(dev);
+               if (ret)
+                       goto probe_failed;
+       }
+
        if (dev->bus->probe) {
                ret = dev->bus->probe(dev);
                if (ret)

Quoted 114 lines · Rafael J. Wysocki

Quoted 104 lines · Ulf Hansson

Quoted 82 lines · Rafael J. Wysocki

Quoted 81 lines · Ulf Hansson

There may be more than one device in a PM domain which then will be probed at different points in time.

Depending on timing and runtime PM support, in for the device related driver/subsystem, a PM domain may be advised to power off after a successful probe sequence.

A general requirement for a device within a PM domain, is that the PM domain must stay powered during the probe sequence. To cope with such requirement, let's add the dev_pm_domain_get|put() APIs.

These APIs are intended to be invoked from subsystem-level code and the calls between get/put needs to be balanced.

dev_pm_domain_get(), tells the PM domain that it needs to increase a usage count and to keep supplying power. dev_pm_domain_put(), does the opposite.

For a PM domain to support this feature, it must implement the optional ->get|put() callbacks.

Signed-off-by Ulf Hansson <[email protected]>

---
 drivers/base/power/common.c | 40 ++++++++++++++++++++++++++++++++++++++++
 include/linux/pm.h          |  2 ++
 include/linux/pm_domain.h   |  4 ++++
 3 files changed, 46 insertions(+)

drivers/base/power/common.c +40 -0

index f32b802..99225af 100644
@@ -128,3 +128,43 @@ void dev_pm_domain_detach(struct device *dev, bool power_off)
              dev->pm_domain->detach(dev, power_off);
 }
 EXPORT_SYMBOL_GPL(dev_pm_domain_detach);
+
+/**
+ * dev_pm_domain_get - Increase usage count to keep a PM domain powered.
+ * @domain: The PM domain to operate on.
+ *
+ * This function will not by itself increase the usage count, that's up to each
+ * PM domain implementation to support. Typically it should be invoked from
+ * subsystem level code prior drivers starts probing.
+ *
+ * Do note, it's optional to implement the ->get() callback for a PM domain.
+ *
+ * Returns 0 on successfully increased usage count or negative error code.
+ */
+int dev_pm_domain_get(struct dev_pm_domain *domain)
+{
+     int ret = 0;
+
+     if (domain && domain->get)
+             ret = domain->get(domain);
+
+     return ret;
+}
+EXPORT_SYMBOL_GPL(dev_pm_domain_get);
+
+/**
+ * dev_pm_domain_put - Decrease usage count to allow a PM domain to power off.
+ * @domain: The PM domain to operate on.
+ *
+ * This function will not by itself decrease the usage count, that's up to each
+ * PM domain implementation to support. Typically it should be invoked from
+ * subsystem level code after drivers has finished probing.
+ *
+ * Do note, it's optional to implement the ->put() callback for a PM domain.
+ */
+void dev_pm_domain_put(struct dev_pm_domain *domain)
+{
+     if (domain && domain->put)
+             domain->put(domain);
+}
+EXPORT_SYMBOL_GPL(dev_pm_domain_put);

include/linux/pm.h +0 -0

index e2f1be6..e62330b 100644
@@ -607,6 +607,8 @@ extern void dev_pm_put_subsys_data(struct device *dev);

Quoted 8 lines

include/linux/pm.h extern void dev_pm_put_subsys_data(struc

Quoted 5 lines

include/linux/pm.h +2 -0

 struct dev_pm_domain {
      struct dev_pm_ops       ops;
      void (*detach)(struct device *dev, bool power_off);
+     int (*get)(struct dev_pm_domain *domain);
+     void (*put)(struct dev_pm_domain *domain);

I don't like these names. They suggest that it's always going to be about reference counting which doesn't have to be the case in principle.

I am happy to change, you don't happen to have a proposal? :-)

For genpd we already have these related APIs:

pm_genpd_poweron()
pm_genpd_name_poweron()
pm_genpd_poweroff_unused()

Theoretically we should be able to replace these with dev_pm_domain_get|put() or whatever we decide to name them to.

Quoted 1 line

Quoted 8 lines

Quoted 1 line

Also what about calling these from the driver core, ie. really_probe()?

I like that!

That also implies moving the calls to dev_pm_domain_attach|detach() out of the buses and into the drivercore, since we need the PM domain to be attached before calling dev_pm_domain_get|put(). I assume that's what you also propose me to change, right?

Quoted 7 lines

Not really. I don't want to inflict the ACPI PM domain on every bus type that may not be prepared for using it or even may not want to use it (like PCI or PNP). That applies to genpd too to some extent.

So bus types need to be able to opt in for using "default" PM domains, but at the same time if they do, the core is a better place to invoke the callbacks.

Okay!

Quoted 7 lines

The patch below shows how that can be done. For the bus types using genpd or the ACPI PM domain today ->pm_domain_init and ->pm_domain_exit may point to the routines initializing/detaching those (which also will help to reduce code duplication somewhat) and that guarantees that the domains will be attached to devices before probing and the core can do the ->pre_probe and ->post_probe things for everybody.

Overall, this seem like a very good idea!

Quoted 33 lines

Rafael

---
 drivers/base/bus.c     |   12 +++++++++++-
 drivers/base/dd.c      |    9 +++++++++
 include/linux/device.h |    3 +++
 include/linux/pm.h     |    2 ++
 4 files changed, 25 insertions(+), 1 deletion(-)

Index: linux-pm/drivers/base/bus.c
===================================================================

drivers/base/bus.c +6 -1

@@ -509,10 +509,15 @@ int bus_add_device(struct device *dev)
        int error = 0;
 
        if (bus) {
+               if (bus->pm_domain_init) {
+                       error = bus->pm_domain_init(dev);
+                       if (error)
+                               goto out_put;
+               }
                pr_debug("bus: '%s': add device %s\n", bus->name, dev_name(dev));
                error = device_add_attrs(bus, dev);
                if (error)
-                       goto out_put;
+                       goto out_pm_domain;
                error = device_add_groups(dev, bus->dev_groups);
                if (error)
                        goto out_groups;

drivers/base/bus.c out_groups: · line 547

Quoted 9 lines

drivers/base/bus.c +3 -0

@@ -534,6 +539,9 @@ out_groups:
        device_remove_groups(dev, bus->dev_groups);
 out_id:
        device_remove_attrs(bus, dev);
+out_pm_domain:
+       if (bus->pm_domain_exit)
+               bus->pm_domain_exit(dev);
 out_put:
        bus_put(dev->bus);
        return error;

To make this work for bus_add_device(), we first need to change the registration procedure for genpd DT providers. Currently that's done when "PM domain drivers" invokes the __of_genpd_add_provider() API, from SoC specific code and from different init levels.

We need to have the available gendp DT providers registered when the ->pm_domain_init() callback is invoked.

Besides the changes above, genpd also needs to deal with attached devices, but which don't have a corresponding driver bound. I believe we have some corner cases to fix around this as well.

As an intermediate step, how about adding the similar code as above but into really_probe()? For the code in bus_remove_device() below, we could add that into __device_release_driver()?

Well, I wouldn't really like to add new callbacks to struct bus_type for intermediate steps, because that's guaranteed to lead to confusion.

So I think the infrastructure is better added first and users may be switched over to it gradually.

I don't see any particular problems with moving the ACPI PM domain attach/detach to bus_add/remove_device(), so that can be done as the first step. As for genpd, it can implement the ->post_probe thing first and do the rest in the bus type ->probe until the generic code is ready.

Quoted 6 lines · Rafael J. Wysocki

Well, I wouldn't really like to add new callbacks to struct bus_type for intermediate steps, because that's guaranteed to lead to confusion.

So I think the infrastructure is better added first and users may be switched over to it gradually.

Quoted 4 lines

I don't see any particular problems with moving the ACPI PM domain attach/detach to bus_add/remove_device(), so that can be done as the first step. As for genpd, it can implement the ->post_probe thing first and do the rest in the bus type ->probe until the generic code is ready.

At what point do you see the genpd binding taking place - remembering that this can fail with -EPROBE_DEFER when DT specifies a genpd, but the domain hasn't been registered yet?

I'm guessing that needs to happen either in the bus type ->probe or the ->pre_probe callback. ->pre_probe sounds better as it can be a standard genpd function which bus types hook directly into that method.

Quoted 186 lines · Ulf Hansson

Quoted 114 lines · Rafael J. Wysocki

Quoted 104 lines · Ulf Hansson

Quoted 82 lines · Rafael J. Wysocki

Quoted 81 lines · Ulf Hansson

There may be more than one device in a PM domain which then will be probed at different points in time.

Depending on timing and runtime PM support, in for the device related driver/subsystem, a PM domain may be advised to power off after a successful probe sequence.

A general requirement for a device within a PM domain, is that the PM domain must stay powered during the probe sequence. To cope with such requirement, let's add the dev_pm_domain_get|put() APIs.

These APIs are intended to be invoked from subsystem-level code and the calls between get/put needs to be balanced.

dev_pm_domain_get(), tells the PM domain that it needs to increase a usage count and to keep supplying power. dev_pm_domain_put(), does the opposite.

For a PM domain to support this feature, it must implement the optional ->get|put() callbacks.

Signed-off-by Ulf Hansson <[email protected]>

---
 drivers/base/power/common.c | 40 ++++++++++++++++++++++++++++++++++++++++
 include/linux/pm.h          |  2 ++
 include/linux/pm_domain.h   |  4 ++++
 3 files changed, 46 insertions(+)

drivers/base/power/common.c +40 -0

index f32b802..99225af 100644
@@ -128,3 +128,43 @@ void dev_pm_domain_detach(struct device *dev, bool power_off)
              dev->pm_domain->detach(dev, power_off);
 }
 EXPORT_SYMBOL_GPL(dev_pm_domain_detach);
+
+/**
+ * dev_pm_domain_get - Increase usage count to keep a PM domain powered.
+ * @domain: The PM domain to operate on.
+ *
+ * This function will not by itself increase the usage count, that's up to each
+ * PM domain implementation to support. Typically it should be invoked from
+ * subsystem level code prior drivers starts probing.
+ *
+ * Do note, it's optional to implement the ->get() callback for a PM domain.
+ *
+ * Returns 0 on successfully increased usage count or negative error code.
+ */
+int dev_pm_domain_get(struct dev_pm_domain *domain)
+{
+     int ret = 0;
+
+     if (domain && domain->get)
+             ret = domain->get(domain);
+
+     return ret;
+}
+EXPORT_SYMBOL_GPL(dev_pm_domain_get);
+
+/**
+ * dev_pm_domain_put - Decrease usage count to allow a PM domain to power off.
+ * @domain: The PM domain to operate on.
+ *
+ * This function will not by itself decrease the usage count, that's up to each
+ * PM domain implementation to support. Typically it should be invoked from
+ * subsystem level code after drivers has finished probing.
+ *
+ * Do note, it's optional to implement the ->put() callback for a PM domain.
+ */
+void dev_pm_domain_put(struct dev_pm_domain *domain)
+{
+     if (domain && domain->put)
+             domain->put(domain);
+}
+EXPORT_SYMBOL_GPL(dev_pm_domain_put);

include/linux/pm.h +0 -0

index e2f1be6..e62330b 100644
@@ -607,6 +607,8 @@ extern void dev_pm_put_subsys_data(struct device *dev);

Quoted 8 lines

include/linux/pm.h extern void dev_pm_put_subsys_data(struc

Quoted 5 lines

include/linux/pm.h +2 -0

 struct dev_pm_domain {
      struct dev_pm_ops       ops;
      void (*detach)(struct device *dev, bool power_off);
+     int (*get)(struct dev_pm_domain *domain);
+     void (*put)(struct dev_pm_domain *domain);

I don't like these names. They suggest that it's always going to be about reference counting which doesn't have to be the case in principle.

I am happy to change, you don't happen to have a proposal? :-)

For genpd we already have these related APIs:

pm_genpd_poweron()
pm_genpd_name_poweron()
pm_genpd_poweroff_unused()

Theoretically we should be able to replace these with dev_pm_domain_get|put() or whatever we decide to name them to.

Quoted 1 line

Quoted 8 lines

Quoted 1 line

Also what about calling these from the driver core, ie. really_probe()?

I like that!

That also implies moving the calls to dev_pm_domain_attach|detach() out of the buses and into the drivercore, since we need the PM domain to be attached before calling dev_pm_domain_get|put(). I assume that's what you also propose me to change, right?

Quoted 7 lines

Not really. I don't want to inflict the ACPI PM domain on every bus type that may not be prepared for using it or even may not want to use it (like PCI or PNP). That applies to genpd too to some extent.

So bus types need to be able to opt in for using "default" PM domains, but at the same time if they do, the core is a better place to invoke the callbacks.

Okay!

Quoted 7 lines

The patch below shows how that can be done. For the bus types using genpd or the ACPI PM domain today ->pm_domain_init and ->pm_domain_exit may point to the routines initializing/detaching those (which also will help to reduce code duplication somewhat) and that guarantees that the domains will be attached to devices before probing and the core can do the ->pre_probe and ->post_probe things for everybody.

Overall, this seem like a very good idea!

Quoted 33 lines

Rafael

---
 drivers/base/bus.c     |   12 +++++++++++-
 drivers/base/dd.c      |    9 +++++++++
 include/linux/device.h |    3 +++
 include/linux/pm.h     |    2 ++
 4 files changed, 25 insertions(+), 1 deletion(-)

Index: linux-pm/drivers/base/bus.c
===================================================================

drivers/base/bus.c +6 -1

@@ -509,10 +509,15 @@ int bus_add_device(struct device *dev)
        int error = 0;
 
        if (bus) {
+               if (bus->pm_domain_init) {
+                       error = bus->pm_domain_init(dev);
+                       if (error)
+                               goto out_put;
+               }
                pr_debug("bus: '%s': add device %s\n", bus->name, dev_name(dev));
                error = device_add_attrs(bus, dev);
                if (error)
-                       goto out_put;
+                       goto out_pm_domain;
                error = device_add_groups(dev, bus->dev_groups);
                if (error)
                        goto out_groups;

drivers/base/bus.c out_groups: · line 547

Quoted 9 lines

drivers/base/bus.c +3 -0

@@ -534,6 +539,9 @@ out_groups:
        device_remove_groups(dev, bus->dev_groups);
 out_id:
        device_remove_attrs(bus, dev);
+out_pm_domain:
+       if (bus->pm_domain_exit)
+               bus->pm_domain_exit(dev);
 out_put:
        bus_put(dev->bus);
        return error;

To make this work for bus_add_device(), we first need to change the registration procedure for genpd DT providers. Currently that's done when "PM domain drivers" invokes the __of_genpd_add_provider() API, from SoC specific code and from different init levels.

We need to have the available gendp DT providers registered when the ->pm_domain_init() callback is invoked.

Quoted 7 lines

Besides the changes above, genpd also needs to deal with attached devices, but which don't have a corresponding driver bound. I believe we have some corner cases to fix around this as well.

As an intermediate step, how about adding the similar code as above but into really_probe()? For the code in bus_remove_device() below, we could add that into __device_release_driver()?

Well, I wouldn't really like to add new callbacks to struct bus_type for intermediate steps, because that's guaranteed to lead to confusion.

So I think the infrastructure is better added first and users may be switched over to it gradually.

I don't see any particular problems with moving the ACPI PM domain attach/detach to bus_add/remove_device(), so that can be done as the first step. As for genpd, it can implement the ->post_probe thing first and do the rest in the bus type ->probe until the generic code is ready.

Quoted 201 lines · Rafael J. Wysocki

Quoted 186 lines · Ulf Hansson

Quoted 114 lines · Rafael J. Wysocki

Quoted 104 lines · Ulf Hansson

Quoted 82 lines · Rafael J. Wysocki

Quoted 81 lines · Ulf Hansson

There may be more than one device in a PM domain which then will be probed at different points in time.

Depending on timing and runtime PM support, in for the device related driver/subsystem, a PM domain may be advised to power off after a successful probe sequence.

A general requirement for a device within a PM domain, is that the PM domain must stay powered during the probe sequence. To cope with such requirement, let's add the dev_pm_domain_get|put() APIs.

These APIs are intended to be invoked from subsystem-level code and the calls between get/put needs to be balanced.

dev_pm_domain_get(), tells the PM domain that it needs to increase a usage count and to keep supplying power. dev_pm_domain_put(), does the opposite.

For a PM domain to support this feature, it must implement the optional ->get|put() callbacks.

Signed-off-by Ulf Hansson <[email protected]>

---
 drivers/base/power/common.c | 40 ++++++++++++++++++++++++++++++++++++++++
 include/linux/pm.h          |  2 ++
 include/linux/pm_domain.h   |  4 ++++
 3 files changed, 46 insertions(+)

drivers/base/power/common.c +40 -0

index f32b802..99225af 100644
@@ -128,3 +128,43 @@ void dev_pm_domain_detach(struct device *dev, bool power_off)
              dev->pm_domain->detach(dev, power_off);
 }
 EXPORT_SYMBOL_GPL(dev_pm_domain_detach);
+
+/**
+ * dev_pm_domain_get - Increase usage count to keep a PM domain powered.
+ * @domain: The PM domain to operate on.
+ *
+ * This function will not by itself increase the usage count, that's up to each
+ * PM domain implementation to support. Typically it should be invoked from
+ * subsystem level code prior drivers starts probing.
+ *
+ * Do note, it's optional to implement the ->get() callback for a PM domain.
+ *
+ * Returns 0 on successfully increased usage count or negative error code.
+ */
+int dev_pm_domain_get(struct dev_pm_domain *domain)
+{
+     int ret = 0;
+
+     if (domain && domain->get)
+             ret = domain->get(domain);
+
+     return ret;
+}
+EXPORT_SYMBOL_GPL(dev_pm_domain_get);
+
+/**
+ * dev_pm_domain_put - Decrease usage count to allow a PM domain to power off.
+ * @domain: The PM domain to operate on.
+ *
+ * This function will not by itself decrease the usage count, that's up to each
+ * PM domain implementation to support. Typically it should be invoked from
+ * subsystem level code after drivers has finished probing.
+ *
+ * Do note, it's optional to implement the ->put() callback for a PM domain.
+ */
+void dev_pm_domain_put(struct dev_pm_domain *domain)
+{
+     if (domain && domain->put)
+             domain->put(domain);
+}
+EXPORT_SYMBOL_GPL(dev_pm_domain_put);

include/linux/pm.h +0 -0

index e2f1be6..e62330b 100644
@@ -607,6 +607,8 @@ extern void dev_pm_put_subsys_data(struct device *dev);

Quoted 8 lines

include/linux/pm.h extern void dev_pm_put_subsys_data(struc

Quoted 5 lines

include/linux/pm.h +2 -0

 struct dev_pm_domain {
      struct dev_pm_ops       ops;
      void (*detach)(struct device *dev, bool power_off);
+     int (*get)(struct dev_pm_domain *domain);
+     void (*put)(struct dev_pm_domain *domain);

I don't like these names. They suggest that it's always going to be about reference counting which doesn't have to be the case in principle.

I am happy to change, you don't happen to have a proposal? :-)

For genpd we already have these related APIs:

pm_genpd_poweron()
pm_genpd_name_poweron()
pm_genpd_poweroff_unused()

Theoretically we should be able to replace these with dev_pm_domain_get|put() or whatever we decide to name them to.

Quoted 1 line

Quoted 8 lines

Quoted 1 line

Also what about calling these from the driver core, ie. really_probe()?

I like that!

That also implies moving the calls to dev_pm_domain_attach|detach() out of the buses and into the drivercore, since we need the PM domain to be attached before calling dev_pm_domain_get|put(). I assume that's what you also propose me to change, right?

Quoted 7 lines

Not really. I don't want to inflict the ACPI PM domain on every bus type that may not be prepared for using it or even may not want to use it (like PCI or PNP). That applies to genpd too to some extent.

So bus types need to be able to opt in for using "default" PM domains, but at the same time if they do, the core is a better place to invoke the callbacks.

Okay!

Quoted 7 lines

The patch below shows how that can be done. For the bus types using genpd or the ACPI PM domain today ->pm_domain_init and ->pm_domain_exit may point to the routines initializing/detaching those (which also will help to reduce code duplication somewhat) and that guarantees that the domains will be attached to devices before probing and the core can do the ->pre_probe and ->post_probe things for everybody.

Overall, this seem like a very good idea!

Quoted 33 lines

Rafael

---
 drivers/base/bus.c     |   12 +++++++++++-
 drivers/base/dd.c      |    9 +++++++++
 include/linux/device.h |    3 +++
 include/linux/pm.h     |    2 ++
 4 files changed, 25 insertions(+), 1 deletion(-)

Index: linux-pm/drivers/base/bus.c
===================================================================

drivers/base/bus.c +6 -1

@@ -509,10 +509,15 @@ int bus_add_device(struct device *dev)
        int error = 0;
 
        if (bus) {
+               if (bus->pm_domain_init) {
+                       error = bus->pm_domain_init(dev);
+                       if (error)
+                               goto out_put;
+               }
                pr_debug("bus: '%s': add device %s\n", bus->name, dev_name(dev));
                error = device_add_attrs(bus, dev);
                if (error)
-                       goto out_put;
+                       goto out_pm_domain;
                error = device_add_groups(dev, bus->dev_groups);
                if (error)
                        goto out_groups;

drivers/base/bus.c out_groups: · line 547

Quoted 9 lines

drivers/base/bus.c +3 -0

@@ -534,6 +539,9 @@ out_groups:
        device_remove_groups(dev, bus->dev_groups);
 out_id:
        device_remove_attrs(bus, dev);
+out_pm_domain:
+       if (bus->pm_domain_exit)
+               bus->pm_domain_exit(dev);
 out_put:
        bus_put(dev->bus);
        return error;

To make this work for bus_add_device(), we first need to change the registration procedure for genpd DT providers. Currently that's done when "PM domain drivers" invokes the __of_genpd_add_provider() API, from SoC specific code and from different init levels.

We need to have the available gendp DT providers registered when the ->pm_domain_init() callback is invoked.

Quoted 7 lines

Besides the changes above, genpd also needs to deal with attached devices, but which don't have a corresponding driver bound. I believe we have some corner cases to fix around this as well.

As an intermediate step, how about adding the similar code as above but into really_probe()? For the code in bus_remove_device() below, we could add that into __device_release_driver()?

Well, I wouldn't really like to add new callbacks to struct bus_type for intermediate steps, because that's guaranteed to lead to confusion.

So I think the infrastructure is better added first and users may be switched over to it gradually.

Quoted 4 lines

I don't see any particular problems with moving the ACPI PM domain attach/detach to bus_add/remove_device(), so that can be done as the first step. As for genpd, it can implement the ->post_probe thing first and do the rest in the bus type ->probe until the generic code is ready.

Yes, that works.

I will cook a new version of the patchset according to your suggestions.

Quoted 207 lines · Ulf Hansson

Quoted 205 lines · Rafael J. Wysocki

Quoted 186 lines · Ulf Hansson

Quoted 114 lines · Rafael J. Wysocki

Quoted 104 lines · Ulf Hansson

Quoted 82 lines · Rafael J. Wysocki

Quoted 81 lines · Ulf Hansson

There may be more than one device in a PM domain which then will be probed at different points in time.

Depending on timing and runtime PM support, in for the device related driver/subsystem, a PM domain may be advised to power off after a successful probe sequence.

A general requirement for a device within a PM domain, is that the PM domain must stay powered during the probe sequence. To cope with such requirement, let's add the dev_pm_domain_get|put() APIs.

These APIs are intended to be invoked from subsystem-level code and the calls between get/put needs to be balanced.

dev_pm_domain_get(), tells the PM domain that it needs to increase a usage count and to keep supplying power. dev_pm_domain_put(), does the opposite.

For a PM domain to support this feature, it must implement the optional ->get|put() callbacks.

Signed-off-by Ulf Hansson <[email protected]>

---
 drivers/base/power/common.c | 40 ++++++++++++++++++++++++++++++++++++++++
 include/linux/pm.h          |  2 ++
 include/linux/pm_domain.h   |  4 ++++
 3 files changed, 46 insertions(+)

drivers/base/power/common.c +40 -0

index f32b802..99225af 100644
@@ -128,3 +128,43 @@ void dev_pm_domain_detach(struct device *dev, bool power_off)
              dev->pm_domain->detach(dev, power_off);
 }
 EXPORT_SYMBOL_GPL(dev_pm_domain_detach);
+
+/**
+ * dev_pm_domain_get - Increase usage count to keep a PM domain powered.
+ * @domain: The PM domain to operate on.
+ *
+ * This function will not by itself increase the usage count, that's up to each
+ * PM domain implementation to support. Typically it should be invoked from
+ * subsystem level code prior drivers starts probing.
+ *
+ * Do note, it's optional to implement the ->get() callback for a PM domain.
+ *
+ * Returns 0 on successfully increased usage count or negative error code.
+ */
+int dev_pm_domain_get(struct dev_pm_domain *domain)
+{
+     int ret = 0;
+
+     if (domain && domain->get)
+             ret = domain->get(domain);
+
+     return ret;
+}
+EXPORT_SYMBOL_GPL(dev_pm_domain_get);
+
+/**
+ * dev_pm_domain_put - Decrease usage count to allow a PM domain to power off.
+ * @domain: The PM domain to operate on.
+ *
+ * This function will not by itself decrease the usage count, that's up to each
+ * PM domain implementation to support. Typically it should be invoked from
+ * subsystem level code after drivers has finished probing.
+ *
+ * Do note, it's optional to implement the ->put() callback for a PM domain.
+ */
+void dev_pm_domain_put(struct dev_pm_domain *domain)
+{
+     if (domain && domain->put)
+             domain->put(domain);
+}
+EXPORT_SYMBOL_GPL(dev_pm_domain_put);

include/linux/pm.h +0 -0

index e2f1be6..e62330b 100644
@@ -607,6 +607,8 @@ extern void dev_pm_put_subsys_data(struct device *dev);

Quoted 8 lines

include/linux/pm.h extern void dev_pm_put_subsys_data(struc

Quoted 5 lines

include/linux/pm.h +2 -0

 struct dev_pm_domain {
      struct dev_pm_ops       ops;
      void (*detach)(struct device *dev, bool power_off);
+     int (*get)(struct dev_pm_domain *domain);
+     void (*put)(struct dev_pm_domain *domain);

I don't like these names. They suggest that it's always going to be about reference counting which doesn't have to be the case in principle.

I am happy to change, you don't happen to have a proposal? :-)

For genpd we already have these related APIs:

pm_genpd_poweron()
pm_genpd_name_poweron()
pm_genpd_poweroff_unused()

Theoretically we should be able to replace these with dev_pm_domain_get|put() or whatever we decide to name them to.

Quoted 1 line

Quoted 8 lines

Quoted 1 line

Also what about calling these from the driver core, ie. really_probe()?

I like that!

That also implies moving the calls to dev_pm_domain_attach|detach() out of the buses and into the drivercore, since we need the PM domain to be attached before calling dev_pm_domain_get|put(). I assume that's what you also propose me to change, right?

Quoted 7 lines

Not really. I don't want to inflict the ACPI PM domain on every bus type that may not be prepared for using it or even may not want to use it (like PCI or PNP). That applies to genpd too to some extent.

So bus types need to be able to opt in for using "default" PM domains, but at the same time if they do, the core is a better place to invoke the callbacks.

Okay!

Quoted 7 lines

The patch below shows how that can be done. For the bus types using genpd or the ACPI PM domain today ->pm_domain_init and ->pm_domain_exit may point to the routines initializing/detaching those (which also will help to reduce code duplication somewhat) and that guarantees that the domains will be attached to devices before probing and the core can do the ->pre_probe and ->post_probe things for everybody.

Overall, this seem like a very good idea!

Quoted 33 lines

Rafael

---
 drivers/base/bus.c     |   12 +++++++++++-
 drivers/base/dd.c      |    9 +++++++++
 include/linux/device.h |    3 +++
 include/linux/pm.h     |    2 ++
 4 files changed, 25 insertions(+), 1 deletion(-)

Index: linux-pm/drivers/base/bus.c
===================================================================

drivers/base/bus.c +6 -1

@@ -509,10 +509,15 @@ int bus_add_device(struct device *dev)
        int error = 0;
 
        if (bus) {
+               if (bus->pm_domain_init) {
+                       error = bus->pm_domain_init(dev);
+                       if (error)
+                               goto out_put;
+               }
                pr_debug("bus: '%s': add device %s\n", bus->name, dev_name(dev));
                error = device_add_attrs(bus, dev);
                if (error)
-                       goto out_put;
+                       goto out_pm_domain;
                error = device_add_groups(dev, bus->dev_groups);
                if (error)
                        goto out_groups;

drivers/base/bus.c out_groups: · line 547

Quoted 9 lines

drivers/base/bus.c +3 -0

@@ -534,6 +539,9 @@ out_groups:
        device_remove_groups(dev, bus->dev_groups);
 out_id:
        device_remove_attrs(bus, dev);
+out_pm_domain:
+       if (bus->pm_domain_exit)
+               bus->pm_domain_exit(dev);
 out_put:
        bus_put(dev->bus);
        return error;

To make this work for bus_add_device(), we first need to change the registration procedure for genpd DT providers. Currently that's done when "PM domain drivers" invokes the __of_genpd_add_provider() API, from SoC specific code and from different init levels.

We need to have the available gendp DT providers registered when the ->pm_domain_init() callback is invoked.

Quoted 7 lines

Besides the changes above, genpd also needs to deal with attached devices, but which don't have a corresponding driver bound. I believe we have some corner cases to fix around this as well.

As an intermediate step, how about adding the similar code as above but into really_probe()? For the code in bus_remove_device() below, we could add that into __device_release_driver()?

Well, I wouldn't really like to add new callbacks to struct bus_type for intermediate steps, because that's guaranteed to lead to confusion.

So I think the infrastructure is better added first and users may be switched over to it gradually.

I don't see any particular problems with moving the ACPI PM domain attach/detach to bus_add/remove_device(), so that can be done as the first step. As for genpd, it can implement the ->post_probe thing first and do the rest in the bus type ->probe until the generic code is ready.

Quoted 3 lines

Yes, that works.

I will cook a new version of the patchset according to your suggestions.

I'll send a cleaner version of my patch tomorrow (I actually would like to use different names for the new callbacks).

Also I can do the ACPI part of the work, please let me know if you want me to.

From: Rafael J. Wysocki <[email protected]>

If PM domains are in use, it may be necessary to prepare the code handling a PM domain for driver probing. For example, in some cases device drivers rely on the ability to power on the devices with the help of the IO runtime PM framework and the PM domain code needs to be ready for that. Also, if that code has not been fully initialized yet, the driver probing should be deferred.

Moreover, after the probing is complete, it may be necessary to put the PM domain in question into the state reflecting the current needs of the devices in it, for example, to prevent power from being drawn in vain.

For these reasons, introduce new PM domain callbacks, ->activate and ->sync, called, respectively, before probing for a device driver and after the probing has been completed.

That is not sufficient, however, because the device's PM domain pointer has to be populated for the ->activate callback to be executed, so setting it in bus type ->probe callback routines would be too late. Also, there are bus types where PM domains are not used at all and the core should not attempt to set the pm_domain pointer for the devices on those buses.

To overcome that difficulty, introduce two new bus type callbacks, ->init and ->release, called by bus_add_device() and bus_remove_device(), respectively. That will allow ->init to be used to populate the pm_domain pointer for the bus types that want to do that and ->release will be useful for any cleanup that may be necessary after removing a device that was part of a PM domain.

Signed-off-by Rafael J. Wysocki <[email protected]>

---

It occured to me that we might want to ->sync regardless of whether or not the probing had been succenssful, so I changed the code in really_probe() along these lines. Please let me know if that's not OK.

---
 drivers/base/bus.c     |   12 +++++++++++-
 drivers/base/dd.c      |   20 ++++++++++++++------
 include/linux/device.h |    5 +++++
 include/linux/pm.h     |    6 ++++++
 4 files changed, 36 insertions(+), 7 deletions(-)

Index: linux-pm/drivers/base/bus.c
===================================================================

drivers/base/bus.c +9 -1

@@ -509,10 +509,15 @@ int bus_add_device(struct device *dev)
        int error = 0;
 
        if (bus) {
+               if (bus->init) {
+                       error = bus->init(dev);
+                       if (error)
+                               goto out_put;
+               }
                pr_debug("bus: '%s': add device %s\n", bus->name, dev_name(dev));
                error = device_add_attrs(bus, dev);
                if (error)
-                       goto out_put;
+                       goto out_release;
                error = device_add_groups(dev, bus->dev_groups);
                if (error)
                        goto out_groups;

@@ -534,6 +539,9 @@ out_groups:
        device_remove_groups(dev, bus->dev_groups);
 out_id:
        device_remove_attrs(bus, dev);
+out_release:
+       if (bus->release)
+               bus->release(dev);
 out_put:
        bus_put(dev->bus);
        return error;

drivers/base/bus.c +0 -0

        pr_debug("bus: '%s': remove device %s\n",
                 dev->bus->name, dev_name(dev));

Index: linux-pm/include/linux/device.h
===================================================================

include/linux/device.h +5 -0

@@ -69,6 +69,8 @@ extern void bus_remove_file(struct bus_t
  * @bus_groups:        Default attributes of the bus.
  * @dev_groups:        Default attributes of the devices on the bus.
  * @drv_groups: Default attributes of the device drivers on the bus.
+ * @init:      Called when registering a device on this bus.
+ * @release:   Called when unregistering a device on this bus.
  * @match:     Called, perhaps multiple times, whenever a new device or driver
  *             is added for this bus. It should return a nonzero value if the
  *             given device can be handled by the given driver.

@@ -111,6 +113,9 @@ struct bus_type {
        const struct attribute_group **dev_groups;
        const struct attribute_group **drv_groups;
 
+       int (*init)(struct device *dev);
+       void (*release)(struct device *dev);
+
        int (*match)(struct device *dev, struct device_driver *drv);
        int (*uevent)(struct device *dev, struct kobj_uevent_env *env);
        int (*probe)(struct device *dev);

Index: linux-pm/include/linux/pm.h
===================================================================

include/linux/pm.h +6 -0

@@ -603,10 +603,16 @@ extern void dev_pm_put_subsys_data(struc
  * Power domains provide callbacks that are executed during system suspend,
  * hibernation, system resume and during runtime PM transitions along with
  * subsystem-level and driver-level callbacks.
+ *
+ * @detach: Called when removing a device from the domain.
+ * @activate: Called before executing probe routines for bus types and drivers.
+ * @sync: Called after successful execiton of the probe routines.
  */
 struct dev_pm_domain {
        struct dev_pm_ops       ops;
        void (*detach)(struct device *dev, bool power_off);
+       int (*activate)(struct device *dev);
+       void (*sync)(struct device *dev);
 };
 
 /*

Index: linux-pm/drivers/base/dd.c
===================================================================

drivers/base/dd.c +7 -6

@@ -279,6 +279,7 @@ static int really_probe(struct device *d
 {
        int ret = 0;
        int local_trigger_count = atomic_read(&deferred_trigger_count);
+       struct dev_pm_domain *pm_domain = dev->pm_domain;
 
        atomic_inc(&probe_count);
        pr_debug("bus: '%s': %s: probing driver %s with device %s\n",

@@ -298,16 +299,23 @@ static int really_probe(struct device *d
                goto probe_failed;
        }
 
-       if (dev->bus->probe) {
-               ret = dev->bus->probe(dev);
-               if (ret)
-                       goto probe_failed;
-       } else if (drv->probe) {
-               ret = drv->probe(dev);
+       if (pm_domain && pm_domain->activate) {
+               ret = pm_domain->activate(dev);
                if (ret)
                        goto probe_failed;
        }
 
+       if (dev->bus->probe)
+               ret = dev->bus->probe(dev);
+       else if (drv->probe)
+               ret = drv->probe(dev);

Quoted 33 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

If PM domains are in use, it may be necessary to prepare the code handling a PM domain for driver probing. For example, in some cases device drivers rely on the ability to power on the devices with the help of the IO runtime PM framework and the PM domain code needs to be ready for that. Also, if that code has not been fully initialized yet, the driver probing should be deferred.

Moreover, after the probing is complete, it may be necessary to put the PM domain in question into the state reflecting the current needs of the devices in it, for example, to prevent power from being drawn in vain.

For these reasons, introduce new PM domain callbacks, ->activate and ->sync, called, respectively, before probing for a device driver and after the probing has been completed.

That is not sufficient, however, because the device's PM domain pointer has to be populated for the ->activate callback to be executed, so setting it in bus type ->probe callback routines would be too late. Also, there are bus types where PM domains are not used at all and the core should not attempt to set the pm_domain pointer for the devices on those buses.

To overcome that difficulty, introduce two new bus type callbacks, ->init and ->release, called by bus_add_device() and bus_remove_device(), respectively. That will allow ->init to be used to populate the pm_domain pointer for the bus types that want to do that and ->release will be useful for any cleanup that may be necessary after removing a device that was part of a PM domain.

Quoted 7 lines

Signed-off-by Rafael J. Wysocki <[email protected]>

---

It occured to me that we might want to ->sync regardless of whether or not the probing had been succenssful, so I changed the code in really_probe() along these lines. Please let me know if that's not OK.

Make perfect sense!

Quoted 42 lines

---
 drivers/base/bus.c     |   12 +++++++++++-
 drivers/base/dd.c      |   20 ++++++++++++++------
 include/linux/device.h |    5 +++++
 include/linux/pm.h     |    6 ++++++
 4 files changed, 36 insertions(+), 7 deletions(-)

Index: linux-pm/drivers/base/bus.c
===================================================================

drivers/base/bus.c +9 -1

@@ -509,10 +509,15 @@ int bus_add_device(struct device *dev)
        int error = 0;
 
        if (bus) {
+               if (bus->init) {
+                       error = bus->init(dev);
+                       if (error)
+                               goto out_put;
+               }
                pr_debug("bus: '%s': add device %s\n", bus->name, dev_name(dev));
                error = device_add_attrs(bus, dev);
                if (error)
-                       goto out_put;
+                       goto out_release;
                error = device_add_groups(dev, bus->dev_groups);
                if (error)
                        goto out_groups;

@@ -534,6 +539,9 @@ out_groups:
        device_remove_groups(dev, bus->dev_groups);
 out_id:
        device_remove_attrs(bus, dev);
+out_release:
+       if (bus->release)
+               bus->release(dev);
 out_put:
        bus_put(dev->bus);
        return error;

@@ -597,6 +605,8 @@ void bus_remove_device(struct device *de
        device_remove_groups(dev, dev->bus->dev_groups);
        if (klist_node_attached(&dev->p->knode_bus))

drivers/base/bus.c void bus_remove_device(struct device *de · line 609

Quoted 3 lines

drivers/base/bus.c +2 -0

                klist_del(&dev->p->knode_bus);
+       if (bus->release)
+               bus->release(dev);

I think we should move this new code block below the call to device_release_driver(), since else the bus'/driver's ->remove() callbacks may be invoked after the ->pm_domain pointer for the device has been removed.

Moving the code below the call to device_release_driver() also means device's devm* managed resources will be freed prior invoking the bus' ->release() callback. Genpd don't have any issues to cope with that I believe that's okay for ACPI as well, but not sure.

Moreover, considering the case where device won't be removed but only its corresponding driver. In that case, the PM domain won't be notified other than through runtime PM transitions. I don't think that's enough, we will likely need to add yet another callback in the struct dev_pm_domain, to be invoked from __device_release_driver().

Quoted 86 lines · Ulf Hansson

Quoted 33 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

If PM domains are in use, it may be necessary to prepare the code handling a PM domain for driver probing. For example, in some cases device drivers rely on the ability to power on the devices with the help of the IO runtime PM framework and the PM domain code needs to be ready for that. Also, if that code has not been fully initialized yet, the driver probing should be deferred.

Moreover, after the probing is complete, it may be necessary to put the PM domain in question into the state reflecting the current needs of the devices in it, for example, to prevent power from being drawn in vain.

For these reasons, introduce new PM domain callbacks, ->activate and ->sync, called, respectively, before probing for a device driver and after the probing has been completed.

That is not sufficient, however, because the device's PM domain pointer has to be populated for the ->activate callback to be executed, so setting it in bus type ->probe callback routines would be too late. Also, there are bus types where PM domains are not used at all and the core should not attempt to set the pm_domain pointer for the devices on those buses.

To overcome that difficulty, introduce two new bus type callbacks, ->init and ->release, called by bus_add_device() and bus_remove_device(), respectively. That will allow ->init to be used to populate the pm_domain pointer for the bus types that want to do that and ->release will be useful for any cleanup that may be necessary after removing a device that was part of a PM domain.

Quoted 7 lines

Signed-off-by Rafael J. Wysocki <[email protected]>

---

It occured to me that we might want to ->sync regardless of whether or not the probing had been succenssful, so I changed the code in really_probe() along these lines. Please let me know if that's not OK.

Make perfect sense!

Quoted 42 lines

---
 drivers/base/bus.c     |   12 +++++++++++-
 drivers/base/dd.c      |   20 ++++++++++++++------
 include/linux/device.h |    5 +++++
 include/linux/pm.h     |    6 ++++++
 4 files changed, 36 insertions(+), 7 deletions(-)

Index: linux-pm/drivers/base/bus.c
===================================================================

drivers/base/bus.c +9 -1

@@ -509,10 +509,15 @@ int bus_add_device(struct device *dev)
        int error = 0;
 
        if (bus) {
+               if (bus->init) {
+                       error = bus->init(dev);
+                       if (error)
+                               goto out_put;
+               }
                pr_debug("bus: '%s': add device %s\n", bus->name, dev_name(dev));
                error = device_add_attrs(bus, dev);
                if (error)
-                       goto out_put;
+                       goto out_release;
                error = device_add_groups(dev, bus->dev_groups);
                if (error)
                        goto out_groups;

@@ -534,6 +539,9 @@ out_groups:
        device_remove_groups(dev, bus->dev_groups);
 out_id:
        device_remove_attrs(bus, dev);
+out_release:
+       if (bus->release)
+               bus->release(dev);
 out_put:
        bus_put(dev->bus);
        return error;

@@ -597,6 +605,8 @@ void bus_remove_device(struct device *de
        device_remove_groups(dev, dev->bus->dev_groups);
        if (klist_node_attached(&dev->p->knode_bus))

Quoted 8 lines

drivers/base/bus.c void bus_remove_device(struct device *de · line 609

Quoted 3 lines

drivers/base/bus.c +2 -0

                klist_del(&dev->p->knode_bus);
+       if (bus->release)
+               bus->release(dev);

I think we should move this new code block below the call to device_release_driver(), since else the bus'/driver's ->remove() callbacks may be invoked after the ->pm_domain pointer for the device has been removed.

Good point, I'll fix that.

Quoted 4 lines

Moving the code below the call to device_release_driver() also means device's devm* managed resources will be freed prior invoking the bus' ->release() callback. Genpd don't have any issues to cope with that I believe that's okay for ACPI as well, but not sure.

It is.

Quoted 5 lines

Moreover, considering the case where device won't be removed but only its corresponding driver. In that case, the PM domain won't be notified other than through runtime PM transitions. I don't think that's enough, we will likely need to add yet another callback in the struct dev_pm_domain, to be invoked from __device_release_driver().

Right, so do we want to ->sync then or do we want a separate callback?

The only use case I have which is not genpd doesn't need that, so genpd will be the only user of it for the time being.

From: Rafael J. Wysocki <[email protected]>

If PM domains are in use, it may be necessary to prepare the code handling a PM domain for driver probing. For example, in some cases device drivers rely on the ability to power on the devices with the help of the IO runtime PM framework and the PM domain code needs to be ready for that. Also, if that code has not been fully initialized yet, the driver probing should be deferred.

Moreover, after the probing is complete, it may be necessary to put the PM domain in question into the state reflecting the current needs of the devices in it, for example, to prevent power from being drawn in vain.

For these reasons, introduce new PM domain callbacks, ->activate and ->sync, called, respectively, before probing for a device driver and after the probing has been completed.

That is not sufficient, however, because the device's PM domain pointer has to be populated for the ->activate callback to be executed, so setting it in bus type ->probe callback routines would be too late. Also, there are bus types where PM domains are not used at all and the core should not attempt to set the pm_domain pointer for the devices on those buses.

To overcome that difficulty, introduce two new bus type callbacks, ->init and ->release, called by bus_add_device() and bus_remove_device(), respectively. That will allow ->init to be used to populate the pm_domain pointer for the bus types that want to do that and ->release will be useful for any cleanup that may be necessary after removing a device that was part of a PM domain.

Signed-off-by Rafael J. Wysocki <[email protected]>

---

It occured to me that we might want to ->sync regardless of whether or not the probing had been succenssful, so I changed the code in really_probe() along these lines. Please let me know if that's not OK.

---
 drivers/base/bus.c     |   12 +++++++++++-
 drivers/base/dd.c      |   20 ++++++++++++++------
 include/linux/device.h |    5 +++++
 include/linux/pm.h     |    6 ++++++
 4 files changed, 36 insertions(+), 7 deletions(-)

Index: linux-pm/drivers/base/bus.c
===================================================================

drivers/base/bus.c +4 -1

 		pr_debug("bus: '%s': add device %s\n", bus->name, dev_name(dev));
 		error = device_add_attrs(bus, dev);
 		if (error)
-			goto out_put;
+			goto out_release;
 		error = device_add_groups(dev, bus->dev_groups);
 		if (error)
 			goto out_groups;

@@ -534,6 +539,9 @@ out_groups:
 	device_remove_groups(dev, bus->dev_groups);
 out_id:
 	device_remove_attrs(bus, dev);
+out_release:
+	if (bus->release)
+		bus->release(dev);
 
 out_put:
 	bus_put(dev->bus);

drivers/base/bus.c +0 -0

 	return error;

Quoted 55 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

If PM domains are in use, it may be necessary to prepare the code handling a PM domain for driver probing. For example, in some cases device drivers rely on the ability to power on the devices with the help of the IO runtime PM framework and the PM domain code needs to be ready for that. Also, if that code has not been fully initialized yet, the driver probing should be deferred.

Moreover, after the probing is complete, it may be necessary to put the PM domain in question into the state reflecting the current needs of the devices in it, for example, to prevent power from being drawn in vain.

For these reasons, introduce new PM domain callbacks, ->activate and ->sync, called, respectively, before probing for a device driver and after the probing has been completed.

That is not sufficient, however, because the device's PM domain pointer has to be populated for the ->activate callback to be executed, so setting it in bus type ->probe callback routines would be too late. Also, there are bus types where PM domains are not used at all and the core should not attempt to set the pm_domain pointer for the devices on those buses.

To overcome that difficulty, introduce two new bus type callbacks, ->init and ->release, called by bus_add_device() and bus_remove_device(), respectively. That will allow ->init to be used to populate the pm_domain pointer for the bus types that want to do that and ->release will be useful for any cleanup that may be necessary after removing a device that was part of a PM domain.

Signed-off-by Rafael J. Wysocki <[email protected]>

---

It occured to me that we might want to ->sync regardless of whether or not the probing had been succenssful, so I changed the code in really_probe() along these lines. Please let me know if that's not OK.

---
 drivers/base/bus.c     |   12 +++++++++++-
 drivers/base/dd.c      |   20 ++++++++++++++------
 include/linux/device.h |    5 +++++
 include/linux/pm.h     |    6 ++++++
 4 files changed, 36 insertions(+), 7 deletions(-)

Index: linux-pm/drivers/base/bus.c
===================================================================

drivers/base/bus.c +0 -0

@@ -509,10 +509,15 @@ int bus_add_device(struct device *dev)
 	int error = 0;
 

drivers/base/bus.c int bus_add_device(struct device *dev) · line 516

Quoted 6 lines

drivers/base/bus.c +5 -0

 	if (bus) {
+		if (bus->init) {
+			error = bus->init(dev);
+			if (error)
+				goto out_put;
+		}

This doesn't make sense to me. A bus just called bus_add_device, it can do whatever it wanted to right before calling this function, no need for another callback.

Quoted 15 lines

drivers/base/bus.c +4 -1

 		pr_debug("bus: '%s': add device %s\n", bus->name, dev_name(dev));
 		error = device_add_attrs(bus, dev);
 		if (error)
-			goto out_put;
+			goto out_release;
 		error = device_add_groups(dev, bus->dev_groups);
 		if (error)
 			goto out_groups;

@@ -534,6 +539,9 @@ out_groups:
 	device_remove_groups(dev, bus->dev_groups);
 out_id:
 	device_remove_attrs(bus, dev);
+out_release:
+	if (bus->release)
+		bus->release(dev);

drivers/base/bus.c void bus_remove_device(struct device *de · line 609

Quoted 9 lines

drivers/base/bus.c +2 -0

 out_put:
 	bus_put(dev->bus);
 	return error;

@@ -597,6 +605,8 @@ void bus_remove_device(struct device *de
 	device_remove_groups(dev, dev->bus->dev_groups);
 	if (klist_node_attached(&dev->p->knode_bus))
 		klist_del(&dev->p->knode_bus);
+	if (bus->release)
+		bus->release(dev);

Same with release(), this happens when a bus wants to remove a device, it controls this, why have a callback right away? These both shouldn't be needed.

sorry if I missed this before, I hadn't noticed these callbacks in previous patches but I wasn't paying much attention.

Quoted 56 lines · Greg Kroah-Hartman

Quoted 55 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

If PM domains are in use, it may be necessary to prepare the code handling a PM domain for driver probing. For example, in some cases device drivers rely on the ability to power on the devices with the help of the IO runtime PM framework and the PM domain code needs to be ready for that. Also, if that code has not been fully initialized yet, the driver probing should be deferred.

Moreover, after the probing is complete, it may be necessary to put the PM domain in question into the state reflecting the current needs of the devices in it, for example, to prevent power from being drawn in vain.

For these reasons, introduce new PM domain callbacks, ->activate and ->sync, called, respectively, before probing for a device driver and after the probing has been completed.

That is not sufficient, however, because the device's PM domain pointer has to be populated for the ->activate callback to be executed, so setting it in bus type ->probe callback routines would be too late. Also, there are bus types where PM domains are not used at all and the core should not attempt to set the pm_domain pointer for the devices on those buses.

To overcome that difficulty, introduce two new bus type callbacks, ->init and ->release, called by bus_add_device() and bus_remove_device(), respectively. That will allow ->init to be used to populate the pm_domain pointer for the bus types that want to do that and ->release will be useful for any cleanup that may be necessary after removing a device that was part of a PM domain.

Signed-off-by Rafael J. Wysocki <[email protected]>

---

It occured to me that we might want to ->sync regardless of whether or not the probing had been succenssful, so I changed the code in really_probe() along these lines. Please let me know if that's not OK.

---
 drivers/base/bus.c     |   12 +++++++++++-
 drivers/base/dd.c      |   20 ++++++++++++++------
 include/linux/device.h |    5 +++++
 include/linux/pm.h     |    6 ++++++
 4 files changed, 36 insertions(+), 7 deletions(-)

Index: linux-pm/drivers/base/bus.c
===================================================================

drivers/base/bus.c +0 -0

@@ -509,10 +509,15 @@ int bus_add_device(struct device *dev)
 	int error = 0;
 

Quoted 10 lines

drivers/base/bus.c int bus_add_device(struct device *dev) · line 516

Quoted 6 lines

drivers/base/bus.c +5 -0

 	if (bus) {
+		if (bus->init) {
+			error = bus->init(dev);
+			if (error)
+				goto out_put;
+		}

This doesn't make sense to me. A bus just called bus_add_device, it can do whatever it wanted to right before calling this function, no need for another callback.

The only caller of bus_add_device() is device_add().

What do you mean by "bus just called bus_add_device"? Do you think that the pm_domain pointer should be populated before calling device_add()?

That wouldn't work for the ACPI PM domain at least, because ACPI companions are generally added during device_add() and we arguably cannot point a device to the ACPI PM domain before its ACPI companion is set.

Quoted 16 lines

Quoted 15 lines

drivers/base/bus.c +4 -1

 		pr_debug("bus: '%s': add device %s\n", bus->name, dev_name(dev));
 		error = device_add_attrs(bus, dev);
 		if (error)
-			goto out_put;
+			goto out_release;
 		error = device_add_groups(dev, bus->dev_groups);
 		if (error)
 			goto out_groups;

@@ -534,6 +539,9 @@ out_groups:
 	device_remove_groups(dev, bus->dev_groups);
 out_id:
 	device_remove_attrs(bus, dev);
+out_release:
+	if (bus->release)
+		bus->release(dev);

Quoted 13 lines

drivers/base/bus.c void bus_remove_device(struct device *de · line 609

Quoted 9 lines

drivers/base/bus.c +2 -0

 out_put:
 	bus_put(dev->bus);
 	return error;

@@ -597,6 +605,8 @@ void bus_remove_device(struct device *de
 	device_remove_groups(dev, dev->bus->dev_groups);
 	if (klist_node_attached(&dev->p->knode_bus))
 		klist_del(&dev->p->knode_bus);
+	if (bus->release)
+		bus->release(dev);

Same with release(), this happens when a bus wants to remove a device, it controls this, why have a callback right away? These both shouldn't be needed.

This is for symmetry with bus_add_device() and please see the argument there.

Quoted 2 lines

sorry if I missed this before, I hadn't noticed these callbacks in previous patches but I wasn't paying much attention.

No, they were not present before.

There are two alternatives to them. One is to do PM domain attach/detach in the bus type's ->probe and ->remove, but that's suboptimal, because it is then carried out every time a driver is probed/removed for a device. The other one would be to have each interested bus type register a bus type notifier for itself, but that would be rather ugly, wouldn't it?

Quoted 68 lines · Rafael J. Wysocki

Quoted 66 lines · Greg Kroah-Hartman

Quoted 53 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

If PM domains are in use, it may be necessary to prepare the code handling a PM domain for driver probing. For example, in some cases device drivers rely on the ability to power on the devices with the help of the IO runtime PM framework and the PM domain code needs to be ready for that. Also, if that code has not been fully initialized yet, the driver probing should be deferred.

Moreover, after the probing is complete, it may be necessary to put the PM domain in question into the state reflecting the current needs of the devices in it, for example, to prevent power from being drawn in vain.

For these reasons, introduce new PM domain callbacks, ->activate and ->sync, called, respectively, before probing for a device driver and after the probing has been completed.

That is not sufficient, however, because the device's PM domain pointer has to be populated for the ->activate callback to be executed, so setting it in bus type ->probe callback routines would be too late. Also, there are bus types where PM domains are not used at all and the core should not attempt to set the pm_domain pointer for the devices on those buses.

To overcome that difficulty, introduce two new bus type callbacks, ->init and ->release, called by bus_add_device() and bus_remove_device(), respectively. That will allow ->init to be used to populate the pm_domain pointer for the bus types that want to do that and ->release will be useful for any cleanup that may be necessary after removing a device that was part of a PM domain.

Signed-off-by Rafael J. Wysocki <[email protected]>

---

It occured to me that we might want to ->sync regardless of whether or not the probing had been succenssful, so I changed the code in really_probe() along these lines. Please let me know if that's not OK.

---
 drivers/base/bus.c     |   12 +++++++++++-
 drivers/base/dd.c      |   20 ++++++++++++++------
 include/linux/device.h |    5 +++++
 include/linux/pm.h     |    6 ++++++
 4 files changed, 36 insertions(+), 7 deletions(-)

Index: linux-pm/drivers/base/bus.c
===================================================================

drivers/base/bus.c int bus_add_device(struct device *dev) · line 516

Quoted 8 lines

drivers/base/bus.c +5 -0

@@ -509,10 +509,15 @@ int bus_add_device(struct device *dev)
 	int error = 0;
 
 	if (bus) {
+		if (bus->init) {
+			error = bus->init(dev);
+			if (error)
+				goto out_put;
+		}

This doesn't make sense to me. A bus just called bus_add_device, it can do whatever it wanted to right before calling this function, no need for another callback.

Quoted 4 lines

The only caller of bus_add_device() is device_add().

What do you mean by "bus just called bus_add_device"? Do you think that the pm_domain pointer should be populated before calling device_add()?

If it's needed, sure. The bus itself (i.e. PCI, USB, etc.) just called device_add() which calls bus_add_device(), so it could set up the pm_domain pointer as it knows what is up for this device.

Quoted 3 lines

That wouldn't work for the ACPI PM domain at least, because ACPI companions are generally added during device_add() and we arguably cannot point a device to the ACPI PM domain before its ACPI companion is set.

I don't understand, what is not set up at device_add() time that is somehow set up at bus_add_device() time? Our number of callbacks seems to be getting deep and messy :)

What happens in device_pm_add(dev); in device_add()?

Quoted 35 lines

Quoted 16 lines

Quoted 15 lines

drivers/base/bus.c +4 -1

 		pr_debug("bus: '%s': add device %s\n", bus->name, dev_name(dev));
 		error = device_add_attrs(bus, dev);
 		if (error)
-			goto out_put;
+			goto out_release;
 		error = device_add_groups(dev, bus->dev_groups);
 		if (error)
 			goto out_groups;

@@ -534,6 +539,9 @@ out_groups:
 	device_remove_groups(dev, bus->dev_groups);
 out_id:
 	device_remove_attrs(bus, dev);
+out_release:
+	if (bus->release)
+		bus->release(dev);

Quoted 13 lines

drivers/base/bus.c void bus_remove_device(struct device *de · line 609

Quoted 9 lines

drivers/base/bus.c +2 -0

 out_put:
 	bus_put(dev->bus);
 	return error;

@@ -597,6 +605,8 @@ void bus_remove_device(struct device *de
 	device_remove_groups(dev, dev->bus->dev_groups);
 	if (klist_node_attached(&dev->p->knode_bus))
 		klist_del(&dev->p->knode_bus);
+	if (bus->release)
+		bus->release(dev);

Same with release(), this happens when a bus wants to remove a device, it controls this, why have a callback right away? These both shouldn't be needed.

This is for symmetry with bus_add_device() and please see the argument there.

Quoted 2 lines

sorry if I missed this before, I hadn't noticed these callbacks in previous patches but I wasn't paying much attention.

Quoted 7 lines

No, they were not present before.

There are two alternatives to them. One is to do PM domain attach/detach in the bus type's ->probe and ->remove, but that's suboptimal, because it is then carried out every time a driver is probed/removed for a device. The other one would be to have each interested bus type register a bus type notifier for itself, but that would be rather ugly, wouldn't it?

This seems really messy, and you are adding more complexity, isn't there some other easier way to do it with all of the different callbacks and notifications we have today?

Quoted 3 lines

This doesn't make sense to me. A bus just called bus_add_device, it can do whatever it wanted to right before calling this function, no need for another callback.

The only caller of bus_add_device() is device_add().

What do you mean by "bus just called bus_add_device"? Do you think that the pm_domain pointer should be populated before calling device_add()?

That wouldn't work for the ACPI PM domain at least, because ACPI companions are generally added during device_add() and we arguably cannot point a device to the ACPI PM domain before its ACPI companion is set.

There are two alternatives to them. One is to do PM domain attach/detach in the bus type's ->probe and ->remove, but that's suboptimal, because it is then carried out every time a driver is probed/removed for a device. The other one would be to have each interested bus type register a bus type notifier for itself, but that would be rather ugly, wouldn't it?

Quoted 14 lines · Rafael J. Wysocki

Quoted 3 lines

This doesn't make sense to me. A bus just called bus_add_device, it can do whatever it wanted to right before calling this function, no need for another callback.

The only caller of bus_add_device() is device_add().

What do you mean by "bus just called bus_add_device"? Do you think that the pm_domain pointer should be populated before calling device_add()?

That wouldn't work for the ACPI PM domain at least, because ACPI companions are generally added during device_add() and we arguably cannot point a device to the ACPI PM domain before its ACPI companion is set.

Quoted 5 lines

There are two alternatives to them. One is to do PM domain attach/detach in the bus type's ->probe and ->remove, but that's suboptimal, because it is then carried out every time a driver is probed/removed for a device. The other one would be to have each interested bus type register a bus type notifier for itself, but that would be rather ugly, wouldn't it?

Driver probing and removal is not a hot path. Successful probes usually occur only once for each device. I don't know how many unsuccessful probes there are on average, but probably not many.

Doing the PM domain attach/detach in ->probe and ->remove makes sense to me.

Quoted 22 lines · Alan Stern

Quoted 14 lines · Rafael J. Wysocki

Quoted 3 lines

This doesn't make sense to me. A bus just called bus_add_device, it can do whatever it wanted to right before calling this function, no need for another callback.

The only caller of bus_add_device() is device_add().

What do you mean by "bus just called bus_add_device"? Do you think that the pm_domain pointer should be populated before calling device_add()?

That wouldn't work for the ACPI PM domain at least, because ACPI companions are generally added during device_add() and we arguably cannot point a device to the ACPI PM domain before its ACPI companion is set.

Quoted 5 lines

There are two alternatives to them. One is to do PM domain attach/detach in the bus type's ->probe and ->remove, but that's suboptimal, because it is then carried out every time a driver is probed/removed for a device. The other one would be to have each interested bus type register a bus type notifier for itself, but that would be rather ugly, wouldn't it?

Quoted 6 lines

Driver probing and removal is not a hot path. Successful probes usually occur only once for each device. I don't know how many unsuccessful probes there are on average, but probably not many.

Doing the PM domain attach/detach in ->probe and ->remove makes sense to me.

Let me elaborate a bit on the current approach, with PM domain attach/detach in ->probe() and ->remove(). This comes with the following limitations.

1) If the device gets probed earlier than the PM domain has been initialized, we can't return -EPROBE_DEFER since we just don't know anything about the PM domain yet.

2) The PM domain won't be able to operate on a device without having a driver bound to it, since the device's PM domain pointer is removed at ->remove().

Perhaps Rafael want to add something to these? I don't know whether we think it's okay to keep these limitations or not.

Quoted 38 lines · Ulf Hansson

Quoted 22 lines · Alan Stern

Quoted 14 lines · Rafael J. Wysocki

Quoted 3 lines

This doesn't make sense to me. A bus just called bus_add_device, it can do whatever it wanted to right before calling this function, no need for another callback.

The only caller of bus_add_device() is device_add().

What do you mean by "bus just called bus_add_device"? Do you think that the pm_domain pointer should be populated before calling device_add()?

That wouldn't work for the ACPI PM domain at least, because ACPI companions are generally added during device_add() and we arguably cannot point a device to the ACPI PM domain before its ACPI companion is set.

Quoted 5 lines

There are two alternatives to them. One is to do PM domain attach/detach in the bus type's ->probe and ->remove, but that's suboptimal, because it is then carried out every time a driver is probed/removed for a device. The other one would be to have each interested bus type register a bus type notifier for itself, but that would be rather ugly, wouldn't it?

Quoted 6 lines

Driver probing and removal is not a hot path. Successful probes usually occur only once for each device. I don't know how many unsuccessful probes there are on average, but probably not many.

Doing the PM domain attach/detach in ->probe and ->remove makes sense to me.

Let me elaborate a bit on the current approach, with PM domain attach/detach in ->probe() and ->remove(). This comes with the following limitations.

1) If the device gets probed earlier than the PM domain has been initialized, we can't return -EPROBE_DEFER since we just don't know anything about the PM domain yet.

Quoted 6 lines

2) The PM domain won't be able to operate on a device without having a driver bound to it, since the device's PM domain pointer is removed at ->remove().

Perhaps Rafael want to add something to these? I don't know whether we think it's okay to keep these limitations or not.

There are arguments both ways as you can see.

At this point, the most straightforward approach would be to apply the Russell's patch adding the ->sync callback alone.

I will then need the ->activate callback for another use case (a PM domain that may not be ready for the device to be probed), but in that case I actually know that the pm_domain pointer is populated at the really_probe() time.

Going forward we may want to move the attach/detach operations for genpd and the ACPI PM domain out of bus type ->probe, but I think that can wait.

Quoted 46 lines · Rafael J. Wysocki

Quoted 38 lines · Ulf Hansson

Quoted 22 lines · Alan Stern

Quoted 14 lines · Rafael J. Wysocki

Quoted 3 lines

This doesn't make sense to me. A bus just called bus_add_device, it can do whatever it wanted to right before calling this function, no need for another callback.

The only caller of bus_add_device() is device_add().

What do you mean by "bus just called bus_add_device"? Do you think that the pm_domain pointer should be populated before calling device_add()?

That wouldn't work for the ACPI PM domain at least, because ACPI companions are generally added during device_add() and we arguably cannot point a device to the ACPI PM domain before its ACPI companion is set.

Quoted 5 lines

There are two alternatives to them. One is to do PM domain attach/detach in the bus type's ->probe and ->remove, but that's suboptimal, because it is then carried out every time a driver is probed/removed for a device. The other one would be to have each interested bus type register a bus type notifier for itself, but that would be rather ugly, wouldn't it?

Quoted 6 lines

Driver probing and removal is not a hot path. Successful probes usually occur only once for each device. I don't know how many unsuccessful probes there are on average, but probably not many.

Doing the PM domain attach/detach in ->probe and ->remove makes sense to me.

Let me elaborate a bit on the current approach, with PM domain attach/detach in ->probe() and ->remove(). This comes with the following limitations.

1) If the device gets probed earlier than the PM domain has been initialized, we can't return -EPROBE_DEFER since we just don't know anything about the PM domain yet.

Quoted 6 lines

2) The PM domain won't be able to operate on a device without having a driver bound to it, since the device's PM domain pointer is removed at ->remove().

Perhaps Rafael want to add something to these? I don't know whether we think it's okay to keep these limitations or not.

Quoted 8 lines

There are arguments both ways as you can see.

At this point, the most straightforward approach would be to apply the Russell's patch adding the ->sync callback alone.

I will then need the ->activate callback for another use case (a PM domain that may not be ready for the device to be probed), but in that case I actually know that the pm_domain pointer is populated at the really_probe() time.

Then, how about adding only the ->sync() and ->activate() callback in one patch - and invoke them from really_probe()? That's seems to fit both our short and long term approach.

Genpd may then implement the ->sync() callback, which is similar to Russell's patch, except that it will now cover all buses and not just the platform bus as it where before.

Quoted 60 lines · Ulf Hansson

Quoted 46 lines · Rafael J. Wysocki

Quoted 38 lines · Ulf Hansson

Quoted 22 lines · Alan Stern

Quoted 14 lines · Rafael J. Wysocki

Quoted 3 lines

This doesn't make sense to me. A bus just called bus_add_device, it can do whatever it wanted to right before calling this function, no need for another callback.

The only caller of bus_add_device() is device_add().

What do you mean by "bus just called bus_add_device"? Do you think that the pm_domain pointer should be populated before calling device_add()?

That wouldn't work for the ACPI PM domain at least, because ACPI companions are generally added during device_add() and we arguably cannot point a device to the ACPI PM domain before its ACPI companion is set.

Quoted 5 lines

There are two alternatives to them. One is to do PM domain attach/detach in the bus type's ->probe and ->remove, but that's suboptimal, because it is then carried out every time a driver is probed/removed for a device. The other one would be to have each interested bus type register a bus type notifier for itself, but that would be rather ugly, wouldn't it?

Quoted 6 lines

Driver probing and removal is not a hot path. Successful probes usually occur only once for each device. I don't know how many unsuccessful probes there are on average, but probably not many.

Doing the PM domain attach/detach in ->probe and ->remove makes sense to me.

Let me elaborate a bit on the current approach, with PM domain attach/detach in ->probe() and ->remove(). This comes with the following limitations.

1) If the device gets probed earlier than the PM domain has been initialized, we can't return -EPROBE_DEFER since we just don't know anything about the PM domain yet.

Quoted 6 lines

2) The PM domain won't be able to operate on a device without having a driver bound to it, since the device's PM domain pointer is removed at ->remove().

Perhaps Rafael want to add something to these? I don't know whether we think it's okay to keep these limitations or not.

Quoted 8 lines

There are arguments both ways as you can see.

At this point, the most straightforward approach would be to apply the Russell's patch adding the ->sync callback alone.

I will then need the ->activate callback for another use case (a PM domain that may not be ready for the device to be probed), but in that case I actually know that the pm_domain pointer is populated at the really_probe() time.

Then, how about adding only the ->sync() and ->activate() callback in one patch - and invoke them from really_probe()? That's seems to fit both our short and long term approach.

Quoted 3 lines

Genpd may then implement the ->sync() callback, which is similar to Russell's patch, except that it will now cover all buses and not just the platform bus as it where before.

OK

So do we need to do ->sync at the driver removal time too? I guess we do?

Quoted 65 lines · Rafael J. Wysocki

Quoted 63 lines · Ulf Hansson

Quoted 46 lines · Rafael J. Wysocki

Quoted 38 lines · Ulf Hansson

Quoted 22 lines · Alan Stern

Quoted 14 lines · Rafael J. Wysocki

Quoted 3 lines

This doesn't make sense to me. A bus just called bus_add_device, it can do whatever it wanted to right before calling this function, no need for another callback.

The only caller of bus_add_device() is device_add().

What do you mean by "bus just called bus_add_device"? Do you think that the pm_domain pointer should be populated before calling device_add()?

That wouldn't work for the ACPI PM domain at least, because ACPI companions are generally added during device_add() and we arguably cannot point a device to the ACPI PM domain before its ACPI companion is set.

Quoted 5 lines

There are two alternatives to them. One is to do PM domain attach/detach in the bus type's ->probe and ->remove, but that's suboptimal, because it is then carried out every time a driver is probed/removed for a device. The other one would be to have each interested bus type register a bus type notifier for itself, but that would be rather ugly, wouldn't it?

Quoted 6 lines

Driver probing and removal is not a hot path. Successful probes usually occur only once for each device. I don't know how many unsuccessful probes there are on average, but probably not many.

Doing the PM domain attach/detach in ->probe and ->remove makes sense to me.

Let me elaborate a bit on the current approach, with PM domain attach/detach in ->probe() and ->remove(). This comes with the following limitations.

1) If the device gets probed earlier than the PM domain has been initialized, we can't return -EPROBE_DEFER since we just don't know anything about the PM domain yet.

Quoted 6 lines

2) The PM domain won't be able to operate on a device without having a driver bound to it, since the device's PM domain pointer is removed at ->remove().

Perhaps Rafael want to add something to these? I don't know whether we think it's okay to keep these limitations or not.

Quoted 8 lines

There are arguments both ways as you can see.

At this point, the most straightforward approach would be to apply the Russell's patch adding the ->sync callback alone.

I will then need the ->activate callback for another use case (a PM domain that may not be ready for the device to be probed), but in that case I actually know that the pm_domain pointer is populated at the really_probe() time.

Then, how about adding only the ->sync() and ->activate() callback in one patch - and invoke them from really_probe()? That's seems to fit both our short and long term approach.

Genpd may then implement the ->sync() callback, which is similar to Russell's patch, except that it will now cover all buses and not just the platform bus as it where before.

Quoted 3 lines

OK

So do we need to do ->sync at the driver removal time too? I guess we do?

For genpd, it will currently not matter since the device will be detached from its PM domain before that ->sync() would be called.

Though, to prepare for the "long term" solution we could decide to add it...

---

This is an update without the additional bus type callbacks. This should be suffucient at this point for the use cases we have.

I've decided to also call ->sync on driver removal as that is consistent with what happens for failing probe.

---

From: Rafael J. Wysocki <[email protected]>
Subject: driver core / PM: Add PM domain callbacks for device setup/cleanup

If PM domains are in use, it may be necessary to prepare the code handling a PM domain for driver probing. For example, in some cases device drivers rely on the ability to power on the devices with the help of the IO runtime PM framework and the PM domain code needs to be ready for that. Also, if that code has not been fully initialized yet, the driver probing should be deferred.

Moreover, after the probing is complete, it may be necessary to put the PM domain in question into the state reflecting the current needs of the devices in it, for example, so that power is not drawn in vain. The same should be done after removing a driver from a device, as the PM domain state may need to be changed to reflect the new situation.

For these reasons, introduce new PM domain callbacks, ->activate and ->sync, called, respectively, before probing for a device driver and after the probing has been completed or the driver has been removed.

Signed-off-by Rafael J. Wysocki <[email protected]>

---

Quoted 8 lines

---

This is an update without the additional bus type callbacks. This should be suffucient at this point for the use cases we have.

I've decided to also call ->sync on driver removal as that is consistent with what happens for failing probe.

Quoted 1 line

---

One more update.

I've realized that in the PM domain's ->sync callback is called after clearing the device's driver field (in the failed probe or driver removal case) rather than before it, that could be used to distinguish between the two cases (successful probe vs no driver), so I reworked the patch accordingly.

---
 drivers/base/dd.c  |   16 ++++++++++++++++
 include/linux/pm.h |    6 ++++++
 2 files changed, 22 insertions(+)

Index: linux-pm/include/linux/pm.h
===================================================================

include/linux/pm.h +6 -0

@@ -603,10 +603,16 @@ extern void dev_pm_put_subsys_data(struc
  * Power domains provide callbacks that are executed during system suspend,
  * hibernation, system resume and during runtime PM transitions along with
  * subsystem-level and driver-level callbacks.
+ *
+ * @detach: Called when removing a device from the domain.
+ * @activate: Called before executing probe routines for bus types and drivers.
+ * @sync: Called after driver probe and removal.
  */
 struct dev_pm_domain {
        struct dev_pm_ops       ops;
        void (*detach)(struct device *dev, bool power_off);
+       int (*activate)(struct device *dev);
+       void (*sync)(struct device *dev);
 };
 
 /*

Index: linux-pm/drivers/base/dd.c
===================================================================

drivers/base/dd.c +16 -0

@@ -279,6 +279,7 @@ static int really_probe(struct device *d
 {
        int ret = 0;
        int local_trigger_count = atomic_read(&deferred_trigger_count);
+       struct dev_pm_domain *pm_domain = dev->pm_domain;
 
        atomic_inc(&probe_count);
        pr_debug("bus: '%s': %s: probing driver %s with device %s\n",

@@ -298,6 +299,12 @@ static int really_probe(struct device *d
                goto probe_failed;
        }
 
+       if (pm_domain && pm_domain->activate) {
+               ret = pm_domain->activate(dev);
+               if (ret)
+                       goto probe_failed;
+       }
+
        if (dev->bus->probe) {
                ret = dev->bus->probe(dev);
                if (ret)

@@ -308,6 +315,9 @@ static int really_probe(struct device *d
                        goto probe_failed;
        }
 
+       if (pm_domain && pm_domain->sync)
+               pm_domain->sync(dev);
+
        driver_bound(dev);
        ret = 1;
        pr_debug("bus: '%s': %s: bound device %s to driver %s\n",

@@ -319,6 +329,8 @@ probe_failed:
        driver_sysfs_remove(dev);
        dev->driver = NULL;
        dev_set_drvdata(dev, NULL);
+       if (pm_domain && pm_domain->sync)
+               pm_domain->sync(dev);
 
        if (ret == -EPROBE_DEFER) {
                /* Driver requested deferred probing */

@@ -522,9 +534,13 @@ static void __device_release_driver(stru
                        dev->bus->remove(dev);
                else if (drv->remove)
                        drv->remove(dev);
+
                devres_release_all(dev);
                dev->driver = NULL;
                dev_set_drvdata(dev, NULL);
+               if (dev->pm_domain && dev->pm_domain->sync)
+                       dev->pm_domain->sync(dev);
+
                klist_remove(&dev->p->knode_driver);
                if (dev->bus)
                        blocking_notifier_call_chain(&dev->bus->p->bus_notifier,

Quoted 36 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>
Subject: driver core / PM: Add PM domain callbacks for device setup/cleanup

If PM domains are in use, it may be necessary to prepare the code handling a PM domain for driver probing. For example, in some cases device drivers rely on the ability to power on the devices with the help of the IO runtime PM framework and the PM domain code needs to be ready for that. Also, if that code has not been fully initialized yet, the driver probing should be deferred.

Moreover, after the probing is complete, it may be necessary to put the PM domain in question into the state reflecting the current needs of the devices in it, for example, so that power is not drawn in vain. The same should be done after removing a driver from a device, as the PM domain state may need to be changed to reflect the new situation.

For these reasons, introduce new PM domain callbacks, ->activate and ->sync, called, respectively, before probing for a device driver and after the probing has been completed or the driver has been removed.

Signed-off-by Rafael J. Wysocki <[email protected]>

---

Quoted 9 lines

---

This is an update without the additional bus type callbacks. This should be suffucient at this point for the use cases we have.

I've decided to also call ->sync on driver removal as that is consistent with what happens for failing probe.

---

One more update.

Quoted 4 lines

I've realized that in the PM domain's ->sync callback is called after clearing the device's driver field (in the failed probe or driver removal case) rather than before it, that could be used to distinguish between the two cases (successful probe vs no driver), so I reworked the patch accordingly.

That is very non-obvious. dev->drv belongs to driver core and maybe bus, nothing else should really be looking at it. Why can't we add an explicit argument to the callback indicating device state?

Quoted 42 lines · Dmitry Torokhov

Quoted 40 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>
Subject: driver core / PM: Add PM domain callbacks for device setup/cleanup

If PM domains are in use, it may be necessary to prepare the code handling a PM domain for driver probing. For example, in some cases device drivers rely on the ability to power on the devices with the help of the IO runtime PM framework and the PM domain code needs to be ready for that. Also, if that code has not been fully initialized yet, the driver probing should be deferred.

Moreover, after the probing is complete, it may be necessary to put the PM domain in question into the state reflecting the current needs of the devices in it, for example, so that power is not drawn in vain. The same should be done after removing a driver from a device, as the PM domain state may need to be changed to reflect the new situation.

For these reasons, introduce new PM domain callbacks, ->activate and ->sync, called, respectively, before probing for a device driver and after the probing has been completed or the driver has been removed.

Signed-off-by Rafael J. Wysocki <[email protected]>

---

Quoted 9 lines

---

This is an update without the additional bus type callbacks. This should be suffucient at this point for the use cases we have.

I've decided to also call ->sync on driver removal as that is consistent with what happens for failing probe.

---

One more update.

I've realized that in the PM domain's ->sync callback is called after clearing the device's driver field (in the failed probe or driver removal case) rather than before it, that could be used to distinguish between the two cases (successful probe vs no driver), so I reworked the patch accordingly.

Quoted 3 lines

That is very non-obvious. dev->drv belongs to driver core and maybe bus, nothing else should really be looking at it. Why can't we add an explicit argument to the callback indicating device state?

Well, if that's a concern, I'd rather introduce a separate callback to be run on driver removal/failing probe, like in the patch below, as it is then clear what the conditions for each callback's execution are (and one can always define them as wrappers around the same routine if need be).

I still think that this ordering of code is the right one, though.

Rafael

---
 drivers/base/dd.c  |   16 ++++++++++++++++
 include/linux/pm.h |    8 ++++++++
 2 files changed, 24 insertions(+)

Index: linux-pm/include/linux/pm.h
===================================================================

include/linux/pm.h +8 -0

@@ -603,10 +603,18 @@ extern void dev_pm_put_subsys_data(struc
  * Power domains provide callbacks that are executed during system suspend,
  * hibernation, system resume and during runtime PM transitions along with
  * subsystem-level and driver-level callbacks.
+ *
+ * @detach: Called when removing a device from the domain.
+ * @activate: Called before executing probe routines for bus types and drivers.
+ * @sync: Called after successful driver probe.
+ * @dismiss: Called after unsuccessful driver probe and after driver removal.
  */
 struct dev_pm_domain {
        struct dev_pm_ops       ops;
        void (*detach)(struct device *dev, bool power_off);
+       int (*activate)(struct device *dev);
+       void (*sync)(struct device *dev);
+       void (*dismiss)(struct device *dev);
 };
 
 /*

Index: linux-pm/drivers/base/dd.c
===================================================================

drivers/base/dd.c +16 -0

@@ -279,6 +279,7 @@ static int really_probe(struct device *d
 {
        int ret = 0;
        int local_trigger_count = atomic_read(&deferred_trigger_count);
+       struct dev_pm_domain *pm_domain = dev->pm_domain;
 
        atomic_inc(&probe_count);
        pr_debug("bus: '%s': %s: probing driver %s with device %s\n",

@@ -298,6 +299,12 @@ static int really_probe(struct device *d
                goto probe_failed;
        }
 
+       if (pm_domain && pm_domain->activate) {
+               ret = pm_domain->activate(dev);
+               if (ret)
+                       goto probe_failed;
+       }
+
        if (dev->bus->probe) {
                ret = dev->bus->probe(dev);
                if (ret)

@@ -308,6 +315,9 @@ static int really_probe(struct device *d
                        goto probe_failed;
        }
 
+       if (pm_domain && pm_domain->sync)
+               pm_domain->sync(dev);
+
        driver_bound(dev);
        ret = 1;
        pr_debug("bus: '%s': %s: bound device %s to driver %s\n",

@@ -319,6 +329,8 @@ probe_failed:
        driver_sysfs_remove(dev);
        dev->driver = NULL;
        dev_set_drvdata(dev, NULL);
+       if (pm_domain && pm_domain->dismiss)
+               pm_domain->dismiss(dev);
 
        if (ret == -EPROBE_DEFER) {
                /* Driver requested deferred probing */

@@ -522,9 +534,13 @@ static void __device_release_driver(stru
                        dev->bus->remove(dev);
                else if (drv->remove)
                        drv->remove(dev);
+
                devres_release_all(dev);
                dev->driver = NULL;
                dev_set_drvdata(dev, NULL);
+               if (dev->pm_domain && dev->pm_domain->dismiss)
+                       dev->pm_domain->dismiss(dev);
+
                klist_remove(&dev->p->knode_driver);
                if (dev->bus)
                        blocking_notifier_call_chain(&dev->bus->p->bus_notifier,

From: Rafael J. Wysocki <[email protected]>
Subject: driver core / PM: Add PM domain callbacks for device setup/cleanup

If PM domains are in use, it may be necessary to prepare the code handling a PM domain for driver probing. For example, in some cases device drivers rely on the ability to power on the devices with the help of the IO runtime PM framework and the PM domain code needs to be ready for that. Also, if that code has not been fully initialized yet, the driver probing should be deferred.

Moreover, after the probing is complete, it may be necessary to put the PM domain in question into the state reflecting the current needs of the devices in it, for example, so that power is not drawn in vain. The same should be done after removing a driver from a device, as the PM domain state may need to be changed to reflect the new situation.

For these reasons, introduce new PM domain callbacks, ->activate and ->sync, called, respectively, before probing for a device driver and after the probing has been completed or the driver has been removed.

Signed-off-by Rafael J. Wysocki <[email protected]>

---

Quoted 9 lines

---

This is an update without the additional bus type callbacks. This should be suffucient at this point for the use cases we have.

I've decided to also call ->sync on driver removal as that is consistent with what happens for failing probe.

---

One more update.

I've realized that in the PM domain's ->sync callback is called after clearing the device's driver field (in the failed probe or driver removal case) rather than before it, that could be used to distinguish between the two cases (successful probe vs no driver), so I reworked the patch accordingly.

---
 drivers/base/dd.c  |   16 ++++++++++++++++
 include/linux/pm.h |    6 ++++++
 2 files changed, 22 insertions(+)

Index: linux-pm/include/linux/pm.h
===================================================================

include/linux/pm.h +6 -0

@@ -603,10 +603,16 @@ extern void dev_pm_put_subsys_data(struc
  * Power domains provide callbacks that are executed during system suspend,
  * hibernation, system resume and during runtime PM transitions along with
  * subsystem-level and driver-level callbacks.
+ *
+ * @detach: Called when removing a device from the domain.
+ * @activate: Called before executing probe routines for bus types and drivers.
+ * @sync: Called after driver probe and removal.
  */
 struct dev_pm_domain {
        struct dev_pm_ops       ops;
        void (*detach)(struct device *dev, bool power_off);
+       int (*activate)(struct device *dev);
+       void (*sync)(struct device *dev);
 };
 
 /*

Index: linux-pm/drivers/base/dd.c
===================================================================

drivers/base/dd.c +16 -0

@@ -279,6 +279,7 @@ static int really_probe(struct device *d
 {
        int ret = 0;
        int local_trigger_count = atomic_read(&deferred_trigger_count);
+       struct dev_pm_domain *pm_domain = dev->pm_domain;
 
        atomic_inc(&probe_count);
        pr_debug("bus: '%s': %s: probing driver %s with device %s\n",

@@ -298,6 +299,12 @@ static int really_probe(struct device *d
                goto probe_failed;
        }
 
+       if (pm_domain && pm_domain->activate) {
+               ret = pm_domain->activate(dev);
+               if (ret)
+                       goto probe_failed;
+       }
+
        if (dev->bus->probe) {
                ret = dev->bus->probe(dev);
                if (ret)

@@ -308,6 +315,9 @@ static int really_probe(struct device *d
                        goto probe_failed;
        }
 
+       if (pm_domain && pm_domain->sync)
+               pm_domain->sync(dev);
+
        driver_bound(dev);
        ret = 1;
        pr_debug("bus: '%s': %s: bound device %s to driver %s\n",

@@ -319,6 +329,8 @@ probe_failed:
        driver_sysfs_remove(dev);
        dev->driver = NULL;
        dev_set_drvdata(dev, NULL);
+       if (pm_domain && pm_domain->sync)
+               pm_domain->sync(dev);
 
        if (ret == -EPROBE_DEFER) {
                /* Driver requested deferred probing */

@@ -522,9 +534,13 @@ static void __device_release_driver(stru
                        dev->bus->remove(dev);
                else if (drv->remove)
                        drv->remove(dev);
+
                devres_release_all(dev);
                dev->driver = NULL;
                dev_set_drvdata(dev, NULL);
+               if (dev->pm_domain && dev->pm_domain->sync)
+                       dev->pm_domain->sync(dev);
+
                klist_remove(&dev->p->knode_driver);
                if (dev->bus)
                        blocking_notifier_call_chain(&dev->bus->p->bus_notifier,

Quoted 22 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>
Subject: driver core / PM: Add PM domain callbacks for device setup/cleanup

If PM domains are in use, it may be necessary to prepare the code handling a PM domain for driver probing. For example, in some cases device drivers rely on the ability to power on the devices with the help of the IO runtime PM framework and the PM domain code needs to be ready for that. Also, if that code has not been fully initialized yet, the driver probing should be deferred.

Moreover, after the probing is complete, it may be necessary to put the PM domain in question into the state reflecting the current needs of the devices in it, for example, so that power is not drawn in vain. The same should be done after removing a driver from a device, as the PM domain state may need to be changed to reflect the new situation.

For these reasons, introduce new PM domain callbacks, ->activate and ->sync, called, respectively, before probing for a device driver and after the probing has been completed or the driver has been removed.

Quoted 1 line

Signed-off-by Rafael J. Wysocki <[email protected]>

This looks good to me!

Acked-by Ulf Hansson <[email protected]>

Quoted 61 lines

---

Quoted 9 lines

---

This is an update without the additional bus type callbacks. This should be suffucient at this point for the use cases we have.

I've decided to also call ->sync on driver removal as that is consistent with what happens for failing probe.

---

One more update.

I've realized that in the PM domain's ->sync callback is called after clearing the device's driver field (in the failed probe or driver removal case) rather than before it, that could be used to distinguish between the two cases (successful probe vs no driver), so I reworked the patch accordingly.

---
 drivers/base/dd.c  |   16 ++++++++++++++++
 include/linux/pm.h |    6 ++++++
 2 files changed, 22 insertions(+)

Index: linux-pm/include/linux/pm.h
===================================================================

include/linux/pm.h +6 -0

@@ -603,10 +603,16 @@ extern void dev_pm_put_subsys_data(struc
  * Power domains provide callbacks that are executed during system suspend,
  * hibernation, system resume and during runtime PM transitions along with
  * subsystem-level and driver-level callbacks.
+ *
+ * @detach: Called when removing a device from the domain.
+ * @activate: Called before executing probe routines for bus types and drivers.
+ * @sync: Called after driver probe and removal.
  */
 struct dev_pm_domain {
        struct dev_pm_ops       ops;
        void (*detach)(struct device *dev, bool power_off);
+       int (*activate)(struct device *dev);
+       void (*sync)(struct device *dev);
 };
 
 /*

Index: linux-pm/drivers/base/dd.c
===================================================================

drivers/base/dd.c +1 -0

@@ -279,6 +279,7 @@ static int really_probe(struct device *d
 {
        int ret = 0;
        int local_trigger_count = atomic_read(&deferred_trigger_count);
+       struct dev_pm_domain *pm_domain = dev->pm_domain;
 
        atomic_inc(&probe_count);
        pr_debug("bus: '%s': %s: probing driver %s with device %s\n",

@@ -298,6 +299,12 @@ static int really_probe(struct device *d
                goto probe_failed;
        }
 

drivers/base/dd.c static int really_probe(struct device *d · line 302

Quoted 1 line

drivers/base/dd.c +1 -0

+       if (pm_domain && pm_domain->activate) {

Quoted 34 lines

drivers/base/dd.c +11 -0

+               ret = pm_domain->activate(dev);
+               if (ret)
+                       goto probe_failed;
+       }
+
        if (dev->bus->probe) {
                ret = dev->bus->probe(dev);
                if (ret)

@@ -308,6 +315,9 @@ static int really_probe(struct device *d
                        goto probe_failed;
        }
 
+       if (pm_domain && pm_domain->sync)
+               pm_domain->sync(dev);
+
        driver_bound(dev);
        ret = 1;
        pr_debug("bus: '%s': %s: bound device %s to driver %s\n",

@@ -319,6 +329,8 @@ probe_failed:
        driver_sysfs_remove(dev);
        dev->driver = NULL;
        dev_set_drvdata(dev, NULL);
+       if (pm_domain && pm_domain->sync)
+               pm_domain->sync(dev);
 
        if (ret == -EPROBE_DEFER) {
                /* Driver requested deferred probing */

@@ -522,9 +534,13 @@ static void __device_release_driver(stru
                        dev->bus->remove(dev);
                else if (drv->remove)
                        drv->remove(dev);
+
                devres_release_all(dev);
                dev->driver = NULL;

drivers/base/dd.c static void __device_release_driver(stru · line 546

Quoted 8 lines

drivers/base/dd.c +3 -0

                dev_set_drvdata(dev, NULL);
+               if (dev->pm_domain && dev->pm_domain->sync)
+                       dev->pm_domain->sync(dev);
+
                klist_remove(&dev->p->knode_driver);
                if (dev->bus)
                        blocking_notifier_call_chain(&dev->bus->p->bus_notifier,

Quoted 23 lines · Ulf Hansson

Quoted 22 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>
Subject: driver core / PM: Add PM domain callbacks for device setup/cleanup

If PM domains are in use, it may be necessary to prepare the code handling a PM domain for driver probing. For example, in some cases device drivers rely on the ability to power on the devices with the help of the IO runtime PM framework and the PM domain code needs to be ready for that. Also, if that code has not been fully initialized yet, the driver probing should be deferred.

Moreover, after the probing is complete, it may be necessary to put the PM domain in question into the state reflecting the current needs of the devices in it, for example, so that power is not drawn in vain. The same should be done after removing a driver from a device, as the PM domain state may need to be changed to reflect the new situation.

For these reasons, introduce new PM domain callbacks, ->activate and ->sync, called, respectively, before probing for a device driver and after the probing has been completed or the driver has been removed.

Quoted 5 lines

Quoted 1 line

Signed-off-by Rafael J. Wysocki <[email protected]>

This looks good to me!

Acked-by Ulf Hansson <[email protected]>

I was a bit too quick acking this a minor fix is needed. See below.

Quoted 61 lines

Quoted 61 lines

---

Quoted 9 lines

---

This is an update without the additional bus type callbacks. This should be suffucient at this point for the use cases we have.

I've decided to also call ->sync on driver removal as that is consistent with what happens for failing probe.

---

One more update.

I've realized that in the PM domain's ->sync callback is called after clearing the device's driver field (in the failed probe or driver removal case) rather than before it, that could be used to distinguish between the two cases (successful probe vs no driver), so I reworked the patch accordingly.

---
 drivers/base/dd.c  |   16 ++++++++++++++++
 include/linux/pm.h |    6 ++++++
 2 files changed, 22 insertions(+)

Index: linux-pm/include/linux/pm.h
===================================================================

include/linux/pm.h +6 -0

@@ -603,10 +603,16 @@ extern void dev_pm_put_subsys_data(struc
  * Power domains provide callbacks that are executed during system suspend,
  * hibernation, system resume and during runtime PM transitions along with
  * subsystem-level and driver-level callbacks.
+ *
+ * @detach: Called when removing a device from the domain.
+ * @activate: Called before executing probe routines for bus types and drivers.
+ * @sync: Called after driver probe and removal.
  */
 struct dev_pm_domain {
        struct dev_pm_ops       ops;
        void (*detach)(struct device *dev, bool power_off);
+       int (*activate)(struct device *dev);
+       void (*sync)(struct device *dev);
 };
 
 /*

Index: linux-pm/drivers/base/dd.c
===================================================================

drivers/base/dd.c +1 -0

@@ -279,6 +279,7 @@ static int really_probe(struct device *d
 {
        int ret = 0;
        int local_trigger_count = atomic_read(&deferred_trigger_count);
+       struct dev_pm_domain *pm_domain = dev->pm_domain;
 
        atomic_inc(&probe_count);
        pr_debug("bus: '%s': %s: probing driver %s with device %s\n",

@@ -298,6 +299,12 @@ static int really_probe(struct device *d
                goto probe_failed;
        }
 

Quoted 1 line

drivers/base/dd.c static int really_probe(struct device *d · line 302

Quoted 1 line

drivers/base/dd.c +1 -0

+       if (pm_domain && pm_domain->activate) {

You need to re-fetch the pm_domain pointer at this point, since it will be updated during ->probe().

Maybe it's just safer to always do the following check:

if (dev->pm_domain && dev->pm_domain->activate|sync)

From: Rafael J. Wysocki <[email protected]>

If PM domains are in use, it may be necessary to prepare the code handling a PM domain for driver probing. For example, in some cases device drivers rely on the ability to power on the devices with the help of the IO runtime PM framework and the PM domain code needs to be ready for that. Also, if that code has not been fully initialized yet, the driver probing should be deferred.

Moreover, after the probing is complete, it may be necessary to put the PM domain in question into the state reflecting the current needs of the devices in it, for example, so that power is not drawn in vain. The same should be done after removing a driver from a device, as the PM domain state may need to be changed to reflect the new situation.

For these reasons, introduce new PM domain callbacks, ->activate, ->sync and ->dismiss called, respectively, before probing for a device driver, after the probing has completed successfully and if the probing has failed or the driver has been removed.

Signed-off-by Rafael J. Wysocki <[email protected]>

---

One more update taking Dmitry's comments into account and fixing an overoptimization bug spotted by Ulf.

And this actually contains the patch this time (clicked on "Send" too early last time, sorry about that).

---
 drivers/base/dd.c  |   14 ++++++++++++++
 include/linux/pm.h |    8 ++++++++
 2 files changed, 22 insertions(+)

Index: linux-pm/include/linux/pm.h
===================================================================

include/linux/pm.h +8 -0

@@ -603,10 +603,18 @@ extern void dev_pm_put_subsys_data(struc
  * Power domains provide callbacks that are executed during system suspend,
  * hibernation, system resume and during runtime PM transitions along with
  * subsystem-level and driver-level callbacks.
+ *
+ * @detach: Called when removing a device from the domain.
+ * @activate: Called before executing probe routines for bus types and drivers.
+ * @sync: Called after successful driver probe.
+ * @dismiss: Called after unsuccessful driver probe and after driver removal.
  */
 struct dev_pm_domain {
        struct dev_pm_ops       ops;
        void (*detach)(struct device *dev, bool power_off);
+       int (*activate)(struct device *dev);
+       void (*sync)(struct device *dev);
+       void (*dismiss)(struct device *dev);
 };
 
 /*

Index: linux-pm/drivers/base/dd.c
===================================================================

drivers/base/dd.c +14 -0

@@ -298,6 +298,12 @@ static int really_probe(struct device *d
                goto probe_failed;
        }
 
+       if (dev->pm_domain && dev->pm_domain->activate) {
+               ret = dev->pm_domain->activate(dev);
+               if (ret)
+                       goto probe_failed;
+       }
+
        if (dev->bus->probe) {
                ret = dev->bus->probe(dev);
                if (ret)

@@ -308,6 +314,9 @@ static int really_probe(struct device *d
                        goto probe_failed;
        }
 
+       if (dev->pm_domain && dev->pm_domain->sync)
+               dev->pm_domain->sync(dev);
+
        driver_bound(dev);
        ret = 1;
        pr_debug("bus: '%s': %s: bound device %s to driver %s\n",

@@ -319,6 +328,8 @@ probe_failed:
        driver_sysfs_remove(dev);
        dev->driver = NULL;
        dev_set_drvdata(dev, NULL);
+       if (dev->pm_domain && dev->pm_domain->dismiss)
+               dev->pm_domain->dismiss(dev);
 
        if (ret == -EPROBE_DEFER) {
                /* Driver requested deferred probing */

@@ -525,6 +536,9 @@ static void __device_release_driver(stru
                devres_release_all(dev);
                dev->driver = NULL;
                dev_set_drvdata(dev, NULL);
+               if (dev->pm_domain && dev->pm_domain->dismiss)
+                       dev->pm_domain->dismiss(dev);
+
                klist_remove(&dev->p->knode_driver);
                if (dev->bus)
                        blocking_notifier_call_chain(&dev->bus->p->bus_notifier,

From: Rafael J. Wysocki <[email protected]>

If PM domains are in use, it may be necessary to prepare the code handling a PM domain for driver probing. For example, in some cases device drivers rely on the ability to power on the devices with the help of the IO runtime PM framework and the PM domain code needs to be ready for that. Also, if that code has not been fully initialized yet, the driver probing should be deferred.

Moreover, after the probing is complete, it may be necessary to put the PM domain in question into the state reflecting the current needs of the devices in it, for example, so that power is not drawn in vain. The same should be done after removing a driver from a device, as the PM domain state may need to be changed to reflect the new situation.

For these reasons, introduce new PM domain callbacks, ->activate, ->sync and ->dismiss called, respectively, before probing for a device driver, after the probing has completed successfully and if the probing has failed or the driver has been removed.

Signed-off-by Rafael J. Wysocki <[email protected]>

---

One more update taking Dmitry's comments into account and fixing an overoptimization bug spotted by Ulf.

And this actually contains the patch this time (clicked on "Send" too early last time, sorry about that).

From: Rafael J. Wysocki <[email protected]>

If PM domains are in use, it may be necessary to prepare the code handling a PM domain for driver probing. For example, in some cases device drivers rely on the ability to power on the devices with the help of the IO runtime PM framework and the PM domain code needs to be ready for that. Also, if that code has not been fully initialized yet, the driver probing should be deferred.

Moreover, after the probing is complete, it may be necessary to put the PM domain in question into the state reflecting the current needs of the devices in it, for example, so that power is not drawn in vain. The same should be done after removing a driver from a device, as the PM domain state may need to be changed to reflect the new situation.

For these reasons, introduce new PM domain callbacks, ->activate, ->sync and ->dismiss called, respectively, before probing for a device driver, after the probing has completed successfully and if the probing has failed or the driver has been removed.

Signed-off-by Rafael J. Wysocki <[email protected]>

---

One more update taking Dmitry's comments into account and fixing an overoptimization bug spotted by Ulf.

And this actually contains the patch this time (clicked on "Send" too early last time, sorry about that).

---
 drivers/base/dd.c  |   14 ++++++++++++++
 include/linux/pm.h |    8 ++++++++
 2 files changed, 22 insertions(+)

Index: linux-pm/include/linux/pm.h
===================================================================

include/linux/pm.h +8 -0

@@ -603,10 +603,18 @@ extern void dev_pm_put_subsys_data(struc
  * Power domains provide callbacks that are executed during system suspend,
  * hibernation, system resume and during runtime PM transitions along with
  * subsystem-level and driver-level callbacks.
+ *
+ * @detach: Called when removing a device from the domain.
+ * @activate: Called before executing probe routines for bus types and drivers.
+ * @sync: Called after successful driver probe.
+ * @dismiss: Called after unsuccessful driver probe and after driver removal.
  */
 struct dev_pm_domain {
 	struct dev_pm_ops	ops;
 	void (*detach)(struct device *dev, bool power_off);
+	int (*activate)(struct device *dev);
+	void (*sync)(struct device *dev);
+	void (*dismiss)(struct device *dev);
 };
 
 /*

Index: linux-pm/drivers/base/dd.c
===================================================================

drivers/base/dd.c +14 -1

@@ -298,6 +298,12 @@ static int really_probe(struct device *d
 		goto probe_failed;
 	}
 
+	if (dev->pm_domain && dev->pm_domain->activate) {
+		ret = dev->pm_domain->activate(dev);
+		if (ret)
+			goto probe_failed;
+	}
+
 	if (dev->bus->probe) {
 		ret = dev->bus->probe(dev);
 		if (ret)

@@ -308,6 +314,9 @@ static int really_probe(struct device *d
 			goto probe_failed;
 	}
 
+	if (dev->pm_domain && dev->pm_domain->sync)
+		dev->pm_domain->sync(dev);
+
 	driver_bound(dev);
 	ret = 1;
 	pr_debug("bus: '%s': %s: bound device %s to driver %s\n",

@@ -319,6 +328,8 @@ probe_failed:
 	driver_sysfs_remove(dev);
 	dev->driver = NULL;
 	dev_set_drvdata(dev, NULL);
+	if (dev->pm_domain && dev->pm_domain->dismiss)
+		dev->pm_domain->dismiss(dev);
 
 	if (ret == -EPROBE_DEFER) {
 		/* Driver requested deferred probing */

@@ -525,6 +536,9 @@ static void __device_release_driver(stru
 		devres_release_all(dev);
 		dev->driver = NULL;
 		dev_set_drvdata(dev, NULL);
+		if (dev->pm_domain && dev->pm_domain->dismiss)
+			dev->pm_domain->dismiss(dev);
+
 		klist_remove(&dev->p->knode_driver);
 		if (dev->bus)
 			blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
 
--

To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 27 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

If PM domains are in use, it may be necessary to prepare the code handling a PM domain for driver probing. For example, in some cases device drivers rely on the ability to power on the devices with the help of the IO runtime PM framework and the PM domain code needs to be ready for that. Also, if that code has not been fully initialized yet, the driver probing should be deferred.

Moreover, after the probing is complete, it may be necessary to put the PM domain in question into the state reflecting the current needs of the devices in it, for example, so that power is not drawn in vain. The same should be done after removing a driver from a device, as the PM domain state may need to be changed to reflect the new situation.

For these reasons, introduce new PM domain callbacks, ->activate, ->sync and ->dismiss called, respectively, before probing for a device driver, after the probing has completed successfully and if the probing has failed or the driver has been removed.

Signed-off-by Rafael J. Wysocki <[email protected]>

---

One more update taking Dmitry's comments into account and fixing an overoptimization bug spotted by Ulf.

Quoted 2 lines

And this actually contains the patch this time (clicked on "Send" too early last time, sorry about that).

Hi Greg,

Since everybody seems to like this one, do you have any objectctions against it?

If not, would it be a problem if I queued it up for 4.1?

Quoted 5 lines

There are two alternatives to them. One is to do PM domain attach/detach in the bus type's ->probe and ->remove, but that's suboptimal, because it is then carried out every time a driver is probed/removed for a device. The other one would be to have each interested bus type register a bus type notifier for itself, but that would be rather ugly, wouldn't it?

Driver probing and removal is not a hot path. Successful probes usually occur only once for each device. I don't know how many unsuccessful probes there are on average, but probably not many.

Quoted 208 lines · Rafael J. Wysocki

Quoted 207 lines · Ulf Hansson

Quoted 205 lines · Rafael J. Wysocki

Quoted 186 lines · Ulf Hansson

Quoted 114 lines · Rafael J. Wysocki

Quoted 104 lines · Ulf Hansson

Quoted 82 lines · Rafael J. Wysocki

Quoted 81 lines · Ulf Hansson

There may be more than one device in a PM domain which then will be probed at different points in time.

Depending on timing and runtime PM support, in for the device related driver/subsystem, a PM domain may be advised to power off after a successful probe sequence.

A general requirement for a device within a PM domain, is that the PM domain must stay powered during the probe sequence. To cope with such requirement, let's add the dev_pm_domain_get|put() APIs.

These APIs are intended to be invoked from subsystem-level code and the calls between get/put needs to be balanced.

dev_pm_domain_get(), tells the PM domain that it needs to increase a usage count and to keep supplying power. dev_pm_domain_put(), does the opposite.

For a PM domain to support this feature, it must implement the optional ->get|put() callbacks.

Signed-off-by Ulf Hansson <[email protected]>

---
 drivers/base/power/common.c | 40 ++++++++++++++++++++++++++++++++++++++++
 include/linux/pm.h          |  2 ++
 include/linux/pm_domain.h   |  4 ++++
 3 files changed, 46 insertions(+)

drivers/base/power/common.c +40 -0

index f32b802..99225af 100644
@@ -128,3 +128,43 @@ void dev_pm_domain_detach(struct device *dev, bool power_off)
              dev->pm_domain->detach(dev, power_off);
 }
 EXPORT_SYMBOL_GPL(dev_pm_domain_detach);
+
+/**
+ * dev_pm_domain_get - Increase usage count to keep a PM domain powered.
+ * @domain: The PM domain to operate on.
+ *
+ * This function will not by itself increase the usage count, that's up to each
+ * PM domain implementation to support. Typically it should be invoked from
+ * subsystem level code prior drivers starts probing.
+ *
+ * Do note, it's optional to implement the ->get() callback for a PM domain.
+ *
+ * Returns 0 on successfully increased usage count or negative error code.
+ */
+int dev_pm_domain_get(struct dev_pm_domain *domain)
+{
+     int ret = 0;
+
+     if (domain && domain->get)
+             ret = domain->get(domain);
+
+     return ret;
+}
+EXPORT_SYMBOL_GPL(dev_pm_domain_get);
+
+/**
+ * dev_pm_domain_put - Decrease usage count to allow a PM domain to power off.
+ * @domain: The PM domain to operate on.
+ *
+ * This function will not by itself decrease the usage count, that's up to each
+ * PM domain implementation to support. Typically it should be invoked from
+ * subsystem level code after drivers has finished probing.
+ *
+ * Do note, it's optional to implement the ->put() callback for a PM domain.
+ */
+void dev_pm_domain_put(struct dev_pm_domain *domain)
+{
+     if (domain && domain->put)
+             domain->put(domain);
+}
+EXPORT_SYMBOL_GPL(dev_pm_domain_put);

include/linux/pm.h +0 -0

index e2f1be6..e62330b 100644
@@ -607,6 +607,8 @@ extern void dev_pm_put_subsys_data(struct device *dev);

Quoted 8 lines

include/linux/pm.h extern void dev_pm_put_subsys_data(struc

Quoted 5 lines

include/linux/pm.h +2 -0

 struct dev_pm_domain {
      struct dev_pm_ops       ops;
      void (*detach)(struct device *dev, bool power_off);
+     int (*get)(struct dev_pm_domain *domain);
+     void (*put)(struct dev_pm_domain *domain);

I don't like these names. They suggest that it's always going to be about reference counting which doesn't have to be the case in principle.

I am happy to change, you don't happen to have a proposal? :-)

For genpd we already have these related APIs:

pm_genpd_poweron()
pm_genpd_name_poweron()
pm_genpd_poweroff_unused()

Theoretically we should be able to replace these with dev_pm_domain_get|put() or whatever we decide to name them to.

Quoted 1 line

Quoted 8 lines

Quoted 1 line

Also what about calling these from the driver core, ie. really_probe()?

I like that!

That also implies moving the calls to dev_pm_domain_attach|detach() out of the buses and into the drivercore, since we need the PM domain to be attached before calling dev_pm_domain_get|put(). I assume that's what you also propose me to change, right?

Quoted 7 lines

Not really. I don't want to inflict the ACPI PM domain on every bus type that may not be prepared for using it or even may not want to use it (like PCI or PNP). That applies to genpd too to some extent.

So bus types need to be able to opt in for using "default" PM domains, but at the same time if they do, the core is a better place to invoke the callbacks.

Okay!

Quoted 7 lines

The patch below shows how that can be done. For the bus types using genpd or the ACPI PM domain today ->pm_domain_init and ->pm_domain_exit may point to the routines initializing/detaching those (which also will help to reduce code duplication somewhat) and that guarantees that the domains will be attached to devices before probing and the core can do the ->pre_probe and ->post_probe things for everybody.

Overall, this seem like a very good idea!

Quoted 33 lines

Rafael

---
 drivers/base/bus.c     |   12 +++++++++++-
 drivers/base/dd.c      |    9 +++++++++
 include/linux/device.h |    3 +++
 include/linux/pm.h     |    2 ++
 4 files changed, 25 insertions(+), 1 deletion(-)

Index: linux-pm/drivers/base/bus.c
===================================================================

drivers/base/bus.c +6 -1

@@ -509,10 +509,15 @@ int bus_add_device(struct device *dev)
        int error = 0;
 
        if (bus) {
+               if (bus->pm_domain_init) {
+                       error = bus->pm_domain_init(dev);
+                       if (error)
+                               goto out_put;
+               }
                pr_debug("bus: '%s': add device %s\n", bus->name, dev_name(dev));
                error = device_add_attrs(bus, dev);
                if (error)
-                       goto out_put;
+                       goto out_pm_domain;
                error = device_add_groups(dev, bus->dev_groups);
                if (error)
                        goto out_groups;

drivers/base/bus.c out_groups: · line 547

Quoted 9 lines

drivers/base/bus.c +3 -0

@@ -534,6 +539,9 @@ out_groups:
        device_remove_groups(dev, bus->dev_groups);
 out_id:
        device_remove_attrs(bus, dev);
+out_pm_domain:
+       if (bus->pm_domain_exit)
+               bus->pm_domain_exit(dev);
 out_put:
        bus_put(dev->bus);
        return error;

To make this work for bus_add_device(), we first need to change the registration procedure for genpd DT providers. Currently that's done when "PM domain drivers" invokes the __of_genpd_add_provider() API, from SoC specific code and from different init levels.

We need to have the available gendp DT providers registered when the ->pm_domain_init() callback is invoked.

Quoted 7 lines

Besides the changes above, genpd also needs to deal with attached devices, but which don't have a corresponding driver bound. I believe we have some corner cases to fix around this as well.

As an intermediate step, how about adding the similar code as above but into really_probe()? For the code in bus_remove_device() below, we could add that into __device_release_driver()?

Well, I wouldn't really like to add new callbacks to struct bus_type for intermediate steps, because that's guaranteed to lead to confusion.

So I think the infrastructure is better added first and users may be switched over to it gradually.

I don't see any particular problems with moving the ACPI PM domain attach/detach to bus_add/remove_device(), so that can be done as the first step. As for genpd, it can implement the ->post_probe thing first and do the rest in the bus type ->probe until the generic code is ready.