Re: [PATCH v3 14/24] dmaengine: dw-edma: Add LL interrupt placement policy

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The logic to enable periodic LL interrupts when 'more issued work follows' is ineffective and causes spurious interrupts because the driver's submission mechanism cannot pipeline multiple descriptors.
--

--- Patch [14]: [PATCH v3 14/24] dmaengine: dw-edma: Add LL interrupt placement policy ---
commit a9910cf03e474780cd4b172ac661f19546045a22
Author: Koichiro Den <[email protected]>

dmaengine: dw-edma: Add LL interrupt placement policy

This commit moves the linked list (LL) interrupt placement policy into the
common core. It configures interrupts at descriptor ends, at the last free
slot, and adds a periodic interrupt every four entries if the descriptor
does not fit or if more work is queued.

> diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> index d4c0be2cbaef1..d28e3633a8665 100644
> --- a/drivers/dma/dw-edma/dw-edma-core.c
> +++ b/drivers/dma/dw-edma/dw-edma-core.c

[ ... ]

> @@ -250,6 +253,30 @@ static bool dw_edma_ll_done_is_stopped(struct dw_edma_chan *chan)
>  	       dw_edma_core_ch_transfer_size(chan) == 0;
>  }
>  
> +static bool dw_edma_core_enable_ll_irq(struct dw_edma_desc *desc, u32 i,
> +				       u32 free)
> +{
> +	struct dw_edma_chan *chan = desc->chan;
> +
> +	/* Always report descriptor ends and the last free slot. */
> +	if (i == desc->nburst - 1 || free == 1)
> +		return true;
> +
> +	/* Keep one progress point per physical ring lap. */
> +	if (chan->ll_head == chan->ll_max - 1)
> +		return true;
> +
> +	/*
> +	 * Add periodic progress points only while this descriptor does not fit
> +	 * in the current free space or more issued work follows it.
> +	 */
> +	if (desc->nburst - i <= free &&
> +	    list_is_last(&desc->vd.node, &chan->vc.desc_issued))
> +		return false;

[Severity: Medium]
Does this logic result in spurious interrupts instead of pipelining the next
descriptor?

When multiple descriptors are queued, !list_is_last() will evaluate to true,
enabling periodic interrupts. However, looking at the submission path,
dw_edma_start_transfer() only fetches the very first descriptor:

dw_edma_start_transfer() {
    ...
    vd = vchan_next_desc(&chan->vc);
    ...
}

Since dw_edma_ll_clean_pending() does not unlink the running descriptor from
the desc_issued list until it is completely finished:

dw_edma_ll_clean_pending() {
    ...
    if (desc->done_burst != desc->nburst)
        break;

    /* Hardware has consumed this descriptor's LL entries. */
    dw_hdma_set_callback_result(vd, DMA_TRANS_NOERROR);
    list_del(&vd->node);
    ...
}

Won't these intermediate periodic interrupts just fetch the same running
descriptor, see it is already fully programmed, and do nothing? 

This appears to prevent subsequent descriptors from being pipelined until the
first one completes, meaning the extra interrupts just add CPU overhead.

> +
> +	return (chan->ll_head + 1) % DW_EDMA_LL_PROGRESS_INTERVAL == 0;
> +}
> +
>  static void dw_edma_core_ll_start(struct dw_edma_desc *desc)
>  {
>  	struct dw_edma_chan *chan = desc->chan;

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