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@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
>
- Péter
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
