Patchwork dma: cppi41: delete channel from pending list when stop channel

login
register
mail settings
Submitter Bin Liu
Date Nov. 12, 2018, 3:43 p.m.
Message ID <20181112154322.24337-1-b-liu@ti.com>
Download mbox | patch
Permalink /patch/654837/
State New
Headers show

Comments

Bin Liu - Nov. 12, 2018, 3:43 p.m.
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(-)
Vinod Koul - Nov. 24, 2018, 2:28 p.m.
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
Bin Liu - Nov. 26, 2018, 1:47 p.m.
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.

Patch

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)