| Message ID | 1455145370-20301-4-git-send-email-tony@atomide.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Feb 10, 2016 at 03:02:46PM -0800, Tony Lindgren wrote: > Mark, I'd like to merge this along with other related fixes via > the ARM SoC tree if no objections, please review and ack if this > look OK to you. Why, I'm not seeing any dependencies here? I'd also expect to be seeing an awful lot more changes like this, this shouldn't be an OMAP thing - do we need to fix the core API and then roll out the transition in a different way?
* Mark Brown <broonie@kernel.org> [160211 03:52]: > On Wed, Feb 10, 2016 at 03:02:46PM -0800, Tony Lindgren wrote: > > > Mark, I'd like to merge this along with other related fixes via > > the ARM SoC tree if no objections, please review and ack if this > > look OK to you. > > Why, I'm not seeing any dependencies here? I'd also expect to be seeing > an awful lot more changes like this, this shouldn't be an OMAP thing - > do we need to fix the core API and then roll out the transition in a > different way? Please feel free to pick up this one if you have other fixes lined up, nothing stopping that. There will be likely more fixes like this to make drivers follow the PM runtime API documentation. These were just the initial ones that were obvious. Regards, Tony
On Thu, Feb 11, 2016 at 07:08:06AM -0800, Tony Lindgren wrote: > * Mark Brown <broonie@kernel.org> [160211 03:52]: > > Why, I'm not seeing any dependencies here? I'd also expect to be seeing > > an awful lot more changes like this, this shouldn't be an OMAP thing - > > do we need to fix the core API and then roll out the transition in a > > different way? > Please feel free to pick up this one if you have other fixes > lined up, nothing stopping that. I'll take it myself unless there is a strong reason to merge it as part of another series. > There will be likely more fixes like this to make drivers follow > the PM runtime API documentation. These were just the initial > ones that were obvious. This does sound like there's been a change in the interface compared to what users are actually doing - is this an actual problem or is it just a divergence from docs?
* Mark Brown <broonie@kernel.org> [160211 07:54]: > On Thu, Feb 11, 2016 at 07:08:06AM -0800, Tony Lindgren wrote: > > * Mark Brown <broonie@kernel.org> [160211 03:52]: > > > > Why, I'm not seeing any dependencies here? I'd also expect to be seeing > > > an awful lot more changes like this, this shouldn't be an OMAP thing - > > > do we need to fix the core API and then roll out the transition in a > > > different way? > > > Please feel free to pick up this one if you have other fixes > > lined up, nothing stopping that. > > I'll take it myself unless there is a strong reason to merge it as part > of another series. OK thanks! > > There will be likely more fixes like this to make drivers follow > > the PM runtime API documentation. These were just the initial > > ones that were obvious. > > This does sound like there's been a change in the interface compared to > what users are actually doing - is this an actual problem or is it just > a divergence from docs? It's an actual problem at least on omaps as the omap_device code is very picky about the hardware state. Depending how the PM runtime is implemented, it may be a problem for some other cases too. For non-omap cases, my guess is that it's mostly a divergence from the docs and non-critical. Regards, Tony
On Thu, Feb 11, 2016 at 09:36:20AM -0800, Tony Lindgren wrote: > * Mark Brown <broonie@kernel.org> [160211 07:54]: > > This does sound like there's been a change in the interface compared to > > what users are actually doing - is this an actual problem or is it just > > a divergence from docs? > It's an actual problem at least on omaps as the omap_device code > is very picky about the hardware state. > Depending how the PM runtime is implemented, it may be a problem > for some other cases too. Or people just aren't testing mainline that well (if this is broken in v4.5 that suggests nobody noticed in -next) - do you know when this broke? It really seems like we may need to spin round on how this is deployed.
* Mark Brown <broonie@kernel.org> [160211 10:38]: > On Thu, Feb 11, 2016 at 09:36:20AM -0800, Tony Lindgren wrote: > > * Mark Brown <broonie@kernel.org> [160211 07:54]: > > > > This does sound like there's been a change in the interface compared to > > > what users are actually doing - is this an actual problem or is it just > > > a divergence from docs? > > > It's an actual problem at least on omaps as the omap_device code > > is very picky about the hardware state. > > > Depending how the PM runtime is implemented, it may be a problem > > for some other cases too. > > Or people just aren't testing mainline that well (if this is broken in > v4.5 that suggests nobody noticed in -next) - do you know when this > broke? It really seems like we may need to spin round on how this is > deployed. I noticed it only with v4.5-rc1 and bisected it to commit 5de85b9d57ab ("PM / runtime: Re-init runtime PM states at probe error and driver unbind") as it broke my PM tests for n900. For Linux next probably was also broken for before that for some time. Unfortunately based on the regressions I'm chasing every merge window I'm suspecting that very few people are actually testing PM runtime with mainline Linux kernel. Or they don't have the PM runtime fully implemented in the mainline kernel for their devices. This is at least for the SoC PM use case. Ulf is working on a generic PM runtime test driver :) I'm hoping we can then use that for basic regression testing on in an arch independent way. Regards, Tony
On 11 February 2016 at 00:02, Tony Lindgren <tony@atomide.com> wrote: > Commit 5de85b9d57ab ("PM / runtime: Re-init runtime PM states at probe > error and driver unbind") introduced pm_runtime_reinit() that is used > to reinitialize PM runtime after -EPROBE_DEFER. This allows shutting > down the device after a failed probe. > > However, for drivers using pm_runtime_use_autosuspend() this can cause > a state where suspend callback is never called after -EPROBE_DEFER. > On the following device driver probe, hardware state is different from > the PM runtime state causing omap_device to produce the following > error: > > omap_device_enable() called from invalid state 1 > > And with omap_device and omap hardware being picky for PM, this will > block any deeper idle states in hardware. > > The solution is to fix the drivers to follow the PM runtime documentation: > > 1. For sections of code that needs the device disabled, use > pm_runtime_put_sync_suspend() if pm_runtime_set_autosuspend() has > been set. > > 2. For driver exit code, use pm_runtime_dont_use_autosuspend() before > pm_runtime_put_sync() if pm_runtime_use_autosuspend() has been > set. > > Fixes: 5de85b9d57ab ("PM / runtime: Re-init runtime PM states at probe > error and driver unbind") > Cc: linux-spi@vger.kernel.org > Cc: Alan Stern <stern@rowland.harvard.edu> > Cc: Kevin Hilman <khilman@baylibre.com> > Cc: Mark Brown <broonie@kernel.org> > Cc: Nishanth Menon <nm@ti.com> > Cc: Rafael J. Wysocki <rafael@kernel.org> > Cc: Ulf Hansson <ulf.hansson@linaro.org> > Cc: Tero Kristo <t-kristo@ti.com> > Signed-off-by: Tony Lindgren <tony@atomide.com> Acked-by: Ulf Hansson <ulf.hansson@linaro.org> Kind regards Uffe > --- > > Mark, I'd like to merge this along with other related fixes via > the ARM SoC tree if no objections, please review and ack if this > look OK to you. > > > --- > drivers/spi/spi-omap2-mcspi.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/spi/spi-omap2-mcspi.c b/drivers/spi/spi-omap2-mcspi.c > index 7273820..0caa3c8 100644 > --- a/drivers/spi/spi-omap2-mcspi.c > +++ b/drivers/spi/spi-omap2-mcspi.c > @@ -1490,6 +1490,8 @@ static int omap2_mcspi_probe(struct platform_device *pdev) > return status; > > disable_pm: > + pm_runtime_dont_use_autosuspend(&pdev->dev); > + pm_runtime_put_sync(&pdev->dev); > pm_runtime_disable(&pdev->dev); > free_master: > spi_master_put(master); > @@ -1501,6 +1503,7 @@ static int omap2_mcspi_remove(struct platform_device *pdev) > struct spi_master *master = platform_get_drvdata(pdev); > struct omap2_mcspi *mcspi = spi_master_get_devdata(master); > > + pm_runtime_dont_use_autosuspend(mcspi->dev); > pm_runtime_put_sync(mcspi->dev); > pm_runtime_disable(&pdev->dev); > > -- > 2.7.0 >
diff --git a/drivers/spi/spi-omap2-mcspi.c b/drivers/spi/spi-omap2-mcspi.c index 7273820..0caa3c8 100644 --- a/drivers/spi/spi-omap2-mcspi.c +++ b/drivers/spi/spi-omap2-mcspi.c @@ -1490,6 +1490,8 @@ static int omap2_mcspi_probe(struct platform_device *pdev) return status; disable_pm: + pm_runtime_dont_use_autosuspend(&pdev->dev); + pm_runtime_put_sync(&pdev->dev); pm_runtime_disable(&pdev->dev); free_master: spi_master_put(master); @@ -1501,6 +1503,7 @@ static int omap2_mcspi_remove(struct platform_device *pdev) struct spi_master *master = platform_get_drvdata(pdev); struct omap2_mcspi *mcspi = spi_master_get_devdata(master); + pm_runtime_dont_use_autosuspend(mcspi->dev); pm_runtime_put_sync(mcspi->dev); pm_runtime_disable(&pdev->dev);
Commit 5de85b9d57ab ("PM / runtime: Re-init runtime PM states at probe error and driver unbind") introduced pm_runtime_reinit() that is used to reinitialize PM runtime after -EPROBE_DEFER. This allows shutting down the device after a failed probe. However, for drivers using pm_runtime_use_autosuspend() this can cause a state where suspend callback is never called after -EPROBE_DEFER. On the following device driver probe, hardware state is different from the PM runtime state causing omap_device to produce the following error: omap_device_enable() called from invalid state 1 And with omap_device and omap hardware being picky for PM, this will block any deeper idle states in hardware. The solution is to fix the drivers to follow the PM runtime documentation: 1. For sections of code that needs the device disabled, use pm_runtime_put_sync_suspend() if pm_runtime_set_autosuspend() has been set. 2. For driver exit code, use pm_runtime_dont_use_autosuspend() before pm_runtime_put_sync() if pm_runtime_use_autosuspend() has been set. Fixes: 5de85b9d57ab ("PM / runtime: Re-init runtime PM states at probe error and driver unbind") Cc: linux-spi@vger.kernel.org Cc: Alan Stern <stern@rowland.harvard.edu> Cc: Kevin Hilman <khilman@baylibre.com> Cc: Mark Brown <broonie@kernel.org> Cc: Nishanth Menon <nm@ti.com> Cc: Rafael J. Wysocki <rafael@kernel.org> Cc: Ulf Hansson <ulf.hansson@linaro.org> Cc: Tero Kristo <t-kristo@ti.com> Signed-off-by: Tony Lindgren <tony@atomide.com> --- Mark, I'd like to merge this along with other related fixes via the ARM SoC tree if no objections, please review and ack if this look OK to you. --- drivers/spi/spi-omap2-mcspi.c | 3 +++ 1 file changed, 3 insertions(+)