[PATCH] dma: cppi41: delete channel from pending list when stop channel

[PATCH] dma: cppi41: delete channel from pending list when stop channel

[Bin Liu]

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

Re: [PATCH] dma: cppi41: delete channel from pending list when stop channel

[Bin Liu]

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
> 

Re: [PATCH] dma: cppi41: delete channel from pending list when stop channel

[Peter Ujfalusi]

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

Re: [PATCH] dma: cppi41: delete channel from pending list when stop channel

[Peter Ujfalusi]

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

Re: [PATCH] dma: cppi41: delete channel from pending list when stop channel

[Bin Liu]

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.

Re: [PATCH] dma: cppi41: delete channel from pending list when stop channel

[Vinod Koul]

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

-- 
~Vinod

[PATCH] dma: cppi41: delete channel from pending list when stop channel

[Bin Liu]

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

Re: [PATCH] dma: cppi41: delete channel from pending list when stop channel

[Vinod Koul]

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

-- 
~Vinod

Re: [PATCH] dma: cppi41: delete channel from pending list when stop channel

[Bin Liu]

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.