From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from lelv0143.ext.ti.com ([198.47.23.248]:59278 "EHLO lelv0143.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728034AbeK1WO7 (ORCPT ); Wed, 28 Nov 2018 17:14:59 -0500 Subject: Re: [PATCH] dma: cppi41: delete channel from pending list when stop channel To: Bin Liu , CC: , Vinod Koul , References: <20181112154049.24129-1-b-liu@ti.com> From: Peter Ujfalusi Message-ID: Date: Wed, 28 Nov 2018 13:15:11 +0200 MIME-Version: 1.0 In-Reply-To: <20181112154049.24129-1-b-liu@ti.com> Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 8bit Sender: stable-owner@vger.kernel.org List-ID: 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 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 > --- > 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