diff mbox series

[v2,2/4] spi: cadence-qspi: fix pointer reference in runtime PM hooks

Message ID 20240205-cdns-qspi-pm-fix-v2-2-2e7bbad49a46@bootlin.com
State New
Headers
Series spi: cadence-qspi: Fix runtime PM and system-wide suspend |

Commit Message

Théo Lebrun Feb. 5, 2024, 2:57 p.m. UTC 
  dev_get_drvdata() gets used to acquire the pointer to cqspi and the SPI
controller. Neither embed the other; this lead to memory corruption.

On a given platform (Mobileye EyeQ5) the memory corruption is hidden
inside cqspi->f_pdata. Also, this uninitialised memory is used as a
mutex (ctlr->bus_lock_mutex) by spi_controller_suspend().

Fixes: 2087e85bb66e ("spi: cadence-quadspi: fix suspend-resume implementations")
Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/spi/spi-cadence-quadspi.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

Comments

Mark Brown Feb. 5, 2024, 3:12 p.m. UTC | #1 
On Mon, Feb 05, 2024 at 03:57:30PM +0100, Théo Lebrun wrote:
> dev_get_drvdata() gets used to acquire the pointer to cqspi and the SPI
> controller. Neither embed the other; this lead to memory corruption.
> 
> On a given platform (Mobileye EyeQ5) the memory corruption is hidden
> inside cqspi->f_pdata. Also, this uninitialised memory is used as a
> mutex (ctlr->bus_lock_mutex) by spi_controller_suspend().

Please place fixes at the start of serieses so that they don't end up
with spurious dependencies on other changes and can more easily be
applied as fixes.
Dhruva Gole Feb. 7, 2024, 8:39 a.m. UTC | #2 
Hi Mark,

On Feb 05, 2024 at 15:12:10 +0000, Mark Brown wrote:
> On Mon, Feb 05, 2024 at 03:57:30PM +0100, Théo Lebrun wrote:
> > dev_get_drvdata() gets used to acquire the pointer to cqspi and the SPI
> > controller. Neither embed the other; this lead to memory corruption.
> > 
> > On a given platform (Mobileye EyeQ5) the memory corruption is hidden
> > inside cqspi->f_pdata. Also, this uninitialised memory is used as a
> > mutex (ctlr->bus_lock_mutex) by spi_controller_suspend().
> 
> Please place fixes at the start of serieses so that they don't end up
> with spurious dependencies on other changes and can more easily be
> applied as fixes.

Didn't really understand the comment here, aren't the 1,2 and 3 patches
fixes and the last one the non-fix? Thus fixes are indeed placed at
start of this series right?

Can you help understand with some example series?
Dhruva Gole Feb. 7, 2024, 8:42 a.m. UTC | #3 
On Feb 05, 2024 at 15:57:30 +0100, Théo Lebrun wrote:
> dev_get_drvdata() gets used to acquire the pointer to cqspi and the SPI
> controller. Neither embed the other; this lead to memory corruption.
> 
> On a given platform (Mobileye EyeQ5) the memory corruption is hidden
> inside cqspi->f_pdata. Also, this uninitialised memory is used as a
> mutex (ctlr->bus_lock_mutex) by spi_controller_suspend().
> 
> Fixes: 2087e85bb66e ("spi: cadence-quadspi: fix suspend-resume implementations")
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> ---
>  drivers/spi/spi-cadence-quadspi.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
> index 720b28d2980c..1a27987638f0 100644
> --- a/drivers/spi/spi-cadence-quadspi.c
> +++ b/drivers/spi/spi-cadence-quadspi.c
> @@ -1930,10 +1930,9 @@ static void cqspi_remove(struct platform_device *pdev)
>  static int cqspi_runtime_suspend(struct device *dev)
>  {
>  	struct cqspi_st *cqspi = dev_get_drvdata(dev);
> -	struct spi_controller *host = dev_get_drvdata(dev);

Or you could do:
+	struct spi_controller *host = cqspi->host;

>  	int ret;
>  
> -	ret = spi_controller_suspend(host);
> +	ret = spi_controller_suspend(cqspi->host);

And avoid changing these?

>  	cqspi_controller_enable(cqspi, 0);
>  
>  	clk_disable_unprepare(cqspi->clk);
> @@ -1944,7 +1943,6 @@ static int cqspi_runtime_suspend(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,7 +1951,7 @@ static int cqspi_runtime_resume(struct device *dev)
>  	cqspi->current_cs = -1;
>  	cqspi->sclk = 0;
>  
> -	return spi_controller_resume(host);
> +	return spi_controller_resume(cqspi->host);

ditto.

Thanks,
Dhruva Gole <d-gole@ti.com>

>  }
>  
>  static DEFINE_RUNTIME_DEV_PM_OPS(cqspi_dev_pm_ops, cqspi_runtime_suspend,
> 
> -- 
> 2.43.0
> 
>
Théo Lebrun Feb. 7, 2024, 9:28 a.m. UTC | #4 
Hello,

On Wed Feb 7, 2024 at 9:42 AM CET, Dhruva Gole wrote:
> On Feb 05, 2024 at 15:57:30 +0100, Théo Lebrun wrote:
> > dev_get_drvdata() gets used to acquire the pointer to cqspi and the SPI
> > controller. Neither embed the other; this lead to memory corruption.
> > 
> > On a given platform (Mobileye EyeQ5) the memory corruption is hidden
> > inside cqspi->f_pdata. Also, this uninitialised memory is used as a
> > mutex (ctlr->bus_lock_mutex) by spi_controller_suspend().
> > 
> > Fixes: 2087e85bb66e ("spi: cadence-quadspi: fix suspend-resume implementations")
> > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> > ---
> >  drivers/spi/spi-cadence-quadspi.c | 6 ++----
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
> > index 720b28d2980c..1a27987638f0 100644
> > --- a/drivers/spi/spi-cadence-quadspi.c
> > +++ b/drivers/spi/spi-cadence-quadspi.c
> > @@ -1930,10 +1930,9 @@ static void cqspi_remove(struct platform_device *pdev)
> >  static int cqspi_runtime_suspend(struct device *dev)
> >  {
> >  	struct cqspi_st *cqspi = dev_get_drvdata(dev);
> > -	struct spi_controller *host = dev_get_drvdata(dev);
>
> Or you could do:
> +	struct spi_controller *host = cqspi->host;

Indeed. I preferred minimizing line count as I didn't see a benefit to
introducing a new variable. It goes away new patch anyway. If you
prefer it this way tell me and I'll fix it for next revision.

Thanks Dhruva,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Mark Brown Feb. 7, 2024, 9:50 a.m. UTC | #5 
On Wed, Feb 07, 2024 at 02:09:02PM +0530, Dhruva Gole wrote:
> On Feb 05, 2024 at 15:12:10 +0000, Mark Brown wrote:

> > Please place fixes at the start of serieses so that they don't end up
> > with spurious dependencies on other changes and can more easily be
> > applied as fixes.

> Didn't really understand the comment here, aren't the 1,2 and 3 patches
> fixes and the last one the non-fix? Thus fixes are indeed placed at
> start of this series right?

Patch 1 is a rename, this is obviously cosmetic and not a bug fix.
Dhruva Gole Feb. 7, 2024, 10:12 a.m. UTC | #6 
On Feb 07, 2024 at 10:28:59 +0100, Théo Lebrun wrote:
> Hello,
> 
> On Wed Feb 7, 2024 at 9:42 AM CET, Dhruva Gole wrote:
> > On Feb 05, 2024 at 15:57:30 +0100, Théo Lebrun wrote:
> > > dev_get_drvdata() gets used to acquire the pointer to cqspi and the SPI
> > > controller. Neither embed the other; this lead to memory corruption.
> > > 
> > > On a given platform (Mobileye EyeQ5) the memory corruption is hidden
> > > inside cqspi->f_pdata. Also, this uninitialised memory is used as a
> > > mutex (ctlr->bus_lock_mutex) by spi_controller_suspend().
> > > 
> > > Fixes: 2087e85bb66e ("spi: cadence-quadspi: fix suspend-resume implementations")
> > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> > > ---
> > >  drivers/spi/spi-cadence-quadspi.c | 6 ++----
> > >  1 file changed, 2 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
> > > index 720b28d2980c..1a27987638f0 100644
> > > --- a/drivers/spi/spi-cadence-quadspi.c
> > > +++ b/drivers/spi/spi-cadence-quadspi.c
> > > @@ -1930,10 +1930,9 @@ static void cqspi_remove(struct platform_device *pdev)
> > >  static int cqspi_runtime_suspend(struct device *dev)
> > >  {
> > >  	struct cqspi_st *cqspi = dev_get_drvdata(dev);
> > > -	struct spi_controller *host = dev_get_drvdata(dev);
> >
> > Or you could do:
> > +	struct spi_controller *host = cqspi->host;
> 
> Indeed. I preferred minimizing line count as I didn't see a benefit to
> introducing a new variable. It goes away new patch anyway. If you
> prefer it this way tell me and I'll fix it for next revision.

I mean since you're going to have to respin then do make this change, it
will further minimise the number of lines of change right?

It goes away in last patch but if atall in some older kernel only
suspend resume support is there then only this will get picked so it's
still not useless code.
Dhruva Gole Feb. 7, 2024, 10:14 a.m. UTC | #7 
Hey,

On Feb 07, 2024 at 09:50:16 +0000, Mark Brown wrote:
> On Wed, Feb 07, 2024 at 02:09:02PM +0530, Dhruva Gole wrote:
> > On Feb 05, 2024 at 15:12:10 +0000, Mark Brown wrote:
> 
> > > Please place fixes at the start of serieses so that they don't end up
> > > with spurious dependencies on other changes and can more easily be
> > > applied as fixes.
> 
> > Didn't really understand the comment here, aren't the 1,2 and 3 patches
> > fixes and the last one the non-fix? Thus fixes are indeed placed at
> > start of this series right?
> 
> Patch 1 is a rename, this is obviously cosmetic and not a bug fix.


Well, Theo, seems like you better fix the first patch, then reorder and
send a v3 :)
diff mbox series

Patch

diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
index 720b28d2980c..1a27987638f0 100644
--- a/drivers/spi/spi-cadence-quadspi.c
+++ b/drivers/spi/spi-cadence-quadspi.c
@@ -1930,10 +1930,9 @@  static void cqspi_remove(struct platform_device *pdev)
 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);
+	ret = spi_controller_suspend(cqspi->host);
 	cqspi_controller_enable(cqspi, 0);
 
 	clk_disable_unprepare(cqspi->clk);
@@ -1944,7 +1943,6 @@  static int cqspi_runtime_suspend(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,7 +1951,7 @@  static int cqspi_runtime_resume(struct device *dev)
 	cqspi->current_cs = -1;
 	cqspi->sclk = 0;
 
-	return spi_controller_resume(host);
+	return spi_controller_resume(cqspi->host);
 }
 
 static DEFINE_RUNTIME_DEV_PM_OPS(cqspi_dev_pm_ops, cqspi_runtime_suspend,