spi: cadence-qspi: stop calling system-wide PM helpers for runtime PM

  The ->runtime_suspend() and ->runtime_resume() callbacks are not
expected to call spi_controller_suspend() and spi_controller_resume().
Remove calls to those in the cadence-qspi driver.

Those helpers have two roles currently:
 - They stop/start the queue, including dealing with the kworker.
 - They toggle the SPI controller SPI_CONTROLLER_SUSPENDED flag. It
   requires acquiring ctlr->bus_lock_mutex.

The cadence-qspi ->exec_op() implementation bumps the usage counter at
its start. It might therefore run our ->runtime_resume()
implementation. However, ctlr->bus_lock_mutex is acquired by
spi_mem_exec_op() while ->exec_op() is being called.

Here is a brief call tree highlighting the issue:

spi_mem_exec_op()
        ...
        spi_mem_access_start()
                mutex_lock(&ctlr->bus_lock_mutex)

        cqspi_exec_mem_op()
                pm_runtime_resume_and_get()
                        cqspi_resume()
                                spi_controller_resume()
                                        mutex_lock(&ctlr->bus_lock_mutex)
                ...

        spi_mem_access_end()
                mutex_unlock(&ctlr->bus_lock_mutex)
        ...

The fatal conclusion of this is a deadlock: we acquire a lock on each
operation but while running the operation, we might want to runtime
resume and acquire the same lock.

Anyway, those helpers (spi_controller_{suspend,resume}) are aimed at
system-wide suspend and resume and should NOT be called at runtime
suspend & resume.

Side note: the previous implementation had a second issue. It acquired a
pointer to both `struct cqspi_st` and `struct spi_controller` using
dev_get_drvdata(). Neither embed the other. This lead to memory
corruption that was being hidden inside the big cqspi->f_pdata array on
my setup. It was working until I tried changing the array side to its
theorical max of 4, which lead to the discovery of this gnarly bug.

Fixes: 0578a6dbfe75 ("spi: spi-cadence-quadspi: add runtime pm support")
Fixes: 2087e85bb66e ("spi: cadence-quadspi: fix suspend-resume implementations")
Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
Hi,

This is a draft patch highlighting a serious bug in the
->runtime_suspend() and ->runtime_resume() implementations of
cadence-qspi. Seeing how runtime PM and autosuspend are enabled by
default, I believe this affects all users of the driver.

I've tried my best to be exhaustive in the commit message. Have I missed
something that could explain how the current implementations could have
been functional in the last few revisions of the kernel?

The MIPS platform at hand, used for debugging and testing, is currently
not supported by the driver. It is the Mobileye EyeQ5 [0]. No code
changes are required for support, only a new compatible and appropriate
match data + flags. That will come later, with some performance-related
patches.

Conclusion being: feedback from maintainers & others that know the
driver and subsystem would be useful to bring this forward.

Thanks all,
Théo

[0]: https://lore.kernel.org/lkml/20240118155252.397947-1-gregory.clement@bootlin.com/
---
 drivers/spi/spi-cadence-quadspi.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)


---
base-commit: 27470aa9b51a348f7edfb99641b5a9004f81e3e6
change-id: 20240202-cdns-qspi-pm-fix-29600cc6d7bf

Best regards,
  

Hello Théo,

theo.lebrun@bootlin.com wrote on Fri, 02 Feb 2024 18:29:40 +0100:

