* [RFC PATCH 0/3] Add prepare/unprepare method in spi_controller_mem_ops @ 2020-05-21 11:23 Yicong Yang 2020-05-21 11:23 ` [RFC PATCH 1/3] spi: spi-mem: add optional prepare/unprepare methods " Yicong Yang ` (2 more replies) 0 siblings, 3 replies; 14+ messages in thread From: Yicong Yang @ 2020-05-21 11:23 UTC (permalink / raw) To: broonie, tudor.ambarus, linux-spi, linux-mtd Cc: richard, john.garry, vigneshr, miquel.raynal In this series: - Add optional prepare/unprepare methods in spi_controller_mem_ops to allow the controller do specific works before/after a set of operations, like spi-nor flash's read/write/erase/lock/unlock. This is equivalent mechanism to spi_nor_controller_ops' prepare/unprepare methods. - Implement prepare/unprepare methods for hisi-sfc-v3xx controller. This will solve a race condition between the kernel driver and firmware. Yicong Yang (3): spi: spi-mem: add optional prepare/unprepare methods in spi_controller_mem_ops mtd: spi-nor: Add prepare/unprepare support for spimem device spi: hisi-sfc-v3xx: Add prepare/unprepare methods to avoid race condition drivers/mtd/spi-nor/core.c | 8 ++++++-- drivers/spi/spi-hisi-sfc-v3xx.c | 41 ++++++++++++++++++++++++++++++++++++++++- drivers/spi/spi-mem.c | 20 ++++++++++++++++++++ include/linux/spi/spi-mem.h | 11 +++++++++++ 4 files changed, 77 insertions(+), 3 deletions(-) -- 2.8.1 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 14+ messages in thread
* [RFC PATCH 1/3] spi: spi-mem: add optional prepare/unprepare methods in spi_controller_mem_ops 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 ` 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 2 siblings, 0 replies; 14+ messages in thread From: Yicong Yang @ 2020-05-21 11:23 UTC (permalink / raw) To: broonie, tudor.ambarus, linux-spi, linux-mtd Cc: richard, john.garry, vigneshr, miquel.raynal Some prepare/unprepare works may be necessary before/after a set of operations of the spimem device. For example, before/after spi-nor flash's read/write/erase/lock/unlock. Add optional prepare/unprepare methods in spi_controller_mem_ops to allow the controller to do specific works after/before the operations. The upper user can use spi_mem_{prepare, unprepare}() to call the methods of the controller. Signed-off-by: Yicong Yang <yangyicong@hisilicon.com> --- drivers/spi/spi-mem.c | 20 ++++++++++++++++++++ include/linux/spi/spi-mem.h | 11 +++++++++++ 2 files changed, 31 insertions(+) diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c index adaa0c4..f09cef1 100644 --- a/drivers/spi/spi-mem.c +++ b/drivers/spi/spi-mem.c @@ -220,6 +220,26 @@ bool spi_mem_supports_op(struct spi_mem *mem, const struct spi_mem_op *op) } EXPORT_SYMBOL_GPL(spi_mem_supports_op); +int spi_mem_prepare(struct spi_mem *mem) +{ + struct spi_controller *ctlr = mem->spi->controller; + + if (ctlr->mem_ops && ctlr->mem_ops->prepare) + return ctlr->mem_ops->prepare(mem); + + return 0; +} +EXPORT_SYMBOL_GPL(spi_mem_prepare); + +void spi_mem_unprepare(struct spi_mem *mem) +{ + struct spi_controller *ctlr = mem->spi->controller; + + if (ctlr->mem_ops && ctlr->mem_ops->unprepare) + ctlr->mem_ops->unprepare(mem); +} +EXPORT_SYMBOL_GPL(spi_mem_unprepare); + static int spi_mem_access_start(struct spi_mem *mem) { struct spi_controller *ctlr = mem->spi->controller; diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h index af9ff2f..e40b5c3 100644 --- a/include/linux/spi/spi-mem.h +++ b/include/linux/spi/spi-mem.h @@ -215,6 +215,12 @@ static inline void *spi_mem_get_drvdata(struct spi_mem *mem) * limitations) * @supports_op: check if an operation is supported by the controller * @exec_op: execute a SPI memory operation + * @prepare: do some prepare works before a set of operations. for example, + * read/write/erase/lock/unlock operations of spi-nor flash. This + * method is optional. + * @unprepare: do some post works after a set of operations. for example, + * read/write/erase/lock/unlock operations of spi-nor flash. This + * method is optional. * @get_name: get a custom name for the SPI mem device from the controller. * This might be needed if the controller driver has been ported * to use the SPI mem layer and a custom name is used to keep @@ -253,6 +259,8 @@ struct spi_controller_mem_ops { int (*adjust_op_size)(struct spi_mem *mem, struct spi_mem_op *op); bool (*supports_op)(struct spi_mem *mem, const struct spi_mem_op *op); + int (*prepare)(struct spi_mem *mem); + void (*unprepare)(struct spi_mem *mem); int (*exec_op)(struct spi_mem *mem, const struct spi_mem_op *op); const char *(*get_name)(struct spi_mem *mem); @@ -329,6 +337,9 @@ int spi_mem_adjust_op_size(struct spi_mem *mem, struct spi_mem_op *op); bool spi_mem_supports_op(struct spi_mem *mem, const struct spi_mem_op *op); +int spi_mem_prepare(struct spi_mem *mem); +void spi_mem_unprepare(struct spi_mem *mem); + int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op); -- 2.8.1 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [RFC PATCH 2/3] mtd: spi-nor: Add prepare/unprepare support for spimem device 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 ` 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 2 siblings, 0 replies; 14+ messages in thread From: Yicong Yang @ 2020-05-21 11:23 UTC (permalink / raw) To: broonie, tudor.ambarus, linux-spi, linux-mtd Cc: richard, john.garry, vigneshr, miquel.raynal spi-nor flash's read/write/erase/lock/unlock may be composed of a set of operations, and some prepare/unprepare works need to be done before/ after these operations in spi_nor_{lock, unlock}_and_{prep, unprep}(). Previously we only call spi-nor controllers' prepare/unprepare method in the functions, without spimem devices'. Add spimem devices' prepare/unprepare support. Call spi_mem_{prepare, unprepare}() function if it's a spimem device. Signed-off-by: Yicong Yang <yangyicong@hisilicon.com> --- drivers/mtd/spi-nor/core.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c index cc68ea8..3a7e40a 100644 --- a/drivers/mtd/spi-nor/core.c +++ b/drivers/mtd/spi-nor/core.c @@ -1103,7 +1103,9 @@ int spi_nor_lock_and_prep(struct spi_nor *nor) mutex_lock(&nor->lock); - if (nor->controller_ops && nor->controller_ops->prepare) { + if (nor->spimem) { + ret = spi_mem_prepare(nor->spimem); + } else if (nor->controller_ops && nor->controller_ops->prepare) { ret = nor->controller_ops->prepare(nor); if (ret) { mutex_unlock(&nor->lock); @@ -1115,7 +1117,9 @@ int spi_nor_lock_and_prep(struct spi_nor *nor) void spi_nor_unlock_and_unprep(struct spi_nor *nor) { - if (nor->controller_ops && nor->controller_ops->unprepare) + if (nor->spimem) + spi_mem_unprepare(nor->spimem); + else if (nor->controller_ops && nor->controller_ops->unprepare) nor->controller_ops->unprepare(nor); mutex_unlock(&nor->lock); } -- 2.8.1 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [RFC PATCH 3/3] spi: hisi-sfc-v3xx: Add prepare/unprepare methods to avoid race condition 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 ` Yicong Yang 2020-05-25 16:14 ` Pratyush Yadav 2020-05-26 9:43 ` Boris Brezillon 2 siblings, 2 replies; 14+ messages in thread From: Yicong Yang @ 2020-05-21 11:23 UTC (permalink / raw) To: broonie, tudor.ambarus, linux-spi, linux-mtd Cc: richard, john.garry, vigneshr, miquel.raynal 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. 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; + + 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; 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, }; -- 2.8.1 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [RFC PATCH 3/3] spi: hisi-sfc-v3xx: Add prepare/unprepare methods to avoid race condition 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-27 8:18 ` Yicong Yang 2020-05-26 9:43 ` Boris Brezillon 1 sibling, 2 replies; 14+ messages in thread From: Pratyush Yadav @ 2020-05-25 16:14 UTC (permalink / raw) To: Yicong Yang Cc: vigneshr, tudor.ambarus, richard, john.garry, linux-spi, broonie, linux-mtd, miquel.raynal 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/ ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC PATCH 3/3] spi: hisi-sfc-v3xx: Add prepare/unprepare methods to avoid race condition 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 1 sibling, 1 reply; 14+ messages in thread From: Boris Brezillon @ 2020-05-26 9:27 UTC (permalink / raw) To: Pratyush Yadav Cc: vigneshr, tudor.ambarus, richard, john.garry, linux-spi, broonie, linux-mtd, miquel.raynal, Yicong Yang On Mon, 25 May 2020 21:44:36 +0530 Pratyush Yadav <me@yadavpratyush.com> 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; > > 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? Like Pratyush said, I don't see why you need to expose new prepare/unprepare steps. Looks like something entirely controller specific. ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC PATCH 3/3] spi: hisi-sfc-v3xx: Add prepare/unprepare methods to avoid race condition 2020-05-26 9:27 ` Boris Brezillon @ 2020-05-26 9:30 ` Boris Brezillon 0 siblings, 0 replies; 14+ messages in thread From: Boris Brezillon @ 2020-05-26 9:30 UTC (permalink / raw) To: Pratyush Yadav Cc: vigneshr, tudor.ambarus, richard, john.garry, linux-spi, broonie, linux-mtd, miquel.raynal, Yicong Yang On Tue, 26 May 2020 11:27:52 +0200 Boris Brezillon <boris.brezillon@collabora.com> wrote: > On Mon, 25 May 2020 21:44:36 +0530 > Pratyush Yadav <me@yadavpratyush.com> 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; > > > > 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? > > Like Pratyush said, I don't see why you need to expose new > prepare/unprepare steps. Looks like something entirely controller > specific. Sorry, this comment is misplaced, just like my understanding of the problem :-). ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC PATCH 3/3] spi: hisi-sfc-v3xx: Add prepare/unprepare methods to avoid race condition 2020-05-25 16:14 ` Pratyush Yadav 2020-05-26 9:27 ` Boris Brezillon @ 2020-05-27 8:18 ` Yicong Yang 2020-05-27 9:33 ` Pratyush Yadav 1 sibling, 1 reply; 14+ messages in thread From: Yicong Yang @ 2020-05-27 8:18 UTC (permalink / raw) To: Pratyush Yadav Cc: vigneshr, tudor.ambarus, richard, john.garry, linux-spi, broonie, linux-mtd, miquel.raynal 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. 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. > > 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? yes. It do give up so early here. :) > >> + >> + 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? 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. Thanks, Yicong > >> 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. > ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC PATCH 3/3] spi: hisi-sfc-v3xx: Add prepare/unprepare methods to avoid race condition 2020-05-27 8:18 ` Yicong Yang @ 2020-05-27 9:33 ` Pratyush Yadav 2020-05-27 10:33 ` Yicong Yang 0 siblings, 1 reply; 14+ messages in thread From: Pratyush Yadav @ 2020-05-27 9:33 UTC (permalink / raw) To: Yicong Yang Cc: vigneshr, tudor.ambarus, richard, john.garry, linux-spi, broonie, linux-mtd, miquel.raynal 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/ ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC PATCH 3/3] spi: hisi-sfc-v3xx: Add prepare/unprepare methods to avoid race condition 2020-05-27 9:33 ` Pratyush Yadav @ 2020-05-27 10:33 ` Yicong Yang 0 siblings, 0 replies; 14+ messages in thread From: Yicong Yang @ 2020-05-27 10:33 UTC (permalink / raw) To: Pratyush Yadav Cc: vigneshr, tudor.ambarus, richard, john.garry, linux-spi, broonie, linux-mtd, miquel.raynal On 2020/5/27 17:33, Pratyush Yadav wrote: > 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. okay. we'd better not make such assumption and do what hardware suggests. > > 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. I know your worries. But it won't reduce the performance as we only do lock and unlock in the beginning or end. See what have implemented in spi-nor framework, as for read: ->spi_nor_read() --->spi_nor_lock_and_prep() // lock the device if necessary --->spi_nor_read_data() // maybe called several times to read wanted bytes --->spi_nor_unlock_and_unprep() // unlock the device we don't call lock/unlock at every spi_nor_read_data(), but just in the beginning /ending of the whole sequence. And we can do the same thing in nand framework to avoid performance reduction, if prepare/unprepare is also needed. > >> 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. sure. it do worth discussion and maybe we need more suggestions. > >>> 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. > ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC PATCH 3/3] spi: hisi-sfc-v3xx: Add prepare/unprepare methods to avoid race condition 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:43 ` Boris Brezillon 2020-05-27 8:55 ` Yicong Yang 1 sibling, 1 reply; 14+ messages in thread From: Boris Brezillon @ 2020-05-26 9:43 UTC (permalink / raw) To: Yicong Yang Cc: vigneshr, tudor.ambarus, richard, john.garry, linux-spi, broonie, linux-mtd, miquel.raynal On Thu, 21 May 2020 19:23:51 +0800 Yicong Yang <yangyicong@hisilicon.com> 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. Okay, so it looks like what we really need is a way to pass sequences (multiple operations) that are expected to be issued without interruptions. I'd prefer extending the spi_mem interface to allow that: int spi_mem_exec_sequence(struct spi_mem *spimem, unsigned int num_ops, const struct spi_mem_op *ops); struct spi_controller_mem_ops { ... int (*exec_sequence)(struct spi_mem *mem, unsigned int num_ops, const struct spi_mem_op *op); ... }; The prepare/unprepare hooks are a bit too vague. Alternatively, we could add functions to grab/release the controller lock, but I'm not sure that's what we want since some controllers might be able to address several devices in parallel, and locking the whole controller at the spi-nor level would prevent that. BTW, I don't know all the details about this lock or what this FW is exactly (where it's running, what's his priority, what kind of synchronization exists between Linux and the FW, ...), but I'm worried about potential deadlocks here. > > 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; > + > + 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; > > 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, > }; > ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC PATCH 3/3] spi: hisi-sfc-v3xx: Add prepare/unprepare methods to avoid race condition 2020-05-26 9:43 ` Boris Brezillon @ 2020-05-27 8:55 ` Yicong Yang 2020-05-27 9:20 ` Boris Brezillon 0 siblings, 1 reply; 14+ messages in thread From: Yicong Yang @ 2020-05-27 8:55 UTC (permalink / raw) To: Boris Brezillon Cc: vigneshr, tudor.ambarus, richard, john.garry, linux-spi, broonie, linux-mtd, miquel.raynal Hi Boris, On 2020/5/26 17:43, Boris Brezillon wrote: > On Thu, 21 May 2020 19:23:51 +0800 > Yicong Yang <yangyicong@hisilicon.com> 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. > Okay, so it looks like what we really need is a way to pass sequences > (multiple operations) that are expected to be issued without > interruptions. I'd prefer extending the spi_mem interface to allow that: > > int spi_mem_exec_sequence(struct spi_mem *spimem, > unsigned int num_ops, > const struct spi_mem_op *ops); > > struct spi_controller_mem_ops { > ... > int (*exec_sequence)(struct spi_mem *mem, > unsigned int num_ops, > const struct spi_mem_op *op); > ... > }; The prepare/unprepare hooks is just like what spi_nor_controller_ops provides. Alternatively we can use the interface you suggested, and it'll require upper layer(spi-nor framework, etc) to pack the operations before call spi_mem_exec_sequence(). > > The prepare/unprepare hooks are a bit too vague. Alternatively, we > could add functions to grab/release the controller lock, but I'm not > sure that's what we want since some controllers might be able to address > several devices in parallel, and locking the whole controller at the > spi-nor level would prevent that. I suppose the method is optional and device may choose to use it or not following their own design. And the implementation is rather controller specific, they may choose to lock the whole controller or only the desired device to operate. > > BTW, I don't know all the details about this lock or what this FW is > exactly (where it's running, what's his priority, what kind of > synchronization exists between Linux and the FW, ...), but I'm worried > about potential deadlocks here. For SFC controller, both firmware and the kernel driver will require the lock before a sequence of operations, and single operations like register access for spi-nor flash is implemented atomically. Once the lock is held by firmware/driver, then the controller cannot perform the operations sent by the other one unless the lock is released. Thanks, Yicong > >> 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; >> + >> + 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; >> >> 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, >> }; >> > . > ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC PATCH 3/3] spi: hisi-sfc-v3xx: Add prepare/unprepare methods to avoid race condition 2020-05-27 8:55 ` Yicong Yang @ 2020-05-27 9:20 ` Boris Brezillon 2020-05-27 10:16 ` Yicong Yang 0 siblings, 1 reply; 14+ messages in thread From: Boris Brezillon @ 2020-05-27 9:20 UTC (permalink / raw) To: Yicong Yang Cc: vigneshr, tudor.ambarus, richard, john.garry, linux-spi, broonie, linux-mtd, miquel.raynal On Wed, 27 May 2020 16:55:00 +0800 Yicong Yang <yangyicong@hisilicon.com> wrote: > Hi Boris, > > > On 2020/5/26 17:43, Boris Brezillon wrote: > > On Thu, 21 May 2020 19:23:51 +0800 > > Yicong Yang <yangyicong@hisilicon.com> 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. > > Okay, so it looks like what we really need is a way to pass sequences > > (multiple operations) that are expected to be issued without > > interruptions. I'd prefer extending the spi_mem interface to allow that: > > > > int spi_mem_exec_sequence(struct spi_mem *spimem, > > unsigned int num_ops, > > const struct spi_mem_op *ops); > > > > struct spi_controller_mem_ops { > > ... > > int (*exec_sequence)(struct spi_mem *mem, > > unsigned int num_ops, > > const struct spi_mem_op *op); > > ... > > }; > > The prepare/unprepare hooks is just like what spi_nor_controller_ops provides. > Alternatively we can use the interface you suggested, and it'll require > upper layer(spi-nor framework, etc) to pack the operations before call > spi_mem_exec_sequence(). We have to patch the upper layers anyway, right? > > > > > > The prepare/unprepare hooks are a bit too vague. Alternatively, we > > could add functions to grab/release the controller lock, but I'm not > > sure that's what we want since some controllers might be able to address > > several devices in parallel, and locking the whole controller at the > > spi-nor level would prevent that. > > I suppose the method is optional and device may choose to use it or not > following their own design. And the implementation is rather controller > specific, they may choose to lock the whole controller or only the desired > device to operate. Yes, this is what I'm complaining about. How can the upper layer know when it should call prepare/unprepare? Let's take the SPI NAND case, should we prepare before loading a page in the cache and unprepare after we're done reading the page, or should we unprepare just after the page has been loaded in the cache? BTW, you've not patched the SPI NAND layer to call ->prepare/unprepare(). > > > > > > BTW, I don't know all the details about this lock or what this FW is > > exactly (where it's running, what's his priority, what kind of > > synchronization exists between Linux and the FW, ...), but I'm worried > > about potential deadlocks here. > > For SFC controller, both firmware and the kernel driver will require the > lock before a sequence of operations, and single operations like register > access for spi-nor flash is implemented atomically. Once the lock is held > by firmware/driver, then the controller cannot perform the operations sent > by the other one unless the lock is released. Yes, that's my point. What prevents the FW from preempting Linux while it's holding the lock and waiting indefinitely on this lock. Is the FW running on a separate core? Don't you have other IPs with the same kind of locks leading to issues if locks are not taken/released in the same order? ... ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC PATCH 3/3] spi: hisi-sfc-v3xx: Add prepare/unprepare methods to avoid race condition 2020-05-27 9:20 ` Boris Brezillon @ 2020-05-27 10:16 ` Yicong Yang 0 siblings, 0 replies; 14+ messages in thread From: Yicong Yang @ 2020-05-27 10:16 UTC (permalink / raw) To: Boris Brezillon Cc: vigneshr, tudor.ambarus, richard, john.garry, linux-spi, broonie, linux-mtd, miquel.raynal On 2020/5/27 17:20, Boris Brezillon wrote: > On Wed, 27 May 2020 16:55:00 +0800 > Yicong Yang <yangyicong@hisilicon.com> wrote: > >> Hi Boris, >> >> >> On 2020/5/26 17:43, Boris Brezillon wrote: >>> On Thu, 21 May 2020 19:23:51 +0800 >>> Yicong Yang <yangyicong@hisilicon.com> 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. >>> Okay, so it looks like what we really need is a way to pass sequences >>> (multiple operations) that are expected to be issued without >>> interruptions. I'd prefer extending the spi_mem interface to allow that: >>> >>> int spi_mem_exec_sequence(struct spi_mem *spimem, >>> unsigned int num_ops, >>> const struct spi_mem_op *ops); >>> >>> struct spi_controller_mem_ops { >>> ... >>> int (*exec_sequence)(struct spi_mem *mem, >>> unsigned int num_ops, >>> const struct spi_mem_op *op); >>> ... >>> }; >> The prepare/unprepare hooks is just like what spi_nor_controller_ops provides. >> Alternatively we can use the interface you suggested, and it'll require >> upper layer(spi-nor framework, etc) to pack the operations before call >> spi_mem_exec_sequence(). > We have to patch the upper layers anyway, right? sure. >>> The prepare/unprepare hooks are a bit too vague. Alternatively, we >>> could add functions to grab/release the controller lock, but I'm not >>> sure that's what we want since some controllers might be able to address >>> several devices in parallel, and locking the whole controller at the >>> spi-nor level would prevent that. >> I suppose the method is optional and device may choose to use it or not >> following their own design. And the implementation is rather controller >> specific, they may choose to lock the whole controller or only the desired >> device to operate. > Yes, this is what I'm complaining about. How can the upper layer know > when it should call prepare/unprepare? Let's take the SPI NAND case, > should we prepare before loading a page in the cache and unprepare > after we're done reading the page, or should we unprepare just after > the page has been loaded in the cache? BTW, you've not patched the SPI > NAND layer to call ->prepare/unprepare(). It's already implemented in spi-nor framework. As for sequential operations, taking read as an example, the call stack looks like: ->spi_nor_read() ---->spi_nor_lock_and_prep() ------->spi_nor_controller_ops->prepare() or spi_mem_prepare() in PATCH 1/3 ... ---->spi_nor_read_data() // maybe called several times ... ---->spi_nor_unlock_and_unprep() ------->spi_nor_controller_ops->unprepare() or spi_mem_unprepare() in PATCH 1/3 As for nand flash, I didn't add it in this RFC as I'm not certain where should prepare/unprepare be called. If we use spi_mem_exec_sequence() seems we'll do more works to adapt, at least at spi-nor side. what do you think? > >> >>> BTW, I don't know all the details about this lock or what this FW is >>> exactly (where it's running, what's his priority, what kind of >>> synchronization exists between Linux and the FW, ...), but I'm worried >>> about potential deadlocks here. >> For SFC controller, both firmware and the kernel driver will require the >> lock before a sequence of operations, and single operations like register >> access for spi-nor flash is implemented atomically. Once the lock is held >> by firmware/driver, then the controller cannot perform the operations sent >> by the other one unless the lock is released. > Yes, that's my point. What prevents the FW from preempting Linux while > it's holding the lock and waiting indefinitely on this lock. Is the FW > running on a separate core? Don't you have other IPs with the same kind > of locks leading to issues if locks are not taken/released in the same > order? ... The firmware is running on a separate co-processor so it may not preempt the linux. Thanks, Yicong > . > ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2020-05-27 10:34 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 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 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
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).