On 3/18/22 20:43, Christian Lamparter wrote:
> 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?
Please send a v2. Or send another patch properly fixing the driver tag
handling as explained in my previous email.
>
> 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 :-).
No ! changing can_queue to something > 32 would be very wrong ! This
really defines the number of tags that can be used, so the device maximum
queue depth. And ATA maximum queue depth is 32 with NCQ commands. It
cannot be anything larger than 32.
>
> Cheers,
> Christian
--
Damien Le Moal
Western Digital Research
