Received-SPF: pass (google.com: domain of linux-kernel+bounces-56289-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Wed, 7 Feb 2024 15:42:01 +0530
From: Dhruva Gole <d-gole@ti.com>
To: =?utf-8?B?VGjDqW8=?= Lebrun <theo.lebrun@bootlin.com>
CC: Mark Brown <broonie@kernel.org>, Apurva Nandan <a-nandan@ti.com>,
        <linux-spi@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        Gregory CLEMENT
	<gregory.clement@bootlin.com>,
        Vladimir Kondratiev
	<vladimir.kondratiev@mobileye.com>,
        Thomas Petazzoni
	<thomas.petazzoni@bootlin.com>,
        Tawfik Bayouk <tawfik.bayouk@mobileye.com>
Subject: Re: [PATCH v2 2/4] spi: cadence-qspi: fix pointer reference in
 runtime PM hooks
Message-ID: <20240207101201.64cpxbns2gnqtoxi@dhruva>
References: <20240205-cdns-qspi-pm-fix-v2-0-2e7bbad49a46@bootlin.com>
 <20240205-cdns-qspi-pm-fix-v2-2-2e7bbad49a46@bootlin.com>
 <20240207084253.fxrnoskda5x6usqo@dhruva>
 <CYYQOHJ4783D.39ZQKS2IK40RV@bootlin.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset="iso-8859-1"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CYYQOHJ4783D.39ZQKS2IK40RV@bootlin.com>

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.


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