diff mbox series

[imx_4.9.y,1/2] PM / Domains: Fix error path during attach in genpd

Message ID 071b6a4681c682759a41a91528c5e4a657bda445.1535744197.git.leonard.crestez@nxp.com
State New
Headers show
Series [imx_4.9.y,1/2] PM / Domains: Fix error path during attach in genpd | expand

Commit Message

Leonard Crestez Sept. 3, 2018, 10:16 a.m. UTC 
From: Ulf Hansson <ulf.hansson@linaro.org>


In case the PM domain fails to be powered on in genpd_dev_pm_attach(), it
returns -EPROBE_DEFER, but keeping the device attached to its PM domain.
This leads to problems when the next attempt to attach is re-tried. More
precisely, in that situation an -EEXIST error code is returned, because the
device already has its PM domain pointer assigned, from the first attempt.

Now, because of the sloppy error handling by the existing callers of
dev_pm_domain_attach(), probing is allowed to continue when -EEXIST is
returned. However, in such case there are no guarantees that the PM domain
is powered on by genpd, which may lead to hangs when buses/drivers tried to
access their devices.

Let's fix this behaviour, simply by detaching the device when powering on
fails in genpd_dev_pm_attach().

Cc: v4.11+ <stable@vger.kernel.org> # v4.11+
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>


(cherry picked from commit 72038df3c580c4c326b83c86149d7ac34007532a)

Cherry-picked to imx_4.9.y with minimal conflicts so that we can
properly handle errors from SCFW pm calls
Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>

---
 drivers/base/power/domain.c | 3 +++
 1 file changed, 3 insertions(+)

-- 
2.17.1

Comments

Aisheng Dong Sept. 3, 2018, 12:36 p.m. UTC | #1 
> -----Original Message-----

> From: Leonard Crestez

> Sent: Monday, September 3, 2018 6:16 PM

> To: Anson Huang <anson.huang@nxp.com>; A.s. Dong

> <aisheng.dong@nxp.com>

> Cc: Ranjani Vaidyanathan <ranjani.vaidyanathan@nxp.com>; Nitin Garg

> <nitin.garg@nxp.com>; Jason Liu <jason.hui.liu@nxp.com>; Pete Zhang

> <pete.zhang@nxp.com>; Eagle Zhou <eagle.zhou@nxp.com>; LnxRevLi

> <LnxRevLi@nxp.com>; Ulf Hansson <ulf.hansson@linaro.org>; v4 . 11+

> <stable@vger.kernel.org>; Rafael J . Wysocki <rafael.j.wysocki@intel.com>

> Subject: [PATCH imx_4.9.y 1/2] PM / Domains: Fix error path during attach in

> genpd

> 

> From: Ulf Hansson <ulf.hansson@linaro.org>

> 

> In case the PM domain fails to be powered on in genpd_dev_pm_attach(), it

> returns -EPROBE_DEFER, but keeping the device attached to its PM domain.

> This leads to problems when the next attempt to attach is re-tried. More

> precisely, in that situation an -EEXIST error code is returned, because the device

> already has its PM domain pointer assigned, from the first attempt.

> 

> Now, because of the sloppy error handling by the existing callers of

> dev_pm_domain_attach(), probing is allowed to continue when -EEXIST is

> returned. However, in such case there are no guarantees that the PM domain is

> powered on by genpd, which may lead to hangs when buses/drivers tried to

> access their devices.

> 

> Let's fix this behaviour, simply by detaching the device when powering on fails

> in genpd_dev_pm_attach().

> 

> Cc: v4.11+ <stable@vger.kernel.org> # v4.11+


Although this patch is stated only needed for v4.11+, but it seems to me 4.9 has the same issue
and also needs it.

Reviewed-by: Dong Aisheng <Aisheng.dong@nxp.com>


Hi Leonard,
I guess you could also try to send it to 4.9 LTS kernel maillist to test maintainer later.

Regards
Dong Aisheng

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

> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> 

> (cherry picked from commit 72038df3c580c4c326b83c86149d7ac34007532a)

> 

> Cherry-picked to imx_4.9.y with minimal conflicts so that we can properly

> handle errors from SCFW pm calls

> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>

> ---

>  drivers/base/power/domain.c | 3 +++

>  1 file changed, 3 insertions(+)

> 

> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c index

> 9c3e535795a0..d9681d372997 100644

> --- a/drivers/base/power/domain.c

> +++ b/drivers/base/power/domain.c

> @@ -1936,10 +1936,13 @@ int genpd_dev_pm_attach(struct device *dev)

>  	dev->pm_domain->sync = genpd_dev_pm_sync;

> 

>  	mutex_lock(&pd->lock);

>  	ret = genpd_poweron(pd, 0);

>  	mutex_unlock(&pd->lock);

> +

> +	if (ret)

