Re: [PATCH v2 19/19] dmaengine: dw-edma: Add trace support

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Data truncation of the pending elements count in engine recovery tracepoint.
--

--- Patch [19]: [PATCH v2 19/19] dmaengine: dw-edma: Add trace support ---
commit 13ac2a18e4baa8bbe3759195a89d17ff4f9df2eb
Author: Frank Li <[email protected]>

dmaengine: dw-edma: Add trace support

The circular LL ring has separate producer and consumer positions. Add
tracepoints for LL publication, descriptor append and completion,
IRQ-paired progress, and engine recovery. The common events cover eDMA
and HDMA.

> diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> index 1ccd3783181ec..4b30fa5383d0a 100644
> --- a/drivers/dma/dw-edma/dw-edma-core.c
> +++ b/drivers/dma/dw-edma/dw-edma-core.c

[ ... ]

> @@ -647,6 +655,45 @@ dw_edma_engine_recovery_sync_irqs(struct dw_edma_engine_recovery *rec)
>  	}
>  }
>  
> +static void
> +dw_edma_trace_engine_recovery(struct dw_edma_engine_recovery *rec)
> +{
> +	struct dw_edma *dw = rec->dw;
> +	u16 off = rec->dir == EDMA_DIR_WRITE ? 0 : dw->wr_ch_cnt;
> +	u16 cnt = rec->dir == EDMA_DIR_WRITE ? dw->wr_ch_cnt : dw->rd_ch_cnt;
> +	struct dw_edma_chan *chan;
> +	u16 i;
> +
> +	if (!trace_edma_engine_recovery_enabled())
> +		return;
> +
> +	for (i = 0; i < cnt; i++) {
> +		enum dw_edma_request request;
> +		enum dw_edma_status status;
> +		bool configured_ll;
> +		unsigned int pending;
> +		u32 ll_head, ll_done;
> +
> +		chan = &dw->chan[off + i];
> +		scoped_guard(spinlock_irqsave, &chan->vc.lock) {
> +			configured_ll = chan->configured && !chan->non_ll;
> +			if (configured_ll) {
> +				request = chan->request;
> +				status = chan->status;
> +				ll_head = chan->ll_head;
> +				ll_done = chan->ll_done;
> +				pending = dw_edma_ll_pending(chan);

[Severity: Medium]
Will this truncate the actual count of pending entries to 0 or 1?

Looking at dw_edma_ll_pending() in dw-edma-core.c, it is defined to return a
boolean. Assigning its result to the unsigned int pending variable means the
tracepoint will only ever record 0 or 1 instead of the true integer count of
pending items. 

Could this use dw_edma_core_get_used_num(chan) directly to ensure the correct
statistics are logged?

> +			}
> +		}
> +		if (!configured_ll)
> +			continue;
> +
> +		trace_edma_engine_recovery(chan, dw_edma_core_ch_status(chan),
> +					   request, status, ll_head, ll_done,
> +					   pending);
> +	}
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=19