diff mbox series

[v1,2/2] spi: Check if transfer is mapped before calling DMA sync APIs

Message ID 20240522171018.3362521-3-andriy.shevchenko@linux.intel.com (mailing list archive)
State Accepted
Commit da560097c05612f8d360f86528f6213629b9c395
Headers show
Series soi: Don't call DMA sync API when not needed | expand

Commit Message

Andy Shevchenko May 22, 2024, 5:09 p.m. UTC
The resent update to remove the orig_nents checks revealed
that not all DMA sync backends can cope with the unallocated
SG list, while supplying orig_nents == 0 (the commit 861370f49ce4
("iommu/dma: force bouncing if the size is not cacheline-aligned"),
for example, makes that happen for the IOMMU case). It means
we have to check if the buffers are DMA mapped before trying
to sync them. Re-introduce that check in a form of calling
->can_dma() in the same way as it's done in the DMA mapping loop
for the SPI transfers.

Reported-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
Reported-by: Neil Armstrong <neil.armstrong@linaro.org>
Closes: https://lore.kernel.org/r/8ae675b5-fcf9-4c9b-b06a-4462f70e1322@linaro.org
Closes: https://lore.kernel.org/all/d3679496-2e4e-4a7c-97ed-f193bd53af1d@notapiano
Fixes: 8cc3bad9d9d6 ("spi: Remove unneded check for orig_nents")
Suggested-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
Tested-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/spi/spi.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

Comments

Nícolas F. R. A. Prado May 22, 2024, 6:41 p.m. UTC | #1
On Wed, May 22, 2024 at 08:09:50PM +0300, Andy Shevchenko wrote:
> The resent update to remove the orig_nents checks revealed
> that not all DMA sync backends can cope with the unallocated
> SG list, while supplying orig_nents == 0 (the commit 861370f49ce4
> ("iommu/dma: force bouncing if the size is not cacheline-aligned"),
> for example, makes that happen for the IOMMU case). It means
> we have to check if the buffers are DMA mapped before trying
> to sync them. Re-introduce that check in a form of calling
> ->can_dma() in the same way as it's done in the DMA mapping loop
> for the SPI transfers.
> 
> Reported-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> Reported-by: Neil Armstrong <neil.armstrong@linaro.org>
> Closes: https://lore.kernel.org/r/8ae675b5-fcf9-4c9b-b06a-4462f70e1322@linaro.org
> Closes: https://lore.kernel.org/all/d3679496-2e4e-4a7c-97ed-f193bd53af1d@notapiano
> Fixes: 8cc3bad9d9d6 ("spi: Remove unneded check for orig_nents")
> Suggested-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> Tested-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Hi Andy,

sorry I keep bothering you.

I tested this series and I still get the oops (attached at the end for
reference). When I tried this change originally, I added it on top of the
patches you had supplied. And as it turns out one of them was necessary.
Specifically, if I add 

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index f94420858c22..9bc9fd10d538 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -1220,6 +1220,11 @@ void spi_unmap_buf(struct spi_controller *ctlr, struct device *dev,
        spi_unmap_buf_attrs(ctlr, dev, sgt, dir, 0);
 }

+/* Dummy SG for unidirect transfers */
+static struct scatterlist dummy_sg = {
+       .page_link = SG_END,
+};
+
 static int __spi_map_msg(struct spi_controller *ctlr, struct spi_message *msg)
 {
        struct device *tx_dev, *rx_dev;
@@ -1258,6 +1263,8 @@ static int __spi_map_msg(struct spi_controller *ctlr, struct spi_message *msg)
                                                attrs);
                        if (ret != 0)
                                return ret;
+               } else {
+                       xfer->tx_sg.sgl = &dummy_sg;
                }

                if (xfer->rx_buf != NULL) {
@@ -1271,6 +1278,8 @@ static int __spi_map_msg(struct spi_controller *ctlr, struct spi_message *msg)

                                return ret;
                        }
+               } else {
+                       xfer->rx_sg.sgl = &dummy_sg;
                }
        }
        /* No transfer has been mapped, bail out with success */

on top of this series, then I don't get any oops. (The memset doesn't seem to be
required, or at least in my test it didn't trigger any issue).

