[PATCH v2 8/9] mtd: spi-nor: Enhance locking to support reads while writes
Miquel Raynal
miquel.raynal at bootlin.com
Thu Dec 1 02:54:40 PST 2022
Hello,
miquel.raynal at bootlin.com wrote on Thu, 10 Nov 2022 16:55:12 +0100:
> On devices featuring several banks, the Read While Write (RWW) feature
> is here to improve the overall performance when performing parallel
> reads and writes at different locations (different banks). The following
> constraints have to be taken into account:
> 1#: A single operation can be performed in a given bank.
> 2#: Only a single program or erase operation can happen on the entire chip.
> 3#: The I/O bus is unique and thus is the most constrained resource, all
> spi-nor operations requiring access to the spi bus (through the spi
> controller) must be serialized until the bus exchanges are over. So
> we must ensure a single operation can be "sent" at a time.
> 4#: Any other operation that would not be either a read or a write or an
> erase is considered requiring access to the full chip and cannot be
> parallelized, we then need to ensure the full chip is in the idle
> state when this occurs.
>
> All these constraints can easily be managed with a proper locking model:
> 1#: Is protected by a per-bank mutex. Only a single operation can happen
> in a specific bank at any times. If the bank mutex is not available,
> the operation cannot start.
I just discovered and fixed a bug in this version related to the erase
path, the indexes (at/len) may in some cases be updated by the
function, leading to the locks not being released as expected. I just
discovered that while playing with UBI. I will soon provide a v3 with
this fixed (I am testing now with io_paral which seems happy).
Thanks,
Miquèl
> 2#: Is handled by the pe_mode mutex which is acquired before any write
> or erase, and is released only at the very end of the
> operation. This way, no other destructive operation on the chip can
> start during this time frame.
> 3#: A device-wide mutex is introduced in order to capture and serialize
> bus accessed. This is the one being released "sooner" than before,
> because we only need to protect the chip against other SPI accesses
> during the I/O phase, which for the destructive operations is the
> beginning of the operation (when we send the command cycles and
> eventually the data), while the second part of the operation (the
> erase delay or the programmation delay) is when we can do something
> else with another bank.
> 4#: Is handled by the "generic" helpers which existed before, where
> basically all the locks are taken before the operation can start,
> and all locks are released once done.
>
> As many devices still do not support this feature, the original lock is
> also kept in a union: either the feature is available and we initialize
> and use the new locks, or it is not and we keep using the previous
> logic.
>
> Signed-off-by: Miquel Raynal <miquel.raynal at bootlin.com>
> ---
> drivers/mtd/spi-nor/core.c | 145 ++++++++++++++++++++++++++++++++----
> include/linux/mtd/spi-nor.h | 12 ++-
> 2 files changed, 143 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index dfda350d5056..b467d5bf0f2a 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -1086,6 +1086,61 @@ static void spi_nor_unprep(struct spi_nor *nor)
> nor->controller_ops->unprepare(nor);
> }
>
> +static bool spi_nor_parallel_locking(struct spi_nor *nor)
> +{
> + return nor->info->n_banks > 1 && nor->info->no_sfdp_flags & SPI_NOR_RWW;
> +}
> +
> +static void spi_nor_lock_all_banks(struct spi_nor *nor)
> +{
> + int i;
> +
> + for (i = 0; i < nor->info->n_banks; i++)
> + mutex_lock(&nor->locks.bank[i]);
> +}
> +
> +static void spi_nor_unlock_all_banks(struct spi_nor *nor)
> +{
> + int i;
> +
> + for (i = nor->info->n_banks; i >= 0; i--)
> + mutex_unlock(&nor->locks.bank[i]);
> +}
> +
> +static void spi_nor_lock_banks(struct spi_nor *nor, loff_t start, size_t len)
> +{
> + int first, last, i;
> +
> + first = DIV_ROUND_DOWN_ULL(start, nor->params->bank_size);
> + last = DIV_ROUND_DOWN_ULL(start + len - 1, nor->params->bank_size);
> +
> + for (i = first; i <= last; i++)
> + mutex_lock(&nor->locks.bank[i]);
> +}
> +
> +static void spi_nor_unlock_banks(struct spi_nor *nor, loff_t start, size_t len)
> +{
> + int first, last, i;
> +
> + first = DIV_ROUND_DOWN_ULL(start, nor->params->bank_size);
> + last = DIV_ROUND_DOWN_ULL(start + len - 1, nor->params->bank_size);
> +
> + for (i = last; i >= first; i--)
> + mutex_unlock(&nor->locks.bank[i]);
> +}
> +
> +static void spi_nor_lock_device(struct spi_nor *nor)
> +{
> + if (spi_nor_parallel_locking(nor))
> + mutex_lock(&nor->locks.device);
> +}
> +
> +static void spi_nor_unlock_device(struct spi_nor *nor)
> +{
> + if (spi_nor_parallel_locking(nor))
> + mutex_unlock(&nor->locks.device);
> +}
> +
> /* Generic helpers for internal locking and serialization */
> int spi_nor_lock_and_prep(struct spi_nor *nor)
> {
> @@ -1095,14 +1150,26 @@ int spi_nor_lock_and_prep(struct spi_nor *nor)
> if (ret)
> return ret;
>
> - mutex_lock(&nor->lock);
> + if (!spi_nor_parallel_locking(nor)) {
> + mutex_lock(&nor->lock);
> + } else {
> + spi_nor_lock_all_banks(nor);
> + mutex_lock(&nor->locks.pe_mode);
> + mutex_lock(&nor->locks.device);
> + }
>
> return 0;
> }
>
> void spi_nor_unlock_and_unprep(struct spi_nor *nor)
> {
> - mutex_unlock(&nor->lock);
> + if (!spi_nor_parallel_locking(nor)) {
> + mutex_unlock(&nor->lock);
> + } else {
> + mutex_unlock(&nor->locks.device);
> + mutex_unlock(&nor->locks.pe_mode);
> + spi_nor_unlock_all_banks(nor);
> + }
>
> spi_nor_unprep(nor);
> }
> @@ -1116,14 +1183,24 @@ static int spi_nor_lock_and_prep_pe(struct spi_nor *nor, loff_t start, size_t le
> if (ret)
> return ret;
>
> - mutex_lock(&nor->lock);
> + if (!spi_nor_parallel_locking(nor)) {
> + mutex_lock(&nor->lock);
> + } else {
> + spi_nor_lock_banks(nor, start, len);
> + mutex_lock(&nor->locks.pe_mode);
> + }
>
> return 0;
> }
>
> static void spi_nor_unlock_and_unprep_pe(struct spi_nor *nor, loff_t start, size_t len)
> {
> - mutex_unlock(&nor->lock);
> + if (!spi_nor_parallel_locking(nor)) {
> + mutex_unlock(&nor->lock);
> + } else {
> + mutex_unlock(&nor->locks.pe_mode);
> + spi_nor_unlock_banks(nor, start, len);
> + }
>
> spi_nor_unprep(nor);
> }
> @@ -1137,14 +1214,20 @@ static int spi_nor_lock_and_prep_rd(struct spi_nor *nor, loff_t start, size_t le
> if (ret)
> return ret;
>
> - mutex_lock(&nor->lock);
> + if (!spi_nor_parallel_locking(nor))
> + mutex_lock(&nor->lock);
> + else
> + spi_nor_lock_banks(nor, start, len);
>
> return 0;
> }
>
> static void spi_nor_unlock_and_unprep_rd(struct spi_nor *nor, loff_t start, size_t len)
> {
> - mutex_unlock(&nor->lock);
> + if (!spi_nor_parallel_locking(nor))
> + mutex_unlock(&nor->lock);
> + else
> + spi_nor_unlock_banks(nor, start, len);
>
> spi_nor_unprep(nor);
> }
> @@ -1451,11 +1534,16 @@ static int spi_nor_erase_multi_sectors(struct spi_nor *nor, u64 addr, u32 len)
> dev_vdbg(nor->dev, "erase_cmd->size = 0x%08x, erase_cmd->opcode = 0x%02x, erase_cmd->count = %u\n",
> cmd->size, cmd->opcode, cmd->count);
>
> + spi_nor_lock_device(nor);
> +
> ret = spi_nor_write_enable(nor);
> - if (ret)
> + if (ret) {
> + spi_nor_unlock_device(nor);
> goto destroy_erase_cmd_list;
> + }
>
> ret = spi_nor_erase_sector(nor, addr);
> + spi_nor_unlock_device(nor);
> if (ret)
> goto destroy_erase_cmd_list;
>
> @@ -1508,11 +1596,16 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr)
> if (len == mtd->size && !(nor->flags & SNOR_F_NO_OP_CHIP_ERASE)) {
> unsigned long timeout;
>
> + spi_nor_lock_device(nor);
> +
> ret = spi_nor_write_enable(nor);
> - if (ret)
> + if (ret) {
> + spi_nor_unlock_device(nor);
> goto erase_err;
> + }
>
> ret = spi_nor_erase_chip(nor);
> + spi_nor_unlock_device(nor);
> if (ret)
> goto erase_err;
>
> @@ -1537,11 +1630,16 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr)
> /* "sector"-at-a-time erase */
> } else if (spi_nor_has_uniform_erase(nor)) {
> while (len) {
> + spi_nor_lock_device(nor);
> +
> ret = spi_nor_write_enable(nor);
> - if (ret)
> + if (ret) {
> + spi_nor_unlock_device(nor);
> goto erase_err;
> + }
>
> ret = spi_nor_erase_sector(nor, addr);
> + spi_nor_unlock_device(nor);
> if (ret)
> goto erase_err;
>
> @@ -1733,11 +1831,13 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len,
> size_t *retlen, u_char *buf)
> {
> struct spi_nor *nor = mtd_to_spi_nor(mtd);
> + loff_t from_lock = from;
> + size_t len_lock = len;
> ssize_t ret;
>
> dev_dbg(nor->dev, "from 0x%08x, len %zd\n", (u32)from, len);
>
> - ret = spi_nor_lock_and_prep_rd(nor, from, len);
> + ret = spi_nor_lock_and_prep_rd(nor, from_lock, len_lock);
> if (ret)
> return ret;
>
> @@ -1746,7 +1846,9 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len,
>
> addr = spi_nor_convert_addr(nor, addr);
>
> + spi_nor_lock_device(nor);
> ret = spi_nor_read_data(nor, addr, len, buf);
> + spi_nor_unlock_device(nor);
> if (ret == 0) {
> /* We shouldn't see 0-length reads */
> ret = -EIO;
> @@ -1764,7 +1866,7 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len,
> ret = 0;
>
> read_err:
> - spi_nor_unlock_and_unprep_rd(nor, from, len);
> + spi_nor_unlock_and_unprep_rd(nor, from_lock, len_lock);
>
> return ret;
> }
> @@ -1809,11 +1911,16 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len,
>
> addr = spi_nor_convert_addr(nor, addr);
>
> + spi_nor_lock_device(nor);
> +
> ret = spi_nor_write_enable(nor);
> - if (ret)
> + if (ret) {
> + spi_nor_unlock_device(nor);
> goto write_err;
> + }
>
> ret = spi_nor_write_data(nor, addr, page_remain, buf + i);
> + spi_nor_unlock_device(nor);
> if (ret < 0)
> goto write_err;
> written = ret;
> @@ -3030,7 +3137,19 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
>
> nor->info = info;
>
> - mutex_init(&nor->lock);
> + if (!spi_nor_parallel_locking(nor)) {
> + mutex_init(&nor->lock);
> + } else {
> + mutex_init(&nor->locks.device);
> + mutex_init(&nor->locks.pe_mode);
> + nor->locks.bank = kmalloc(sizeof(struct mutex) * nor->info->n_banks,
> + GFP_KERNEL);
> + if (!nor->locks.bank)
> + return -ENOMEM;
> +
> + for (i = 0; i < nor->info->n_banks; i++)
> + mutex_init(&nor->locks.bank[i]);
> + }
>
> /* Init flash parameters based on flash_info struct and SFDP */
> ret = spi_nor_init_params(nor);
> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> index 42218a1164f6..02c9a3cf924e 100644
> --- a/include/linux/mtd/spi-nor.h
> +++ b/include/linux/mtd/spi-nor.h
> @@ -344,6 +344,9 @@ struct spi_nor_flash_parameter;
> * struct spi_nor - Structure for defining the SPI NOR layer
> * @mtd: an mtd_info structure
> * @lock: the lock for the read/write/erase/lock/unlock operations
> + * @locks.device: the lock for the I/O bus
> + * @locks.pe_mode: the lock for the program/erase mode for RWW operations
> + * @locks.bank: the lock for every available bank
> * @dev: pointer to an SPI device or an SPI NOR controller device
> * @spimem: pointer to the SPI memory device
> * @bouncebuf: bounce buffer used when the buffer passed by the MTD
> @@ -374,7 +377,14 @@ struct spi_nor_flash_parameter;
> */
> struct spi_nor {
> struct mtd_info mtd;
> - struct mutex lock;
> + union {
> + struct mutex lock;
> + struct {
> + struct mutex device;
> + struct mutex pe_mode;
> + struct mutex *bank;
> + } locks;
> + };
> struct device *dev;
> struct spi_mem *spimem;
> u8 *bouncebuf;
More information about the linux-mtd
mailing list