Re: [PATCH] dma: cppi41: delete channel from pending list when stop channel

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 12/11/2018 17.40, Bin Liu wrote:

Can you fix up the subject line to:
dmaengine: ti: cppi4: delete channel from pending list when stop channel

> The driver defines three states for a cppi channel.
> - idle: .chan_busy == 0 && not in .pending list
> - pending: .chan_busy == 0 && in .pending list
> - busy: .chan_busy == 1 && not in .pending list
> 
> There are cases in which the cppi channel could be in the pending state
> when cppi41_dma_issue_pending() is called after cppi41_runtime_suspend()
> is called.
> 
> cppi41_stop_chan() has a bug for these cases to set channels to idle state.
> It only checks the .chan_busy flag, but not the .pending list, then later
> when cppi41_runtime_resume() is called the channels in .pending list will
> be transitioned to busy state.
> 
> Removing channels from the .pending list solves the problem.

So, let me see if I understand this correctly:
- client issued a transfer _after_ the cppi4 driver is suspended
- cppi41_dma_issue_pending() will place it to pending list and will not
start the transfer right away as cdd->is_suspended is true.
- on resume the cppi4 will pick up the pending transfers from the
pending list

This is so far a sane thing to do.

If I guess right, then after the issue_pending the client driver will
call terminate_all, presumably from it's suspend callback?

As per the purpose of terminate_all we should terminated all future
transfers on the channel, so clearing the pending list is the correct
thing to do.

With the fixed subject:
Reviewed-by: Peter Ujfalusi <peter.ujfalusi@xxxxxx>

I have one question:

> Fixes: 975faaeb9985 ("dma: cppi41: start tear down only if channel is busy")
> Cc: stable@xxxxxxxxxxxxxxx # v3.15+
> Signed-off-by: Bin Liu <b-liu@xxxxxx>
> ---
>  drivers/dma/ti/cppi41.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/dma/ti/cppi41.c b/drivers/dma/ti/cppi41.c
> index 1497da367710..e507ec36c0d3 100644
> --- a/drivers/dma/ti/cppi41.c
> +++ b/drivers/dma/ti/cppi41.c
> @@ -723,8 +723,22 @@ static int cppi41_stop_chan(struct dma_chan *chan)
>  
>  	desc_phys = lower_32_bits(c->desc_phys);
>  	desc_num = (desc_phys - cdd->descs_phys) / sizeof(struct cppi41_desc);
> -	if (!cdd->chan_busy[desc_num])
> +	if (!cdd->chan_busy[desc_num]) {
> +		struct cppi41_channel *cc, *_ct;
> +
> +		/*
> +		 * channels might still be in the pendling list if
> +		 * cppi41_dma_issue_pending() is called after
> +		 * cppi41_runtime_suspend() is called
> +		 */
> +		list_for_each_entry_safe(cc, _ct, &cdd->pending, node) {
> +			if (cc != c)
> +				continue;
> +			list_del(&cc->node);

If we delete from the pending list, are we going to leak memory?
I'm not familiar with the cppi4, it might not be an issue for it.

> +			break;
> +		}
>  		return 0;
> +	}
>  
>  	ret = cppi41_tear_down_chan(c);
>  	if (ret)
> 

- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki



[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux