Re: [RFC PATCH 3/3] spi: hisi-sfc-v3xx: Add prepare/unprepare methods to avoid race condition

[Pratyush Yadav <me@xxxxxxxxxxxxxxxxx>]

On 27/05/20 04:18PM, Yicong Yang wrote:
> Hi Pratyush,
> 
> On 2020/5/26 0:14, Pratyush Yadav wrote:
> > Hi Yicong,
> >
> > On 21/05/20 07:23PM, Yicong Yang wrote:
> >> The controller can be shared with the firmware, which may cause race
> >> problems. As most read/write/erase/lock/unlock of spi-nor flash are
> >> composed of a set of operations, while the firmware may use the controller
> >> and start its own operation in the middle of the process started by the
> >> kernel driver, which may lead to the kernel driver's function broken.
> >>
> >> Bit[20] in HISI_SFC_V3XX_CMD_CFG register plays a role of a lock, to
> >> protect the controller from firmware access, which means the firmware
> >> cannot reach the controller if the driver set the bit. Add prepare/
> >> unprepare methods for the controller, we'll hold the lock in prepare
> >> method and release it in unprepare method, which will solve the race
> >> issue.
> > I'm trying to understand the need for this change. What's wrong with
> > performing the lock/unlock procedure in hisi_sfc_v3xx_exec_op()? You can 
> > probably do something like:
> >
> >   hisi_sfc_v3xx_lock();
> >   ret = hisi_sfc_v3xx_generic_exec_op(host, op, chip_select);
> >   hisi_sfc_v3xx_unlock();
> >   return ret;
> 
> if doing like this, suppose we perform a sequential operations like below:
> 
> lock()->exec_op(cmd1)->unlock()->lock()->exec_op(cmd2)->unlock()->lock()->exec_op(cmd3)->unlock()
>                        ^==========^is unlocked          ^==========^is unlocked
> 
> As shown above, we cannot lock the device continuously during the whole operations.

Correct. My argument is based on the assumption that lock() and unlock() 
are cheap/fast operations. If you spend very little time in lock() and 
unlock(), it doesn't make a big difference if you do all 3 operations in 
one go or one at a time.

In other words, since register write should be pretty fast, locking and 
unlocking should be pretty fast. If we don't spend a lot of time in 
lock() and unlock(), we don't gain a lot of performance by reducing 
those calls.

> But if we use upper layer method then it looks like
> 
> prepare()->exec_op(cmd1)->exec_op(cmd2)->exec_op(cmd3)->unprepare()
>         ^locked here                                              ^unlocked here
> 
> we can hold the lock during the all 3 operations' execution.

If you still think doing all operations in one go is a better idea, I  
like Boris's idea of batching operations and its worth considering.
 
> > What's the benefit of making upper layers do this? Acquiring the lock is 
> > a simple register write, so it should be relatively fast. Unless there 
> > is a lot of contention on the lock between the firmware and kernel, I 
> > would expect the performance impact to be minimal. Maybe you can run 
> > some benchmarks and see if there is a real difference.
> >
> >> Signed-off-by: Yicong Yang <yangyicong@xxxxxxxxxxxxx>
> >> ---
> >>  drivers/spi/spi-hisi-sfc-v3xx.c | 41 ++++++++++++++++++++++++++++++++++++++++-
> >>  1 file changed, 40 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/spi/spi-hisi-sfc-v3xx.c b/drivers/spi/spi-hisi-sfc-v3xx.c
> >> index e3b5725..13c161c 100644
> >> --- a/drivers/spi/spi-hisi-sfc-v3xx.c
> >> +++ b/drivers/spi/spi-hisi-sfc-v3xx.c
> >> @@ -163,7 +192,15 @@ static int hisi_sfc_v3xx_generic_exec_op(struct hisi_sfc_v3xx_host *host,
> >>  					 u8 chip_select)
> >>  {
> >>  	int ret, len = op->data.nbytes;
> >> -	u32 config = 0;
> >> +	u32 config;
> >> +
> >> +	/*
> >> +	 * The lock bit is in the command register. Clear the command
> >> +	 * field with lock bit held if it has been set in
> >> +	 * .prepare().
> >> +	 */
> >> +	config = readl(host->regbase + HISI_SFC_V3XX_CMD_CFG);
> >> +	config &= HISI_SFC_V3XX_CMD_CFG_LOCK;
> > This will unlock the controller _before_ the driver issues 
> > hisi_sfc_v3xx_read_databuf(). I'm not very familiar with the hardware, 
> > but to me it seems like it can lead to a race. What if the firmware 
> > issues a command that over-writes the databuf (I assume this is shared 
> > between the two) before the driver gets a chance to copy that data to 
> > the kernel buffer?
> 
> It won't unlock the controller if it has been locked in prepare(). It will clear
> the other bits in the register other than the lock bit. For single operations, as 
> prepare() method is not called, the bit is 0 and it won't change here.

Right. I misread the code. Sorry.

-- 
Regards,
Pratyush Yadav