Re: [PATCH 3/4] dmaengine: qcom: gpi: Ignore cancelled transfer-completion events

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] dma: qcom/gpi: dropped transfers during pause due to CANCELLED events
--

commit 4a0fc29a2e60fc21d8e540e5657013e4f69245b6
Author: Naresh Maramaina <naresh.maramaina@oss.qualcomm.com>

dmaengine: qcom: gpi: Ignore cancelled transfer-completion events

Channel cancellation can generate a transfer-completion event marked
with the cancellation status bit. If the event is processed after
the channel has left ACTIVE_STATE, it triggers a false error report.

Ignore transfer-completion events marked as cancellation events when
the channel is no longer active. Continue to report all other
unexpected completion events received on inactive channels.

> diff --git a/drivers/dma/qcom/gpi.c b/drivers/dma/qcom/gpi.c
> index f72b80bf5553c..7e1fa8d249024 100644
> --- a/drivers/dma/qcom/gpi.c
> +++ b/drivers/dma/qcom/gpi.c

[ ... ]

> @@ -1022,7 +1024,15 @@ static void gpi_process_xfer_compl_event(struct gchan *gchan,
>  
>  	/* only process events on active channel */
>  	if (unlikely(gchan->pm_state != ACTIVE_STATE)) {
> -		dev_err(gpii->gpi_dev->dev, "skipping processing event because ch @ %s state\n",
> +		/*
> +		 * A channel cancel may generate a transfer-completion event after
> +		 * the channel has left ACTIVE_STATE. Ignore such events to avoid
> +		 * reporting a spurious error.
> +		 */
> +		if (compl_event->status & GPI_COMPL_STATUS_CANCELLED)
> +			return;
> +

[Severity: Medium]
Will this properly ignore the cancelled event during a pause?

When a client calls gpi_pause(), it stops the channel which generates a
CANCELLED event, but it leaves gchan->pm_state as ACTIVE_STATE:

drivers/dma/qcom/gpi.c:gpi_pause() {
    ...
	for (i = 0; i < MAX_CHANNELS_PER_GPII; i++) {
		ret = gpi_stop_chan(&gpii->gchan[i]);
    ...
	write_lock_irq(&gpii->pm_lock);
	gpii->pm_state = PAUSE_STATE;
    ...
}

Because gchan->pm_state remains ACTIVE_STATE, gpi_process_xfer_compl_event()
will bypass this new check. The function then proceeds to consume the
CANCELLED event and complete the descriptor, permanently dropping the transfer
and breaking the pause/resume state machine.

Should the check for GPI_COMPL_STATUS_CANCELLED be moved outside the pm_state
check, or should gchan->pm_state be updated in gpi_pause() prior to stopping
the channel?

> +		dev_err(gpii->gpi_dev->dev, "skipping processing xfer completion event because ch @ %s state\n",
>  			TO_GPI_PM_STR(gchan->pm_state));
>  		return;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009-add-gpi-dma-support-for-uart-v1-0-c69e728df686@oss.qualcomm.com?part=3