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 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
>
--
regards,
-grygorii
--
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
