It's been a while since the introduction for the support for multi PM domains
per device in genpd. In this small series, a couple of different improvement
are being made to this code in genpd.
Ulf Hansson (3):
PM / Domains: Don't kfree() the virtual device in the error path
PM / Domains: Allow OF lookup for multi PM domain case from
->attach_dev()
PM / Domains: Enable genpd_dev_pm_attach_by_id|name() for single PM
domain
drivers/base/power/domain.c | 18 ++++++++++--------
1 file changed, 10 insertions(+), 8 deletions(-)
--
2.17.1
A genpd provider that uses the ->attach_dev() callback to lookup resources
for a device, fails to do so when the device has multiple PM domains. This
is because when genpd invokes the ->attach_dev() callback, it passes the
allocated virtual device as the in-parameter.
To address this problem, let's simply assign the dev->of_node for the
virtual device, based upon the original device's OF node.
Signed-off-by: Ulf Hansson <[email protected]>
---
drivers/base/power/domain.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 7fec69aebf46..801f31c87d16 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -2345,6 +2345,7 @@ EXPORT_SYMBOL_GPL(of_genpd_remove_last);
static void genpd_release_dev(struct device *dev)
{
+ of_node_put(dev->of_node);
kfree(dev);
}
@@ -2406,14 +2407,14 @@ static void genpd_dev_pm_sync(struct device *dev)
genpd_queue_power_off_work(pd);
}
-static int __genpd_dev_pm_attach(struct device *dev, struct device_node *np,
- unsigned int index, bool power_on)
+static int __genpd_dev_pm_attach(struct device *dev, unsigned int index,
+ bool power_on)
{
struct of_phandle_args pd_args;
struct generic_pm_domain *pd;
int ret;
- ret = of_parse_phandle_with_args(np, "power-domains",
+ ret = of_parse_phandle_with_args(dev->of_node, "power-domains",
"#power-domain-cells", index, &pd_args);
if (ret < 0)
return ret;
@@ -2481,7 +2482,7 @@ int genpd_dev_pm_attach(struct device *dev)
"#power-domain-cells") != 1)
return 0;
- return __genpd_dev_pm_attach(dev, dev->of_node, 0, true);
+ return __genpd_dev_pm_attach(dev, 0, true);
}
EXPORT_SYMBOL_GPL(genpd_dev_pm_attach);
@@ -2525,6 +2526,7 @@ struct device *genpd_dev_pm_attach_by_id(struct device *dev,
dev_set_name(virt_dev, "genpd:%u:%s", index, dev_name(dev));
virt_dev->bus = &genpd_bus_type;
virt_dev->release = genpd_release_dev;
+ virt_dev->of_node = of_node_get(dev->of_node);
ret = device_register(virt_dev);
if (ret) {
@@ -2533,7 +2535,7 @@ struct device *genpd_dev_pm_attach_by_id(struct device *dev,
}
/* Try to attach the device to the PM domain at the specified index. */
- ret = __genpd_dev_pm_attach(virt_dev, dev->of_node, index, false);
+ ret = __genpd_dev_pm_attach(virt_dev, index, false);
if (ret < 1) {
device_unregister(virt_dev);
return ret ? ERR_PTR(ret) : NULL;
--
2.17.1
If a call to dev_pm_domain_attach() succeeds to attach a device to its
single PM domain, the important point is to prevent following calls to
dev_pm_domain_attach_by_name|id() to fail. This is managed by checking the
dev->pm_domain pointer and then return -EEXIST, rather than continue
calling genpd_dev_pm_attach_by_id|name().
For this reason, let's enable genpd_dev_pm_attach_by_id|name() to be used
for also single PM domains. This simplifies for future users to solely make
use of dev_pm_domain_attach_by_id|name() rather than having to combine it
with dev_pm_domain_attach().
Signed-off-by: Ulf Hansson <[email protected]>
---
drivers/base/power/domain.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 801f31c87d16..1b026704a8fc 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -2512,10 +2512,10 @@ struct device *genpd_dev_pm_attach_by_id(struct device *dev,
if (!dev->of_node)
return NULL;
- /* Deal only with devices using multiple PM domains. */
+ /* Verify that the index is within a valid range. */
num_domains = of_count_phandle_with_args(dev->of_node, "power-domains",
"#power-domain-cells");
- if (num_domains < 2 || index >= num_domains)
+ if (index >= num_domains)
return NULL;
/* Allocate and register device on the genpd bus. */
--
2.17.1
It's not correct to call kfree(dev) when device_register(dev) has failed.
Fix this by calling put_device(dev) instead.
Fixes: 3c095f32a92b ("PM / Domains: Add support for multi PM domains per device to genpd")
Signed-off-by: Ulf Hansson <[email protected]>
---
drivers/base/power/domain.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index ecac03dcc9b2..7fec69aebf46 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -2528,7 +2528,7 @@ struct device *genpd_dev_pm_attach_by_id(struct device *dev,
ret = device_register(virt_dev);
if (ret) {
- kfree(virt_dev);
+ put_device(virt_dev);
return ERR_PTR(ret);
}
--
2.17.1
On 18-04-19, 12:27, Ulf Hansson wrote:
> It's not correct to call kfree(dev) when device_register(dev) has failed.
> Fix this by calling put_device(dev) instead.
>
> Fixes: 3c095f32a92b ("PM / Domains: Add support for multi PM domains per device to genpd")
> Signed-off-by: Ulf Hansson <[email protected]>
> ---
> drivers/base/power/domain.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index ecac03dcc9b2..7fec69aebf46 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -2528,7 +2528,7 @@ struct device *genpd_dev_pm_attach_by_id(struct device *dev,
>
> ret = device_register(virt_dev);
> if (ret) {
> - kfree(virt_dev);
> + put_device(virt_dev);
> return ERR_PTR(ret);
> }
Acked-by: Viresh Kumar <[email protected]>
--
viresh
On 18-04-19, 12:27, Ulf Hansson wrote:
> A genpd provider that uses the ->attach_dev() callback to lookup resources
> for a device, fails to do so when the device has multiple PM domains. This
> is because when genpd invokes the ->attach_dev() callback, it passes the
> allocated virtual device as the in-parameter.
>
> To address this problem, let's simply assign the dev->of_node for the
> virtual device, based upon the original device's OF node.
>
> Signed-off-by: Ulf Hansson <[email protected]>
> ---
> drivers/base/power/domain.c | 12 +++++++-----
> 1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 7fec69aebf46..801f31c87d16 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -2345,6 +2345,7 @@ EXPORT_SYMBOL_GPL(of_genpd_remove_last);
>
> static void genpd_release_dev(struct device *dev)
> {
> + of_node_put(dev->of_node);
> kfree(dev);
> }
>
> @@ -2406,14 +2407,14 @@ static void genpd_dev_pm_sync(struct device *dev)
> genpd_queue_power_off_work(pd);
> }
>
> -static int __genpd_dev_pm_attach(struct device *dev, struct device_node *np,
> - unsigned int index, bool power_on)
> +static int __genpd_dev_pm_attach(struct device *dev, unsigned int index,
> + bool power_on)
> {
> struct of_phandle_args pd_args;
> struct generic_pm_domain *pd;
> int ret;
>
> - ret = of_parse_phandle_with_args(np, "power-domains",
> + ret = of_parse_phandle_with_args(dev->of_node, "power-domains",
> "#power-domain-cells", index, &pd_args);
> if (ret < 0)
> return ret;
> @@ -2481,7 +2482,7 @@ int genpd_dev_pm_attach(struct device *dev)
> "#power-domain-cells") != 1)
> return 0;
>
> - return __genpd_dev_pm_attach(dev, dev->of_node, 0, true);
> + return __genpd_dev_pm_attach(dev, 0, true);
> }
> EXPORT_SYMBOL_GPL(genpd_dev_pm_attach);
>
> @@ -2525,6 +2526,7 @@ struct device *genpd_dev_pm_attach_by_id(struct device *dev,
> dev_set_name(virt_dev, "genpd:%u:%s", index, dev_name(dev));
> virt_dev->bus = &genpd_bus_type;
> virt_dev->release = genpd_release_dev;
> + virt_dev->of_node = of_node_get(dev->of_node);
>
> ret = device_register(virt_dev);
> if (ret) {
> @@ -2533,7 +2535,7 @@ struct device *genpd_dev_pm_attach_by_id(struct device *dev,
> }
>
> /* Try to attach the device to the PM domain at the specified index. */
> - ret = __genpd_dev_pm_attach(virt_dev, dev->of_node, index, false);
> + ret = __genpd_dev_pm_attach(virt_dev, index, false);
> if (ret < 1) {
> device_unregister(virt_dev);
> return ret ? ERR_PTR(ret) : NULL;
Acked-by: Viresh Kumar <[email protected]>
--
viresh
On 18-04-19, 12:27, Ulf Hansson wrote:
> If a call to dev_pm_domain_attach() succeeds to attach a device to its
> single PM domain, the important point is to prevent following calls to
> dev_pm_domain_attach_by_name|id() to fail. This is managed by checking the
> dev->pm_domain pointer and then return -EEXIST, rather than continue
> calling genpd_dev_pm_attach_by_id|name().
>
> For this reason, let's enable genpd_dev_pm_attach_by_id|name() to be used
> for also single PM domains. This simplifies for future users to solely make
> use of dev_pm_domain_attach_by_id|name() rather than having to combine it
> with dev_pm_domain_attach().
>
> Signed-off-by: Ulf Hansson <[email protected]>
> ---
> drivers/base/power/domain.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 801f31c87d16..1b026704a8fc 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -2512,10 +2512,10 @@ struct device *genpd_dev_pm_attach_by_id(struct device *dev,
> if (!dev->of_node)
> return NULL;
>
> - /* Deal only with devices using multiple PM domains. */
> + /* Verify that the index is within a valid range. */
> num_domains = of_count_phandle_with_args(dev->of_node, "power-domains",
> "#power-domain-cells");
> - if (num_domains < 2 || index >= num_domains)
> + if (index >= num_domains)
> return NULL;
>
> /* Allocate and register device on the genpd bus. */
Acked-by: Viresh Kumar <[email protected]>
--
viresh
On Thu, Apr 18, 2019 at 12:27:57PM +0200, Ulf Hansson wrote:
> If a call to dev_pm_domain_attach() succeeds to attach a device to its
> single PM domain, the important point is to prevent following calls to
nit: s/prevent/ensure/
> dev_pm_domain_attach_by_name|id() to fail. This is managed by checking the
> dev->pm_domain pointer and then return -EEXIST, rather than continue
> calling genpd_dev_pm_attach_by_id|name().
>
> For this reason, let's enable genpd_dev_pm_attach_by_id|name() to be used
> for also single PM domains. This simplifies for future users to solely make
> use of dev_pm_domain_attach_by_id|name() rather than having to combine it
> with dev_pm_domain_attach().
>
> Signed-off-by: Ulf Hansson <[email protected]>
> ---
> drivers/base/power/domain.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 801f31c87d16..1b026704a8fc 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -2512,10 +2512,10 @@ struct device *genpd_dev_pm_attach_by_id(struct device *dev,
> if (!dev->of_node)
> return NULL;
>
> - /* Deal only with devices using multiple PM domains. */
> + /* Verify that the index is within a valid range. */
> num_domains = of_count_phandle_with_args(dev->of_node, "power-domains",
> "#power-domain-cells");
> - if (num_domains < 2 || index >= num_domains)
> + if (index >= num_domains)
> return NULL;
>
> /* Allocate and register device on the genpd bus. */
> --
> 2.17.1
>
Acked-by: Niklas Cassel <[email protected]>
On Thu, Apr 18, 2019 at 12:27:56PM +0200, Ulf Hansson wrote:
> A genpd provider that uses the ->attach_dev() callback to lookup resources
> for a device, fails to do so when the device has multiple PM domains. This
> is because when genpd invokes the ->attach_dev() callback, it passes the
> allocated virtual device as the in-parameter.
>
> To address this problem, let's simply assign the dev->of_node for the
> virtual device, based upon the original device's OF node.
>
> Signed-off-by: Ulf Hansson <[email protected]>
> ---
> drivers/base/power/domain.c | 12 +++++++-----
> 1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 7fec69aebf46..801f31c87d16 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -2345,6 +2345,7 @@ EXPORT_SYMBOL_GPL(of_genpd_remove_last);
>
> static void genpd_release_dev(struct device *dev)
> {
> + of_node_put(dev->of_node);
> kfree(dev);
> }
>
> @@ -2406,14 +2407,14 @@ static void genpd_dev_pm_sync(struct device *dev)
> genpd_queue_power_off_work(pd);
> }
>
> -static int __genpd_dev_pm_attach(struct device *dev, struct device_node *np,
> - unsigned int index, bool power_on)
> +static int __genpd_dev_pm_attach(struct device *dev, unsigned int index,
> + bool power_on)
> {
> struct of_phandle_args pd_args;
> struct generic_pm_domain *pd;
> int ret;
>
> - ret = of_parse_phandle_with_args(np, "power-domains",
> + ret = of_parse_phandle_with_args(dev->of_node, "power-domains",
> "#power-domain-cells", index, &pd_args);
> if (ret < 0)
> return ret;
> @@ -2481,7 +2482,7 @@ int genpd_dev_pm_attach(struct device *dev)
> "#power-domain-cells") != 1)
> return 0;
>
> - return __genpd_dev_pm_attach(dev, dev->of_node, 0, true);
> + return __genpd_dev_pm_attach(dev, 0, true);
> }
> EXPORT_SYMBOL_GPL(genpd_dev_pm_attach);
>
> @@ -2525,6 +2526,7 @@ struct device *genpd_dev_pm_attach_by_id(struct device *dev,
> dev_set_name(virt_dev, "genpd:%u:%s", index, dev_name(dev));
> virt_dev->bus = &genpd_bus_type;
> virt_dev->release = genpd_release_dev;
> + virt_dev->of_node = of_node_get(dev->of_node);
>
> ret = device_register(virt_dev);
> if (ret) {
> @@ -2533,7 +2535,7 @@ struct device *genpd_dev_pm_attach_by_id(struct device *dev,
> }
>
> /* Try to attach the device to the PM domain at the specified index. */
> - ret = __genpd_dev_pm_attach(virt_dev, dev->of_node, index, false);
> + ret = __genpd_dev_pm_attach(virt_dev, index, false);
> if (ret < 1) {
> device_unregister(virt_dev);
> return ret ? ERR_PTR(ret) : NULL;
> --
> 2.17.1
>
Acked-by: Niklas Cassel <[email protected]>
On Thu, Apr 18, 2019 at 12:27:55PM +0200, Ulf Hansson wrote:
> It's not correct to call kfree(dev) when device_register(dev) has failed.
> Fix this by calling put_device(dev) instead.
>
> Fixes: 3c095f32a92b ("PM / Domains: Add support for multi PM domains per device to genpd")
> Signed-off-by: Ulf Hansson <[email protected]>
> ---
> drivers/base/power/domain.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index ecac03dcc9b2..7fec69aebf46 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -2528,7 +2528,7 @@ struct device *genpd_dev_pm_attach_by_id(struct device *dev,
>
> ret = device_register(virt_dev);
> if (ret) {
> - kfree(virt_dev);
> + put_device(virt_dev);
> return ERR_PTR(ret);
> }
>
> --
> 2.17.1
>
Acked-by: Niklas Cassel <[email protected]>
On Thursday, April 18, 2019 12:27:54 PM CEST Ulf Hansson wrote:
> It's been a while since the introduction for the support for multi PM domains
> per device in genpd. In this small series, a couple of different improvement
> are being made to this code in genpd.
>
> Ulf Hansson (3):
> PM / Domains: Don't kfree() the virtual device in the error path
> PM / Domains: Allow OF lookup for multi PM domain case from
> ->attach_dev()
> PM / Domains: Enable genpd_dev_pm_attach_by_id|name() for single PM
> domain
>
> drivers/base/power/domain.c | 18 ++++++++++--------
> 1 file changed, 10 insertions(+), 8 deletions(-)
All [1-3/3] applied, thanks!