Re: [PATCH] dmaengine: cppi41: Fix oops in cppi41_runtime_resume

[Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>]

* Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> [170112 16:04]:
> * Grygorii Strashko <grygorii.strashko-l0cyMroinI0@public.gmane.org> [170112 15:43]:
> > @@ -457,38 +449,36 @@ static void push_desc_queue(struct cppi41_channel *c)
> >         cppi_writel(reg, cdd->qmgr_mem + QMGR_QUEUE_D(c->q_num));
> >  }
> >  
> > -static void pending_desc(struct cppi41_channel *c)
> > +static void cppi41_dma_issue_pending(struct dma_chan *chan)
> >  {
> > +       struct cppi41_channel *c = to_cpp41_chan(chan);
> >         struct cppi41_dd *cdd = c->cdd;
> > +       int error;
> > +       struct cppi41_channel *_c;
> >         unsigned long flags;
> >  
> >         spin_lock_irqsave(&cdd->lock, flags);
> >         list_add_tail(&c->node, &cdd->pending);
> > -       spin_unlock_irqrestore(&cdd->lock, flags);
> > -}
> > -
> > -static void cppi41_dma_issue_pending(struct dma_chan *chan)
> > -{
> > -       struct cppi41_channel *c = to_cpp41_chan(chan);
> > -       struct cppi41_dd *cdd = c->cdd;
> > -       int error;
> >  
> >         error = pm_runtime_get(cdd->ddev.dev);
> > -       if ((error != -EINPROGRESS) && error < 0) {
> > +       if (error < 0) {
> >                 pm_runtime_put_noidle(cdd->ddev.dev);
> >                 dev_err(cdd->ddev.dev, "Failed to pm_runtime_get: %i\n",
> >                         error);
> > -
> > +               spin_unlock_irqrestore(&cdd->lock, flags);
> >                 return;
> >         }
> >  
> > -       if (likely(pm_runtime_active(cdd->ddev.dev)))
> > -               push_desc_queue(c);
> > -       else
> > -               pending_desc(c);
> > +       if (!cdd->is_suspended) {
> > +               list_for_each_entry_safe(c, _c, &cdd->pending, node) {
> > +                       push_desc_queue(c);
> > +                       list_del(&c->node);
> > +               };
> > +       }
> >  
> >         pm_runtime_mark_last_busy(cdd->ddev.dev);
> >         pm_runtime_put_autosuspend(cdd->ddev.dev);
> > +       spin_lock_irqsave(&cdd->lock, flags);
> >  }
> 
> So always add to the queue no matter, then always flush the queue
> directly if active? Yeah that might work :)
> 
> Don't we need spinlock in the list_for_each_entry_safe() to prevent
> it racing with runtime_resume() though?

I could not apply for me as looks like your mail server replaced tabs
with spaces it seems :(

But anyways here's your basic idea plugged into my recent patch and
it seems to work. I maybe have missed something though while reading
so please check.

The pm_runtime_get/mark_last_busy/put_autosuspend and WARN_ON() we
want to keep in cppi41_irq() at least for now :)

And I'm thinking we must also callcppi41_run_queue() with spinlock
held to prevent out of order triggering of the DMA transfers.

Does this look OK to you?

Regards,

Tony

8< -----------------------
diff --git a/drivers/dma/cppi41.c b/drivers/dma/cppi41.c
index d5ba43a87a68..6ee593eb2acb 100644
--- 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 is_suspended;
 };
 
 #define FIST_COMPLETION_QUEUE	93
@@ -316,11 +318,12 @@ static irqreturn_t cppi41_irq(int irq, void *data)
 			__iormb();
 
 		while (val) {
+			unsigned long flags;
 			u32 desc, len;
 			int error;
 
 			error = pm_runtime_get(cdd->ddev.dev);
-			if (error < 0)
+			if ((error != -EINPROGRESS) && error < 0)
 				dev_err(cdd->ddev.dev, "%s pm runtime get: %i\n",
 					__func__, error);
 
@@ -340,6 +343,11 @@ static irqreturn_t cppi41_irq(int irq, void *data)
 			else
 				len = pd_trans_len(c->desc->pd0);
 
+			/* This warning should never trigger */
+			spin_lock_irqsave(&cdd->lock, flags);
+			WARN_ON(cdd->is_suspended);
+			spin_unlock_irqrestore(&cdd->lock, flags);
+
 			c->residue = pd_trans_len(c->desc->pd6) - len;
 			dma_cookie_complete(&c->txd);
 			dmaengine_desc_get_callback_invoke(&c->txd, NULL);
@@ -457,20 +465,26 @@ static void push_desc_queue(struct cppi41_channel *c)
 	cppi_writel(reg, cdd->qmgr_mem + QMGR_QUEUE_D(c->q_num));
 }
 
-static void pending_desc(struct cppi41_channel *c)
+/*
+ * Caller must hold cdd->lock to prevent push_desc_queue()
+ * getting called out of order. We have both cppi41_dma_issue_pending()
+ * and cppi41_runtime_resume() call this function.
+ */
+static void cppi41_run_queue(struct cppi41_dd *cdd)
 {
-	struct cppi41_dd *cdd = c->cdd;
-	unsigned long flags;
+	struct cppi41_channel *c, *_c;
 
-	spin_lock_irqsave(&cdd->lock, flags);
-	list_add_tail(&c->node, &cdd->pending);
-	spin_unlock_irqrestore(&cdd->lock, flags);
+	list_for_each_entry_safe(c, _c, &cdd->pending, node) {
+		push_desc_queue(c);
+		list_del(&c->node);
+	}
 }
 
 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;
 	int error;
 
 	error = pm_runtime_get(cdd->ddev.dev);
@@ -482,10 +496,11 @@ static void cppi41_dma_issue_pending(struct dma_chan *chan)
 		return;
 	}
 
-	if (likely(pm_runtime_active(cdd->ddev.dev)))
-		push_desc_queue(c);
-	else
-		pending_desc(c);
+	spin_lock_irqsave(&cdd->lock, flags);
+	list_add_tail(&c->node, &cdd->pending);
+	if (!cdd->is_suspended)
+		cppi41_run_queue(cdd);
+	spin_unlock_irqrestore(&cdd->lock, flags);
 
 	pm_runtime_mark_last_busy(cdd->ddev.dev);
 	pm_runtime_put_autosuspend(cdd->ddev.dev);
@@ -1150,6 +1165,11 @@ static int __maybe_unused cppi41_resume(struct device *dev)
 static int __maybe_unused cppi41_runtime_suspend(struct device *dev)
 {
 	struct cppi41_dd *cdd = dev_get_drvdata(dev);
+	unsigned long flags;
+
+	spin_lock_irqsave(&cdd->lock, flags);
+	cdd->is_suspended = true;
+	spin_unlock_irqrestore(&cdd->lock, flags);
 
 	WARN_ON(!list_empty(&cdd->pending));
 
@@ -1159,14 +1179,11 @@ 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;
 	unsigned long flags;
 
 	spin_lock_irqsave(&cdd->lock, flags);
-	list_for_each_entry_safe(c, _c, &cdd->pending, node) {
-		push_desc_queue(c);
-		list_del(&c->node);
-	}
+	cdd->is_suspended = false;
+	cppi41_run_queue(cdd);
 	spin_unlock_irqrestore(&cdd->lock, flags);
 
 	return 0;
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html