| Message ID | 20250331161542.3040005-1-liwei.song.lsong@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] mtd: core: add sync between read/write and unbind device | expand |
Hi Maintainer, could you give some suggestions about this unbind issue? Thanks, Liwei. On Tue, Apr 1, 2025 at 12:16 AM Liwei Song <liwei.song.lsong@gmail.com> wrote: > > When unbind mtd device or qspi controller with a high frequency > reading to /dev/mtd0 device, there will be Calltrace as below: > > $ while true; do cat /dev/mtd0 >/dev/null; done & > $ echo ff8d2000.spi > /sys/bus/platform/drivers/cadence-qspi/unbind > > Internal error: synchronous external abort: 0000000096000210 [#1] PREEMPT SMP > Modules linked in: > CPU: 3 UID: 0 PID: 466 Comm: cat Not tainted 6.14.0-rc7-yocto-standard+ #1 > Hardware name: SoCFPGA Stratix 10 SoCDK (DT) > pc : cqspi_indirect_read_execute.isra.0+0x188/0x330 > lr : cqspi_indirect_read_execute.isra.0+0x21c/0x330 > Call trace: > cqspi_indirect_read_execute.isra.0+0x188/0x330 (P) > cqspi_exec_mem_op+0x8bc/0xe40 > spi_mem_exec_op+0x3e0/0x478 > spi_mem_no_dirmap_read+0xa8/0xc8 > spi_mem_dirmap_read+0xdc/0x150 > spi_nor_read_data+0x120/0x198 > spi_nor_read+0xf0/0x280 > mtd_read_oob_std+0x80/0x98 > mtd_read_oob+0x9c/0x168 > mtd_read+0x6c/0xd8 > mtdchar_read+0xdc/0x288 > vfs_read+0xc8/0x2f8 > ksys_read+0x70/0x110 > __arm64_sys_read+0x24/0x38 > invoke_syscall+0x5c/0x130 > el0_svc_common.constprop.0+0x48/0xf8 > do_el0_svc+0x28/0x40 > el0_svc+0x30/0xd0 > el0t_64_sync_handler+0x144/0x168 > el0t_64_sync+0x198/0x1a0 > Code: 927e7442 aa1a03e0 8b020342 d503201f (b9400321) > ---[ end trace 0000000000000000 ]--- > > Or: > $ while true; do cat /dev/mtd0 >/dev/null; done & > $ echo spi0.0 > /sys/class/mtd/mtd0/device/driver/unbind > > Unable to handle kernel paging request at virtual address 00000000000012e8 > Internal error: Oops: 0000000096000004 [#1] PREEMPT SMP > Modules linked in: > CPU: 2 UID: 0 PID: 459 Comm: cat Not tainted 6.14.0-rc7-yocto-standard+ #1 > Hardware name: SoCFPGA Stratix 10 SoCDK (DT) > pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--) > pc : spi_mem_exec_op+0x3e8/0x478 > lr : spi_mem_exec_op+0x3e0/0x478 > Call trace: > spi_mem_exec_op+0x3e8/0x478 (P) > spi_mem_no_dirmap_read+0xa8/0xc8 > spi_mem_dirmap_read+0xdc/0x150 > spi_nor_read_data+0x120/0x198 > spi_nor_read+0xf0/0x280 > mtd_read_oob_std+0x80/0x98 > mtd_read_oob+0x9c/0x168 > mtd_read+0x6c/0xd8 > mtdchar_read+0xdc/0x288 > vfs_read+0xc8/0x2f8 > ksys_read+0x70/0x110 > __arm64_sys_read+0x24/0x38 > invoke_syscall+0x5c/0x130 > el0_svc_common.constprop.0+0x48/0xf8 > do_el0_svc+0x28/0x40 > el0_svc+0x30/0xd0 > el0t_64_sync_handler+0x144/0x168 > el0t_64_sync+0x198/0x1a0 > Code: f9400842 d63f0040 2a0003f4 f94002a1 (f9417437) > ---[ end trace 0000000000000000 ]--- > > when unbind is running, the memory allocated to qspi controller and > mtd device is freed during unbinding, but open/close and reading device > are still running, if the reading process get read lock and start > excuting, there will be above illegal memory access. This issue also > can be repruduced on many other platforms like ls1046 and nxpimx8 which > have qspi flash. > > In this patch, register a spi bus notifier which will be called before > unbind process freeing device memory, add a new member mtd_event_remove > to block mtd open/read, then waiting for the running task to be finished, > after that, memory is safe to be free. > > Signed-off-by: Liwei Song <liwei.song.lsong@gmail.com> > --- > > Hi Maintainer, > > This is an improved patch compared with the original one: > (https://patchwork.ozlabs.org/project/linux-mtd/patch/20250325133954.3699535-1-liwei.song.lsong@gmail.com/), > This v2 patch move notifier to spi-nor to avoid crash other types of flash. > now this patch only aim at fixing nor-flash "bind/unbind while reading" calltrace, > but for other types of flash like nand also have this issue. > > Thanks, > Liwei. > > --- > drivers/mtd/mtdcore.c | 3 +++ > drivers/mtd/spi-nor/core.c | 46 +++++++++++++++++++++++++++++++++++++ > include/linux/mtd/mtd.h | 1 + > include/linux/mtd/spi-nor.h | 1 + > 4 files changed, 51 insertions(+) > > diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c > index 724f917f91ba..a78044ee603e 100644 > --- a/drivers/mtd/mtdcore.c > +++ b/drivers/mtd/mtdcore.c > @@ -1243,6 +1243,9 @@ int __get_mtd_device(struct mtd_info *mtd) > struct mtd_info *master = mtd_get_master(mtd); > int err; > > + if (master->mtd_event_remove) > + return -ENODEV; > + > if (master->_get_device) { > err = master->_get_device(mtd); > if (err) > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c > index 19eb98bd6821..ae879d445046 100644 > --- a/drivers/mtd/spi-nor/core.c > +++ b/drivers/mtd/spi-nor/core.c > @@ -16,6 +16,7 @@ > #include <linux/mtd/mtd.h> > #include <linux/mtd/spi-nor.h> > #include <linux/mutex.h> > +#include <linux/of_device.h> > #include <linux/of_platform.h> > #include <linux/regulator/consumer.h> > #include <linux/sched/task_stack.h> > @@ -44,6 +45,9 @@ > #define SPI_NOR_SRST_SLEEP_MIN 200 > #define SPI_NOR_SRST_SLEEP_MAX 400 > > +static int spi_nor_remove_notifier_call(struct notifier_block *nb, > + unsigned long event, void *data); > + > /** > * spi_nor_get_cmd_ext() - Get the command opcode extension based on the > * extension type. > @@ -1191,6 +1195,9 @@ static int spi_nor_prep(struct spi_nor *nor) > if (nor->controller_ops && nor->controller_ops->prepare) > ret = nor->controller_ops->prepare(nor); > > + if (nor->mtd.mtd_event_remove) > + return -ENODEV; > + > return ret; > } > > @@ -3649,6 +3656,11 @@ static int spi_nor_probe(struct spi_mem *spimem) > if (ret) > return ret; > > + if (!nor->spi_nor_remove_nb.notifier_call) { > + nor->spi_nor_remove_nb.notifier_call = spi_nor_remove_notifier_call; > + bus_register_notifier(&spi_bus_type, &nor->spi_nor_remove_nb); > + } > + > return mtd_device_register(&nor->mtd, data ? data->parts : NULL, > data ? data->nr_parts : 0); > } > @@ -3659,6 +3671,9 @@ static int spi_nor_remove(struct spi_mem *spimem) > > spi_nor_restore(nor); > > + bus_unregister_notifier(&spi_bus_type, &nor->spi_nor_remove_nb); > + memset(&nor->spi_nor_remove_nb, 0, sizeof(nor->spi_nor_remove_nb)); > + > /* Clean up MTD stuff. */ > return mtd_device_unregister(&nor->mtd); > } > @@ -3737,6 +3752,37 @@ static const struct of_device_id spi_nor_of_table[] = { > }; > MODULE_DEVICE_TABLE(of, spi_nor_of_table); > > +static int spi_nor_remove_notifier_call(struct notifier_block *nb, > + unsigned long event, void *data) > +{ > + struct device *dev = data; > + struct spi_device *spi; > + struct spi_mem *mem; > + struct spi_nor *nor; > + > + if (!of_match_device(spi_nor_of_table, dev)) > + return 0; > + > + switch (event) { > + case BUS_NOTIFY_DEL_DEVICE: > + case BUS_NOTIFY_UNBIND_DRIVER: > + spi = to_spi_device(dev); > + mem = spi_get_drvdata(spi); > + if (!mem) > + return NOTIFY_DONE; > + nor = spi_mem_get_drvdata(mem); > + > + mutex_lock(&nor->lock); > + nor->mtd.mtd_event_remove = true; > + mutex_unlock(&nor->lock); > + msleep(300); > + > + break; > + } > + > + return NOTIFY_DONE; > +} > + > /* > * REVISIT: many of these chips have deep power-down modes, which > * should clearly be entered on suspend() to minimize power use. > diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h > index 8d10d9d2e830..134bfa6fcf76 100644 > --- a/include/linux/mtd/mtd.h > +++ b/include/linux/mtd/mtd.h > @@ -290,6 +290,7 @@ struct mtd_info { > /* Kernel-only stuff starts here. */ > const char *name; > int index; > + bool mtd_event_remove; > > /* OOB layout description */ > const struct mtd_ooblayout_ops *ooblayout; > diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h > index cdcfe0fd2e7d..d176af8fe2f2 100644 > --- a/include/linux/mtd/spi-nor.h > +++ b/include/linux/mtd/spi-nor.h > @@ -420,6 +420,7 @@ struct spi_nor { > } dirmap; > > void *priv; > + struct notifier_block spi_nor_remove_nb; > }; > > static inline void spi_nor_set_flash_node(struct spi_nor *nor, > -- > 2.40.0 >
Hello Liwei, On 01/04/2025 at 00:15:20 +08, Liwei Song <liwei.song.lsong@gmail.com> wrote: > When unbind mtd device or qspi controller with a high frequency > reading to /dev/mtd0 device, there will be Calltrace as below: > > $ while true; do cat /dev/mtd0 >/dev/null; done & > $ echo ff8d2000.spi > /sys/bus/platform/drivers/cadence-qspi/unbind > > Internal error: synchronous external abort: 0000000096000210 [#1] PREEMPT SMP > Modules linked in: > CPU: 3 UID: 0 PID: 466 Comm: cat Not tainted 6.14.0-rc7-yocto-standard+ #1 > Hardware name: SoCFPGA Stratix 10 SoCDK (DT) > pc : cqspi_indirect_read_execute.isra.0+0x188/0x330 > lr : cqspi_indirect_read_execute.isra.0+0x21c/0x330 > Call trace: > cqspi_indirect_read_execute.isra.0+0x188/0x330 (P) > cqspi_exec_mem_op+0x8bc/0xe40 > spi_mem_exec_op+0x3e0/0x478 > spi_mem_no_dirmap_read+0xa8/0xc8 > spi_mem_dirmap_read+0xdc/0x150 > spi_nor_read_data+0x120/0x198 > spi_nor_read+0xf0/0x280 > mtd_read_oob_std+0x80/0x98 > mtd_read_oob+0x9c/0x168 > mtd_read+0x6c/0xd8 > mtdchar_read+0xdc/0x288 > vfs_read+0xc8/0x2f8 > ksys_read+0x70/0x110 > __arm64_sys_read+0x24/0x38 > invoke_syscall+0x5c/0x130 > el0_svc_common.constprop.0+0x48/0xf8 > do_el0_svc+0x28/0x40 > el0_svc+0x30/0xd0 > el0t_64_sync_handler+0x144/0x168 > el0t_64_sync+0x198/0x1a0 > Code: 927e7442 aa1a03e0 8b020342 d503201f (b9400321) > ---[ end trace 0000000000000000 ]--- > > Or: > $ while true; do cat /dev/mtd0 >/dev/null; done & > $ echo spi0.0 > /sys/class/mtd/mtd0/device/driver/unbind > > Unable to handle kernel paging request at virtual address 00000000000012e8 > Internal error: Oops: 0000000096000004 [#1] PREEMPT SMP > Modules linked in: > CPU: 2 UID: 0 PID: 459 Comm: cat Not tainted 6.14.0-rc7-yocto-standard+ #1 > Hardware name: SoCFPGA Stratix 10 SoCDK (DT) > pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--) > pc : spi_mem_exec_op+0x3e8/0x478 > lr : spi_mem_exec_op+0x3e0/0x478 > Call trace: > spi_mem_exec_op+0x3e8/0x478 (P) > spi_mem_no_dirmap_read+0xa8/0xc8 > spi_mem_dirmap_read+0xdc/0x150 > spi_nor_read_data+0x120/0x198 > spi_nor_read+0xf0/0x280 > mtd_read_oob_std+0x80/0x98 > mtd_read_oob+0x9c/0x168 > mtd_read+0x6c/0xd8 > mtdchar_read+0xdc/0x288 > vfs_read+0xc8/0x2f8 > ksys_read+0x70/0x110 > __arm64_sys_read+0x24/0x38 > invoke_syscall+0x5c/0x130 > el0_svc_common.constprop.0+0x48/0xf8 > do_el0_svc+0x28/0x40 > el0_svc+0x30/0xd0 > el0t_64_sync_handler+0x144/0x168 > el0t_64_sync+0x198/0x1a0 > Code: f9400842 d63f0040 2a0003f4 f94002a1 (f9417437) > ---[ end trace 0000000000000000 ]--- > > when unbind is running, the memory allocated to qspi controller and > mtd device is freed during unbinding, but open/close and reading device > are still running, if the reading process get read lock and start > excuting, there will be above illegal memory access. This issue also > can be repruduced on many other platforms like ls1046 and nxpimx8 which > have qspi flash. > > In this patch, register a spi bus notifier which will be called before > unbind process freeing device memory, add a new member mtd_event_remove > to block mtd open/read, then waiting for the running task to be finished, > after that, memory is safe to be free. > > Signed-off-by: Liwei Song <liwei.song.lsong@gmail.com> > --- > > Hi Maintainer, > > This is an improved patch compared with the original one: > (https://patchwork.ozlabs.org/project/linux-mtd/patch/20250325133954.3699535-1-liwei.song.lsong@gmail.com/), > This v2 patch move notifier to spi-nor to avoid crash other types of flash. > now this patch only aim at fixing nor-flash "bind/unbind while reading" calltrace, > but for other types of flash like nand also have this issue. While I agree with the observation and also the conclusion of adding some kind of notifier, I'd like to understand the rationale behind choosing to fix only spi-nor in v2? If any spi memory registered in the mtd subsystem is subject to this failure, we should find a generic approach (or if it's too difficult, at least have the fix in both spi nor and spi nand). Looking at your implementation, maybe it could fit in spi-mem (I'm not sure). ... > +static int spi_nor_remove_notifier_call(struct notifier_block *nb, > + unsigned long event, void > *data); I believe spi nor maitainers would prefer to avoid forward declarations. > + > /** > * spi_nor_get_cmd_ext() - Get the command opcode extension based on the > * extension type. > @@ -1191,6 +1195,9 @@ static int spi_nor_prep(struct spi_nor *nor) > if (nor->controller_ops && nor->controller_ops->prepare) > ret = nor->controller_ops->prepare(nor); > > + if (nor->mtd.mtd_event_remove) > + return -ENODEV; > + > return ret; > } ... > +static int spi_nor_remove_notifier_call(struct notifier_block *nb, > + unsigned long event, void *data) > +{ > + struct device *dev = data; > + struct spi_device *spi; > + struct spi_mem *mem; > + struct spi_nor *nor; > + > + if (!of_match_device(spi_nor_of_table, dev)) > + return 0; > + > + switch (event) { > + case BUS_NOTIFY_DEL_DEVICE: > + case BUS_NOTIFY_UNBIND_DRIVER: > + spi = to_spi_device(dev); > + mem = spi_get_drvdata(spi); > + if (!mem) > + return NOTIFY_DONE; > + nor = spi_mem_get_drvdata(mem); > + > + mutex_lock(&nor->lock); > + nor->mtd.mtd_event_remove = true; > + mutex_unlock(&nor->lock); > + msleep(300); What is this sleep for? > + > + break; > + } > + > + return NOTIFY_DONE; > +} > + > /* > * REVISIT: many of these chips have deep power-down modes, which > * should clearly be entered on suspend() to minimize power use. > diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h > index 8d10d9d2e830..134bfa6fcf76 100644 > --- a/include/linux/mtd/mtd.h > +++ b/include/linux/mtd/mtd.h > @@ -290,6 +290,7 @@ struct mtd_info { > /* Kernel-only stuff starts here. */ > const char *name; > int index; > + bool mtd_event_remove; No need to repeat 'mtd' here, you are already in the mtd_info structure, so mtd->mtd_event_remove would be redundant. > /* OOB layout description */ > const struct mtd_ooblayout_ops *ooblayout; Thanks, Miquèl
Hi Miquèl, On Tue, Apr 29, 2025 at 3:55 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > Hello Liwei, > > On 01/04/2025 at 00:15:20 +08, Liwei Song <liwei.song.lsong@gmail.com> wrote: > > > When unbind mtd device or qspi controller with a high frequency > > reading to /dev/mtd0 device, there will be Calltrace as below: > > > > $ while true; do cat /dev/mtd0 >/dev/null; done & > > $ echo ff8d2000.spi > /sys/bus/platform/drivers/cadence-qspi/unbind > > > > Internal error: synchronous external abort: 0000000096000210 [#1] PREEMPT SMP > > Modules linked in: > > CPU: 3 UID: 0 PID: 466 Comm: cat Not tainted 6.14.0-rc7-yocto-standard+ #1 > > Hardware name: SoCFPGA Stratix 10 SoCDK (DT) > > pc : cqspi_indirect_read_execute.isra.0+0x188/0x330 > > lr : cqspi_indirect_read_execute.isra.0+0x21c/0x330 > > Call trace: > > cqspi_indirect_read_execute.isra.0+0x188/0x330 (P) > > cqspi_exec_mem_op+0x8bc/0xe40 > > spi_mem_exec_op+0x3e0/0x478 > > spi_mem_no_dirmap_read+0xa8/0xc8 > > spi_mem_dirmap_read+0xdc/0x150 > > spi_nor_read_data+0x120/0x198 > > spi_nor_read+0xf0/0x280 > > mtd_read_oob_std+0x80/0x98 > > mtd_read_oob+0x9c/0x168 > > mtd_read+0x6c/0xd8 > > mtdchar_read+0xdc/0x288 > > vfs_read+0xc8/0x2f8 > > ksys_read+0x70/0x110 > > __arm64_sys_read+0x24/0x38 > > invoke_syscall+0x5c/0x130 > > el0_svc_common.constprop.0+0x48/0xf8 > > do_el0_svc+0x28/0x40 > > el0_svc+0x30/0xd0 > > el0t_64_sync_handler+0x144/0x168 > > el0t_64_sync+0x198/0x1a0 > > Code: 927e7442 aa1a03e0 8b020342 d503201f (b9400321) > > ---[ end trace 0000000000000000 ]--- > > > > Or: > > $ while true; do cat /dev/mtd0 >/dev/null; done & > > $ echo spi0.0 > /sys/class/mtd/mtd0/device/driver/unbind > > > > Unable to handle kernel paging request at virtual address 00000000000012e8 > > Internal error: Oops: 0000000096000004 [#1] PREEMPT SMP > > Modules linked in: > > CPU: 2 UID: 0 PID: 459 Comm: cat Not tainted 6.14.0-rc7-yocto-standard+ #1 > > Hardware name: SoCFPGA Stratix 10 SoCDK (DT) > > pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--) > > pc : spi_mem_exec_op+0x3e8/0x478 > > lr : spi_mem_exec_op+0x3e0/0x478 > > Call trace: > > spi_mem_exec_op+0x3e8/0x478 (P) > > spi_mem_no_dirmap_read+0xa8/0xc8 > > spi_mem_dirmap_read+0xdc/0x150 > > spi_nor_read_data+0x120/0x198 > > spi_nor_read+0xf0/0x280 > > mtd_read_oob_std+0x80/0x98 > > mtd_read_oob+0x9c/0x168 > > mtd_read+0x6c/0xd8 > > mtdchar_read+0xdc/0x288 > > vfs_read+0xc8/0x2f8 > > ksys_read+0x70/0x110 > > __arm64_sys_read+0x24/0x38 > > invoke_syscall+0x5c/0x130 > > el0_svc_common.constprop.0+0x48/0xf8 > > do_el0_svc+0x28/0x40 > > el0_svc+0x30/0xd0 > > el0t_64_sync_handler+0x144/0x168 > > el0t_64_sync+0x198/0x1a0 > > Code: f9400842 d63f0040 2a0003f4 f94002a1 (f9417437) > > ---[ end trace 0000000000000000 ]--- > > > > when unbind is running, the memory allocated to qspi controller and > > mtd device is freed during unbinding, but open/close and reading device > > are still running, if the reading process get read lock and start > > excuting, there will be above illegal memory access. This issue also > > can be repruduced on many other platforms like ls1046 and nxpimx8 which > > have qspi flash. > > > > In this patch, register a spi bus notifier which will be called before > > unbind process freeing device memory, add a new member mtd_event_remove > > to block mtd open/read, then waiting for the running task to be finished, > > after that, memory is safe to be free. > > > > Signed-off-by: Liwei Song <liwei.song.lsong@gmail.com> > > --- > > > > Hi Maintainer, > > > > This is an improved patch compared with the original one: > > (https://patchwork.ozlabs.org/project/linux-mtd/patch/20250325133954.3699535-1-liwei.song.lsong@gmail.com/), > > This v2 patch move notifier to spi-nor to avoid crash other types of flash. > > now this patch only aim at fixing nor-flash "bind/unbind while reading" calltrace, > > but for other types of flash like nand also have this issue. > > While I agree with the observation and also the conclusion of adding > some kind of notifier, I'd like to understand the rationale behind > choosing to fix only spi-nor in v2? If any spi memory registered in the My original plan is to fix nand in another patch if the idea of this patch is acceptable, but after some investigation, it also can be done in this patch together, because for nand device, the existing driver will call mtd_device_unregister() directly to remove device when unbinding, new adding "mtd_event_remove" can be set there, and check "mtd_event_remove" in mtd_read(), on my board exist below call trace: for nand unbind: mtd_device_unregister+0x50/0x90 denali_remove+0x58/0x108 denali_dt_remove+0x24/0x88 platform_remove+0x34/0x80 device_remove+0x54/0x90 device_release_driver_internal+0x1d4/0x238 device_driver_detach+0x20/0x38 unbind_store+0xbc/0xc8 for nand read: denali_dma_xfer+0x140/0x218 denali_read_page+0x5c/0x3c0 nand_read_oob+0x2b4/0x8a0 mtd_read_oob_std+0x60/0x98 mtd_read_oob+0x9c/0x168 mtd_read+0x6c/0xb0 mtdchar_read+0xdc/0x288 for spi_nor read: cqspi_exec_mem_op+0x8d4/0xfbc spi_mem_exec_op+0x3dc/0x45c spi_mem_no_dirmap_read+0xa0/0xc0 spi_mem_dirmap_read+0xdc/0x144 spi_nor_read_data+0x114/0x180 spi_nor_read+0xbc/0x164 mtd_read_oob_std+0x80/0x90 mtd_read_oob+0x8c/0x150 mtd_read+0x6c/0xb0 mtdchar_read+0xdc/0x2a0 > mtd subsystem is subject to this failure, we should find a generic > approach (or if it's too difficult, at least have the fix in both > spi nor and spi nand). Looking at your implementation, maybe it could > fit in spi-mem (I'm not sure). Thanks for your suggestion, I will have a try to move the code there. > > ... > > > +static int spi_nor_remove_notifier_call(struct notifier_block *nb, > > + unsigned long event, void > > *data); > > I believe spi nor maitainers would prefer to avoid forward declarations. Got it, thanks, will drop this kind of declaration. > > > + > > /** > > * spi_nor_get_cmd_ext() - Get the command opcode extension based on the > > * extension type. > > @@ -1191,6 +1195,9 @@ static int spi_nor_prep(struct spi_nor *nor) > > if (nor->controller_ops && nor->controller_ops->prepare) > > ret = nor->controller_ops->prepare(nor); > > > > + if (nor->mtd.mtd_event_remove) > > + return -ENODEV; > > + > > return ret; > > } > > ... > > > +static int spi_nor_remove_notifier_call(struct notifier_block *nb, > > + unsigned long event, void *data) > > +{ > > + struct device *dev = data; > > + struct spi_device *spi; > > + struct spi_mem *mem; > > + struct spi_nor *nor; > > + > > + if (!of_match_device(spi_nor_of_table, dev)) > > + return 0; > > + > > + switch (event) { > > + case BUS_NOTIFY_DEL_DEVICE: > > + case BUS_NOTIFY_UNBIND_DRIVER: > > + spi = to_spi_device(dev); > > + mem = spi_get_drvdata(spi); > > + if (!mem) > > + return NOTIFY_DONE; > > + nor = spi_mem_get_drvdata(mem); > > + > > + mutex_lock(&nor->lock); > > + nor->mtd.mtd_event_remove = true; > > + mutex_unlock(&nor->lock); > > + msleep(300); > > What is this sleep for? The sleep is to wait the process which already got the lock and running in reading routine can be finished before memory is released, show in below scenario: without sleep: -------------------------------------------------------------------- mtd.mtd_event_remove = false; reading start; mtd.mtd_event_remove = true; release memory reading end; -------------------------------------------------------------------- with sleep: ------------------------------------------------------------------- mtd.mtd_event_remove = false; reading start; mtd.mtd_event_remove = true; sleep() start reading end; sleep() end release memory ------------------------------------------------------------------- > > > + > > + break; > > + } > > + > > + return NOTIFY_DONE; > > +} > > + > > /* > > * REVISIT: many of these chips have deep power-down modes, which > > * should clearly be entered on suspend() to minimize power use. > > diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h > > index 8d10d9d2e830..134bfa6fcf76 100644 > > --- a/include/linux/mtd/mtd.h > > +++ b/include/linux/mtd/mtd.h > > @@ -290,6 +290,7 @@ struct mtd_info { > > /* Kernel-only stuff starts here. */ > > const char *name; > > int index; > > + bool mtd_event_remove; > > No need to repeat 'mtd' here, you are already in the mtd_info structure, > so mtd->mtd_event_remove would be redundant. Got it, will remove the "mtd" prefix. Thanks, Liwei. > > > /* OOB layout description */ > > const struct mtd_ooblayout_ops *ooblayout; > > Thanks, > Miquèl
+Cc Mark and linux-spi@ On Wed, Apr 30 2025, liwei song wrote: > Hi Miquèl, > > On Tue, Apr 29, 2025 at 3:55 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote: >> >> Hello Liwei, >> >> On 01/04/2025 at 00:15:20 +08, Liwei Song <liwei.song.lsong@gmail.com> wrote: >> >> > When unbind mtd device or qspi controller with a high frequency >> > reading to /dev/mtd0 device, there will be Calltrace as below: >> > >> > $ while true; do cat /dev/mtd0 >/dev/null; done & >> > $ echo ff8d2000.spi > /sys/bus/platform/drivers/cadence-qspi/unbind >> > >> > Internal error: synchronous external abort: 0000000096000210 [#1] PREEMPT SMP >> > Modules linked in: >> > CPU: 3 UID: 0 PID: 466 Comm: cat Not tainted 6.14.0-rc7-yocto-standard+ #1 >> > Hardware name: SoCFPGA Stratix 10 SoCDK (DT) >> > pc : cqspi_indirect_read_execute.isra.0+0x188/0x330 >> > lr : cqspi_indirect_read_execute.isra.0+0x21c/0x330 >> > Call trace: >> > cqspi_indirect_read_execute.isra.0+0x188/0x330 (P) >> > cqspi_exec_mem_op+0x8bc/0xe40 >> > spi_mem_exec_op+0x3e0/0x478 >> > spi_mem_no_dirmap_read+0xa8/0xc8 >> > spi_mem_dirmap_read+0xdc/0x150 >> > spi_nor_read_data+0x120/0x198 >> > spi_nor_read+0xf0/0x280 >> > mtd_read_oob_std+0x80/0x98 >> > mtd_read_oob+0x9c/0x168 >> > mtd_read+0x6c/0xd8 >> > mtdchar_read+0xdc/0x288 >> > vfs_read+0xc8/0x2f8 >> > ksys_read+0x70/0x110 >> > __arm64_sys_read+0x24/0x38 >> > invoke_syscall+0x5c/0x130 >> > el0_svc_common.constprop.0+0x48/0xf8 >> > do_el0_svc+0x28/0x40 >> > el0_svc+0x30/0xd0 >> > el0t_64_sync_handler+0x144/0x168 >> > el0t_64_sync+0x198/0x1a0 >> > Code: 927e7442 aa1a03e0 8b020342 d503201f (b9400321) >> > ---[ end trace 0000000000000000 ]--- >> > >> > Or: >> > $ while true; do cat /dev/mtd0 >/dev/null; done & >> > $ echo spi0.0 > /sys/class/mtd/mtd0/device/driver/unbind >> > >> > Unable to handle kernel paging request at virtual address 00000000000012e8 >> > Internal error: Oops: 0000000096000004 [#1] PREEMPT SMP >> > Modules linked in: >> > CPU: 2 UID: 0 PID: 459 Comm: cat Not tainted 6.14.0-rc7-yocto-standard+ #1 >> > Hardware name: SoCFPGA Stratix 10 SoCDK (DT) >> > pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--) >> > pc : spi_mem_exec_op+0x3e8/0x478 >> > lr : spi_mem_exec_op+0x3e0/0x478 >> > Call trace: >> > spi_mem_exec_op+0x3e8/0x478 (P) >> > spi_mem_no_dirmap_read+0xa8/0xc8 >> > spi_mem_dirmap_read+0xdc/0x150 >> > spi_nor_read_data+0x120/0x198 >> > spi_nor_read+0xf0/0x280 >> > mtd_read_oob_std+0x80/0x98 >> > mtd_read_oob+0x9c/0x168 >> > mtd_read+0x6c/0xd8 >> > mtdchar_read+0xdc/0x288 >> > vfs_read+0xc8/0x2f8 >> > ksys_read+0x70/0x110 >> > __arm64_sys_read+0x24/0x38 >> > invoke_syscall+0x5c/0x130 >> > el0_svc_common.constprop.0+0x48/0xf8 >> > do_el0_svc+0x28/0x40 >> > el0_svc+0x30/0xd0 >> > el0t_64_sync_handler+0x144/0x168 >> > el0t_64_sync+0x198/0x1a0 >> > Code: f9400842 d63f0040 2a0003f4 f94002a1 (f9417437) >> > ---[ end trace 0000000000000000 ]--- >> > >> > when unbind is running, the memory allocated to qspi controller and >> > mtd device is freed during unbinding, but open/close and reading device >> > are still running, if the reading process get read lock and start >> > excuting, there will be above illegal memory access. This issue also >> > can be repruduced on many other platforms like ls1046 and nxpimx8 which >> > have qspi flash. >> > >> > In this patch, register a spi bus notifier which will be called before >> > unbind process freeing device memory, add a new member mtd_event_remove >> > to block mtd open/read, then waiting for the running task to be finished, >> > after that, memory is safe to be free. >> > >> > Signed-off-by: Liwei Song <liwei.song.lsong@gmail.com> >> > --- >> > >> > Hi Maintainer, >> > >> > This is an improved patch compared with the original one: >> > (https://patchwork.ozlabs.org/project/linux-mtd/patch/20250325133954.3699535-1-liwei.song.lsong@gmail.com/), >> > This v2 patch move notifier to spi-nor to avoid crash other types of flash. >> > now this patch only aim at fixing nor-flash "bind/unbind while reading" calltrace, >> > but for other types of flash like nand also have this issue. >> >> While I agree with the observation and also the conclusion of adding >> some kind of notifier, I'd like to understand the rationale behind >> choosing to fix only spi-nor in v2? If any spi memory registered in the [...] >> > +static int spi_nor_remove_notifier_call(struct notifier_block *nb, >> > + unsigned long event, void *data) >> > +{ >> > + struct device *dev = data; >> > + struct spi_device *spi; >> > + struct spi_mem *mem; >> > + struct spi_nor *nor; >> > + >> > + if (!of_match_device(spi_nor_of_table, dev)) >> > + return 0; >> > + >> > + switch (event) { >> > + case BUS_NOTIFY_DEL_DEVICE: >> > + case BUS_NOTIFY_UNBIND_DRIVER: >> > + spi = to_spi_device(dev); >> > + mem = spi_get_drvdata(spi); >> > + if (!mem) >> > + return NOTIFY_DONE; >> > + nor = spi_mem_get_drvdata(mem); >> > + >> > + mutex_lock(&nor->lock); >> > + nor->mtd.mtd_event_remove = true; >> > + mutex_unlock(&nor->lock); >> > + msleep(300); >> >> What is this sleep for? > > The sleep is to wait the process which already got the lock and > running in reading > routine can be finished before memory is released, show in below scenario: > > without sleep: > -------------------------------------------------------------------- > mtd.mtd_event_remove = false; > reading start; > mtd.mtd_event_remove = true; > release memory > reading end; > -------------------------------------------------------------------- > > with sleep: > ------------------------------------------------------------------- > mtd.mtd_event_remove = false; > reading start; > mtd.mtd_event_remove = true; > sleep() start > reading end; > sleep() end > release memory > ------------------------------------------------------------------- This is not how we should manage lifetimes. Adding arbitrary sleeps to hope races go away is flaky and will break in strange ways that would be impossible to debug. For example, what if a read happens to take longer than 300ms? Instead, we should have a way to properly manage the lifetimes and sequence operations. I always thought that by the time spi_unregister_controller() returns, there are no longer any in-flight operations so freeing stuff after it would be safe. Seems not to be the case. After a quick look, seems like all SPI MEM devices are doing similar things in their remove callbacks, so I think they are affected by the same race. For example, nxp_fspi_remove() unmaps its AHB area, so in flight SPI MEM operations would fail. Similarly, spi-stm32 will release its DMA channels, and so on with others. I suspect that this notifier thing is not the proper way to manage the lifetimes here and there should be something better. We need a way for the driver to make sure the underlying MTD device is no longer active before it frees its memory. Mark/SPI folks, what do you think is the proper way of doing this? [...]
diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c index 724f917f91ba..a78044ee603e 100644 --- a/drivers/mtd/mtdcore.c +++ b/drivers/mtd/mtdcore.c @@ -1243,6 +1243,9 @@ int __get_mtd_device(struct mtd_info *mtd) struct mtd_info *master = mtd_get_master(mtd); int err; + if (master->mtd_event_remove) + return -ENODEV; + if (master->_get_device) { err = master->_get_device(mtd); if (err) diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c index 19eb98bd6821..ae879d445046 100644 --- a/drivers/mtd/spi-nor/core.c +++ b/drivers/mtd/spi-nor/core.c @@ -16,6 +16,7 @@ #include <linux/mtd/mtd.h> #include <linux/mtd/spi-nor.h> #include <linux/mutex.h> +#include <linux/of_device.h> #include <linux/of_platform.h> #include <linux/regulator/consumer.h> #include <linux/sched/task_stack.h> @@ -44,6 +45,9 @@ #define SPI_NOR_SRST_SLEEP_MIN 200 #define SPI_NOR_SRST_SLEEP_MAX 400 +static int spi_nor_remove_notifier_call(struct notifier_block *nb, + unsigned long event, void *data); + /** * spi_nor_get_cmd_ext() - Get the command opcode extension based on the * extension type. @@ -1191,6 +1195,9 @@ static int spi_nor_prep(struct spi_nor *nor) if (nor->controller_ops && nor->controller_ops->prepare) ret = nor->controller_ops->prepare(nor); + if (nor->mtd.mtd_event_remove) + return -ENODEV; + return ret; } @@ -3649,6 +3656,11 @@ static int spi_nor_probe(struct spi_mem *spimem) if (ret) return ret; + if (!nor->spi_nor_remove_nb.notifier_call) { + nor->spi_nor_remove_nb.notifier_call = spi_nor_remove_notifier_call; + bus_register_notifier(&spi_bus_type, &nor->spi_nor_remove_nb); + } + return mtd_device_register(&nor->mtd, data ? data->parts : NULL, data ? data->nr_parts : 0); } @@ -3659,6 +3671,9 @@ static int spi_nor_remove(struct spi_mem *spimem) spi_nor_restore(nor); + bus_unregister_notifier(&spi_bus_type, &nor->spi_nor_remove_nb); + memset(&nor->spi_nor_remove_nb, 0, sizeof(nor->spi_nor_remove_nb)); + /* Clean up MTD stuff. */ return mtd_device_unregister(&nor->mtd); } @@ -3737,6 +3752,37 @@ static const struct of_device_id spi_nor_of_table[] = { }; MODULE_DEVICE_TABLE(of, spi_nor_of_table); +static int spi_nor_remove_notifier_call(struct notifier_block *nb, + unsigned long event, void *data) +{ + struct device *dev = data; + struct spi_device *spi; + struct spi_mem *mem; + struct spi_nor *nor; + + if (!of_match_device(spi_nor_of_table, dev)) + return 0; + + switch (event) { + case BUS_NOTIFY_DEL_DEVICE: + case BUS_NOTIFY_UNBIND_DRIVER: + spi = to_spi_device(dev); + mem = spi_get_drvdata(spi); + if (!mem) + return NOTIFY_DONE; + nor = spi_mem_get_drvdata(mem); + + mutex_lock(&nor->lock); + nor->mtd.mtd_event_remove = true; + mutex_unlock(&nor->lock); + msleep(300); + + break; + } + + return NOTIFY_DONE; +} + /* * REVISIT: many of these chips have deep power-down modes, which * should clearly be entered on suspend() to minimize power use. diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h index 8d10d9d2e830..134bfa6fcf76 100644 --- a/include/linux/mtd/mtd.h +++ b/include/linux/mtd/mtd.h @@ -290,6 +290,7 @@ struct mtd_info { /* Kernel-only stuff starts here. */ const char *name; int index; + bool mtd_event_remove; /* OOB layout description */ const struct mtd_ooblayout_ops *ooblayout; diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h index cdcfe0fd2e7d..d176af8fe2f2 100644 --- a/include/linux/mtd/spi-nor.h +++ b/include/linux/mtd/spi-nor.h @@ -420,6 +420,7 @@ struct spi_nor { } dirmap; void *priv; + struct notifier_block spi_nor_remove_nb; }; static inline void spi_nor_set_flash_node(struct spi_nor *nor,
When unbind mtd device or qspi controller with a high frequency reading to /dev/mtd0 device, there will be Calltrace as below: $ while true; do cat /dev/mtd0 >/dev/null; done & $ echo ff8d2000.spi > /sys/bus/platform/drivers/cadence-qspi/unbind Internal error: synchronous external abort: 0000000096000210 [#1] PREEMPT SMP Modules linked in: CPU: 3 UID: 0 PID: 466 Comm: cat Not tainted 6.14.0-rc7-yocto-standard+ #1 Hardware name: SoCFPGA Stratix 10 SoCDK (DT) pc : cqspi_indirect_read_execute.isra.0+0x188/0x330 lr : cqspi_indirect_read_execute.isra.0+0x21c/0x330 Call trace: cqspi_indirect_read_execute.isra.0+0x188/0x330 (P) cqspi_exec_mem_op+0x8bc/0xe40 spi_mem_exec_op+0x3e0/0x478 spi_mem_no_dirmap_read+0xa8/0xc8 spi_mem_dirmap_read+0xdc/0x150 spi_nor_read_data+0x120/0x198 spi_nor_read+0xf0/0x280 mtd_read_oob_std+0x80/0x98 mtd_read_oob+0x9c/0x168 mtd_read+0x6c/0xd8 mtdchar_read+0xdc/0x288 vfs_read+0xc8/0x2f8 ksys_read+0x70/0x110 __arm64_sys_read+0x24/0x38 invoke_syscall+0x5c/0x130 el0_svc_common.constprop.0+0x48/0xf8 do_el0_svc+0x28/0x40 el0_svc+0x30/0xd0 el0t_64_sync_handler+0x144/0x168 el0t_64_sync+0x198/0x1a0 Code: 927e7442 aa1a03e0 8b020342 d503201f (b9400321) ---[ end trace 0000000000000000 ]--- Or: $ while true; do cat /dev/mtd0 >/dev/null; done & $ echo spi0.0 > /sys/class/mtd/mtd0/device/driver/unbind Unable to handle kernel paging request at virtual address 00000000000012e8 Internal error: Oops: 0000000096000004 [#1] PREEMPT SMP Modules linked in: CPU: 2 UID: 0 PID: 459 Comm: cat Not tainted 6.14.0-rc7-yocto-standard+ #1 Hardware name: SoCFPGA Stratix 10 SoCDK (DT) pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--) pc : spi_mem_exec_op+0x3e8/0x478 lr : spi_mem_exec_op+0x3e0/0x478 Call trace: spi_mem_exec_op+0x3e8/0x478 (P) spi_mem_no_dirmap_read+0xa8/0xc8 spi_mem_dirmap_read+0xdc/0x150 spi_nor_read_data+0x120/0x198 spi_nor_read+0xf0/0x280 mtd_read_oob_std+0x80/0x98 mtd_read_oob+0x9c/0x168 mtd_read+0x6c/0xd8 mtdchar_read+0xdc/0x288 vfs_read+0xc8/0x2f8 ksys_read+0x70/0x110 __arm64_sys_read+0x24/0x38 invoke_syscall+0x5c/0x130 el0_svc_common.constprop.0+0x48/0xf8 do_el0_svc+0x28/0x40 el0_svc+0x30/0xd0 el0t_64_sync_handler+0x144/0x168 el0t_64_sync+0x198/0x1a0 Code: f9400842 d63f0040 2a0003f4 f94002a1 (f9417437) ---[ end trace 0000000000000000 ]--- when unbind is running, the memory allocated to qspi controller and mtd device is freed during unbinding, but open/close and reading device are still running, if the reading process get read lock and start excuting, there will be above illegal memory access. This issue also can be repruduced on many other platforms like ls1046 and nxpimx8 which have qspi flash. In this patch, register a spi bus notifier which will be called before unbind process freeing device memory, add a new member mtd_event_remove to block mtd open/read, then waiting for the running task to be finished, after that, memory is safe to be free. Signed-off-by: Liwei Song <liwei.song.lsong@gmail.com> --- Hi Maintainer, This is an improved patch compared with the original one: (https://patchwork.ozlabs.org/project/linux-mtd/patch/20250325133954.3699535-1-liwei.song.lsong@gmail.com/), This v2 patch move notifier to spi-nor to avoid crash other types of flash. now this patch only aim at fixing nor-flash "bind/unbind while reading" calltrace, but for other types of flash like nand also have this issue. Thanks, Liwei. --- drivers/mtd/mtdcore.c | 3 +++ drivers/mtd/spi-nor/core.c | 46 +++++++++++++++++++++++++++++++++++++ include/linux/mtd/mtd.h | 1 + include/linux/mtd/spi-nor.h | 1 + 4 files changed, 51 insertions(+)