I guess this is needed because the can_dma "flag" is shared between rx and tx,
but either one of them might not have a buffer assigned (for unidirectional
transfers).

Sorry about the confusion.

Thanks,
Nícolas

Oops:

[    3.085228] Unable to handle kernel NULL pointer dereference at virtual address 000000000000001c
[    3.096144] Unable to handle kernel NULL pointer dereference at virtual address 000000000000001c
[    3.101468] Mem abort info:
[    3.110363] Mem abort info:
[    3.113239]   ESR = 0x0000000096000004
[    3.116114]   ESR = 0x0000000096000004
[    3.119969]   EC = 0x25: DABT (current EL), IL = 32 bits
[    3.123827]   EC = 0x25: DABT (current EL), IL = 32 bits
[    3.129284]   SET = 0, FnV = 0
[    3.134744]   SET = 0, FnV = 0
[    3.137881]   EA = 0, S1PTW = 0
[    3.141022]   EA = 0, S1PTW = 0
[    3.144247]   FSC = 0x04: level 0 translation fault
[    3.147475]   FSC = 0x04: level 0 translation fault
[    3.152491] Data abort info:
[    3.157505] Data abort info:
[    3.160468]   ISV = 0, ISS = 0x00000004, ISS2 = 0x00000000
[    3.163434]   ISV = 0, ISS = 0x00000004, ISS2 = 0x00000000
[    3.169065]   CM = 0, WnR = 0, TnD = 0, TagAccess = 0
[    3.169069]   GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
[    3.169073] [000000000000001c] user address but active_mm is swapper
[    3.169078] Internal error: Oops: 0000000096000004 [#1] PREEMPT SMP
[    3.174711]   CM = 0, WnR = 0, TnD = 0, TagAccess = 0
[    3.179896] Modules linked in:
[    3.179903] CPU: 6 PID: 68 Comm: kworker/u32:2 Not tainted 6.9.0-next-20240515-00006-g6c6aba391be0 #427
[    3.185352]   GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
[    3.191869] Hardware name: Google Kingoftown (DT)
[    3.191872] Workqueue: events_unbound deferred_probe_work_func
[    3.198309] [000000000000001c] user address but active_mm is swapper
[    3.203495]
[    3.203497] pstate: 80400009 (Nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[    3.247739] pc : iommu_dma_sync_sg_for_device+0x28/0x100
[    3.253204] lr : __dma_sync_sg_for_device+0x28/0x4c
[    3.258220] sp : ffff800080942dd0
[    3.261624] x29: ffff800080942dd0 x28: ffff1a58c1012010 x27: ffff1a58c1012010
[    3.268953] x26: ffff1a58c31a0800 x25: ffff1a58c31a0c80 x24: 00000000000186a0
[    3.276285] x23: ffff1a58c1012010 x22: 0000000000000001 x21: 0000000000000000
[    3.283615] x20: ffffc3561c10c718 x19: 0000000000000000 x18: ffffc3561cde8948
[    3.290943] x17: 0000000000010108 x16: 0000000000000000 x15: 0000000000000002
[    3.298274] x14: ffff1a58c09156c0 x13: 0000000000000001 x12: 0000000000000000
[    3.305602] x11: 071c71c71c71c71c x10: 0000000000000aa0 x9 : ffff800080942c90
[    3.312932] x8 : ffff1a5a36da4800 x7 : 4000000000000000 x6 : ffff1a5a36da4828
[    3.320265] x5 : ffffc3561b51a780 x4 : ffffc3561a704acc x3 : 0000000000000001
[    3.327596] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff1a58c1012010
[    3.334927] Call trace:
[    3.337442]  iommu_dma_sync_sg_for_device+0x28/0x100
[    3.342540]  __dma_sync_sg_for_device+0x28/0x4c
[    3.347203]  spi_transfer_one_message+0x3a8/0x700
[    3.352042]  __spi_pump_transfer_message+0x198/0x4dc
[    3.357143]  __spi_sync+0x2a0/0x3c4
[    3.360738]  spi_sync+0x30/0x54
[    3.363971]  spi_mem_exec_op+0x26c/0x41c
[    3.368008]  spi_nor_spimem_read_data+0x148/0x158
[    3.372848]  spi_nor_read_data+0x30/0x3c
[    3.376881]  spi_nor_read_sfdp+0x74/0xe4
[    3.380916]  spi_nor_parse_sfdp+0x120/0x11d0
[    3.385314]  spi_nor_sfdp_init_params_deprecated+0x3c/0x8c
[    3.390951]  spi_nor_scan+0x7ac/0xef8
[    3.394721]  spi_nor_probe+0x94/0x2ec
[    3.398490]  spi_mem_probe+0x6c/0xac
[    3.402171]  spi_probe+0x84/0xe4
[    3.405491]  really_probe+0xbc/0x2a0
[    3.409173]  __driver_probe_device+0x78/0x12c
[    3.413654]  driver_probe_device+0x40/0x160
[    3.417961]  __device_attach_driver+0xb8/0x134
[    3.422533]  bus_for_each_drv+0x84/0xe0
[    3.426478]  __device_attach+0xa8/0x1b0
[    3.430423]  device_initial_probe+0x14/0x20
[    3.434720]  bus_probe_device+0xa8/0xac
[    3.438664]  device_add+0x590/0x750
[    3.442257]  __spi_add_device+0x138/0x208
[    3.446378]  of_register_spi_device+0x394/0x57c
[    3.451037]  spi_register_controller+0x394/0x760
[    3.455787]  qcom_qspi_probe+0x328/0x390
[    3.459826]  platform_probe+0x68/0xd8
[    3.463595]  really_probe+0xbc/0x2a0
[    3.467277]  __driver_probe_device+0x78/0x12c
[    3.471760]  driver_probe_device+0x40/0x160
[    3.476068]  __device_attach_driver+0xb8/0x134
[    3.480641]  bus_for_each_drv+0x84/0xe0
[    3.484585]  __device_attach+0xa8/0x1b0
[    3.488530]  device_initial_probe+0x14/0x20
[    3.492838]  bus_probe_device+0xa8/0xac
[    3.496784]  deferred_probe_work_func+0x88/0xc0
[    3.501442]  process_one_work+0x154/0x298
[    3.505568]  worker_thread+0x304/0x408
[    3.509425]  kthread+0x118/0x11c
[    3.512745]  ret_from_fork+0x10/0x20
[    3.516431] Code: 2a0203f5 2a0303f6 a90363f7 aa0003f7 (b9401c20)
[    3.522686] ---[ end trace 0000000000000000 ]---

[    3.527428] Internal error: Oops: 0000000096000004 [#2] PREEMPT SMP
[    3.533868] Modules linked in:
[    3.537013] CPU: 2 PID: 102 Comm: cros_ec_spi_hig Tainted: G      D            6.9.0-next-20240515-00006-g6c6aba391be0 #427
[    3.548431] Hardware name: Google Kingoftown (DT)
[    3.553267] pstate: 80400009 (Nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[    3.560412] pc : iommu_dma_sync_sg_for_device+0x28/0x100
[    3.565877] lr : __dma_sync_sg_for_device+0x28/0x4c
[    3.570899] sp : ffff8000809fb990
[    3.574312] x29: ffff8000809fb990 x28: ffff1a58c0ff8010 x27: ffff1a58c0ff8010
[    3.581644] x26: ffff1a58c4d39800 x25: ffff1a58c4d39c80 x24: 00000000000186a0
[    3.588976] x23: ffff1a58c0ff8010 x22: 0000000000000001 x21: 0000000000000000
[    3.596307] x20: ffffc3561c10c3d8 x19: 0000000000000000 x18: 0000000000000000
[    3.603639] x17: 000000040044ffff x16: 005000f2b5503510 x15: 0000000000000002
[    3.610972] x14: 0000000000000001 x13: 0000000000162b7a x12: 0000000000000001
[    3.618304] x11: ffff8000809fb8a0 x10: ffff1a58c1f93ff8 x9 : ffff1a58c4d39c69
[    3.625630] x8 : ffff1a58c102db04 x7 : 0000000000000000 x6 : 0000000000000001
[    3.632962] x5 : ffffc3561b51a780 x4 : ffffc3561a704acc x3 : 0000000000000001
[    3.640291] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff1a58c0ff8010
[    3.647623] Call trace:
[    3.650148]  iommu_dma_sync_sg_for_device+0x28/0x100
[    3.655249]  __dma_sync_sg_for_device+0x28/0x4c
[    3.659903]  spi_transfer_one_message+0x3a8/0x700
[    3.664746]  __spi_pump_transfer_message+0x198/0x4dc
[    3.669853]  __spi_sync+0x2a0/0x3c4
[    3.673441]  spi_sync_locked+0x10/0x1c
[    3.677299]  receive_n_bytes+0xc0/0x118
[    3.681248]  do_cros_ec_pkt_xfer_spi+0x3c0/0x530
[    3.685997]  cros_ec_xfer_high_pri_work+0x20/0x34
[    3.690835]  kthread_worker_fn+0xcc/0x184
[    3.694960]  kthread+0x118/0x11c
[    3.698282]  ret_from_fork+0x10/0x20
[    3.701964] Code: 2a0203f5 2a0303f6 a90363f7 aa0003f7 (b9401c20)
[    3.708221] ---[ end trace 0000000000000000 ]---
Mark Brown May 23, 2024, 12:01 p.m. UTC | #2
On Wed, May 22, 2024 at 02:41:48PM -0400, Nícolas F. R. A. Prado wrote:

> I tested this series and I still get the oops (attached at the end for
> reference). When I tried this change originally, I added it on top of the
> patches you had supplied. And as it turns out one of them was necessary.
> Specifically, if I add 
> 
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index f94420858c22..9bc9fd10d538 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -1220,6 +1220,11 @@ void spi_unmap_buf(struct spi_controller *ctlr, struct device *dev,
>         spi_unmap_buf_attrs(ctlr, dev, sgt, dir, 0);

The other two patches are already queued, could you send this one
incrementally please Andy?
Nícolas F. R. A. Prado May 29, 2024, 12:35 p.m. UTC | #3
On Wed, May 22, 2024 at 02:41:51PM -0400, Nícolas F. R. A. Prado wrote:
> On Wed, May 22, 2024 at 08:09:50PM +0300, Andy Shevchenko wrote:
[..]
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index f94420858c22..9bc9fd10d538 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -1220,6 +1220,11 @@ void spi_unmap_buf(struct spi_controller *ctlr, struct device *dev,
>         spi_unmap_buf_attrs(ctlr, dev, sgt, dir, 0);
>  }
> 
> +/* Dummy SG for unidirect transfers */
> +static struct scatterlist dummy_sg = {
> +       .page_link = SG_END,
> +};
> +
>  static int __spi_map_msg(struct spi_controller *ctlr, struct spi_message *msg)
>  {
>         struct device *tx_dev, *rx_dev;
> @@ -1258,6 +1263,8 @@ static int __spi_map_msg(struct spi_controller *ctlr, struct spi_message *msg)
>                                                 attrs);
>                         if (ret != 0)
>                                 return ret;
> +               } else {
> +                       xfer->tx_sg.sgl = &dummy_sg;
>                 }
> 
>                 if (xfer->rx_buf != NULL) {
> @@ -1271,6 +1278,8 @@ static int __spi_map_msg(struct spi_controller *ctlr, struct spi_message *msg)
> 
>                                 return ret;
>                         }
> +               } else {
> +                       xfer->rx_sg.sgl = &dummy_sg;
>                 }
>         }
>         /* No transfer has been mapped, bail out with success */

Hi Andy,

I can send this patch to the list myself with your authorship, I just need your
SoB.

Thanks,
Nícolas
Andy Shevchenko May 29, 2024, 12:48 p.m. UTC | #4
On Wed, May 29, 2024 at 08:35:26AM -0400, Nícolas F. R. A. Prado wrote:
> On Wed, May 22, 2024 at 02:41:51PM -0400, Nícolas F. R. A. Prado wrote:
> > On Wed, May 22, 2024 at 08:09:50PM +0300, Andy Shevchenko wrote:

[..]

> I can send this patch to the list myself with your authorship, I just need
> your SoB.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

P.S. Sorry for the delay, I was and still is on a sick leave.
Nícolas F. R. A. Prado May 29, 2024, 12:56 p.m. UTC | #5
On Wed, May 29, 2024 at 03:48:05PM +0300, Andy Shevchenko wrote:
> On Wed, May 29, 2024 at 08:35:26AM -0400, Nícolas F. R. A. Prado wrote:
> > On Wed, May 22, 2024 at 02:41:51PM -0400, Nícolas F. R. A. Prado wrote:
> > > On Wed, May 22, 2024 at 08:09:50PM +0300, Andy Shevchenko wrote:
> 
> [..]
> 
> > I can send this patch to the list myself with your authorship, I just need
> > your SoB.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> P.S. Sorry for the delay, I was and still is on a sick leave.

No need to be sorry, health comes first. Hope you get better soon! :)

Thanks,
Nícolas
diff mbox series

Patch

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 51811f04e463..cc8bb7d5ba1a 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -1311,7 +1311,7 @@  static int __spi_unmap_msg(struct spi_controller *ctlr, struct spi_message *msg)
 	return 0;
 }
 
