Hi,
On 22/05/2024 13:53, Neil Armstrong wrote:
On 22/05/2024 13:33, Andy Shevchenko wrote:
On Wed, May 22, 2024 at 12:03:33PM +0200, Neil Armstrong wrote:
On 15/05/2024 23:09, Nícolas F. R. A. Prado wrote:
On Tue, May 07, 2024 at 11:10:27PM +0300, Andy Shevchenko wrote:
Both dma_unmap_sgtable() and sg_free_table() in spi_unmap_buf_attrs()
have checks for orig_nents against 0. No need to duplicate this.
All the same applies to other DMA mapping API calls.
Also note, there is no other user in the kernel that does this kind of
checks.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
Hi,
this commit caused a regression which I reported here:
https://lore.kernel.org/all/d3679496-2e4e-4a7c-97ed-f193bd53af1d@notapiano
along with some thoughts on the cause and a possible solution, though I'm not
familiar with this code base at all and would really appreciate any feedback you
may have.
I also see the same regression on the SM8550 and SM8650 platforms,
please CC linux-arm-msm@xxxxxxxxxxxxxxx and me for a potential fix to test on those platforms.
There is still no answer from IOMMU patch author. Do you have the same trace
due to IOMMU calls? Anyway, I guess it would be nice to see it.
Yes :
[ 6.404623] Unable to handle kernel NULL pointer dereference at virtual address 000000000000001c
<snip>
[ 6.641597] lr : __dma_sync_sg_for_device+0x3c/0x40
<snip>
[ 6.688286] Call trace:
[ 6.688287] iommu_dma_sync_sg_for_device+0x28/0x100
[ 6.717582] __dma_sync_sg_for_device+0x3c/0x40
[ 6.717585] spi_transfer_one_message+0x358/0x680
[ 6.732229] __spi_pump_transfer_message+0x188/0x494
[ 6.732232] __spi_sync+0x2a8/0x3c4
[ 6.732234] spi_sync+0x30/0x54
[ 6.732236] goodix_berlin_spi_write+0xf8/0x164 [goodix_berlin_spi]
[ 6.739854] _regmap_raw_write_impl+0x538/0x674
[ 6.750053] _regmap_raw_write+0xb4/0x144
[ 6.750056] regmap_raw_write+0x7c/0xc0
[ 6.750058] goodix_berlin_power_on+0xb0/0x1b0 [goodix_berlin_core]
[ 6.765520] goodix_berlin_probe+0xc0/0x660 [goodix_berlin_core]
[ 6.765522] goodix_berlin_spi_probe+0x12c/0x14c [goodix_berlin_spi]
[ 6.772339] spi_probe+0x84/0xe4
[ 6.772342] really_probe+0xbc/0x29c
[ 6.784313] __driver_probe_device+0x78/0x12c
[ 6.784316] driver_probe_device+0x3c/0x15c
[ 6.784319] __driver_attach+0x90/0x19c
[ 6.784322] bus_for_each_dev+0x7c/0xdc
[ 6.794520] driver_attach+0x24/0x30
[ 6.794523] bus_add_driver+0xe4/0x208
[ 6.794526] driver_register+0x5c/0x124
[ 6.802586] __spi_register_driver+0xa4/0xe4
[ 6.802589] goodix_berlin_spi_driver_init+0x20/0x1000 [goodix_berlin_spi]
[ 6.802591] do_one_initcall+0x80/0x1c8
[ 6.902310] do_init_module+0x60/0x218
[ 6.921988] load_module+0x1bcc/0x1d8c
[ 6.925847] init_module_from_file+0x88/0xcc
[ 6.930238] __arm64_sys_finit_module+0x1dc/0x2e4
[ 6.935074] invoke_syscall+0x48/0x114
[ 6.938944] el0_svc_common.constprop.0+0xc0/0xe0
[ 6.943781] do_el0_svc+0x1c/0x28
[ 6.947195] el0_svc+0x34/0xd8
[ 6.950348] el0t_64_sync_handler+0x120/0x12c
[ 6.954833] el0t_64_sync+0x190/0x194
[ 6.958600] Code: 2a0203f5 2a0303f6 a90363f7 aa0003f7 (b9401c2
Reverting 8cc3bad9d9d6 ("spi: Remove unneded check for orig_nents") removes the crash.
Meanwhile, I have three changes I posted in the replies to the initial report,
can you combine them all and test? This will be a plan B (? or A, depending on
the culprit).
I'll try to apply them and test.
I stacked the 3 changes, and it works:
Tested-by: Neil Armstrong <neil.armstrong@xxxxxxxxxx> # on SM8650-QRD
For reference, the changeset looks like:
============><============================================================================================
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 289feccca376..0851c5e1fd1f 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;
@@ -1243,6 +1248,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;
@@ -1257,6 +1263,9 @@ static int __spi_map_msg(struct spi_controller *ctlr, struct spi_message *msg)
attrs);
if (ret != 0)
return ret;
+ } else {
+ memset(&xfer->tx_sg, 0, sizeof(xfer->tx_sg));
+ xfer->tx_sg.sgl = &dummy_sg;
}
if (xfer->rx_buf != NULL) {
@@ -1270,8 +1279,14 @@ static int __spi_map_msg(struct spi_controller *ctlr, struct spi_message *msg)
return ret;
}
+ } else {
+ memset(&xfer->rx_sg, 0, sizeof(xfer->rx_sg));
+ xfer->rx_sg.sgl = &dummy_sg;
}
}
+ /* 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;
============><============================================================================================
Thanks,
Neil
Neil
