| Message ID | 20170112213016.19367-1-tony@atomide.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On 01/12/2017 03:30 PM, Tony Lindgren wrote: > Commit fdea2d09b997 ("dmaengine: cppi41: Add basic PM runtime support") > together with recent MUSB changes allowed USB and DMA on BeagleBone to idle > when no cable is connected. But looks like few corner case issues still > remain. > > Looks like just by repluging USB cable about ten or so times on BeagleBone > when configured in USB peripheral mode we can get warnings and eventually > trigger an oops in cppi41 DMA: > > WARNING: CPU: 0 PID: 14 at drivers/dma/cppi41.c:1154 cppi41_runtime_suspend+ > x28/0x38 [cppi41] > ... > > WARNING: CPU: 0 PID: 14 at drivers/dma/cppi41.c:452 > push_desc_queue+0x94/0x9c [cppi41] > ... > > Unable to handle kernel NULL pointer dereference at virtual > address 00000104 > pgd = c0004000 > [00000104] *pgd=00000000 > Internal error: Oops: 805 [#1] SMP ARM > ... > [<bf0d92cc>] (cppi41_runtime_resume [cppi41]) from [<c0589838>] > (__rpm_callback+0xc0/0x214) > [<c0589838>] (__rpm_callback) from [<c05899ac>] (rpm_callback+0x20/0x80) > [<c05899ac>] (rpm_callback) from [<c0589460>] (rpm_resume+0x504/0x78c) > [<c0589460>] (rpm_resume) from [<c058a1a0>] (pm_runtime_work+0x60/0xa8) > [<c058a1a0>] (pm_runtime_work) from [<c0156120>] (process_one_work+0x2b4/0x808) > > This is because of a race with PM runtime and cppi41_dma_issue_pending() > as reported by Alexandre Bailon <abailon@baylibre.com> in earlier > set of patches. Based on mailing list discussions we however came to the > conclusion that a different fix from Alexandre's fix is needed in order > to guarantee that DMA is really active when we try to use it. > > To fix the issue, we need to add a driver specific flag as we otherwise > can have -EINPROGRESS state set by PM runtime and can't rely on > pm_runtime_active() to tell us when we can use the DMA. > > For reference, this is also documented in Documentation/power/runtime_pm.txt > in the example at the end of the file as pointed out by Grygorii Strashko > <grygorii.strashko@ti.com>. > > So let's fix the issue by introducing atomic_t active that gets toggled > in runtime_suspend() and runtime_resume(). And then let's replace the > pm_runtime_active() checks with atomic_read(). > > Note that we may also get transactions queued while in -EINPROGRESS > state. So we need to check for new elements in cppi41_runtime_resume() > by replacing list_for_each_entry_safe() with the commonly used test for > while (!list_empty(&cdd->pending)). > > Fixes: fdea2d09b997 ("dmaengine: cppi41: Add basic PM runtime support") > Cc: Andy Shevchenko <andy.shevchenko@gmail.com> > Cc: Bin Liu <b-liu@ti.com> > Cc: Grygorii Strashko <grygorii.strashko@ti.com> > Cc: Kevin Hilman <khilman@baylibre.com> > Cc: Patrick Titiano <ptitiano@baylibre.com> > Cc: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> > Reported-by: Alexandre Bailon <abailon@baylibre.com> > Signed-off-by: Tony Lindgren <tony@atomide.com> > --- > drivers/dma/cppi41.c | 23 ++++++++++++++++++----- > 1 file changed, 18 insertions(+), 5 deletions(-) > > diff --git a/drivers/dma/cppi41.c b/drivers/dma/cppi41.c > --- a/drivers/dma/cppi41.c > +++ b/drivers/dma/cppi41.c > @@ -1,3 +1,4 @@ > +#include <linux/atomic.h> > #include <linux/delay.h> > #include <linux/dmaengine.h> > #include <linux/dma-mapping.h> > @@ -153,6 +154,8 @@ struct cppi41_dd { > > /* context for suspend/resume */ > unsigned int dma_tdfdq; > + > + atomic_t active; > }; > > #define FIST_COMPLETION_QUEUE 93 > @@ -320,7 +323,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) || > + !atomic_read(&cdd->active)) > dev_err(cdd->ddev.dev, "%s pm runtime get: %i\n", > __func__, error); > > @@ -482,7 +486,7 @@ static void cppi41_dma_issue_pending(struct dma_chan *chan) > return; > } > Sry, but even if it looks better it might still race with PM runtime :( > - if (likely(pm_runtime_active(cdd->ddev.dev))) > + if (likely(atomic_read(&cdd->active))) > push_desc_queue(c); > else - CPU is here (-EINPROGRESS and active == 0) and then preempted - PM runtime will finish cppi41_runtime_resume and clean-up pending descs - CPU return here and adds desc to the pending queue - oops Am I wrong? > pending_desc(c); > @@ -1003,6 +1007,7 @@ static int cppi41_dma_probe(struct platform_device *pdev) > cdd->ddev.dst_addr_widths = CPPI41_DMA_BUSWIDTHS; > cdd->ddev.residue_granularity = DMA_RESIDUE_GRANULARITY_BURST; > cdd->ddev.dev = dev; > + atomic_set(&cdd->active, 0); > INIT_LIST_HEAD(&cdd->ddev.channels); > cpp41_dma_info.dma_cap = cdd->ddev.cap_mask; > > @@ -1151,6 +1156,7 @@ static int __maybe_unused cppi41_runtime_suspend(struct device *dev) > { > struct cppi41_dd *cdd = dev_get_drvdata(dev); > > + atomic_set(&cdd->active, 0); > WARN_ON(!list_empty(&cdd->pending)); > > return 0; > @@ -1159,13 +1165,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; > > + atomic_set(&cdd->active, 1); > + > 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); > >
* Grygorii Strashko <grygorii.strashko@ti.com> [170112 13:54]: > On 01/12/2017 03:30 PM, Tony Lindgren wrote: > > Sry, but even if it looks better it might still race with PM runtime :( > > > - if (likely(pm_runtime_active(cdd->ddev.dev))) > > + if (likely(atomic_read(&cdd->active))) > > push_desc_queue(c); > > else > > > - CPU is here (-EINPROGRESS and active == 0) and then preempted > - PM runtime will finish cppi41_runtime_resume and clean-up pending descs > - CPU return here and adds desc to the pending queue > - oops > > Am I wrong? We had cppi41_dma_issue_pending() getting called from atomic contex and cppi41_runtime_resume() getting preempted where cppi41_dma_issue_pending() would add to the queue. If what you're saying can happen in some cases, then we again see the WARN_ON(!list_empty(&cdd->pending)) in cppi41_runtime_suspend(). At least so far I have no longer been able to make happen today. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/12/2017 04:19 PM, Tony Lindgren wrote: > * Grygorii Strashko <grygorii.strashko@ti.com> [170112 13:54]: >> On 01/12/2017 03:30 PM, Tony Lindgren wrote: >> >> Sry, but even if it looks better it might still race with PM runtime :( >> >>> - if (likely(pm_runtime_active(cdd->ddev.dev))) >>> + if (likely(atomic_read(&cdd->active))) >>> push_desc_queue(c); >>> else >> >> >> - CPU is here (-EINPROGRESS and active == 0) and then preempted >> - PM runtime will finish cppi41_runtime_resume and clean-up pending descs >> - CPU return here and adds desc to the pending queue >> - oops >> >> Am I wrong? > > We had cppi41_dma_issue_pending() getting called from atomic contex and > cppi41_runtime_resume() getting preempted where cppi41_dma_issue_pending() > would add to the queue. Again, I can be mistaken but cppi41_configure_channel() seems not atomic. cppi41_configure_channel()->dma_async_issue_pending() + documentation says "This function can be called in an interrupt context" And definitely it will be preemptive on RT :( > > If what you're saying can happen in some cases, then we again see the > WARN_ON(!list_empty(&cdd->pending)) in cppi41_runtime_suspend(). At least > so far I have no longer been able to make happen today.
* Grygorii Strashko <grygorii.strashko@ti.com> [170112 14:53]: > > > On 01/12/2017 04:19 PM, Tony Lindgren wrote: > > * Grygorii Strashko <grygorii.strashko@ti.com> [170112 13:54]: > >> On 01/12/2017 03:30 PM, Tony Lindgren wrote: > >> > >> Sry, but even if it looks better it might still race with PM runtime :( > >> > >>> - if (likely(pm_runtime_active(cdd->ddev.dev))) > >>> + if (likely(atomic_read(&cdd->active))) > >>> push_desc_queue(c); > >>> else > >> > >> > >> - CPU is here (-EINPROGRESS and active == 0) and then preempted > >> - PM runtime will finish cppi41_runtime_resume and clean-up pending descs > >> - CPU return here and adds desc to the pending queue > >> - oops > >> > >> Am I wrong? > > > > We had cppi41_dma_issue_pending() getting called from atomic contex and > > cppi41_runtime_resume() getting preempted where cppi41_dma_issue_pending() > > would add to the queue. > > Again, I can be mistaken but cppi41_configure_channel() seems not atomic. > cppi41_configure_channel()->dma_async_issue_pending() > + documentation says "This function can be called in an interrupt context" > > And definitely it will be preemptive on RT :( Hmm OK. So are you thinking we should add a spinlock around the test in cppi41_dma_issue_pending() and when modifying cdd->active? Something like: spin_lock_irqsave(&cdd->lock, flags); if (likely(cdd->active)) push_desc_queue(c); else list_add_tail(&c->node, &cdd->pending); spin_unlock_irqrestore(&cdd->lock, flags); Or do you have something better in mind? Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello! On 1/13/2017 12:30 AM, Tony Lindgren wrote: > Commit fdea2d09b997 ("dmaengine: cppi41: Add basic PM runtime support") > together with recent MUSB changes allowed USB and DMA on BeagleBone to idle > when no cable is connected. But looks like few corner case issues still > remain. > > Looks like just by repluging USB cable about ten or so times on BeagleBone Re-plugging. > when configured in USB peripheral mode we can get warnings and eventually > trigger an oops in cppi41 DMA: > > WARNING: CPU: 0 PID: 14 at drivers/dma/cppi41.c:1154 cppi41_runtime_suspend+ > x28/0x38 [cppi41] > ... > > WARNING: CPU: 0 PID: 14 at drivers/dma/cppi41.c:452 > push_desc_queue+0x94/0x9c [cppi41] > ... > > Unable to handle kernel NULL pointer dereference at virtual > address 00000104 > pgd = c0004000 > [00000104] *pgd=00000000 > Internal error: Oops: 805 [#1] SMP ARM > ... > [<bf0d92cc>] (cppi41_runtime_resume [cppi41]) from [<c0589838>] > (__rpm_callback+0xc0/0x214) > [<c0589838>] (__rpm_callback) from [<c05899ac>] (rpm_callback+0x20/0x80) > [<c05899ac>] (rpm_callback) from [<c0589460>] (rpm_resume+0x504/0x78c) > [<c0589460>] (rpm_resume) from [<c058a1a0>] (pm_runtime_work+0x60/0xa8) > [<c058a1a0>] (pm_runtime_work) from [<c0156120>] (process_one_work+0x2b4/0x808) > > This is because of a race with PM runtime and cppi41_dma_issue_pending() > as reported by Alexandre Bailon <abailon@baylibre.com> in earlier > set of patches. Based on mailing list discussions we however came to the > conclusion that a different fix from Alexandre's fix is needed in order > to guarantee that DMA is really active when we try to use it. > > To fix the issue, we need to add a driver specific flag as we otherwise > can have -EINPROGRESS state set by PM runtime and can't rely on Runtime PM. > pm_runtime_active() to tell us when we can use the DMA. > > For reference, this is also documented in Documentation/power/runtime_pm.txt > in the example at the end of the file as pointed out by Grygorii Strashko > <grygorii.strashko@ti.com>. > > So let's fix the issue by introducing atomic_t active that gets toggled > in runtime_suspend() and runtime_resume(). And then let's replace the > pm_runtime_active() checks with atomic_read(). > > Note that we may also get transactions queued while in -EINPROGRESS > state. So we need to check for new elements in cppi41_runtime_resume() > by replacing list_for_each_entry_safe() with the commonly used test for > while (!list_empty(&cdd->pending)). > > Fixes: fdea2d09b997 ("dmaengine: cppi41: Add basic PM runtime support") > Cc: Andy Shevchenko <andy.shevchenko@gmail.com> > Cc: Bin Liu <b-liu@ti.com> > Cc: Grygorii Strashko <grygorii.strashko@ti.com> > Cc: Kevin Hilman <khilman@baylibre.com> > Cc: Patrick Titiano <ptitiano@baylibre.com> > Cc: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> > Reported-by: Alexandre Bailon <abailon@baylibre.com> > Signed-off-by: Tony Lindgren <tony@atomide.com> > --- > drivers/dma/cppi41.c | 23 ++++++++++++++++++----- > 1 file changed, 18 insertions(+), 5 deletions(-) > > diff --git a/drivers/dma/cppi41.c b/drivers/dma/cppi41.c > --- a/drivers/dma/cppi41.c > +++ b/drivers/dma/cppi41.c [...] > @@ -320,7 +323,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) || Hum, the innermost parens are inconsistent: around != but not around <. [...] MBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> [170113 01:25]: > > @@ -320,7 +323,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) || > > Hum, the innermost parens are inconsistent: around != but not around <. Oops just noticed I missed this one in the latest version, will update for the next revision. The typos I already fixed. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/dma/cppi41.c b/drivers/dma/cppi41.c --- a/drivers/dma/cppi41.c +++ b/drivers/dma/cppi41.c @@ -1,3 +1,4 @@ +#include <linux/atomic.h> #include <linux/delay.h> #include <linux/dmaengine.h> #include <linux/dma-mapping.h> @@ -153,6 +154,8 @@ struct cppi41_dd { /* context for suspend/resume */ unsigned int dma_tdfdq; + + atomic_t active; }; #define FIST_COMPLETION_QUEUE 93 @@ -320,7 +323,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) || + !atomic_read(&cdd->active)) dev_err(cdd->ddev.dev, "%s pm runtime get: %i\n", __func__, error); @@ -482,7 +486,7 @@ static void cppi41_dma_issue_pending(struct dma_chan *chan) return; } - if (likely(pm_runtime_active(cdd->ddev.dev))) + if (likely(atomic_read(&cdd->active))) push_desc_queue(c); else pending_desc(c); @@ -1003,6 +1007,7 @@ static int cppi41_dma_probe(struct platform_device *pdev) cdd->ddev.dst_addr_widths = CPPI41_DMA_BUSWIDTHS; cdd->ddev.residue_granularity = DMA_RESIDUE_GRANULARITY_BURST; cdd->ddev.dev = dev; + atomic_set(&cdd->active, 0); INIT_LIST_HEAD(&cdd->ddev.channels); cpp41_dma_info.dma_cap = cdd->ddev.cap_mask; @@ -1151,6 +1156,7 @@ static int __maybe_unused cppi41_runtime_suspend(struct device *dev) { struct cppi41_dd *cdd = dev_get_drvdata(dev); + atomic_set(&cdd->active, 0); WARN_ON(!list_empty(&cdd->pending)); return 0; @@ -1159,13 +1165,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; + atomic_set(&cdd->active, 1); + 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);
Commit fdea2d09b997 ("dmaengine: cppi41: Add basic PM runtime support") together with recent MUSB changes allowed USB and DMA on BeagleBone to idle when no cable is connected. But looks like few corner case issues still remain. Looks like just by repluging USB cable about ten or so times on BeagleBone when configured in USB peripheral mode we can get warnings and eventually trigger an oops in cppi41 DMA: WARNING: CPU: 0 PID: 14 at drivers/dma/cppi41.c:1154 cppi41_runtime_suspend+ x28/0x38 [cppi41] ... WARNING: CPU: 0 PID: 14 at drivers/dma/cppi41.c:452 push_desc_queue+0x94/0x9c [cppi41] ... Unable to handle kernel NULL pointer dereference at virtual address 00000104 pgd = c0004000 [00000104] *pgd=00000000 Internal error: Oops: 805 [#1] SMP ARM ... [<bf0d92cc>] (cppi41_runtime_resume [cppi41]) from [<c0589838>] (__rpm_callback+0xc0/0x214) [<c0589838>] (__rpm_callback) from [<c05899ac>] (rpm_callback+0x20/0x80) [<c05899ac>] (rpm_callback) from [<c0589460>] (rpm_resume+0x504/0x78c) [<c0589460>] (rpm_resume) from [<c058a1a0>] (pm_runtime_work+0x60/0xa8) [<c058a1a0>] (pm_runtime_work) from [<c0156120>] (process_one_work+0x2b4/0x808) This is because of a race with PM runtime and cppi41_dma_issue_pending() as reported by Alexandre Bailon <abailon@baylibre.com> in earlier set of patches. Based on mailing list discussions we however came to the conclusion that a different fix from Alexandre's fix is needed in order to guarantee that DMA is really active when we try to use it. To fix the issue, we need to add a driver specific flag as we otherwise can have -EINPROGRESS state set by PM runtime and can't rely on pm_runtime_active() to tell us when we can use the DMA. For reference, this is also documented in Documentation/power/runtime_pm.txt in the example at the end of the file as pointed out by Grygorii Strashko <grygorii.strashko@ti.com>. So let's fix the issue by introducing atomic_t active that gets toggled in runtime_suspend() and runtime_resume(). And then let's replace the pm_runtime_active() checks with atomic_read(). Note that we may also get transactions queued while in -EINPROGRESS state. So we need to check for new elements in cppi41_runtime_resume() by replacing list_for_each_entry_safe() with the commonly used test for while (!list_empty(&cdd->pending)). Fixes: fdea2d09b997 ("dmaengine: cppi41: Add basic PM runtime support") Cc: Andy Shevchenko <andy.shevchenko@gmail.com> Cc: Bin Liu <b-liu@ti.com> Cc: Grygorii Strashko <grygorii.strashko@ti.com> Cc: Kevin Hilman <khilman@baylibre.com> Cc: Patrick Titiano <ptitiano@baylibre.com> Cc: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> Reported-by: Alexandre Bailon <abailon@baylibre.com> Signed-off-by: Tony Lindgren <tony@atomide.com> --- drivers/dma/cppi41.c | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-)