Re: [PATCH 3/3] dmaengine: dw-axi-dmac: Decode errors in the transfer failure log

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] dmaengine: dw-axi-dmac: torn reads of hardware registers during error logging
--

commit 6d61e30d4aa304e36fef59b004f04b0033e152dd
Author: Roland Dreier <rolanddreier@rivian.com>
dmaengine: dw-axi-dmac: Decode errors in the transfer failure log

This commit decodes errors in the transfer failure log for the dw-axi-dmac
driver. It reports the first matching error name and includes the SAR, DAR,
and LLP registers for diagnostic context, avoiding dumping every LLI on
standard data errors.

> diff --git a/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c b/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c
> index 88362f1812071..e5a259e79a386 100644
> --- a/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c
> +++ b/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c

[ ... ]

> @@ -1078,11 +1108,19 @@ static noinline void axi_chan_handle_err(struct axi_dma_chan *chan, u32 status)
>  	/* Remove the completed descriptor from issued list */
>  	list_del(&vd->node);
>  
> -	/* WARN about bad descriptor */
> +	/*
> +	 * SAR and DAR advance as each burst is issued, so with bursts still
> +	 * outstanding they can be a little past the one that failed.
> +	 */
>  	dev_err(chan2dev(chan),
> -		"Bad descriptor submitted for %s, cookie: %d, irq: 0x%08x\n",
> -		axi_chan_name(chan), vd->tx.cookie, status);
> -	axi_chan_list_dump_lli(chan, vd_to_axi_desc(vd));
> +		"%s for %s, cookie: %d, irq: 0x%08x, SAR: 0x%llx DAR: 0x%llx LLP: 0x%llx\n",
> +		axi_chan_err_str(status), axi_chan_name(chan), vd->tx.cookie,
> +		status, lo_hi_readq(chan->chan_regs + CH_SAR),
> +		lo_hi_readq(chan->chan_regs + CH_DAR),
> +		lo_hi_readq(chan->chan_regs + CH_LLP));

[Severity: Medium]
Can reading the SAR, DAR, and LLP registers using lo_hi_readq result in torn
reads here?

The comment notes that bursts might still be outstanding and the hardware
registers can actively advance concurrently. 

Because lo_hi_readq performs two independent non-atomic 32-bit reads, if the
hardware advances the register across a 32-bit boundary between the low and
high reads, the resulting 64-bit value will be torn.

Could this mislead debugging efforts by logging bogus, corrupted addresses in
the diagnostic output?

> +	/* Only dump LLIs on non-data errors */
> +	if (status & (DWAXIDMAC_IRQ_ALL_ERR & ~AXI_DMA_DATA_ERR))
> +		axi_chan_list_dump_lli(chan, vd_to_axi_desc(vd));
>  
>  	if (status & (DWAXIDMAC_IRQ_SRC_DEC_ERR | DWAXIDMAC_IRQ_SRC_SLV_ERR))
>  		vd->tx_result.result = DMA_TRANS_READ_FAILED;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261006-dw-axi-fixes-v1-0-3b319e456d93@rivian.com?part=3