| Message ID | 20181112154049.24129-1-b-liu@ti.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | dma: cppi41: delete channel from pending list when stop channel | expand |
Sorry, please ignore this. Used incorrect Vinod email address. On Mon, Nov 12, 2018 at 09:40:49AM -0600, Bin Liu wrote: > 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. > > 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); > + break; > + } > return 0; > + } > > ret = cppi41_tear_down_chan(c); > if (ret) > -- > 2.17.1 >
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
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
On 28-11-18, 13:15, Peter Ujfalusi wrote: > > > 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> Thanks Peter, Applied after fixing the title, thanks
Peter, On Wed, Nov 28, 2018 at 01:16:32PM +0200, Peter Ujfalusi wrote: > 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. Here is no memory leak. The elements added to the pending list are cppi41 channels which are allocated in driver _probe(). No dynamic memory allocation happening when operating this pending list. Regards, -Bin.
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); + break; + } return 0; + } ret = cppi41_tear_down_chan(c); if (ret)
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. 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(-)