-static void spi_dma_sync_for_device(struct spi_controller *ctlr,
+static void spi_dma_sync_for_device(struct spi_controller *ctlr, struct spi_message *msg,
 				    struct spi_transfer *xfer)
 {
 	struct device *rx_dev = ctlr->cur_rx_dma_dev;
@@ -1320,11 +1320,14 @@  static void spi_dma_sync_for_device(struct spi_controller *ctlr,
 	if (!ctlr->cur_msg_mapped)
 		return;
 
+	if (!ctlr->can_dma(ctlr, msg->spi, xfer))
+		return;
+
 	dma_sync_sgtable_for_device(tx_dev, &xfer->tx_sg, DMA_TO_DEVICE);
 	dma_sync_sgtable_for_device(rx_dev, &xfer->rx_sg, DMA_FROM_DEVICE);
 }
 
-static void spi_dma_sync_for_cpu(struct spi_controller *ctlr,
+static void spi_dma_sync_for_cpu(struct spi_controller *ctlr, struct spi_message *msg,
 				 struct spi_transfer *xfer)
 {
 	struct device *rx_dev = ctlr->cur_rx_dma_dev;
@@ -1333,6 +1336,9 @@  static void spi_dma_sync_for_cpu(struct spi_controller *ctlr,
 	if (!ctlr->cur_msg_mapped)
 		return;
 
+	if (!ctlr->can_dma(ctlr, msg->spi, xfer))
+		return;
+
 	dma_sync_sgtable_for_cpu(rx_dev, &xfer->rx_sg, DMA_FROM_DEVICE);
 	dma_sync_sgtable_for_cpu(tx_dev, &xfer->tx_sg, DMA_TO_DEVICE);
 }
@@ -1350,11 +1356,13 @@  static inline int __spi_unmap_msg(struct spi_controller *ctlr,
 }
 
 static void spi_dma_sync_for_device(struct spi_controller *ctrl,
+				    struct spi_message *msg,
 				    struct spi_transfer *xfer)
 {
 }
 
 static void spi_dma_sync_for_cpu(struct spi_controller *ctrl,
+				 struct spi_message *msg,
 				 struct spi_transfer *xfer)
 {
 }
@@ -1626,10 +1634,10 @@  static int spi_transfer_one_message(struct spi_controller *ctlr,
 			reinit_completion(&ctlr->xfer_completion);
 
 fallback_pio:
-			spi_dma_sync_for_device(ctlr, xfer);
+			spi_dma_sync_for_device(ctlr, msg, xfer);
 			ret = ctlr->transfer_one(ctlr, msg->spi, xfer);
 			if (ret < 0) {
-				spi_dma_sync_for_cpu(ctlr, xfer);
+				spi_dma_sync_for_cpu(ctlr, msg, xfer);
 
 				if (ctlr->cur_msg_mapped &&
 				   (xfer->error & SPI_TRANS_FAIL_NO_START)) {
@@ -1654,7 +1662,7 @@  static int spi_transfer_one_message(struct spi_controller *ctlr,
 					msg->status = ret;
 			}
 
-			spi_dma_sync_for_cpu(ctlr, xfer);
+			spi_dma_sync_for_cpu(ctlr, msg, xfer);
 		} else {
 			if (xfer->len)
 				dev_err(&msg->spi->dev,