| Message ID | 20170109170337.6957-3-abailon@baylibre.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
* Alexandre Bailon <abailon@baylibre.com> [170109 09:04]: > We can occasionally get -EINPROGRESS for pm_runtime_get. > This is happening when an interrupt is fired before PM runtime had time > to update the PM state to RESUMED. > In that case, don't print any error. Hmm if the cppi41 interrupt fires, the device has resumed :) I think we should have a cppi41.c specific flag that we can set at the end of cppi41_resume() instead of list_empty() or pm_runtime_active(). Regars, Tony > Signed-off-by: Alexandre Bailon <abailon@baylibre.com> > --- > drivers/dma/cppi41.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/drivers/dma/cppi41.c b/drivers/dma/cppi41.c > index 025fee4..2306020 100644 > --- a/drivers/dma/cppi41.c > +++ b/drivers/dma/cppi41.c > @@ -320,7 +320,7 @@ 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) > dev_err(cdd->ddev.dev, "%s pm runtime get: %i\n", > __func__, error); > > @@ -492,8 +492,7 @@ static void cppi41_dma_issue_pending(struct dma_chan *chan) > */ > if (error == -EINPROGRESS) { > spin_lock_irqsave(&cdd->lock, flags); > - if (list_empty(&cdd->pending)) > - active = true; > + active = !!list_empty(&cdd->pending); > spin_unlock_irqrestore(&cdd->lock, flags); > } > } > -- > 2.10.2 > -- 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/09/2017 12:39 PM, Tony Lindgren wrote: > * Alexandre Bailon <abailon@baylibre.com> [170109 09:04]: >> We can occasionally get -EINPROGRESS for pm_runtime_get. >> This is happening when an interrupt is fired before PM runtime had time >> to update the PM state to RESUMED. >> In that case, don't print any error. > > Hmm if the cppi41 interrupt fires, the device has resumed :) > > I think we should have a cppi41.c specific flag that we can set > at the end of cppi41_resume() instead of list_empty() or > pm_runtime_active(). > > Regars, > > Tony > > >> Signed-off-by: Alexandre Bailon <abailon@baylibre.com> >> --- >> drivers/dma/cppi41.c | 5 ++--- >> 1 file changed, 2 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/dma/cppi41.c b/drivers/dma/cppi41.c >> index 025fee4..2306020 100644 >> --- a/drivers/dma/cppi41.c >> +++ b/drivers/dma/cppi41.c >> @@ -320,7 +320,7 @@ 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) >> dev_err(cdd->ddev.dev, "%s pm runtime get: %i\n", >> __func__, error); Another question of curiosity :) -EINPROGRESS and pm_runtime_get() in general doesn't guarantee that device is powered on and accessible immediately after this call, but few lines below there are code which try to access registers desc = cppi41_pop_desc(cdd, q_num); Is it ok? >> >> @@ -492,8 +492,7 @@ static void cppi41_dma_issue_pending(struct dma_chan *chan) >> */ >> if (error == -EINPROGRESS) { >> spin_lock_irqsave(&cdd->lock, flags); >> - if (list_empty(&cdd->pending)) >> - active = true; >> + active = !!list_empty(&cdd->pending); >> spin_unlock_irqrestore(&cdd->lock, flags); >> } >> } >> -- >> 2.10.2 >
On 01/09/2017 08:11 PM, Grygorii Strashko wrote: > > > On 01/09/2017 12:39 PM, Tony Lindgren wrote: >> * Alexandre Bailon <abailon@baylibre.com> [170109 09:04]: >>> We can occasionally get -EINPROGRESS for pm_runtime_get. >>> This is happening when an interrupt is fired before PM runtime had time >>> to update the PM state to RESUMED. >>> In that case, don't print any error. >> >> Hmm if the cppi41 interrupt fires, the device has resumed :) >> >> I think we should have a cppi41.c specific flag that we can set >> at the end of cppi41_resume() instead of list_empty() or >> pm_runtime_active(). >> >> Regars, >> >> Tony >> >> >>> Signed-off-by: Alexandre Bailon <abailon@baylibre.com> >>> --- >>> drivers/dma/cppi41.c | 5 ++--- >>> 1 file changed, 2 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/dma/cppi41.c b/drivers/dma/cppi41.c >>> index 025fee4..2306020 100644 >>> --- a/drivers/dma/cppi41.c >>> +++ b/drivers/dma/cppi41.c >>> @@ -320,7 +320,7 @@ 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) >>> dev_err(cdd->ddev.dev, "%s pm runtime get: %i\n", >>> __func__, error); > > Another question of curiosity :) -EINPROGRESS and pm_runtime_get() in general doesn't > guarantee that device is powered on and accessible immediately after this call, but > few lines below there are code which try to access registers > desc = cppi41_pop_desc(cdd, q_num); > Is it ok? I guess it is ok. I think the interrupt can only be fired if the device is active. And the interrupt happen because the one descriptor queued during the resume has been moved to completion queue (so device is clocked and operational). > >>> >>> @@ -492,8 +492,7 @@ static void cppi41_dma_issue_pending(struct dma_chan *chan) >>> */ >>> if (error == -EINPROGRESS) { >>> spin_lock_irqsave(&cdd->lock, flags); >>> - if (list_empty(&cdd->pending)) >>> - active = true; >>> + active = !!list_empty(&cdd->pending); >>> spin_unlock_irqrestore(&cdd->lock, flags); >>> } >>> } >>> -- >>> 2.10.2 > >> > -- 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 Tue, Jan 10, 2017 at 1:34 PM, Alexandre Bailon <abailon@baylibre.com> wrote: > On 01/09/2017 08:11 PM, Grygorii Strashko wrote: >>> Hmm if the cppi41 interrupt fires, the device has resumed :) >>> >>> I think we should have a cppi41.c specific flag that we can set >>> at the end of cppi41_resume() instead of list_empty() or >>> pm_runtime_active(). > I think the interrupt can only be fired if the device is active. I have no idea about this particular case, but believe me there are cases where above is not true. I think Tony keeps it in mind after good discussion about thread IRQ enforcement on shared interrupt handlers. > And the interrupt happen because the one descriptor queued during the > resume has been moved to completion queue > (so device is clocked and operational). Doesn't really matter in some cases.
diff --git a/drivers/dma/cppi41.c b/drivers/dma/cppi41.c index 025fee4..2306020 100644 --- a/drivers/dma/cppi41.c +++ b/drivers/dma/cppi41.c @@ -320,7 +320,7 @@ 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) dev_err(cdd->ddev.dev, "%s pm runtime get: %i\n", __func__, error); @@ -492,8 +492,7 @@ static void cppi41_dma_issue_pending(struct dma_chan *chan) */ if (error == -EINPROGRESS) { spin_lock_irqsave(&cdd->lock, flags); - if (list_empty(&cdd->pending)) - active = true; + active = !!list_empty(&cdd->pending); spin_unlock_irqrestore(&cdd->lock, flags); } }
We can occasionally get -EINPROGRESS for pm_runtime_get. This is happening when an interrupt is fired before PM runtime had time to update the PM state to RESUMED. In that case, don't print any error. Signed-off-by: Alexandre Bailon <abailon@baylibre.com> --- drivers/dma/cppi41.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)