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 dmaengine" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
