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