| Message ID | 20181112154322.24337-1-b-liu@ti.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | dma: cppi41: delete channel from pending list when stop channel | expand |
On 12-11-18, 09:43, 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. I would like some testing, given that intent is to go to stable. Peter..? > > 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
Hi, On Sat, Nov 24, 2018 at 07:58:03PM +0530, Vinod Koul wrote: > On 12-11-18, 09:43, 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. > > I would like some testing, given that intent is to go to stable. > Peter..? FYI, this cppi41 dma driver is *only* used by musb controller driver. In the past I received multiple reports for this issue, but I wasn't able to reproduce it using similar test cases. The only way I could trigger the issue is to do system suspend/resume test on AM335x with a USB hub attached to the usb host port. This issue only happens once in the very *first* time suspend/resume test after reboot. 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(-)