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;}- }
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