Re: [PATCH v2 1/2] dmaengine: cppi41: Fix list not empty warning on runtime suspend

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 01/12/2017 06:23 PM, Grygorii Strashko wrote:
> 
> 
> On 01/12/2017 11:09 AM, Tony Lindgren wrote:
>> * Tony Lindgren <tony@xxxxxxxxxxx> [170109 10:35]:
>>> Hi,
>>>
>>> * Alexandre Bailon <abailon@xxxxxxxxxxxx> [170109 09:04]:
>>>> Sometime, a transfer may not be queued due to a race between runtime pm
>>>> and cppi41_dma_issue_pending().
>>>> Sometime, cppi41_runtime_resume() may be interrupted right before to
>>>> update device PM state to RESUMED.
>>>> When it happens, if a new dma transfer is issued, because the device is not
>>>> in active state, the descriptor will be added to the pendding list.
>>>> But because the descriptors in the pendding list are only queued to cppi41
>>>> on runtime resume, the descriptor will not be queued.
>>>> On runtime suspend, the list is not empty, which is causing a warning.
>>>> Queue the descriptor if the device is active or resuming.
>>>>
>>>> Signed-off-by: Alexandre Bailon <abailon@xxxxxxxxxxxx>
>>>> ---
>>>>  drivers/dma/cppi41.c | 18 +++++++++++++++++-
>>>>  1 file changed, 17 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/dma/cppi41.c b/drivers/dma/cppi41.c
>>>> index d5ba43a..025fee4 100644
>>>> --- a/drivers/dma/cppi41.c
>>>> +++ b/drivers/dma/cppi41.c
>>>> @@ -471,6 +471,8 @@ static void cppi41_dma_issue_pending(struct dma_chan *chan)
>>>>  {
>>>>  	struct cppi41_channel *c = to_cpp41_chan(chan);
>>>>  	struct cppi41_dd *cdd = c->cdd;
>>>> +	unsigned long flags;
>>>> +	bool active;
>>>>  	int error;
>>>>  
>>>>  	error = pm_runtime_get(cdd->ddev.dev);
>>>> @@ -482,7 +484,21 @@ static void cppi41_dma_issue_pending(struct dma_chan *chan)
>>>>  		return;
>>>>  	}
>>>>  
>>>> -	if (likely(pm_runtime_active(cdd->ddev.dev)))
>>>> +	active = pm_runtime_active(cdd->ddev.dev);
>>>> +	if (!active) {
>>>> +		/*
>>>> +		 * Runtime resume may be interrupted before runtime_status
>>>> +		 * has been updated. Test if device has resumed.
>>>> +		 */
>>>> +		if (error == -EINPROGRESS) {
>>>> +			spin_lock_irqsave(&cdd->lock, flags);
>>>> +			if (list_empty(&cdd->pending))
>>>> +				active = true;
>>>> +			spin_unlock_irqrestore(&cdd->lock, flags);
>>>> +		}
>>>> +	}
>>>> +
>>>> +	if (likely(active))
>>>>  		push_desc_queue(c);
>>>>  	else
>>>>  		pending_desc(c);
>>>
>>> What guarantees that the PM runtime state is really active few lines later?
>>>
>>> A safer approach might be to check the queue for new entries by in
>>> cppi41_runtime_resume() using "while (!list_empty())" instead of
>>> list_for_each_entry(). That releases the spinlock between each entry
>>> and rechecks the list.
>>>
>>> And instead of doing WARN_ON(!list_empty(&cdd->pending)) it seems we
>>> should run the queue also on cppi41_runtime_suspend()?
>>
>> Hmm so I started seeing these issues too with Linux next just plugging
>> in USB cable on bbb while configured as a USB peripheral :)
>>
>> Below is what seems to fix issues for me, not seeing any more warnings
>> either.
>>
>> Care to give it a try with your USB headset?
This solves the issue but I still have a bad playback quality.
I don't remember if I have spoken about it so I will describe it.
When I play audio (with your patch or mine), the music cut a lot.
The issue go away when the MUSB driver is built in PIO mode only.
Some experimentation I made today let me think it is related to
PM runtime.
I guess I should probably start another thread about that.
> 
> This looks more close to what is provided in Documentation\power\runtime_pm.txt
> (example at the end of the file),
> but as per this document custom locking is required to access .active var.
> 
> 
>>
>> 8< --------------------------
>> diff --git a/drivers/dma/cppi41.c b/drivers/dma/cppi41.c
>> --- a/drivers/dma/cppi41.c
>> +++ b/drivers/dma/cppi41.c
>> @@ -153,6 +153,8 @@ struct cppi41_dd {
>>  
>>  	/* context for suspend/resume */
>>  	unsigned int dma_tdfdq;
>> +
>> +	bool active;
>>  };
>>  
>>  #define FIST_COMPLETION_QUEUE	93
>> @@ -320,7 +322,8 @@ static irqreturn_t cppi41_irq(int irq, void *data)
>>  			int error;
>>  
>>  			error = pm_runtime_get(cdd->ddev.dev);
>> -			if (error < 0)
>> +			if (((error != -EINPROGRESS) && error < 0) ||
>> +			    !cdd->active)
>>  				dev_err(cdd->ddev.dev, "%s pm runtime get: %i\n",
>>  					__func__, error);
>>  
>> @@ -482,7 +485,7 @@ static void cppi41_dma_issue_pending(struct dma_chan *chan)
>>  		return;
>>  	}
>>  
>> -	if (likely(pm_runtime_active(cdd->ddev.dev)))
>> +	if (likely(cdd->active))
>>  		push_desc_queue(c);
>>  	else
>>  		pending_desc(c);
>> @@ -1151,6 +1154,7 @@ static int __maybe_unused cppi41_runtime_suspend(struct device *dev)
>>  {
>>  	struct cppi41_dd *cdd = dev_get_drvdata(dev);
>>  
>> +	cdd->active = false;
>>  	WARN_ON(!list_empty(&cdd->pending));
>>  
>>  	return 0;
>> @@ -1159,13 +1163,20 @@ static int __maybe_unused cppi41_runtime_suspend(struct device *dev)
>>  static int __maybe_unused cppi41_runtime_resume(struct device *dev)
>>  {
>>  	struct cppi41_dd *cdd = dev_get_drvdata(dev);
>> -	struct cppi41_channel *c, *_c;
>> +	struct cppi41_channel *c;
>>  	unsigned long flags;
>>  
>> +	cdd->active = true;
>> +
>>  	spin_lock_irqsave(&cdd->lock, flags);
>> -	list_for_each_entry_safe(c, _c, &cdd->pending, node) {
>> -		push_desc_queue(c);
>> +	while (!list_empty(&cdd->pending)) {
>> +		c = list_first_entry(&cdd->pending,
>> +				     struct cppi41_channel,
>> +				     node);
>>  		list_del(&c->node);
>> +		spin_unlock_irqrestore(&cdd->lock, flags);
>> +		push_desc_queue(c);
>> +		spin_lock_irqsave(&cdd->lock, flags);
>>  	}
>>  	spin_unlock_irqrestore(&cdd->lock, flags);
>>  
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Arm (vger)]     [ARM Kernel]     [ARM MSM]     [Linux Tegra]     [Linux WPAN Networking]     [Linux Wireless Networking]     [Maemo Users]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux