[PATCH 0/2] spi: fsl-qspi: Fix kernel panic when unbinding the SPI host

[PATCH 0/2] spi: fsl-qspi: Fix kernel panic when unbinding the SPI host

[Kevin Hao]

Hi,

I observed a kernel panic on my imx8mq-evk board. It can be easily
reproduced with the following steps:
  while true; do cat /dev/mtd0 >/dev/null; done &
  echo 30bb0000.spi >/sys/bus/platform/drivers/fsl-quadspi/unbind

The following is the kernel log:
  Unable to handle kernel paging request at virtual address ffffffc082a6015c
  Mem abort info:
    ESR = 0x0000000096000007
    EC = 0x25: DABT (current EL), IL = 32 bits
    SET = 0, FnV = 0
    EA = 0, S1PTW = 0
    FSC = 0x07: level 3 translation fault
  Data abort info:
    ISV = 0, ISS = 0x00000007, ISS2 = 0x00000000
    CM = 0, WnR = 0, TnD = 0, TagAccess = 0
    GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
  swapper pgtable: 4k pages, 39-bit VAs, pgdp=0000000041dc3000
  [ffffffc082a6015c] pgd=1000000042779003, p4d=1000000042779003, pud=1000000042779003, pmd=1000000044631403, pte=0000000000000000
  Internal error: Oops: 0000000096000007 [#1]  SMP
  Modules linked in: 8021q ath10k_pci ath10k_core etnaviv snd_soc_fsl_asoc_card ath snd_soc_imx_audmux gpu_sched snd_soc_fsl_sai snd_soc_fsl_spdif imx_sdma imx_pcm_dma snd_soc_fsl_utils snd_soc_wm8524 sch_fq_codel openvswitch nsh nf_conncount nf_nat fuse configfs nfnetlink
  Hardware name: NXP i.MX8MQ EVK (DT)
  pc : fsl_qspi_exec_op+0xa8/0x7c0
  lr : fsl_qspi_exec_op+0x88/0x7c0
  sp : ffffffc08433b650
  x8 : ffffffc08433b748 x7 : 0000000000000000 x6 : 0000000000000004
  x5 : 0000000000000001 x4 : 0000000000000000 x3 : 0000000000004174
  x2 : 0032724c809254be x1 : 000000000000c2a2 x0 : 000000173a720be8
  Call trace:
   fsl_qspi_exec_op+0xa8/0x7c0 (P)
   spi_mem_exec_op+0x410/0x4a0
   spi_mem_no_dirmap_read+0xb0/0xd0
   spi_mem_dirmap_read+0xdc/0x150
   spi_nor_read_data+0x128/0x1a0
   spi_nor_read+0xf4/0x2c8
   mtd_read_oob_std+0x80/0x98
   mtd_read_oob+0x9c/0x168
   mtd_read+0x70/0xe0
   mtdchar_read+0x224/0x2a8
   vfs_read+0xcc/0x310
   ksys_read+0x78/0x118
   __arm64_sys_read+0x24/0x38
   invoke_syscall+0x5c/0x130
   el0_svc_common.constprop.0+0x48/0xf0
   do_el0_svc+0x24/0x38
   el0_svc+0x30/0xd0
   el0t_64_sync_handler+0x10c/0x138
   el0t_64_sync+0x198/0x1a0

CPU: 1 UID: 0 PID: 527 Comm: cat Not tainted 6.14.0-rc7-next-20250321-yocto-standard+ #11 PREEMPT
pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
x29: ffffffc08433b670 x28: 0000007f8019f000 x27: 000000000000000e
x26: ffffff8004781140 x25: 000000173a814e28 x24: 0000000000000006
x23: ffffffc082a6015c x22: ffffff80046a3d80 x21: ffffff80046a3d98
x20: ffffffc082a60000 x19: ffffffc08433b9b8 x18: 0000000000000000
x17: 0000000000000000 x16: 003135312e373631 x15: 2e3432322e383231
x14: 0000000000000000 x13: ffffff80bf74d940 x12: 0000000000000000
x11: 0000000000000160 x10: 00000000000009b0 x9 : ffffffc08010ea78
Code: d2800141 d2800060 941ecb48 d503203f (b94002e1)
---
Kevin Hao (2):
      spi: fsl-qsi: Optimize fsl_qspi struct
      spi: fsl-qspi: Explicitly unregister SPI host in remove()

 drivers/spi/spi-fsl-qspi.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)
---
base-commit: 9388ec571cb1adba59d1cded2300eeb11827679c
change-id: 20250321-spi-8d7999765767

Best regards,
-- 
Kevin Hao <haokexin@gmail.com>

[PATCH 1/2] spi: fsl-qsi: Optimize fsl_qspi struct

[Kevin Hao]

Reorgize the members of the fsl_qspi struct to:
  - Reduce a hole in the struct.
  - Group members required by each op (e.g., iobase, ahb_addr,
    devtype_data and lock) into the same cacheline.

Before:
struct fsl_qspi {
	[...]

	/* size: 176, cachelines: 3, members: 11 */
	/* sum members: 168, holes: 1, sum holes: 4 */
	/* padding: 4 */
	/* member types with holes: 1, total: 1 */
	/* last cacheline: 48 bytes */
};

after:
struct fsl_qspi {
	void *                     iobase;               /*     0     8 */
	void *                     ahb_addr;             /*     8     8 */
	const struct fsl_qspi_devtype_data  * devtype_data; /*    16     8 */
	struct mutex               lock;                 /*    24    32 */
	struct completion          c;                    /*    56    32 */

	/* XXX last struct has 1 hole */

	/* --- cacheline 1 boundary (64 bytes) was 24 bytes ago --- */
	struct clk *               clk;                  /*    88     8 */
	struct clk *               clk_en;               /*    96     8 */
	struct pm_qos_request      pm_qos_req;           /*   104    48 */
	/* --- cacheline 2 boundary (128 bytes) was 24 bytes ago --- */
	struct device *            dev;                  /*   152     8 */
	int                        selected;             /*   160     4 */
	u32                        memmap_phy;           /*   164     4 */

	/* size: 168, cachelines: 3, members: 11 */
	/* member types with holes: 1, total: 1 */
	/* last cacheline: 40 bytes */
};

Signed-off-by: Kevin Hao <haokexin@gmail.com>
---
 drivers/spi/spi-fsl-qspi.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/spi-fsl-qspi.c b/drivers/spi/spi-fsl-qspi.c
index 355e6a39fb41896f460e2474a90b8f0b42068ff3..efd87f44c63a5b12b76538aa459ca8eb203b9dcd 100644
--- a/drivers/spi/spi-fsl-qspi.c
+++ b/drivers/spi/spi-fsl-qspi.c
@@ -264,14 +264,14 @@ static const struct fsl_qspi_devtype_data ls2080a_data = {
 struct fsl_qspi {
 	void __iomem *iobase;
 	void __iomem *ahb_addr;
-	u32 memmap_phy;
-	struct clk *clk, *clk_en;
-	struct device *dev;
-	struct completion c;
 	const struct fsl_qspi_devtype_data *devtype_data;
 	struct mutex lock;
+	struct completion c;
+	struct clk *clk, *clk_en;
 	struct pm_qos_request pm_qos_req;
+	struct device *dev;
 	int selected;
+	u32 memmap_phy;
 };
 
 static inline int needs_swap_endian(struct fsl_qspi *q)

-- 
2.48.1

[PATCH 2/2] spi: fsl-qspi: Explicitly unregister SPI host in remove()

[Kevin Hao]

Currently, the SPI host is registered using a managed API, which
automatically unregisters it when the device is detached from its driver.
However, this unregistration occurs after the driver's remove() callback.

Since the host is already disabled inside the remove(), any pending IO
from child devices can easily corrupt the kernel.

For example, the following steps on an imx8mq-evk board can trigger a
kernel panic:
  while true; do cat /dev/mtd0 >/dev/null; done &
  echo 30bb0000.spi >/sys/bus/platform/drivers/fsl-quadspi/unbind

To fix this, explicitly call spi_unregister_controller() in the
remove() callback. This ensures that all child devices are properly
removed before the host is disabled.

Cc: stable@vger.kernel.org
Fixes: 8fcb830a00f0 ("spi: spi-fsl-qspi: use devm_spi_register_controller")
Signed-off-by: Kevin Hao <haokexin@gmail.com>
---
 drivers/spi/spi-fsl-qspi.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-fsl-qspi.c b/drivers/spi/spi-fsl-qspi.c
index efd87f44c63a5b12b76538aa459ca8eb203b9dcd..4767d2085510c2f231476ba75e46f83271c4c645 100644
--- a/drivers/spi/spi-fsl-qspi.c
+++ b/drivers/spi/spi-fsl-qspi.c
@@ -272,6 +272,7 @@ struct fsl_qspi {
 	struct device *dev;
 	int selected;
 	u32 memmap_phy;
+	struct spi_controller *host;
 };
 
 static inline int needs_swap_endian(struct fsl_qspi *q)
@@ -862,6 +863,7 @@ static int fsl_qspi_probe(struct platform_device *pdev)
 
 	q = spi_controller_get_devdata(ctlr);
 	q->dev = dev;
+	q->host = ctlr;
 	q->devtype_data = of_device_get_match_data(dev);
 	if (!q->devtype_data) {
 		ret = -ENODEV;
@@ -934,7 +936,7 @@ static int fsl_qspi_probe(struct platform_device *pdev)
 
 	ctlr->dev.of_node = np;
 
-	ret = devm_spi_register_controller(dev, ctlr);
+	ret = spi_register_controller(ctlr);
 	if (ret)
 		goto err_destroy_mutex;
 
@@ -957,6 +959,8 @@ static void fsl_qspi_remove(struct platform_device *pdev)
 {
 	struct fsl_qspi *q = platform_get_drvdata(pdev);
 
+	spi_unregister_controller(q->host);
+
 	/* disable the hardware */
 	qspi_writel(q, QUADSPI_MCR_MDIS_MASK, q->iobase + QUADSPI_MCR);
 	qspi_writel(q, 0x0, q->iobase + QUADSPI_RSER);

-- 
2.48.1

Re: [PATCH 2/2] spi: fsl-qspi: Explicitly unregister SPI host in remove()

[Han Xu]

On 25/03/21 08:40PM, Kevin Hao wrote:
> 
> Currently, the SPI host is registered using a managed API, which
> automatically unregisters it when the device is detached from its driver.
> However, this unregistration occurs after the driver's remove() callback.
> 
> Since the host is already disabled inside the remove(), any pending IO
> from child devices can easily corrupt the kernel.
> 
> For example, the following steps on an imx8mq-evk board can trigger a
> kernel panic:
>   while true; do cat /dev/mtd0 >/dev/null; done &
>   echo 30bb0000.spi >/sys/bus/platform/drivers/fsl-quadspi/unbind
> 
> To fix this, explicitly call spi_unregister_controller() in the
> remove() callback. This ensures that all child devices are properly
> removed before the host is disabled.

If you explicitly remove the child devices, such as 
cd /sys/bus/spi/drivers/spi-nor
echo spi0.0 > unbind
then unbind the fsl-quadspi driver, the kernel panic does not occur.

Not sure if it should be the responsibility of the fsl-quadspi driver to handle
this, IMO it is a common issue with all SPI drivers.

> 
> Cc: stable@vger.kernel.org
> Fixes: 8fcb830a00f0 ("spi: spi-fsl-qspi: use devm_spi_register_controller")
> Signed-off-by: Kevin Hao <haokexin@gmail.com>
> ---
>  drivers/spi/spi-fsl-qspi.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/spi/spi-fsl-qspi.c b/drivers/spi/spi-fsl-qspi.c
> index efd87f44c63a5b12b76538aa459ca8eb203b9dcd..4767d2085510c2f231476ba75e46f83271c4c645 100644
> --- a/drivers/spi/spi-fsl-qspi.c
> +++ b/drivers/spi/spi-fsl-qspi.c
> @@ -272,6 +272,7 @@ struct fsl_qspi {
>         struct device *dev;
>         int selected;
>         u32 memmap_phy;
> +       struct spi_controller *host;
>  };
> 
>  static inline int needs_swap_endian(struct fsl_qspi *q)
> @@ -862,6 +863,7 @@ static int fsl_qspi_probe(struct platform_device *pdev)
> 
>         q = spi_controller_get_devdata(ctlr);
>         q->dev = dev;
> +       q->host = ctlr;
>         q->devtype_data = of_device_get_match_data(dev);
>         if (!q->devtype_data) {
>                 ret = -ENODEV;
> @@ -934,7 +936,7 @@ static int fsl_qspi_probe(struct platform_device *pdev)
> 
>         ctlr->dev.of_node = np;
> 
> -       ret = devm_spi_register_controller(dev, ctlr);
> +       ret = spi_register_controller(ctlr);
>         if (ret)
>                 goto err_destroy_mutex;
> 
> @@ -957,6 +959,8 @@ static void fsl_qspi_remove(struct platform_device *pdev)
>  {
>         struct fsl_qspi *q = platform_get_drvdata(pdev);
> 
> +       spi_unregister_controller(q->host);
> +
>         /* disable the hardware */
>         qspi_writel(q, QUADSPI_MCR_MDIS_MASK, q->iobase + QUADSPI_MCR);
>         qspi_writel(q, 0x0, q->iobase + QUADSPI_RSER);
> 
> --
> 2.48.1
> 

Re: [PATCH 1/2] spi: fsl-qsi: Optimize fsl_qspi struct

[Han Xu]

On 25/03/21 08:40PM, Kevin Hao wrote:
> 
> Reorgize the members of the fsl_qspi struct to:
>   - Reduce a hole in the struct.
>   - Group members required by each op (e.g., iobase, ahb_addr,
>     devtype_data and lock) into the same cacheline.
> 
> Before:
> struct fsl_qspi {
>         [...]
> 
>         /* size: 176, cachelines: 3, members: 11 */
>         /* sum members: 168, holes: 1, sum holes: 4 */
>         /* padding: 4 */
>         /* member types with holes: 1, total: 1 */
>         /* last cacheline: 48 bytes */
> };
> 
> after:
> struct fsl_qspi {
>         void *                     iobase;               /*     0     8 */
>         void *                     ahb_addr;             /*     8     8 */
>         const struct fsl_qspi_devtype_data  * devtype_data; /*    16     8 */
>         struct mutex               lock;                 /*    24    32 */
>         struct completion          c;                    /*    56    32 */
> 
>         /* XXX last struct has 1 hole */
> 
>         /* --- cacheline 1 boundary (64 bytes) was 24 bytes ago --- */
>         struct clk *               clk;                  /*    88     8 */
>         struct clk *               clk_en;               /*    96     8 */
>         struct pm_qos_request      pm_qos_req;           /*   104    48 */
>         /* --- cacheline 2 boundary (128 bytes) was 24 bytes ago --- */
>         struct device *            dev;                  /*   152     8 */
>         int                        selected;             /*   160     4 */
>         u32                        memmap_phy;           /*   164     4 */
> 
>         /* size: 168, cachelines: 3, members: 11 */
>         /* member types with holes: 1, total: 1 */
>         /* last cacheline: 40 bytes */
> };
> 
> Signed-off-by: Kevin Hao <haokexin@gmail.com>
> ---
>  drivers/spi/spi-fsl-qspi.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/spi/spi-fsl-qspi.c b/drivers/spi/spi-fsl-qspi.c
> index 355e6a39fb41896f460e2474a90b8f0b42068ff3..efd87f44c63a5b12b76538aa459ca8eb203b9dcd 100644
> --- a/drivers/spi/spi-fsl-qspi.c
> +++ b/drivers/spi/spi-fsl-qspi.c
> @@ -264,14 +264,14 @@ static const struct fsl_qspi_devtype_data ls2080a_data = {
>  struct fsl_qspi {
>         void __iomem *iobase;
>         void __iomem *ahb_addr;
> -       u32 memmap_phy;
> -       struct clk *clk, *clk_en;
> -       struct device *dev;
> -       struct completion c;
>         const struct fsl_qspi_devtype_data *devtype_data;
>         struct mutex lock;
> +       struct completion c;
> +       struct clk *clk, *clk_en;
>         struct pm_qos_request pm_qos_req;
> +       struct device *dev;
>         int selected;
> +       u32 memmap_phy;
>  };

Reviewed-by: Han Xu <han.xu@nxp.com>

> 
>  static inline int needs_swap_endian(struct fsl_qspi *q)
> 
> --
> 2.48.1
> 

Re: [PATCH 2/2] spi: fsl-qspi: Explicitly unregister SPI host in remove()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2839 bytes --]

On Fri, Mar 21, 2025 at 10:23:07AM -0500, Han Xu wrote:
> On 25/03/21 08:40PM, Kevin Hao wrote:

> > Since the host is already disabled inside the remove(), any pending IO
> > from child devices can easily corrupt the kernel.

...

> > To fix this, explicitly call spi_unregister_controller() in the
> > remove() callback. This ensures that all child devices are properly
> > removed before the host is disabled.

> If you explicitly remove the child devices, such as 
> cd /sys/bus/spi/drivers/spi-nor
> echo spi0.0 > unbind
> then unbind the fsl-quadspi driver, the kernel panic does not occur.

> Not sure if it should be the responsibility of the fsl-quadspi driver to handle
> this, IMO it is a common issue with all SPI drivers.

This is a bug in the driver, it needs to be able to continue supporting
child devices until it is unregistered.  This means that either the
unregistration needs to be manual or any disabling also needs to be done
via devm.

> 
> > 
> > Cc: stable@vger.kernel.org
> > Fixes: 8fcb830a00f0 ("spi: spi-fsl-qspi: use devm_spi_register_controller")
> > Signed-off-by: Kevin Hao <haokexin@gmail.com>
> > ---
> >  drivers/spi/spi-fsl-qspi.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/spi/spi-fsl-qspi.c b/drivers/spi/spi-fsl-qspi.c
> > index efd87f44c63a5b12b76538aa459ca8eb203b9dcd..4767d2085510c2f231476ba75e46f83271c4c645 100644
> > --- a/drivers/spi/spi-fsl-qspi.c
> > +++ b/drivers/spi/spi-fsl-qspi.c
> > @@ -272,6 +272,7 @@ struct fsl_qspi {
> >         struct device *dev;
> >         int selected;
> >         u32 memmap_phy;
> > +       struct spi_controller *host;
> >  };
> > 
> >  static inline int needs_swap_endian(struct fsl_qspi *q)
> > @@ -862,6 +863,7 @@ static int fsl_qspi_probe(struct platform_device *pdev)
> > 
> >         q = spi_controller_get_devdata(ctlr);
> >         q->dev = dev;
> > +       q->host = ctlr;
> >         q->devtype_data = of_device_get_match_data(dev);
> >         if (!q->devtype_data) {
> >                 ret = -ENODEV;
> > @@ -934,7 +936,7 @@ static int fsl_qspi_probe(struct platform_device *pdev)
> > 
> >         ctlr->dev.of_node = np;
> > 
> > -       ret = devm_spi_register_controller(dev, ctlr);
> > +       ret = spi_register_controller(ctlr);
> >         if (ret)
> >                 goto err_destroy_mutex;
> > 
> > @@ -957,6 +959,8 @@ static void fsl_qspi_remove(struct platform_device *pdev)
> >  {
> >         struct fsl_qspi *q = platform_get_drvdata(pdev);
> > 
> > +       spi_unregister_controller(q->host);
> > +
> >         /* disable the hardware */
> >         qspi_writel(q, QUADSPI_MCR_MDIS_MASK, q->iobase + QUADSPI_MCR);
> >         qspi_writel(q, 0x0, q->iobase + QUADSPI_RSER);
> > 
> > --
> > 2.48.1
> > 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]