dma: cppi41: delete channel from pending list when stop channel

[Peter Ujfalusi <peter.ujfalusi@ti.com>]

Hi,

On 28/11/2018 13.15, Peter Ujfalusi wrote:

forgot to fix up Vinod's email address.

> 
> 
> 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@ti.com>
> 
> I have one question:
> 
>> Fixes: 975faaeb9985 ("dma: cppi41: start tear down only if channel is busy")
>> Cc: stable@vger.kernel.org # v3.15+
>> Signed-off-by: Bin Liu <b-liu@ti.com>
>> ---
>>  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
> 

- Péter

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