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

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

Comments

Bin Liu - Nov. 12, 2018, 3:40 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(-)
Bin Liu - Nov. 12, 2018, 3:46 p.m.
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
>
Peter Ujfalusi - Nov. 28, 2018, 11:15 a.m.
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
Peter Ujfalusi - Nov. 28, 2018, 11:16 a.m.
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
Vinod Koul - Dec. 5, 2018, 8:32 a.m.
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
Bin Liu - Dec. 6, 2018, 2:56 p.m.
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.

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)