> The ->runtime_suspend() and ->runtime_resume() callbacks are not
> expected to call spi_controller_suspend() and spi_controller_resume().
> Remove calls to those in the cadence-qspi driver.
> 
> Those helpers have two roles currently:
>  - They stop/start the queue, including dealing with the kworker.
>  - They toggle the SPI controller SPI_CONTROLLER_SUSPENDED flag. It
>    requires acquiring ctlr->bus_lock_mutex.
> 
> The cadence-qspi ->exec_op() implementation bumps the usage counter at
> its start. It might therefore run our ->runtime_resume()
> implementation. However, ctlr->bus_lock_mutex is acquired by
> spi_mem_exec_op() while ->exec_op() is being called.
> 
> Here is a brief call tree highlighting the issue:
> 
> spi_mem_exec_op()
>         ...
>         spi_mem_access_start()
>                 mutex_lock(&ctlr->bus_lock_mutex)
> 
>         cqspi_exec_mem_op()
>                 pm_runtime_resume_and_get()
>                         cqspi_resume()
>                                 spi_controller_resume()
>                                         mutex_lock(&ctlr->bus_lock_mutex)
>                 ...
> 
>         spi_mem_access_end()
>                 mutex_unlock(&ctlr->bus_lock_mutex)
>         ...
> 
> The fatal conclusion of this is a deadlock: we acquire a lock on each
> operation but while running the operation, we might want to runtime
> resume and acquire the same lock.
> 
> Anyway, those helpers (spi_controller_{suspend,resume}) are aimed at
> system-wide suspend and resume and should NOT be called at runtime
> suspend & resume.
> 
> Side note: the previous implementation had a second issue. It acquired a
> pointer to both `struct cqspi_st` and `struct spi_controller` using
> dev_get_drvdata(). Neither embed the other. This lead to memory
> corruption that was being hidden inside the big cqspi->f_pdata array on
> my setup. It was working until I tried changing the array side to its
> theorical max of 4, which lead to the discovery of this gnarly bug.
> 
> Fixes: 0578a6dbfe75 ("spi: spi-cadence-quadspi: add runtime pm support")
> Fixes: 2087e85bb66e ("spi: cadence-quadspi: fix suspend-resume implementations")

Your commit log makes total sense but I believe the diff is gonna break
again the suspend to RAM operation. This is only my understanding
right after quickly going through the whole story, so maybe I'm
totally off topic.

What happened if I understand the two commits blamed above:

- There were PM hooks.
- Someone turned them into runtime PM hooks (breaking regular
  suspend/resume).
- Someone else added the "missing" suspend/resume logic inside the
  runtime PM hooks to fix suspend and resume.
- You are removing this logic because it leads to deadlocks.

There was likely a misconception of what is expected in both cases
(quick and small power savings vs. full power cycle/loosing the whole
configuration).

I would propose instead to create two distinct set of functions:
- One for runtime PM
- One for suspend/resume
This way the runtime PM no longer deadlocks and people using
suspend/resume won't get affected? I don't know if your runtime hooks
*will* always be called during a suspend/resume. I hope so, which would
make the split quite easy and without any code duplication.

Thanks,
Miquèl

> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> ---
> Hi,
> 
> This is a draft patch highlighting a serious bug in the
> ->runtime_suspend() and ->runtime_resume() implementations of
> cadence-qspi. Seeing how runtime PM and autosuspend are enabled by
> default, I believe this affects all users of the driver.
> 
> I've tried my best to be exhaustive in the commit message. Have I missed
> something that could explain how the current implementations could have
> been functional in the last few revisions of the kernel?
> 
> The MIPS platform at hand, used for debugging and testing, is currently
> not supported by the driver. It is the Mobileye EyeQ5 [0]. No code
> changes are required for support, only a new compatible and appropriate
> match data + flags. That will come later, with some performance-related
> patches.
> 
> Conclusion being: feedback from maintainers & others that know the
> driver and subsystem would be useful to bring this forward.
> 
> Thanks all,
> Théo
> 
> [0]: https://lore.kernel.org/lkml/20240118155252.397947-1-gregory.clement@bootlin.com/
> ---
>  drivers/spi/spi-cadence-quadspi.c | 18 ++++++------------
>  1 file changed, 6 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
> index 74647dfcb86c..72f80c77ee35 100644
> --- a/drivers/spi/spi-cadence-quadspi.c
> +++ b/drivers/spi/spi-cadence-quadspi.c
> @@ -1927,24 +1927,18 @@ static void cqspi_remove(struct platform_device *pdev)
>  	pm_runtime_disable(&pdev->dev);
>  }
>  
> -static int cqspi_suspend(struct device *dev)
> +static int cqspi_runtime_suspend(struct device *dev)
>  {
>  	struct cqspi_st *cqspi = dev_get_drvdata(dev);
> -	struct spi_controller *host = dev_get_drvdata(dev);
> -	int ret;
>  
> -	ret = spi_controller_suspend(host);
>  	cqspi_controller_enable(cqspi, 0);
> -
>  	clk_disable_unprepare(cqspi->clk);
> -
> -	return ret;
> +	return 0;
>  }
>  
> -static int cqspi_resume(struct device *dev)
> +static int cqspi_runtime_resume(struct device *dev)
>  {
>  	struct cqspi_st *cqspi = dev_get_drvdata(dev);
> -	struct spi_controller *host = dev_get_drvdata(dev);
>  
>  	clk_prepare_enable(cqspi->clk);
>  	cqspi_wait_idle(cqspi);
> @@ -1953,11 +1947,11 @@ static int cqspi_resume(struct device *dev)
>  	cqspi->current_cs = -1;
>  	cqspi->sclk = 0;
>  
> -	return spi_controller_resume(host);
> +	return 0;
>  }
>  
> -static DEFINE_RUNTIME_DEV_PM_OPS(cqspi_dev_pm_ops, cqspi_suspend,
> -				 cqspi_resume, NULL);
> +static DEFINE_RUNTIME_DEV_PM_OPS(cqspi_dev_pm_ops, cqspi_runtime_suspend,
> +				 cqspi_runtime_resume, NULL);
>  
>  static const struct cqspi_driver_platdata cdns_qspi = {
>  	.quirks = CQSPI_DISABLE_DAC_MODE,
> 
> ---
> base-commit: 27470aa9b51a348f7edfb99641b5a9004f81e3e6
> change-id: 20240202-cdns-qspi-pm-fix-29600cc6d7bf
> 
> Best regards,
  

Hi,

On Mon Feb 5, 2024 at 10:03 AM CET, Miquel Raynal wrote:
> Hello Théo,
>
> theo.lebrun@bootlin.com wrote on Fri, 02 Feb 2024 18:29:40 +0100:
>
> > The ->runtime_suspend() and ->runtime_resume() callbacks are not
> > expected to call spi_controller_suspend() and spi_controller_resume().
> > Remove calls to those in the cadence-qspi driver.
> > 
> > Those helpers have two roles currently:
> >  - They stop/start the queue, including dealing with the kworker.
> >  - They toggle the SPI controller SPI_CONTROLLER_SUSPENDED flag. It
> >    requires acquiring ctlr->bus_lock_mutex.
> > 
> > The cadence-qspi ->exec_op() implementation bumps the usage counter at
> > its start. It might therefore run our ->runtime_resume()
> > implementation. However, ctlr->bus_lock_mutex is acquired by
> > spi_mem_exec_op() while ->exec_op() is being called.
> > 
> > Here is a brief call tree highlighting the issue:
> > 
> > spi_mem_exec_op()
> >         ...
> >         spi_mem_access_start()
> >                 mutex_lock(&ctlr->bus_lock_mutex)
> > 
> >         cqspi_exec_mem_op()
> >                 pm_runtime_resume_and_get()
> >                         cqspi_resume()
> >                                 spi_controller_resume()
> >                                         mutex_lock(&ctlr->bus_lock_mutex)
> >                 ...
> > 
> >         spi_mem_access_end()
> >                 mutex_unlock(&ctlr->bus_lock_mutex)
> >         ...
> > 
> > The fatal conclusion of this is a deadlock: we acquire a lock on each
> > operation but while running the operation, we might want to runtime
> > resume and acquire the same lock.
> > 
> > Anyway, those helpers (spi_controller_{suspend,resume}) are aimed at
> > system-wide suspend and resume and should NOT be called at runtime
> > suspend & resume.
> > 
> > Side note: the previous implementation had a second issue. It acquired a
> > pointer to both `struct cqspi_st` and `struct spi_controller` using
> > dev_get_drvdata(). Neither embed the other. This lead to memory
> > corruption that was being hidden inside the big cqspi->f_pdata array on
> > my setup. It was working until I tried changing the array side to its
> > theorical max of 4, which lead to the discovery of this gnarly bug.
> > 
> > Fixes: 0578a6dbfe75 ("spi: spi-cadence-quadspi: add runtime pm support")
> > Fixes: 2087e85bb66e ("spi: cadence-quadspi: fix suspend-resume implementations")
>
> Your commit log makes total sense but I believe the diff is gonna break
> again the suspend to RAM operation. This is only my understanding
> right after quickly going through the whole story, so maybe I'm
> totally off topic.

The current ->runtime_suspend() implementation would indeed (probably)
work for suspend-to-RAM if it wasn't for the wrong pointers to cqspi
and spi_controller (see side note from commit message).

I've not found a moment where `struct cqspi_st` embed `struct
spi_controller` at its start, so I do not believe this has ever worked.
It might be the result of a mistake while porting a patch from a branch
that included other changes.

> What happened if I understand the two commits blamed above:
>
> - There were PM hooks.
> - Someone turned them into runtime PM hooks (breaking regular
>   suspend/resume).
> - Someone else added the "missing" suspend/resume logic inside the
>   runtime PM hooks to fix suspend and resume.
> - You are removing this logic because it leads to deadlocks.
>
> There was likely a misconception of what is expected in both cases
> (quick and small power savings vs. full power cycle/loosing the whole
> configuration).
>
> I would propose instead to create two distinct set of functions:
> - One for runtime PM
> - One for suspend/resume
> This way the runtime PM no longer deadlocks and people using
> suspend/resume won't get affected? I don't know if your runtime hooks
> *will* always be called during a suspend/resume. I hope so, which would
> make the split quite easy and without any code duplication.

That does indeed sound like the right approach. Runtime hooks can be
called from suspend/resume if needs be. Runtime PM then gets disabled
at the late stage.

I do not believe currently system-wide suspend can be working.
spi_controller_{suspend,resume} are being called with a bogus pointer.
This makes me ask: should the system-wide suspend/resume part be
addressed with this patch or a follow-up? It feels like a separate
concern to me.

The nice thing is that I have easy access to J7200, which uses the same
controller and supports suspend-to-RAM. That should make it a good test
setup.

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
  

Hi Théo,

> > > The fatal conclusion of this is a deadlock: we acquire a lock on each
> > > operation but while running the operation, we might want to runtime
> > > resume and acquire the same lock.
> > > 
> > > Anyway, those helpers (spi_controller_{suspend,resume}) are aimed at
> > > system-wide suspend and resume and should NOT be called at runtime
> > > suspend & resume.
> > > 
> > > Side note: the previous implementation had a second issue. It acquired a
> > > pointer to both `struct cqspi_st` and `struct spi_controller` using
> > > dev_get_drvdata(). Neither embed the other. This lead to memory
> > > corruption that was being hidden inside the big cqspi->f_pdata array on
> > > my setup. It was working until I tried changing the array side to its
> > > theorical max of 4, which lead to the discovery of this gnarly bug.
> > > 
> > > Fixes: 0578a6dbfe75 ("spi: spi-cadence-quadspi: add runtime pm support")
> > > Fixes: 2087e85bb66e ("spi: cadence-quadspi: fix suspend-resume implementations")  
> >
> > Your commit log makes total sense but I believe the diff is gonna break
> > again the suspend to RAM operation. This is only my understanding
> > right after quickly going through the whole story, so maybe I'm
> > totally off topic.  
> 
> The current ->runtime_suspend() implementation would indeed (probably)
> work for suspend-to-RAM if it wasn't for the wrong pointers to cqspi
> and spi_controller (see side note from commit message).

Yeah, this probably needs to be fixed aside.

> I've not found a moment where `struct cqspi_st` embed `struct
> spi_controller` at its start, so I do not believe this has ever worked.
> It might be the result of a mistake while porting a patch from a branch
> that included other changes.
> 
> > What happened if I understand the two commits blamed above:
> >
> > - There were PM hooks.
> > - Someone turned them into runtime PM hooks (breaking regular
> >   suspend/resume).
> > - Someone else added the "missing" suspend/resume logic inside the
> >   runtime PM hooks to fix suspend and resume.
> > - You are removing this logic because it leads to deadlocks.
> >
> > There was likely a misconception of what is expected in both cases
> > (quick and small power savings vs. full power cycle/loosing the whole
> > configuration).
> >
> > I would propose instead to create two distinct set of functions:
> > - One for runtime PM
> > - One for suspend/resume
> > This way the runtime PM no longer deadlocks and people using
> > suspend/resume won't get affected? I don't know if your runtime hooks
> > *will* always be called during a suspend/resume. I hope so, which would
> > make the split quite easy and without any code duplication.  
> 
> That does indeed sound like the right approach. Runtime hooks can be
> called from suspend/resume if needs be. Runtime PM then gets disabled
> at the late stage.

Would make sense indeed.

> I do not believe currently system-wide suspend can be working.
> spi_controller_{suspend,resume} are being called with a bogus pointer.
> This makes me ask: should the system-wide suspend/resume part be
> addressed with this patch or a follow-up? It feels like a separate
> concern to me.

Probably two patches, yes.

Thanks,
Miquèl
  

Hello,

On Feb 05, 2024 at 11:12:54 +0100, Miquel Raynal wrote:
> Hi Théo,
> 
> > > > The fatal conclusion of this is a deadlock: we acquire a lock on each
> > > > operation but while running the operation, we might want to runtime
> > > > resume and acquire the same lock.
> > > > 
> > > > Anyway, those helpers (spi_controller_{suspend,resume}) are aimed at
> > > > system-wide suspend and resume and should NOT be called at runtime
> > > > suspend & resume.
> > > > 
> > > > Side note: the previous implementation had a second issue. It acquired a
> > > > pointer to both `struct cqspi_st` and `struct spi_controller` using
> > > > dev_get_drvdata(). Neither embed the other. This lead to memory

Oops, I seem to have overlooked this. I think it should've been
spi_controller_get_devdata()

> > > > corruption that was being hidden inside the big cqspi->f_pdata array on
> > > > my setup. It was working until I tried changing the array side to its
> > > > theorical max of 4, which lead to the discovery of this gnarly bug.
> > > > 
> > > > Fixes: 0578a6dbfe75 ("spi: spi-cadence-quadspi: add runtime pm support")
> > > > Fixes: 2087e85bb66e ("spi: cadence-quadspi: fix suspend-resume implementations")  

Thanks for the fixes.

> > >
> > > Your commit log makes total sense but I believe the diff is gonna break
> > > again the suspend to RAM operation. This is only my understanding
> > > right after quickly going through the whole story, so maybe I'm
> > > totally off topic.  
> > 
> > The current ->runtime_suspend() implementation would indeed (probably)
> > work for suspend-to-RAM if it wasn't for the wrong pointers to cqspi
> > and spi_controller (see side note from commit message).
> 
> Yeah, this probably needs to be fixed aside.
> 
> > I've not found a moment where `struct cqspi_st` embed `struct
> > spi_controller` at its start, so I do not believe this has ever worked.

I don't know how it worked either, but I had definitely tested and
provided logs at the time of posting the series,

https://lore.kernel.org/all/20230417091027.966146-1-d-gole@ti.com/

> > It might be the result of a mistake while porting a patch from a branch
> > that included other changes.

Hmm, could be, not entirely sure now. But I did test it and now don't
know how it had worked with that wrong pointer now that I see that
mistake.

> > 
> > > What happened if I understand the two commits blamed above:
> > >
> > > - There were PM hooks.
> > > - Someone turned them into runtime PM hooks (breaking regular
> > >   suspend/resume).
> > > - Someone else added the "missing" suspend/resume logic inside the
> > >   runtime PM hooks to fix suspend and resume.
> > > - You are removing this logic because it leads to deadlocks.
> > >
> > > There was likely a misconception of what is expected in both cases
> > > (quick and small power savings vs. full power cycle/loosing the whole
> > > configuration).

The context was as follows,

The upstream cqspi driver prior to this:
https://lore.kernel.org/all/20230417091027.966146-1-d-gole@ti.com/
series had buggy suspend resume. That needed fixing hence I added the
first patch that introduced the buggy pointer but somehow still ended up
working after suspend resume.

After that, I also wanted the driver to support runtime_pm. I thought
that both system suspend and runtime pm would have similar requirements
from a driver POV since the IP essentially would turn off and from it's
view would need system suspend like suspend resume calls.

> > >
> > > I would propose instead to create two distinct set of functions:
> > > - One for runtime PM
> > > - One for suspend/resume
> > > This way the runtime PM no longer deadlocks and people using
> > > suspend/resume won't get affected? I don't know if your runtime hooks
> > > *will* always be called during a suspend/resume. I hope so, which would
> > > make the split quite easy and without any code duplication.  
> > 
> > That does indeed sound like the right approach. Runtime hooks can be
> > called from suspend/resume if needs be. Runtime PM then gets disabled
> > at the late stage.
> 
> Would make sense indeed.

Now that I look at it, perhaps it is best to have 2 seperate calls for
runtime and system pm.

> 
> > I do not believe currently system-wide suspend can be working.
> > spi_controller_{suspend,resume} are being called with a bogus pointer.
> > This makes me ask: should the system-wide suspend/resume part be
> > addressed with this patch or a follow-up? It feels like a separate
> > concern to me.
> 
> Probably two patches, yes.

Yes, I think it best that we add a proper system suspend and runtime pm
support for this driver.

Again, thanks for catching this bug and reporting a fix. I also have an
SK-AM62 handy which uses this ospi controller so let me see if I can
help test your patches with system and runtime pm as well whenever you
do post them.