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()?
Regards,
Tony
--
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
