[PATCH] PM / Domains: restore calling of .suspend/resume_noirq() callbacks

[PATCH] PM / Domains: restore calling of .suspend/resume_noirq() callbacks

[Grygorii Strashko]

Now .suspend/resume_noirq() callbacks will not be called during
system wide suspend/resume for devices which belongs to some GPD.
It seems, that this change was accidently introduced by
commit d23b9b00cdde ("PM / Domains: Rework system suspend callback
 routines (v2)").

This patch restores calling of .suspend/resume_noirq() callbacks
for devices from GPD during system wide suspend/resume.

Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
 drivers/base/power/domain.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index fb83d4a..f8c70e6 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1014,6 +1014,7 @@ static int pm_genpd_suspend_late(struct device *dev)
 static int pm_genpd_suspend_noirq(struct device *dev)
 {
 	struct generic_pm_domain *genpd;
+	int ret;
 
 	dev_dbg(dev, "%s()\n", __func__);
 
@@ -1021,8 +1022,14 @@ static int pm_genpd_suspend_noirq(struct device *dev)
 	if (IS_ERR(genpd))
 		return -EINVAL;
 
-	if (genpd->suspend_power_off
-	    || (dev->power.wakeup_path && genpd_dev_active_wakeup(genpd, dev)))
+	if (genpd->suspend_power_off)
+		return 0;
+
+	ret = pm_generic_suspend_noirq(dev);
+	if (ret)
+		return ret;
+
+	if (dev->power.wakeup_path && genpd_dev_active_wakeup(genpd, dev))
 		return 0;
 
 	genpd_stop_dev(genpd, dev);
@@ -1065,8 +1072,9 @@ static int pm_genpd_resume_noirq(struct device *dev)
 	 */
 	pm_genpd_sync_poweron(genpd);
 	genpd->suspended_count--;
+	genpd_start_dev(genpd, dev);
 
-	return genpd_start_dev(genpd, dev);
+	return pm_generic_resume_noirq(dev);
 }
 
 /**
-- 
1.9.1

[PATCH] PM / Domains: restore calling of .suspend/resume_noirq() callbacks

[Rafael J. Wysocki]

On Wednesday, November 12, 2014 06:00:47 PM Grygorii Strashko wrote:
> Now .suspend/resume_noirq() callbacks will not be called during
> system wide suspend/resume for devices which belongs to some GPD.
> It seems, that this change was accidently introduced by
> commit d23b9b00cdde ("PM / Domains: Rework system suspend callback
>  routines (v2)").

I'm not sure if that was really accidentally.

Can you describe the problem that the change below is attempting to
address, without going to much into the history?  IOW, what's that
doesn't work right now?

> This patch restores calling of .suspend/resume_noirq() callbacks
> for devices from GPD during system wide suspend/resume.
> 
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
> ---
>  drivers/base/power/domain.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index fb83d4a..f8c70e6 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -1014,6 +1014,7 @@ static int pm_genpd_suspend_late(struct device *dev)
>  static int pm_genpd_suspend_noirq(struct device *dev)
>  {
>  	struct generic_pm_domain *genpd;
> +	int ret;
>  
>  	dev_dbg(dev, "%s()\n", __func__);
>  
> @@ -1021,8 +1022,14 @@ static int pm_genpd_suspend_noirq(struct device *dev)
>  	if (IS_ERR(genpd))
>  		return -EINVAL;
>  
> -	if (genpd->suspend_power_off
> -	    || (dev->power.wakeup_path && genpd_dev_active_wakeup(genpd, dev)))
> +	if (genpd->suspend_power_off)
> +		return 0;
> +
> +	ret = pm_generic_suspend_noirq(dev);
> +	if (ret)
> +		return ret;
> +
> +	if (dev->power.wakeup_path && genpd_dev_active_wakeup(genpd, dev))
>  		return 0;
>  
>  	genpd_stop_dev(genpd, dev);
> @@ -1065,8 +1072,9 @@ static int pm_genpd_resume_noirq(struct device *dev)
>  	 */
>  	pm_genpd_sync_poweron(genpd);
>  	genpd->suspended_count--;
> +	genpd_start_dev(genpd, dev);
>  
> -	return genpd_start_dev(genpd, dev);
> +	return pm_generic_resume_noirq(dev);
>  }
>  
>  /**
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

[PATCH] PM / Domains: restore calling of .suspend/resume_noirq() callbacks

[Grygorii Strashko]

Hi Rafael,

On 11/13/2014 03:33 AM, Rafael J. Wysocki wrote:
> On Wednesday, November 12, 2014 06:00:47 PM Grygorii Strashko wrote:
>> Now .suspend/resume_noirq() callbacks will not be called during
>> system wide suspend/resume for devices which belongs to some GPD.
>> It seems, that this change was accidentally introduced by
>> commit d23b9b00cdde ("PM / Domains: Rework system suspend callback
>>   routines (v2)").
> 
> I'm not sure if that was really accidentally.
> 
> Can you describe the problem that the change below is attempting to
> address, without going to much into the history?  IOW, what's that
> doesn't work right now?

There are no real issues - now in Kernel there are no users of GPD
which use "noirq" callbacks.

As of now, I'm trying to use GPD for Keyatone2, so 
I'm studying it and playing with it to understand all benefits and 
disadvantages of such solution.

So, I've found this issue, but not found any description of why
calls of these callbacks have been removed.
I'd very appreciate if can point me on?

> 
>> This patch restores calling of .suspend/resume_noirq() callbacks
>> for devices from GPD during system wide suspend/resume.
>>
>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
>> ---
>>   drivers/base/power/domain.c | 14 +++++++++++---
>>   1 file changed, 11 insertions(+), 3 deletions(-)
>>
...

Regards,
-grygorii
 

[PATCH] PM / Domains: restore calling of .suspend/resume_noirq() callbacks

[Geert Uytterhoeven]

On Thu, Nov 13, 2014 at 7:43 PM, Grygorii Strashko
<grygorii.strashko@ti.com> wrote:
> On 11/13/2014 03:33 AM, Rafael J. Wysocki wrote:
>> On Wednesday, November 12, 2014 06:00:47 PM Grygorii Strashko wrote:
>>> Now .suspend/resume_noirq() callbacks will not be called during
>>> system wide suspend/resume for devices which belongs to some GPD.
>>> It seems, that this change was accidentally introduced by
>>> commit d23b9b00cdde ("PM / Domains: Rework system suspend callback
>>>   routines (v2)").
>>
>> I'm not sure if that was really accidentally.
>>
>> Can you describe the problem that the change below is attempting to
>> address, without going to much into the history?  IOW, what's that
>> doesn't work right now?
>
> There are no real issues - now in Kernel there are no users of GPD
> which use "noirq" callbacks.

Indeed.

But as the .suspend_noirq() and .resume_noirq() callbacks are not called
when using the generic PM domain, I had to manually handle interrupt
disable/enable in commit a00d91ea264f974b ("fbdev: sh_mobile_hdmi:
Re-init regs before irq re-enable on resume").

Gr{oetje,eeting}s,

                        Geert

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

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH] PM / Domains: restore calling of .suspend/resume_noirq() callbacks

[Grygorii Strashko]

On 11/13/2014 09:11 PM, Geert Uytterhoeven wrote:
> On Thu, Nov 13, 2014 at 7:43 PM, Grygorii Strashko
> <grygorii.strashko@ti.com> wrote:
>> On 11/13/2014 03:33 AM, Rafael J. Wysocki wrote:
>>> On Wednesday, November 12, 2014 06:00:47 PM Grygorii Strashko wrote:
>>>> Now .suspend/resume_noirq() callbacks will not be called during
>>>> system wide suspend/resume for devices which belongs to some GPD.
>>>> It seems, that this change was accidentally introduced by
>>>> commit d23b9b00cdde ("PM / Domains: Rework system suspend callback
>>>>    routines (v2)").
>>>
>>> I'm not sure if that was really accidentally.
>>>
>>> Can you describe the problem that the change below is attempting to
>>> address, without going to much into the history?  IOW, what's that
>>> doesn't work right now?
>>
>> There are no real issues - now in Kernel there are no users of GPD
>> which use "noirq" callbacks.
> 
> Indeed.
> 
> But as the .suspend_noirq() and .resume_noirq() callbacks are not called
> when using the generic PM domain, I had to manually handle interrupt
> disable/enable in commit a00d91ea264f974b ("fbdev: sh_mobile_hdmi:
> Re-init regs before irq re-enable on resume").

^ Honestly, this is very useful practice, because with SMP enabled
the IRQ can be triggered after .suspend() and before .suspend_noirq()
which, in turn, may schedule some kthread/work or even threaded_irq_handler
on secondary cpus (disable_nonboot_cpus() is called after suspend_noirq stage).

Funny things may happen after that :P like: some work/thread which is 
servicing request from I2C device (for example) will be frozen in the middle of
its execution and then it will be resumed right after enable_nonboot_cpus().

^Issue from real life.

regards,
-grygorii