On 18/03/2022 10:34, Damien Le Moal wrote:
On 3/18/22 18:03, Christian Lamparter wrote:
the driver uses libata's "tag" values from in various arrays.
Since the mentioned patch bumped the ATA_TAG_INTERNAL to 32,
the value of the SATA_DWC_QCMD_MAX needs to be bumped to 33.
Otherwise ATA_TAG_INTERNAL cause a crash like this:
| BUG: Kernel NULL pointer dereference at 0x00000000
| Faulting instruction address: 0xc03ed4b8
| Oops: Kernel access of bad area, sig: 11 [#1]
| BE PAGE_SIZE=4K PowerPC 44x Platform
| CPU: 0 PID: 362 Comm: scsi_eh_1 Not tainted 5.4.163 #0
| NIP: c03ed4b8 LR: c03d27e8 CTR: c03ed36c
| REGS: cfa59950 TRAP: 0300 Not tainted (5.4.163)
| MSR: 00021000 <CE,ME> CR: 42000222 XER: 00000000
| DEAR: 00000000 ESR: 00000000
| GPR00: c03d27e8 cfa59a08 cfa55fe0 00000000 0fa46bc0 [...]
| [..]
| NIP [c03ed4b8] sata_dwc_qc_issue+0x14c/0x254
| LR [c03d27e8] ata_qc_issue+0x1c8/0x2dc
| Call Trace:
| [cfa59a08] [c003f4e0] __cancel_work_timer+0x124/0x194 (unreliable)
| [cfa59a78] [c03d27e8] ata_qc_issue+0x1c8/0x2dc
| [cfa59a98] [c03d2b3c] ata_exec_internal_sg+0x240/0x524
| [cfa59b08] [c03d2e98] ata_exec_internal+0x78/0xe0
| [cfa59b58] [c03d30fc] ata_read_log_page.part.38+0x1dc/0x204
| [cfa59bc8] [c03d324c] ata_identify_page_supported+0x68/0x130
| [...]
this is because sata_dwc_dma_xfer_complete() NULLs the
dma_pending's next neighbour "chan" (a *dma_chan struct) in
this '32' case right here (line ~735):
hsdevp->dma_pending[tag] = SATA_DWC_DMA_PENDING_NONE;
Then the next time, a dma gets issued; dma_dwc_xfer_setup() passes
the NULL'd hsdevp->chan to the dmaengine_slave_config() which then
causes the crash.
Reported-by: ticerex (OpenWrt Forum)
I would remove this since you have the link to the bug report below.
Without a real name & email, this does not make much sense.
Ok.
Fixes: 28361c403683c ("libata: add extra internal command")
Cc: stable@xxxxxxxxxx # 4.18+
Link: https://forum.openwrt.org/t/my-book-live-duo-reboot-loop/122464
Signed-off-by: Christian Lamparter <chunkeey@xxxxxxxxx>
---
ticerex said when I've asked him about his real name+email for the patches:
"Please use my forum nick."
<https://forum.openwrt.org/t/my-book-live-duo-reboot-loop/122464/14>
(I know checkpatch.pl complains about that. But what can you do?)
---
drivers/ata/sata_dwc_460ex.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/ata/sata_dwc_460ex.c b/drivers/ata/sata_dwc_460ex.c
index bec33d781ae0..061b27584667 100644
--- a/drivers/ata/sata_dwc_460ex.c
+++ b/drivers/ata/sata_dwc_460ex.c
@@ -137,7 +137,7 @@ struct sata_dwc_device {
#endif
};
-#define SATA_DWC_QCMD_MAX 32
+#define SATA_DWC_QCMD_MAX 33
This is really ugly, but I do not see a better way to do it simply.
But at least, let's do something like this to avoid confusion and to show
that this driver is not doing some black magic with ATA drives:
/*
* Allow one extra special slot for commands and DMA management to
* account for libata internal commands.
*/
#define SATA_DWC_QCMD_MAX (ATA_MAX_QUEUE + 1)
Thoughts ?
Yes, this works. That ATA_TAG_INTERNAL value has remained unchanged
since Jens' change from 2018. How do you want to proceed?
Should I make a v2, or do you update the patch locally?
The way I understand it. this ATA_TAG_INTERNAL has the special MAGIC
value so DMA "qc issues" do not interfere with possibly concurrent NCQ
"qc issues" on the involved controllers.
sata_dwc_460ex's NCQ is disabled/gimped by line 1093:
| /* .can_queue = ATA_MAX_QUEUE, */)
Reassiging the ATA_TAG_INTERNAL could have been done.
But just increasing the arrays worked :-).
Cheers,
Christian
