| Message ID | 1415808047-23455-1-git-send-email-grygorii.strashko@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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); > } > > /** >
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
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@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
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
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); } /**
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(-)