diff mbox

[v2] PM / Domains: defer dev_pm_domain_set() until genpd->attach_dev succeeds if present

Message ID 1500029508-7103-1-git-send-email-sudeep.holla@arm.com (mailing list archive)
State Accepted, archived
Delegated to: Rafael Wysocki
Headers show

Commit Message

Sudeep Holla July 14, 2017, 10:51 a.m. UTC 
If the genpd->attach_dev or genpd->power_on fails, genpd_dev_pm_attach
may return -EPROBE_DEFER initially. However genpd_alloc_dev_data sets
the PM domain for the device unconditionally.

When subsequent attempts are made to call genpd_dev_pm_attach, it may
return -EEXISTS checking dev->pm_domain without re-attempting to call
attach_dev or power_on.

platform_drv_probe then attempts to call drv->probe as the return value
-EEXIST != -EPROBE_DEFER, which may end up in a situation where the
device is accessed without it's power domain switched on.

Fixes: f104e1e5ef57 ("PM / Domains: Re-order initialization of generic_pm_domain_data")
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Kevin Hilman <khilman@kernel.org>
Cc: Ulf Hansson <ulf.hansson@linaro.org>
Cc: <stable@vger.kernel.org> # v4.0+
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/base/power/domain.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Hi,

I have added Cc/Fixes tags based on the original commit that caused the issue.
I need to workout a patch that can cleanly apply before commit 989561de9b51
("PM / Domains: add setter for dev.pm_domain"), i.e. for v4.0.x to v4.4.x
This patch should apply from v4.5.x onwards.

Let me know if I got anything wrong.

v1->v2: Moved dev_pm_domain_set(dev, NULL) from genpd_free_dev_data

Regards,
Sudeep

Comments

Rafael J. Wysocki July 14, 2017, 10:06 p.m. UTC | #1 
On Friday, July 14, 2017 11:51:48 AM Sudeep Holla wrote:
> If the genpd->attach_dev or genpd->power_on fails, genpd_dev_pm_attach
> may return -EPROBE_DEFER initially. However genpd_alloc_dev_data sets
> the PM domain for the device unconditionally.
> 
> When subsequent attempts are made to call genpd_dev_pm_attach, it may
> return -EEXISTS checking dev->pm_domain without re-attempting to call
> attach_dev or power_on.
> 
> platform_drv_probe then attempts to call drv->probe as the return value
> -EEXIST != -EPROBE_DEFER, which may end up in a situation where the
> device is accessed without it's power domain switched on.
> 
> Fixes: f104e1e5ef57 ("PM / Domains: Re-order initialization of generic_pm_domain_data")
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Kevin Hilman <khilman@kernel.org>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: <stable@vger.kernel.org> # v4.0+
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

Ulf, Kevin?

