A couple of fixes to avoid calling DMA sync API when it's not needed.
This doesn't stop from discussing if IOMMU code is doing the right thing,
i.e. dereferences SG list when orig_nents == 0, but this is a separate
story.
Andy Shevchenko (2):
spi: Don't mark message DMA mapped when no transfer in it is
spi: Check if transfer is mapped before calling DMA sync APIs
drivers/spi/spi.c | 22 +++++++++++++++++-----
1 file changed, 17 insertions(+), 5 deletions(-)
--
2.43.0.rc1.1336.g36b5255a03ac
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 <[email protected]>
Reported-by: Neil Armstrong <[email protected]>
Closes: https://lore.kernel.org/r/[email protected]
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 <[email protected]>
Tested-by: Nícolas F. R. A. Prado <[email protected]>
Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/spi/spi.c | 18 +++++++++++++-----
1 file changed, 13 insertions(+), 5 deletions(-)
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,
--
2.43.0.rc1.1336.g36b5255a03ac
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 <[email protected]>
> Reported-by: Neil Armstrong <[email protected]>
> Closes: https://lore.kernel.org/r/[email protected]
> 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 <[email protected]>
> Tested-by: N?colas F. R. A. Prado <[email protected]>
> Signed-off-by: Andy Shevchenko <[email protected]>
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 ]---
There is no need to set the DMA mapped flag of the message if it has
no mapped transfers. Moreover, it may give the code a chance to take
the wrong paths, i.e. to exercise DMA related APIs on unmapped data.
Make __spi_map_msg() to bail earlier on the above mentioned cases.
Fixes: 99adef310f68 ("spi: Provide core support for DMA mapping transfers")
Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/spi/spi.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index b2efd4964f7c..51811f04e463 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -1243,6 +1243,7 @@ static int __spi_map_msg(struct spi_controller *ctlr, struct spi_message *msg)
else
rx_dev = ctlr->dev.parent;
+ ret = -ENOMSG;
list_for_each_entry(xfer, &msg->transfers, transfer_list) {
/* The sync is done before each transfer. */
unsigned long attrs = DMA_ATTR_SKIP_CPU_SYNC;
@@ -1272,6 +1273,9 @@ static int __spi_map_msg(struct spi_controller *ctlr, struct spi_message *msg)
}
}
}
+ /* No transfer has been mapped, bail out with success */
+ if (ret)
+ return 0;
ctlr->cur_rx_dma_dev = rx_dev;
ctlr->cur_tx_dma_dev = tx_dev;
--
2.43.0.rc1.1336.g36b5255a03ac
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?
On Wed, 22 May 2024 20:09:48 +0300, Andy Shevchenko wrote:
> A couple of fixes to avoid calling DMA sync API when it's not needed.
> This doesn't stop from discussing if IOMMU code is doing the right thing,
> i.e. dereferences SG list when orig_nents == 0, but this is a separate
> story.
>
> Andy Shevchenko (2):
> spi: Don't mark message DMA mapped when no transfer in it is
> spi: Check if transfer is mapped before calling DMA sync APIs
>
> [...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next
Thanks!
[1/2] spi: Don't mark message DMA mapped when no transfer in it is
commit: 9f788ba457b45b0ce422943fcec9fa35c4587764
[2/2] spi: Check if transfer is mapped before calling DMA sync APIs
commit: da560097c05612f8d360f86528f6213629b9c395
All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying
to this mail.
Thanks,
Mark
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
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 <[email protected]>
P.S. Sorry for the delay, I was and still is on a sick leave.
--
With Best Regards,
Andy Shevchenko
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 <[email protected]>
>
> 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