> +		genpd_remove_device(pd, dev);

>  out:

>  	return ret ? -EPROBE_DEFER : 0;

>  }

>  EXPORT_SYMBOL_GPL(genpd_dev_pm_attach);

>  #endif /* CONFIG_PM_GENERIC_DOMAINS_OF */

> --

> 2.17.1
Aisheng Dong Sept. 3, 2018, 12:39 p.m. UTC | #2 
> -----Original Message-----

> From: Leonard Crestez

> Sent: Monday, September 3, 2018 6:16 PM

> To: Anson Huang <anson.huang@nxp.com>; A.s. Dong

> <aisheng.dong@nxp.com>

> Cc: Ranjani Vaidyanathan <ranjani.vaidyanathan@nxp.com>; Nitin Garg

> <nitin.garg@nxp.com>; Jason Liu <jason.hui.liu@nxp.com>; Pete Zhang

> <pete.zhang@nxp.com>; Eagle Zhou <eagle.zhou@nxp.com>; LnxRevLi

> <LnxRevLi@nxp.com>; Ulf Hansson <ulf.hansson@linaro.org>; v4 . 11+

> <stable@vger.kernel.org>; Rafael J . Wysocki <rafael.j.wysocki@intel.com>

> Subject: [PATCH imx_4.9.y 1/2] PM / Domains: Fix error path during attach in

> genpd

> 

> From: Ulf Hansson <ulf.hansson@linaro.org>

> 

> In case the PM domain fails to be powered on in genpd_dev_pm_attach(), it

> returns -EPROBE_DEFER, but keeping the device attached to its PM domain.

> This leads to problems when the next attempt to attach is re-tried. More

> precisely, in that situation an -EEXIST error code is returned, because the device

> already has its PM domain pointer assigned, from the first attempt.

> 

> Now, because of the sloppy error handling by the existing callers of

> dev_pm_domain_attach(), probing is allowed to continue when -EEXIST is

> returned. However, in such case there are no guarantees that the PM domain is

> powered on by genpd, which may lead to hangs when buses/drivers tried to

> access their devices.

> 

> Let's fix this behaviour, simply by detaching the device when powering on fails

> in genpd_dev_pm_attach().

> 

> Cc: v4.11+ <stable@vger.kernel.org> # v4.11+

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

> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> 

> (cherry picked from commit 72038df3c580c4c326b83c86149d7ac34007532a)

> 

> Cherry-picked to imx_4.9.y with minimal conflicts so that we can properly

> handle errors from SCFW pm calls

> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>

> ---

>  drivers/base/power/domain.c | 3 +++

>  1 file changed, 3 insertions(+)

> 

> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c index

> 9c3e535795a0..d9681d372997 100644

> --- a/drivers/base/power/domain.c

> +++ b/drivers/base/power/domain.c

> @@ -1936,10 +1936,13 @@ int genpd_dev_pm_attach(struct device *dev)

>  	dev->pm_domain->sync = genpd_dev_pm_sync;

> 

>  	mutex_lock(&pd->lock);

>  	ret = genpd_poweron(pd, 0);

>  	mutex_unlock(&pd->lock);

> +

> +	if (ret)

> +		genpd_remove_device(pd, dev);

>  out:

>  	return ret ? -EPROBE_DEFER : 0;

>  }

>  EXPORT_SYMBOL_GPL(genpd_dev_pm_attach);

>  #endif /* CONFIG_PM_GENERIC_DOMAINS_OF */



BTW, it looks to me this is not a complete fix.

static int platform_drv_probe(struct device *_dev)
{

        ret = dev_pm_domain_attach(_dev, true);
        if (ret != -EPROBE_DEFER) {	<---- here is risk as it's only hanlle EPROBE_DEFFER. Latest upstream is already changed. However, it may not cause real issue for our case.
                if (drv->probe) {
                        ret = drv->probe(dev);
                        if (ret)
                                dev_pm_domain_detach(_dev, true);
                } else {
                        /* don't fail if just dev_pm_domain_attach failed */
                        ret = 0;
                }
        }       
        ...

        return ret; 
}

> --

> 2.17.1
diff mbox series

Patch

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 9c3e535795a0..d9681d372997 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1936,10 +1936,13 @@  int genpd_dev_pm_attach(struct device *dev)
 	dev->pm_domain->sync = genpd_dev_pm_sync;
 
 	mutex_lock(&pd->lock);
 	ret = genpd_poweron(pd, 0);
 	mutex_unlock(&pd->lock);
+
+	if (ret)
+		genpd_remove_device(pd, dev);
 out:
 	return ret ? -EPROBE_DEFER : 0;
 }
 EXPORT_SYMBOL_GPL(genpd_dev_pm_attach);
 #endif /* CONFIG_PM_GENERIC_DOMAINS_OF */