> ---
>  drivers/base/power/domain.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> Hi,
> 
> I have added Cc/Fixes tags based on the original commit that caused the issue.
> I need to workout a patch that can cleanly apply before commit 989561de9b51
> ("PM / Domains: add setter for dev.pm_domain"), i.e. for v4.0.x to v4.4.x
> This patch should apply from v4.5.x onwards.
> 
> Let me know if I got anything wrong.
> 
> v1->v2: Moved dev_pm_domain_set(dev, NULL) from genpd_free_dev_data
> 
> Regards,
> Sudeep
> 
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index da49a8383dc3..2d9178660061 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -1168,8 +1168,6 @@ static struct generic_pm_domain_data *genpd_alloc_dev_data(struct device *dev,
>  
>  	spin_unlock_irq(&dev->power.lock);
>  
> -	dev_pm_domain_set(dev, &genpd->domain);
> -
>  	return gpd_data;
>  
>   err_free:
> @@ -1183,8 +1181,6 @@ static struct generic_pm_domain_data *genpd_alloc_dev_data(struct device *dev,
>  static void genpd_free_dev_data(struct device *dev,
>  				struct generic_pm_domain_data *gpd_data)
>  {
> -	dev_pm_domain_set(dev, NULL);
> -
>  	spin_lock_irq(&dev->power.lock);
>  
>  	dev->power.subsys_data->domain_data = NULL;
> @@ -1221,6 +1217,8 @@ static int genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
>  	if (ret)
>  		goto out;
>  
> +	dev_pm_domain_set(dev, &genpd->domain);
> +
>  	genpd->device_count++;
>  	genpd->max_off_time_changed = true;
>  
> @@ -1282,6 +1280,8 @@ static int genpd_remove_device(struct generic_pm_domain *genpd,
>  	if (genpd->detach_dev)
>  		genpd->detach_dev(genpd, dev);
>  
> +	dev_pm_domain_set(dev, NULL);
> +
>  	list_del_init(&pdd->list_node);
>  
>  	genpd_unlock(genpd);
>
Ulf Hansson July 17, 2017, 7:01 a.m. UTC | #2 
On 14 July 2017 at 12:51, Sudeep Holla <sudeep.holla@arm.com> wrote:
> If the genpd->attach_dev or genpd->power_on fails, genpd_dev_pm_attach
> may return -EPROBE_DEFER initially. However genpd_alloc_dev_data sets
> the PM domain for the device unconditionally.
>
> When subsequent attempts are made to call genpd_dev_pm_attach, it may
> return -EEXISTS checking dev->pm_domain without re-attempting to call
> attach_dev or power_on.
>
> platform_drv_probe then attempts to call drv->probe as the return value
> -EEXIST != -EPROBE_DEFER, which may end up in a situation where the
> device is accessed without it's power domain switched on.
>
> Fixes: f104e1e5ef57 ("PM / Domains: Re-order initialization of generic_pm_domain_data")
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Kevin Hilman <khilman@kernel.org>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: <stable@vger.kernel.org> # v4.0+
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

Acked-by: Ulf Hansson <ulf.hansson@linaro.org>

> ---
>  drivers/base/power/domain.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> Hi,
>
> I have added Cc/Fixes tags based on the original commit that caused the issue.
> I need to workout a patch that can cleanly apply before commit 989561de9b51
> ("PM / Domains: add setter for dev.pm_domain"), i.e. for v4.0.x to v4.4.x
> This patch should apply from v4.5.x onwards.

Thanks for checking this.

Perhaps we should change the stable tag to # v4.5+ instead then?
Otherwise the stable people will just report problems when trying to
apply it.

>
> Let me know if I got anything wrong.
>
> v1->v2: Moved dev_pm_domain_set(dev, NULL) from genpd_free_dev_data
>
> Regards,
> Sudeep

[...]

Kind regards
Uffe
Sudeep Holla July 17, 2017, 10:34 a.m. UTC | #3 
On 17/07/17 08:01, Ulf Hansson wrote:
> On 14 July 2017 at 12:51, Sudeep Holla <sudeep.holla@arm.com> wrote:
>> If the genpd->attach_dev or genpd->power_on fails, genpd_dev_pm_attach
>> may return -EPROBE_DEFER initially. However genpd_alloc_dev_data sets
>> the PM domain for the device unconditionally.
>>
>> When subsequent attempts are made to call genpd_dev_pm_attach, it may
>> return -EEXISTS checking dev->pm_domain without re-attempting to call
>> attach_dev or power_on.
>>
>> platform_drv_probe then attempts to call drv->probe as the return value
>> -EEXIST != -EPROBE_DEFER, which may end up in a situation where the
>> device is accessed without it's power domain switched on.
>>
>> Fixes: f104e1e5ef57 ("PM / Domains: Re-order initialization of generic_pm_domain_data")
>> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
>> Cc: Kevin Hilman <khilman@kernel.org>
>> Cc: Ulf Hansson <ulf.hansson@linaro.org>
>> Cc: <stable@vger.kernel.org> # v4.0+
>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> 
> Acked-by: Ulf Hansson <ulf.hansson@linaro.org>
> 
>> ---
>>  drivers/base/power/domain.c | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> Hi,
>>
>> I have added Cc/Fixes tags based on the original commit that caused the issue.
>> I need to workout a patch that can cleanly apply before commit 989561de9b51
>> ("PM / Domains: add setter for dev.pm_domain"), i.e. for v4.0.x to v4.4.x
>> This patch should apply from v4.5.x onwards.
> 
> Thanks for checking this.
> 
> Perhaps we should change the stable tag to # v4.5+ instead then?
> Otherwise the stable people will just report problems when trying to
> apply it.
> 

Yes, I know. I thought then I can work out the solution for whatever
version they need. Instead of trying to post patch for all versions from
v4.0 to v4.4
If that's not acceptable, then yes it should be #4.5+
Rafael, can you just update that if required ?

I am fine to work a patch for v4.[0-4].x when requested.
Rafael J. Wysocki July 17, 2017, 11:36 a.m. UTC | #4 
On Monday, July 17, 2017 11:34:57 AM Sudeep Holla wrote:
> 
> On 17/07/17 08:01, Ulf Hansson wrote:
> > On 14 July 2017 at 12:51, Sudeep Holla <sudeep.holla@arm.com> wrote:
> >> If the genpd->attach_dev or genpd->power_on fails, genpd_dev_pm_attach
> >> may return -EPROBE_DEFER initially. However genpd_alloc_dev_data sets
> >> the PM domain for the device unconditionally.
> >>
> >> When subsequent attempts are made to call genpd_dev_pm_attach, it may
> >> return -EEXISTS checking dev->pm_domain without re-attempting to call
> >> attach_dev or power_on.
> >>
> >> platform_drv_probe then attempts to call drv->probe as the return value
> >> -EEXIST != -EPROBE_DEFER, which may end up in a situation where the
> >> device is accessed without it's power domain switched on.
> >>
> >> Fixes: f104e1e5ef57 ("PM / Domains: Re-order initialization of generic_pm_domain_data")
> >> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> >> Cc: Kevin Hilman <khilman@kernel.org>
> >> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> >> Cc: <stable@vger.kernel.org> # v4.0+
> >> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > 
> > Acked-by: Ulf Hansson <ulf.hansson@linaro.org>
> > 
> >> ---
> >>  drivers/base/power/domain.c | 8 ++++----
> >>  1 file changed, 4 insertions(+), 4 deletions(-)
> >>
> >> Hi,
> >>
> >> I have added Cc/Fixes tags based on the original commit that caused the issue.
> >> I need to workout a patch that can cleanly apply before commit 989561de9b51
> >> ("PM / Domains: add setter for dev.pm_domain"), i.e. for v4.0.x to v4.4.x
> >> This patch should apply from v4.5.x onwards.
> > 
> > Thanks for checking this.
> > 
> > Perhaps we should change the stable tag to # v4.5+ instead then?
> > Otherwise the stable people will just report problems when trying to
> > apply it.
> > 
> 
> Yes, I know. I thought then I can work out the solution for whatever
> version they need. Instead of trying to post patch for all versions from
> v4.0 to v4.4
> If that's not acceptable, then yes it should be #4.5+
> Rafael, can you just update that if required ?

I can.

> I am fine to work a patch for v4.[0-4].x when requested.

That depends on how much of a problem this can be for the users of those
kernels, but I guess 4.4 is used by Android, so maybe it's worth the effort.

Thanks,
Rafael
diff mbox

Patch

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index da49a8383dc3..2d9178660061 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1168,8 +1168,6 @@  static struct generic_pm_domain_data *genpd_alloc_dev_data(struct device *dev,
 
 	spin_unlock_irq(&dev->power.lock);
 
-	dev_pm_domain_set(dev, &genpd->domain);
-
 	return gpd_data;
 
  err_free:
@@ -1183,8 +1181,6 @@  static struct generic_pm_domain_data *genpd_alloc_dev_data(struct device *dev,
 static void genpd_free_dev_data(struct device *dev,
 				struct generic_pm_domain_data *gpd_data)
 {
-	dev_pm_domain_set(dev, NULL);
-
 	spin_lock_irq(&dev->power.lock);
 
 	dev->power.subsys_data->domain_data = NULL;
@@ -1221,6 +1217,8 @@  static int genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
 	if (ret)
 		goto out;
 
+	dev_pm_domain_set(dev, &genpd->domain);
+
 	genpd->device_count++;
 	genpd->max_off_time_changed = true;
 
@@ -1282,6 +1280,8 @@  static int genpd_remove_device(struct generic_pm_domain *genpd,
 	if (genpd->detach_dev)
 		genpd->detach_dev(genpd, dev);
 
+	dev_pm_domain_set(dev, NULL);
+
 	list_del_init(&pdd->list_node);
 
 	genpd_unlock(genpd);