| Message ID | 1384448716-3186-1-git-send-email-nm@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On Thu, Nov 14, 2013 at 11:05:16AM -0600, Nishanth Menon wrote: > OMAP device hooks around suspend|resume_noirq ensures that hwmod > devices are forced to idle using omap_device_idle/enable as part of > the last stage of suspend activity. > > For a device such as i2c who uses autosuspend, it is possible to enter > the suspend path with dev->power.runtime_status = RPM_ACTIVE. > > As part of the suspend flow, the generic runtime logic would increment > it's dev->power.disable_depth to 1. This should prevent further > pm_runtime_get_sync from succeeding once the runtime_status has been > set to RPM_SUSPENDED. > > Now, as part of the suspend_noirq handler in omap_device, we force the > following: if the device status is !suspended, we force the device > to idle using omap_device_idle (clocks are cut etc..). This ensures > that from a hardware perspective, the device is "suspended". However, > runtime_status is left to be active. > > *if* an operation is attempted after this point to > pm_runtime_get_sync, runtime framework depends on runtime_status to > indicate accurately the device status, and since it sees it to be > ACTIVE, it assumes the module is functional and returns a non-error > value. As a result the user will see pm_runtime_get succeed, however a > register access will crash due to the lack of clocks. > > To prevent this from happening, we should ensure that runtime_status > exactly indicates the device status. As a result of this change > any further calls to pm_runtime_get* would return -EACCES (since > disable_depth is 1). On resume, we restore the clocks and runtime > status exactly as we suspended with. These operations are not expected > to fail as we update the states after the core runtime framework has > suspended itself and restore before the core runtime framework has > resumed. > > Reported-by: J Keerthy <j-keerthy@ti.com> > Signed-off-by: Nishanth Menon <nm@ti.com> > Acked-by: Rajendra Nayak <rnayak@ti.com> > Acked-by: Kevin Hilman <khilman@linaro.org> > --- > Changes in V3: > - Added WARN in case of unexpected failure of runtime pm status restore > v2: https://patchwork.kernel.org/patch/3176501/ > v1: https://patchwork.kernel.org/patch/3154501/ > > patch baseline: V3.12 tag (also applies on linux-next next-20131107 tag) > > arch/arm/mach-omap2/omap_device.c | 13 +++++++++++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > diff --git a/arch/arm/mach-omap2/omap_device.c b/arch/arm/mach-omap2/omap_device.c > index b69dd9a..53f0735 100644 > --- a/arch/arm/mach-omap2/omap_device.c > +++ b/arch/arm/mach-omap2/omap_device.c > @@ -621,6 +621,7 @@ static int _od_suspend_noirq(struct device *dev) > > if (!ret && !pm_runtime_status_suspended(dev)) { > if (pm_generic_runtime_suspend(dev) == 0) { > + pm_runtime_set_suspended(dev); > omap_device_idle(pdev); > od->flags |= OMAP_DEVICE_SUSPENDED; > } > @@ -634,10 +635,18 @@ static int _od_resume_noirq(struct device *dev) > struct platform_device *pdev = to_platform_device(dev); > struct omap_device *od = to_omap_device(pdev); > > - if ((od->flags & OMAP_DEVICE_SUSPENDED) && > - !pm_runtime_status_suspended(dev)) { > + if (od->flags & OMAP_DEVICE_SUSPENDED) { > od->flags &= ~OMAP_DEVICE_SUSPENDED; > omap_device_enable(pdev); > + /* > + * XXX: we run before core runtime pm has resumed itself. At > + * this point in time, we just restore the runtime pm state and > + * considering symmetric operations in resume, we donot expect > + * to fail. If we failed, something changed in core runtime_pm > + * framework OR some device driver messed things up, hence, WARN > + */ > + WARN(pm_runtime_set_active(dev), > + "Could not set %s runtime state active\n", dev_name(dev)); if you want to print the device name, how about dev_WARN() ? no strong feelings though: Reviewed-by: Felipe Balbi <balbi@ti.com>
On 11/14/2013 12:55 PM, Felipe Balbi wrote: > Hi, > > On Thu, Nov 14, 2013 at 11:05:16AM -0600, Nishanth Menon wrote: >> OMAP device hooks around suspend|resume_noirq ensures that hwmod >> devices are forced to idle using omap_device_idle/enable as part of >> the last stage of suspend activity. >> >> For a device such as i2c who uses autosuspend, it is possible to enter >> the suspend path with dev->power.runtime_status = RPM_ACTIVE. >> >> As part of the suspend flow, the generic runtime logic would increment >> it's dev->power.disable_depth to 1. This should prevent further >> pm_runtime_get_sync from succeeding once the runtime_status has been >> set to RPM_SUSPENDED. >> >> Now, as part of the suspend_noirq handler in omap_device, we force the >> following: if the device status is !suspended, we force the device >> to idle using omap_device_idle (clocks are cut etc..). This ensures >> that from a hardware perspective, the device is "suspended". However, >> runtime_status is left to be active. >> >> *if* an operation is attempted after this point to >> pm_runtime_get_sync, runtime framework depends on runtime_status to >> indicate accurately the device status, and since it sees it to be >> ACTIVE, it assumes the module is functional and returns a non-error >> value. As a result the user will see pm_runtime_get succeed, however a >> register access will crash due to the lack of clocks. >> >> To prevent this from happening, we should ensure that runtime_status >> exactly indicates the device status. As a result of this change >> any further calls to pm_runtime_get* would return -EACCES (since >> disable_depth is 1). On resume, we restore the clocks and runtime >> status exactly as we suspended with. These operations are not expected >> to fail as we update the states after the core runtime framework has >> suspended itself and restore before the core runtime framework has >> resumed. >> >> Reported-by: J Keerthy <j-keerthy@ti.com> >> Signed-off-by: Nishanth Menon <nm@ti.com> >> Acked-by: Rajendra Nayak <rnayak@ti.com> >> Acked-by: Kevin Hilman <khilman@linaro.org> >> --- >> Changes in V3: >> - Added WARN in case of unexpected failure of runtime pm status restore >> v2: https://patchwork.kernel.org/patch/3176501/ >> v1: https://patchwork.kernel.org/patch/3154501/ >> >> patch baseline: V3.12 tag (also applies on linux-next next-20131107 tag) >> >> arch/arm/mach-omap2/omap_device.c | 13 +++++++++++-- >> 1 file changed, 11 insertions(+), 2 deletions(-) >> >> diff --git a/arch/arm/mach-omap2/omap_device.c b/arch/arm/mach-omap2/omap_device.c >> index b69dd9a..53f0735 100644 >> --- a/arch/arm/mach-omap2/omap_device.c >> +++ b/arch/arm/mach-omap2/omap_device.c >> @@ -621,6 +621,7 @@ static int _od_suspend_noirq(struct device *dev) >> >> if (!ret && !pm_runtime_status_suspended(dev)) { >> if (pm_generic_runtime_suspend(dev) == 0) { >> + pm_runtime_set_suspended(dev); >> omap_device_idle(pdev); >> od->flags |= OMAP_DEVICE_SUSPENDED; >> } >> @@ -634,10 +635,18 @@ static int _od_resume_noirq(struct device *dev) >> struct platform_device *pdev = to_platform_device(dev); >> struct omap_device *od = to_omap_device(pdev); >> >> - if ((od->flags & OMAP_DEVICE_SUSPENDED) && >> - !pm_runtime_status_suspended(dev)) { >> + if (od->flags & OMAP_DEVICE_SUSPENDED) { >> od->flags &= ~OMAP_DEVICE_SUSPENDED; >> omap_device_enable(pdev); >> + /* >> + * XXX: we run before core runtime pm has resumed itself. At >> + * this point in time, we just restore the runtime pm state and >> + * considering symmetric operations in resume, we donot expect >> + * to fail. If we failed, something changed in core runtime_pm >> + * framework OR some device driver messed things up, hence, WARN >> + */ >> + WARN(pm_runtime_set_active(dev), >> + "Could not set %s runtime state active\n", dev_name(dev)); > > if you want to print the device name, how about dev_WARN() ? > > no strong feelings though: I would like to stick with WARN as dev_WARN would probably need an condition check.. unless someone has a strong opinion, I dont see it adding value here. > > Reviewed-by: Felipe Balbi <balbi@ti.com> >
On Thu, 14 Nov 2013, Nishanth Menon wrote: > OMAP device hooks around suspend|resume_noirq ensures that hwmod > devices are forced to idle using omap_device_idle/enable as part of > the last stage of suspend activity. > > For a device such as i2c who uses autosuspend, it is possible to enter > the suspend path with dev->power.runtime_status = RPM_ACTIVE. > > As part of the suspend flow, the generic runtime logic would increment > it's dev->power.disable_depth to 1. This should prevent further > pm_runtime_get_sync from succeeding once the runtime_status has been > set to RPM_SUSPENDED. > > Now, as part of the suspend_noirq handler in omap_device, we force the > following: if the device status is !suspended, we force the device > to idle using omap_device_idle (clocks are cut etc..). This ensures > that from a hardware perspective, the device is "suspended". However, > runtime_status is left to be active. > > *if* an operation is attempted after this point to > pm_runtime_get_sync, runtime framework depends on runtime_status to > indicate accurately the device status, and since it sees it to be > ACTIVE, it assumes the module is functional and returns a non-error > value. As a result the user will see pm_runtime_get succeed, however a > register access will crash due to the lack of clocks. > > To prevent this from happening, we should ensure that runtime_status > exactly indicates the device status. As a result of this change > any further calls to pm_runtime_get* would return -EACCES (since > disable_depth is 1). On resume, we restore the clocks and runtime > status exactly as we suspended with. These operations are not expected > to fail as we update the states after the core runtime framework has > suspended itself and restore before the core runtime framework has > resumed. > > Reported-by: J Keerthy <j-keerthy@ti.com> > Signed-off-by: Nishanth Menon <nm@ti.com> > Acked-by: Rajendra Nayak <rnayak@ti.com> > Acked-by: Kevin Hilman <khilman@linaro.org> Looks reasonable to me. Looks like this should be considered for -stable - Nishanth, what do you think? Tony or Kevin, do you want to take this one, or want me to? - Paul -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/15/2013 02:07 AM, Paul Walmsley wrote: > On Thu, 14 Nov 2013, Nishanth Menon wrote: > >> OMAP device hooks around suspend|resume_noirq ensures that hwmod >> devices are forced to idle using omap_device_idle/enable as part of >> the last stage of suspend activity. >> >> For a device such as i2c who uses autosuspend, it is possible to enter >> the suspend path with dev->power.runtime_status = RPM_ACTIVE. >> >> As part of the suspend flow, the generic runtime logic would increment >> it's dev->power.disable_depth to 1. This should prevent further >> pm_runtime_get_sync from succeeding once the runtime_status has been >> set to RPM_SUSPENDED. >> >> Now, as part of the suspend_noirq handler in omap_device, we force the >> following: if the device status is !suspended, we force the device >> to idle using omap_device_idle (clocks are cut etc..). This ensures >> that from a hardware perspective, the device is "suspended". However, >> runtime_status is left to be active. >> >> *if* an operation is attempted after this point to >> pm_runtime_get_sync, runtime framework depends on runtime_status to >> indicate accurately the device status, and since it sees it to be >> ACTIVE, it assumes the module is functional and returns a non-error >> value. As a result the user will see pm_runtime_get succeed, however a >> register access will crash due to the lack of clocks. >> >> To prevent this from happening, we should ensure that runtime_status >> exactly indicates the device status. As a result of this change >> any further calls to pm_runtime_get* would return -EACCES (since >> disable_depth is 1). On resume, we restore the clocks and runtime >> status exactly as we suspended with. These operations are not expected >> to fail as we update the states after the core runtime framework has >> suspended itself and restore before the core runtime framework has >> resumed. >> >> Reported-by: J Keerthy <j-keerthy@ti.com> >> Signed-off-by: Nishanth Menon <nm@ti.com> >> Acked-by: Rajendra Nayak <rnayak@ti.com> >> Acked-by: Kevin Hilman <khilman@linaro.org> > > Looks reasonable to me. Looks like this should be considered for -stable > - Nishanth, what do you think? Every product kernel since 3.4 needed to be hacked (we have hacked in different ways so far) to work around this (since we never spend time digging deeper :( ), So, I do agree with your view that a -stable tag will be most beneficial. > > Tony or Kevin, do you want to take this one, or want me to? > > > - Paul >
* Nishanth Menon <nm@ti.com> [131115 05:30]: > On 11/15/2013 02:07 AM, Paul Walmsley wrote: > > On Thu, 14 Nov 2013, Nishanth Menon wrote: > > > >> OMAP device hooks around suspend|resume_noirq ensures that hwmod > >> devices are forced to idle using omap_device_idle/enable as part of > >> the last stage of suspend activity. > >> > >> For a device such as i2c who uses autosuspend, it is possible to enter > >> the suspend path with dev->power.runtime_status = RPM_ACTIVE. > >> > >> As part of the suspend flow, the generic runtime logic would increment > >> it's dev->power.disable_depth to 1. This should prevent further > >> pm_runtime_get_sync from succeeding once the runtime_status has been > >> set to RPM_SUSPENDED. > >> > >> Now, as part of the suspend_noirq handler in omap_device, we force the > >> following: if the device status is !suspended, we force the device > >> to idle using omap_device_idle (clocks are cut etc..). This ensures > >> that from a hardware perspective, the device is "suspended". However, > >> runtime_status is left to be active. > >> > >> *if* an operation is attempted after this point to > >> pm_runtime_get_sync, runtime framework depends on runtime_status to > >> indicate accurately the device status, and since it sees it to be > >> ACTIVE, it assumes the module is functional and returns a non-error > >> value. As a result the user will see pm_runtime_get succeed, however a > >> register access will crash due to the lack of clocks. > >> > >> To prevent this from happening, we should ensure that runtime_status > >> exactly indicates the device status. As a result of this change > >> any further calls to pm_runtime_get* would return -EACCES (since > >> disable_depth is 1). On resume, we restore the clocks and runtime > >> status exactly as we suspended with. These operations are not expected > >> to fail as we update the states after the core runtime framework has > >> suspended itself and restore before the core runtime framework has > >> resumed. > >> > >> Reported-by: J Keerthy <j-keerthy@ti.com> > >> Signed-off-by: Nishanth Menon <nm@ti.com> > >> Acked-by: Rajendra Nayak <rnayak@ti.com> > >> Acked-by: Kevin Hilman <khilman@linaro.org> > > > > Looks reasonable to me. Looks like this should be considered for -stable > > - Nishanth, what do you think? > > Every product kernel since 3.4 needed to be hacked (we have hacked in > different ways so far) to work around this (since we never spend time > digging deeper :( ), So, I do agree with your view that a -stable tag > will be most beneficial. > > > > > Tony or Kevin, do you want to take this one, or want me to? I can take it unless you have other fixes pending right now. Tony -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 15 Nov 2013, Tony Lindgren wrote: > I can take it unless you have other fixes pending right now. Ran a quick test with the patch applied on v3.12, results here: http://www.pwsan.com/omap/testlogs/test_nm_omap_device_fix_v3.12-rc/20131115123132/ No other fixes queued here right now, so: Acked-by: Paul Walmsley <paul@pwsan.com> - Paul -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Tony Lindgren <tony@atomide.com> writes: > * Nishanth Menon <nm@ti.com> [131115 05:30]: >> On 11/15/2013 02:07 AM, Paul Walmsley wrote: >> > On Thu, 14 Nov 2013, Nishanth Menon wrote: >> > >> >> OMAP device hooks around suspend|resume_noirq ensures that hwmod >> >> devices are forced to idle using omap_device_idle/enable as part of >> >> the last stage of suspend activity. >> >> >> >> For a device such as i2c who uses autosuspend, it is possible to enter >> >> the suspend path with dev->power.runtime_status = RPM_ACTIVE. >> >> >> >> As part of the suspend flow, the generic runtime logic would increment >> >> it's dev->power.disable_depth to 1. This should prevent further >> >> pm_runtime_get_sync from succeeding once the runtime_status has been >> >> set to RPM_SUSPENDED. >> >> >> >> Now, as part of the suspend_noirq handler in omap_device, we force the >> >> following: if the device status is !suspended, we force the device >> >> to idle using omap_device_idle (clocks are cut etc..). This ensures >> >> that from a hardware perspective, the device is "suspended". However, >> >> runtime_status is left to be active. >> >> >> >> *if* an operation is attempted after this point to >> >> pm_runtime_get_sync, runtime framework depends on runtime_status to >> >> indicate accurately the device status, and since it sees it to be >> >> ACTIVE, it assumes the module is functional and returns a non-error >> >> value. As a result the user will see pm_runtime_get succeed, however a >> >> register access will crash due to the lack of clocks. >> >> >> >> To prevent this from happening, we should ensure that runtime_status >> >> exactly indicates the device status. As a result of this change >> >> any further calls to pm_runtime_get* would return -EACCES (since >> >> disable_depth is 1). On resume, we restore the clocks and runtime >> >> status exactly as we suspended with. These operations are not expected >> >> to fail as we update the states after the core runtime framework has >> >> suspended itself and restore before the core runtime framework has >> >> resumed. >> >> >> >> Reported-by: J Keerthy <j-keerthy@ti.com> >> >> Signed-off-by: Nishanth Menon <nm@ti.com> >> >> Acked-by: Rajendra Nayak <rnayak@ti.com> >> >> Acked-by: Kevin Hilman <khilman@linaro.org> >> > >> > Looks reasonable to me. Looks like this should be considered for -stable >> > - Nishanth, what do you think? >> >> Every product kernel since 3.4 needed to be hacked (we have hacked in >> different ways so far) to work around this (since we never spend time >> digging deeper :( ), So, I do agree with your view that a -stable tag >> will be most beneficial. >> >> > >> > Tony or Kevin, do you want to take this one, or want me to? > > I can take it unless you have other fixes pending right now. This version looks good to me. Reviewed-by: Kevin Hilman <khilman@linaro.org> -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 15 November 2013 23:03, Kevin Hilman <khilman@linaro.org> wrote: > Tony Lindgren <tony@atomide.com> writes: > >> * Nishanth Menon <nm@ti.com> [131115 05:30]: >>> On 11/15/2013 02:07 AM, Paul Walmsley wrote: >>> > On Thu, 14 Nov 2013, Nishanth Menon wrote: >>> > >>> >> OMAP device hooks around suspend|resume_noirq ensures that hwmod >>> >> devices are forced to idle using omap_device_idle/enable as part of >>> >> the last stage of suspend activity. >>> >> >>> >> For a device such as i2c who uses autosuspend, it is possible to enter >>> >> the suspend path with dev->power.runtime_status = RPM_ACTIVE. >>> >> >>> >> As part of the suspend flow, the generic runtime logic would increment >>> >> it's dev->power.disable_depth to 1. This should prevent further >>> >> pm_runtime_get_sync from succeeding once the runtime_status has been >>> >> set to RPM_SUSPENDED. >>> >> >>> >> Now, as part of the suspend_noirq handler in omap_device, we force the >>> >> following: if the device status is !suspended, we force the device >>> >> to idle using omap_device_idle (clocks are cut etc..). This ensures >>> >> that from a hardware perspective, the device is "suspended". However, >>> >> runtime_status is left to be active. >>> >> >>> >> *if* an operation is attempted after this point to >>> >> pm_runtime_get_sync, runtime framework depends on runtime_status to >>> >> indicate accurately the device status, and since it sees it to be >>> >> ACTIVE, it assumes the module is functional and returns a non-error >>> >> value. As a result the user will see pm_runtime_get succeed, however a >>> >> register access will crash due to the lack of clocks. >>> >> >>> >> To prevent this from happening, we should ensure that runtime_status >>> >> exactly indicates the device status. As a result of this change >>> >> any further calls to pm_runtime_get* would return -EACCES (since >>> >> disable_depth is 1). On resume, we restore the clocks and runtime >>> >> status exactly as we suspended with. These operations are not expected >>> >> to fail as we update the states after the core runtime framework has >>> >> suspended itself and restore before the core runtime framework has >>> >> resumed. >>> >> >>> >> Reported-by: J Keerthy <j-keerthy@ti.com> >>> >> Signed-off-by: Nishanth Menon <nm@ti.com> >>> >> Acked-by: Rajendra Nayak <rnayak@ti.com> >>> >> Acked-by: Kevin Hilman <khilman@linaro.org> Hi Kevin, I am wondering if OMAP would benefit from letting the PM core allow runtime suspends during system suspend, which is not the case as of now. My impression is that it could simplify OMAP drivers and the power domain, but it would be interesting to hear your opinion in this. It somewhat touches this patch. I guess. The reason for bringing this up here, is that I wanted to highlight that we are at the moment discussing the following RFC on the linux-pm mailing list: "[RFC PATCH] PM / Runtime: Allow to inactivate devices during system suspend". Kind regards Ulf Hansson >>> > >>> > Looks reasonable to me. Looks like this should be considered for -stable >>> > - Nishanth, what do you think? >>> >>> Every product kernel since 3.4 needed to be hacked (we have hacked in >>> different ways so far) to work around this (since we never spend time >>> digging deeper :( ), So, I do agree with your view that a -stable tag >>> will be most beneficial. >>> >>> > >>> > Tony or Kevin, do you want to take this one, or want me to? >> >> I can take it unless you have other fixes pending right now. > > This version looks good to me. > > Reviewed-by: Kevin Hilman <khilman@linaro.org> > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/arm/mach-omap2/omap_device.c b/arch/arm/mach-omap2/omap_device.c index b69dd9a..53f0735 100644 --- a/arch/arm/mach-omap2/omap_device.c +++ b/arch/arm/mach-omap2/omap_device.c @@ -621,6 +621,7 @@ static int _od_suspend_noirq(struct device *dev) if (!ret && !pm_runtime_status_suspended(dev)) { if (pm_generic_runtime_suspend(dev) == 0) { + pm_runtime_set_suspended(dev); omap_device_idle(pdev); od->flags |= OMAP_DEVICE_SUSPENDED; } @@ -634,10 +635,18 @@ static int _od_resume_noirq(struct device *dev) struct platform_device *pdev = to_platform_device(dev); struct omap_device *od = to_omap_device(pdev); - if ((od->flags & OMAP_DEVICE_SUSPENDED) && - !pm_runtime_status_suspended(dev)) { + if (od->flags & OMAP_DEVICE_SUSPENDED) { od->flags &= ~OMAP_DEVICE_SUSPENDED; omap_device_enable(pdev); + /* + * XXX: we run before core runtime pm has resumed itself. At + * this point in time, we just restore the runtime pm state and + * considering symmetric operations in resume, we donot expect + * to fail. If we failed, something changed in core runtime_pm + * framework OR some device driver messed things up, hence, WARN + */ + WARN(pm_runtime_set_active(dev), + "Could not set %s runtime state active\n", dev_name(dev)); pm_generic_runtime_resume(dev); }