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

[Pratyush Yadav <me@xxxxxxxxxxxxxxxxx>]

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;

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
> @@ -18,6 +18,7 @@
>  #define HISI_SFC_V3XX_VERSION (0x1f8)
>  
>  #define HISI_SFC_V3XX_CMD_CFG (0x300)
> +#define HISI_SFC_V3XX_CMD_CFG_LOCK BIT(20)
>  #define HISI_SFC_V3XX_CMD_CFG_DUAL_IN_DUAL_OUT (1 << 17)
>  #define HISI_SFC_V3XX_CMD_CFG_DUAL_IO (2 << 17)
>  #define HISI_SFC_V3XX_CMD_CFG_FULL_DIO (3 << 17)
> @@ -41,6 +42,34 @@ struct hisi_sfc_v3xx_host {
>  	int max_cmd_dword;
>  };
>  
> +int hisi_sfc_v3xx_op_prepare(struct spi_mem *mem)
> +{
> +	struct spi_device *spi = mem->spi;
> +	struct hisi_sfc_v3xx_host *host;
> +	u32 reg = HISI_SFC_V3XX_CMD_CFG_LOCK;
> +
> +	host = spi_controller_get_devdata(spi->master);
> +
> +	writel(reg, host->regbase + HISI_SFC_V3XX_CMD_CFG);
> +
> +	reg = readl(host->regbase + HISI_SFC_V3XX_CMD_CFG);
> +	if (!(reg & HISI_SFC_V3XX_CMD_CFG_LOCK))
> +		return -EIO;

IIUC, you are checking if you actually got the lock, and you won't get 
the lock if the firmware is using the controller. So, is it a good idea 
to give up so easily? Maybe we should do this in a loop at some 
intervals, and only error out when we reach a number of failed attempts?

> +
> +	return 0;
> +}
> +
> +void hisi_sfc_v3xx_op_unprepare(struct spi_mem *mem)
> +{
> +	struct spi_device *spi = mem->spi;
> +	struct hisi_sfc_v3xx_host *host;
> +
> +	host = spi_controller_get_devdata(spi->master);
> +
> +	/* Release the lock and clear the command register. */
> +	writel(0, host->regbase + HISI_SFC_V3XX_CMD_CFG);
> +}
> +
>  #define HISI_SFC_V3XX_WAIT_TIMEOUT_US		1000000
>  #define HISI_SFC_V3XX_WAIT_POLL_INTERVAL_US	10
>  
> @@ -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?
  
>  	if (op->addr.nbytes)
>  		config |= HISI_SFC_V3XX_CMD_CFG_ADDR_EN_MSK;
> @@ -248,6 +285,8 @@ static int hisi_sfc_v3xx_exec_op(struct spi_mem *mem,
>  
>  static const struct spi_controller_mem_ops hisi_sfc_v3xx_mem_ops = {
>  	.adjust_op_size = hisi_sfc_v3xx_adjust_op_size,
> +	.prepare	= hisi_sfc_v3xx_op_prepare,
> +	.unprepare	= hisi_sfc_v3xx_op_unprepare,
>  	.exec_op = hisi_sfc_v3xx_exec_op,
>  };
>  

FWIW, the other two patches in the series look good to me given you can 
justify the need for having the API.

-- 
Regards,
Pratyush Yadav