diff mbox

[3/7] spi: omap2-mcspi: Fix PM regression with deferred probe for pm_runtime_reinit

Message ID 1455145370-20301-4-git-send-email-tony@atomide.com (mailing list archive)
State Accepted
Commit 0e6f357a5deba4b81b1a65acabaa51f2cbd2e2cd
Headers show

Commit Message

Tony Lindgren Feb. 10, 2016, 11:02 p.m. UTC
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(+)

Comments

Mark Brown Feb. 11, 2016, 11:51 a.m. UTC | #1
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?
Tony Lindgren Feb. 11, 2016, 3:08 p.m. UTC | #2
* 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
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown Feb. 11, 2016, 3:52 p.m. UTC | #3
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?
Tony Lindgren Feb. 11, 2016, 5:36 p.m. UTC | #4
* 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
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown Feb. 11, 2016, 6:36 p.m. UTC | #5
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.
Tony Lindgren Feb. 11, 2016, 6:52 p.m. UTC | #6
* 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

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ulf Hansson Feb. 12, 2016, 12:51 p.m. UTC | #7
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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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);