From: Pratyush Yadav <me@yadavpratyush.com>
To: Yicong Yang <yangyicong@hisilicon.com>
Cc: vigneshr@ti.com, tudor.ambarus@microchip.com, richard@nod.at,
john.garry@huawei.com, linux-spi@vger.kernel.org,
broonie@kernel.org, linux-mtd@lists.infradead.org,
miquel.raynal@bootlin.com
Subject: Re: [RFC PATCH 3/3] spi: hisi-sfc-v3xx: Add prepare/unprepare methods to avoid race condition
Date: Mon, 25 May 2020 21:44:36 +0530 [thread overview]
Message-ID: <20200525161436.c5h6d27pm3jptwbo@yadavpratyush.com> (raw)
In-Reply-To: <1590060231-23242-4-git-send-email-yangyicong@hisilicon.com>
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@hisilicon.com>
> ---
> 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
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
next prev parent reply other threads:[~2020-05-25 16:15 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-21 11:23 [RFC PATCH 0/3] Add prepare/unprepare method in spi_controller_mem_ops Yicong Yang
2020-05-21 11:23 ` [RFC PATCH 1/3] spi: spi-mem: add optional prepare/unprepare methods " Yicong Yang
2020-05-21 11:23 ` [RFC PATCH 2/3] mtd: spi-nor: Add prepare/unprepare support for spimem device Yicong Yang
2020-05-21 11:23 ` [RFC PATCH 3/3] spi: hisi-sfc-v3xx: Add prepare/unprepare methods to avoid race condition Yicong Yang
2020-05-25 16:14 ` Pratyush Yadav [this message]
2020-05-26 9:27 ` Boris Brezillon
2020-05-26 9:30 ` Boris Brezillon
2020-05-27 8:18 ` Yicong Yang
2020-05-27 9:33 ` Pratyush Yadav
2020-05-27 10:33 ` Yicong Yang
2020-05-26 9:43 ` Boris Brezillon
2020-05-27 8:55 ` Yicong Yang
2020-05-27 9:20 ` Boris Brezillon
2020-05-27 10:16 ` Yicong Yang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200525161436.c5h6d27pm3jptwbo@yadavpratyush.com \
--to=me@yadavpratyush.com \
--cc=broonie@kernel.org \
--cc=john.garry@huawei.com \
--cc=linux-mtd@lists.infradead.org \
--cc=linux-spi@vger.kernel.org \
--cc=miquel.raynal@bootlin.com \
--cc=richard@nod.at \
--cc=tudor.ambarus@microchip.com \
--cc=vigneshr@ti.com \
--cc=yangyicong@hisilicon.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).