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: Wed, 27 May 2020 15:03:25 +0530 [thread overview]
Message-ID: <20200527093325.247l6tnxaicsqdst@yadavpratyush.com> (raw)
In-Reply-To: <6a41fb13-e746-54f3-24ef-197384dde6ab@hisilicon.com>
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@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
> >> @@ -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
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
next prev parent reply other threads:[~2020-05-27 9:33 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
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 [this message]
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=20200527093325.247l6tnxaicsqdst@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).