| Message ID | 20170120200753.GM7403@atomide.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On Fri, Jan 20, 2017 at 12:07:53PM -0800, Tony Lindgren wrote: > With patches "dmaengine: cppi41: Fix runtime PM timeouts with USB mass > storage", and "dmaengine: cppi41: Fix oops in cppi41_runtime_resume", > the pm_runtime_get/put() in cppi41_irq() is no longer needed. We now > guarantee that cppi41 is enabled when dma is in use. > > We can still get pointless error -115 when musb is configured as a > usb peripheral. That's because we should now check for the state of > is_suspended instead. I am not sure I understand this paragraph. Do you mean we still get harmless -115 in peripheral mode? If so how is it caused by is_suspended check? And the comment below for the check implies the WARN_ON() never happens... Regards, -Bin. > > Let's just remove the now useless code and replace it with a WARN(). > > Signed-off-by: Tony Lindgren <tony@atomide.com> > --- > > Found one more cosmetic issue. With patches 1/2 and 2/2 fixing the > problems, this can now wait if considered not suitable for the -rc > cycle. > > --- > drivers/dma/cppi41.c | 13 +++++-------- > 1 file changed, 5 insertions(+), 8 deletions(-) > > diff --git a/drivers/dma/cppi41.c b/drivers/dma/cppi41.c > --- a/drivers/dma/cppi41.c > +++ b/drivers/dma/cppi41.c > @@ -323,12 +323,12 @@ static irqreturn_t cppi41_irq(int irq, void *data) > > while (val) { > u32 desc, len; > - int error; > > - error = pm_runtime_get(cdd->ddev.dev); > - if (error < 0) > - dev_err(cdd->ddev.dev, "%s pm runtime get: %i\n", > - __func__, error); > + /* > + * This should never trigger, see the comments in > + * push_desc_queue() > + */ > + WARN_ON(cdd->is_suspended); > > q_num = __fls(val); > val &= ~(1 << q_num); > @@ -349,9 +349,6 @@ static irqreturn_t cppi41_irq(int irq, void *data) > c->residue = pd_trans_len(c->desc->pd6) - len; > dma_cookie_complete(&c->txd); > dmaengine_desc_get_callback_invoke(&c->txd, NULL); > - > - pm_runtime_mark_last_busy(cdd->ddev.dev); > - pm_runtime_put_autosuspend(cdd->ddev.dev); > } > } > return IRQ_HANDLED; > -- > 2.11.0 -- 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
* Bin Liu <b-liu@ti.com> [170120 13:08]: > On Fri, Jan 20, 2017 at 12:07:53PM -0800, Tony Lindgren wrote: > > With patches "dmaengine: cppi41: Fix runtime PM timeouts with USB mass > > storage", and "dmaengine: cppi41: Fix oops in cppi41_runtime_resume", > > the pm_runtime_get/put() in cppi41_irq() is no longer needed. We now > > guarantee that cppi41 is enabled when dma is in use. > > > > We can still get pointless error -115 when musb is configured as a > > usb peripheral. That's because we should now check for the state of > > is_suspended instead. > > I am not sure I understand this paragraph. Do you mean we still get > harmless -115 in peripheral mode? If so how is it caused by is_suspended > check? And the comment below for the check implies the WARN_ON() never > happens... Yes I noticed we can still get it in peripheral mode. And it's a bogus warning now because we should now be using the new cdd->is_suspended instead. It happens because cppi41_runtime_resume() has not yet completed and is calling cppi41_run_queue() that produces the interrupt. So that that point we have cppi41 active with !cdd->is_suspended, but pm_runtime_get() still returns -EINPROGRESS (115). 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 @@ -323,12 +323,12 @@ static irqreturn_t cppi41_irq(int irq, void *data) while (val) { u32 desc, len; - int error; - error = pm_runtime_get(cdd->ddev.dev); - if (error < 0) - dev_err(cdd->ddev.dev, "%s pm runtime get: %i\n", - __func__, error); + /* + * This should never trigger, see the comments in + * push_desc_queue() + */ + WARN_ON(cdd->is_suspended); q_num = __fls(val); val &= ~(1 << q_num); @@ -349,9 +349,6 @@ static irqreturn_t cppi41_irq(int irq, void *data) c->residue = pd_trans_len(c->desc->pd6) - len; dma_cookie_complete(&c->txd); dmaengine_desc_get_callback_invoke(&c->txd, NULL); - - pm_runtime_mark_last_busy(cdd->ddev.dev); - pm_runtime_put_autosuspend(cdd->ddev.dev); } } return IRQ_HANDLED;
With patches "dmaengine: cppi41: Fix runtime PM timeouts with USB mass storage", and "dmaengine: cppi41: Fix oops in cppi41_runtime_resume", the pm_runtime_get/put() in cppi41_irq() is no longer needed. We now guarantee that cppi41 is enabled when dma is in use. We can still get pointless error -115 when musb is configured as a usb peripheral. That's because we should now check for the state of is_suspended instead. Let's just remove the now useless code and replace it with a WARN(). Signed-off-by: Tony Lindgren <tony@atomide.com> --- Found one more cosmetic issue. With patches 1/2 and 2/2 fixing the problems, this can now wait if considered not suitable for the -rc cycle. --- drivers/dma/cppi41.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-)