| Message ID | 20161111192852.25399-1-tony@atomide.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On Fri, Nov 11, 2016 at 11:28:52AM -0800, Tony Lindgren wrote: > If we return early on pm_runtime_get() error, we need to also call > pm_runtime_put_noidle() as pointed out in a musb related thread > by Johan Hovold <johan@kernel.org>. This is to keep the PM runtime > use counts happy. Applied, thanks
On Fri, Nov 11, 2016 at 11:28:52AM -0800, Tony Lindgren wrote: > If we return early on pm_runtime_get() error, we need to also call > pm_runtime_put_noidle() as pointed out in a musb related thread > by Johan Hovold <johan@kernel.org>. This is to keep the PM runtime > use counts happy. > > Fixes: fdea2d09b997 ("dmaengine: cppi41: Add basic PM runtime support") > Cc: Johan Hovold <johan@kernel.org> > Signed-off-by: Tony Lindgren <tony@atomide.com> > --- > drivers/dma/cppi41.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > @@ -466,6 +472,7 @@ static void cppi41_dma_issue_pending(struct dma_chan *chan) > > error = pm_runtime_get(cdd->ddev.dev); > if ((error != -EINPROGRESS) && error < 0) { > + pm_runtime_put_noidle(cdd->ddev.dev); > dev_err(cdd->ddev.dev, "Failed to pm_runtime_get: %i\n", > error); Will this chunk not introduce rather than fix an imbalance, though? An error is never returned above, and the corresponding put is done unconditionally as far as I can tell. Johan -- 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 Mon, Nov 14, 2016 at 03:34:54PM +0100, Johan Hovold wrote: > On Fri, Nov 11, 2016 at 11:28:52AM -0800, Tony Lindgren wrote: > > If we return early on pm_runtime_get() error, we need to also call > > pm_runtime_put_noidle() as pointed out in a musb related thread > > by Johan Hovold <johan@kernel.org>. This is to keep the PM runtime > > use counts happy. > > > > Fixes: fdea2d09b997 ("dmaengine: cppi41: Add basic PM runtime support") > > Cc: Johan Hovold <johan@kernel.org> > > Signed-off-by: Tony Lindgren <tony@atomide.com> > > --- > > drivers/dma/cppi41.c | 11 +++++++++-- > > 1 file changed, 9 insertions(+), 2 deletions(-) > > > @@ -466,6 +472,7 @@ static void cppi41_dma_issue_pending(struct dma_chan *chan) > > > > error = pm_runtime_get(cdd->ddev.dev); > > if ((error != -EINPROGRESS) && error < 0) { > > + pm_runtime_put_noidle(cdd->ddev.dev); > > dev_err(cdd->ddev.dev, "Failed to pm_runtime_get: %i\n", > > error); > > Will this chunk not introduce rather than fix an imbalance, though? An > error is never returned above, and the corresponding put is done > unconditionally as far as I can tell. Ah, that's no longer an issue after "dma: cppi41: Fix unpaired pm runtime when only a USB hub is connected" from last week. Sorry for the noise. Johan -- 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
Hi, * Johan Hovold <johan@kernel.org> [161114 06:35]: > On Fri, Nov 11, 2016 at 11:28:52AM -0800, Tony Lindgren wrote: > > If we return early on pm_runtime_get() error, we need to also call > > pm_runtime_put_noidle() as pointed out in a musb related thread > > by Johan Hovold <johan@kernel.org>. This is to keep the PM runtime > > use counts happy. > > > > Fixes: fdea2d09b997 ("dmaengine: cppi41: Add basic PM runtime support") > > Cc: Johan Hovold <johan@kernel.org> > > Signed-off-by: Tony Lindgren <tony@atomide.com> > > --- > > drivers/dma/cppi41.c | 11 +++++++++-- > > 1 file changed, 9 insertions(+), 2 deletions(-) > > > @@ -466,6 +472,7 @@ static void cppi41_dma_issue_pending(struct dma_chan *chan) > > > > error = pm_runtime_get(cdd->ddev.dev); > > if ((error != -EINPROGRESS) && error < 0) { > > + pm_runtime_put_noidle(cdd->ddev.dev); > > dev_err(cdd->ddev.dev, "Failed to pm_runtime_get: %i\n", > > error); > > Will this chunk not introduce rather than fix an imbalance, though? An > error is never returned above, and the corresponding put is done > unconditionally as far as I can tell. There is already an early return in cppi41_dma_issue_pending() on error. 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 Mon, Nov 14, 2016 at 06:47:31AM -0800, Tony Lindgren wrote: > Hi, > > * Johan Hovold <johan@kernel.org> [161114 06:35]: > > On Fri, Nov 11, 2016 at 11:28:52AM -0800, Tony Lindgren wrote: > > > If we return early on pm_runtime_get() error, we need to also call > > > pm_runtime_put_noidle() as pointed out in a musb related thread > > > by Johan Hovold <johan@kernel.org>. This is to keep the PM runtime > > > use counts happy. > > > > > > Fixes: fdea2d09b997 ("dmaengine: cppi41: Add basic PM runtime support") > > > Cc: Johan Hovold <johan@kernel.org> > > > Signed-off-by: Tony Lindgren <tony@atomide.com> > > > --- > > > drivers/dma/cppi41.c | 11 +++++++++-- > > > 1 file changed, 9 insertions(+), 2 deletions(-) > > > > > @@ -466,6 +472,7 @@ static void cppi41_dma_issue_pending(struct dma_chan *chan) > > > > > > error = pm_runtime_get(cdd->ddev.dev); > > > if ((error != -EINPROGRESS) && error < 0) { > > > + pm_runtime_put_noidle(cdd->ddev.dev); > > > dev_err(cdd->ddev.dev, "Failed to pm_runtime_get: %i\n", > > > error); > > > > Will this chunk not introduce rather than fix an imbalance, though? An > > error is never returned above, and the corresponding put is done > > unconditionally as far as I can tell. > > There is already an early return in cppi41_dma_issue_pending() on > error. Indeed, but before "dma: cppi41: Fix unpaired pm runtime when only a USB hub is connected" from last week, the corresponding put in cppi41_irq() was done unconditionally and would have required an unconditional get here. Thanks, Johan -- 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
* Johan Hovold <johan@kernel.org> [161114 06:59]: > On Mon, Nov 14, 2016 at 06:47:31AM -0800, Tony Lindgren wrote: > > Hi, > > > > * Johan Hovold <johan@kernel.org> [161114 06:35]: > > > On Fri, Nov 11, 2016 at 11:28:52AM -0800, Tony Lindgren wrote: > > > > If we return early on pm_runtime_get() error, we need to also call > > > > pm_runtime_put_noidle() as pointed out in a musb related thread > > > > by Johan Hovold <johan@kernel.org>. This is to keep the PM runtime > > > > use counts happy. > > > > > > > > Fixes: fdea2d09b997 ("dmaengine: cppi41: Add basic PM runtime support") > > > > Cc: Johan Hovold <johan@kernel.org> > > > > Signed-off-by: Tony Lindgren <tony@atomide.com> > > > > --- > > > > drivers/dma/cppi41.c | 11 +++++++++-- > > > > 1 file changed, 9 insertions(+), 2 deletions(-) > > > > > > > @@ -466,6 +472,7 @@ static void cppi41_dma_issue_pending(struct dma_chan *chan) > > > > > > > > error = pm_runtime_get(cdd->ddev.dev); > > > > if ((error != -EINPROGRESS) && error < 0) { > > > > + pm_runtime_put_noidle(cdd->ddev.dev); > > > > dev_err(cdd->ddev.dev, "Failed to pm_runtime_get: %i\n", > > > > error); > > > > > > Will this chunk not introduce rather than fix an imbalance, though? An > > > error is never returned above, and the corresponding put is done > > > unconditionally as far as I can tell. > > > > There is already an early return in cppi41_dma_issue_pending() on > > error. > > Indeed, but before > > "dma: cppi41: Fix unpaired pm runtime when only a USB hub is > connected" > > from last week, the corresponding put in cppi41_irq() was done > unconditionally and would have required an unconditional get here. Oh yeah that's right as the pm_runtime_mark_last_busy() and pm_runtime_put_autosuspend() got moved. 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 Saturday 12 November 2016 12:58 AM, Tony Lindgren wrote: > If we return early on pm_runtime_get() error, we need to also call > pm_runtime_put_noidle() as pointed out in a musb related thread > by Johan Hovold <johan@kernel.org>. This is to keep the PM runtime > use counts happy. > > Fixes: fdea2d09b997 ("dmaengine: cppi41: Add basic PM runtime support") > Cc: Johan Hovold <johan@kernel.org> > Signed-off-by: Tony Lindgren <tony@atomide.com> > --- > drivers/dma/cppi41.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/drivers/dma/cppi41.c b/drivers/dma/cppi41.c > --- a/drivers/dma/cppi41.c > +++ b/drivers/dma/cppi41.c > @@ -366,8 +366,11 @@ static int cppi41_dma_alloc_chan_resources(struct dma_chan *chan) > int error; > > error = pm_runtime_get_sync(cdd->ddev.dev); > - if (error < 0) > + if (error < 0) { > + pm_runtime_put_noidle(cdd->ddev.dev); > + If pm_runtime_get_sync() fails due to callback error, then rpm_callback() sets dev.power.runtime_error to an error value which gets cleared by an explicit call to pm_runtime_set_suspended(). This will tell the framework that the status of device is suspended. Else, the failure will be sticky and on subsequent attempts, rpm_resume() will keep returning early instead of trying to resume the device again. This is as far as I can gather from code. So, I believe the recovery path should be: if (error < 0) { pm_runtime_set_suspended(cdd->ddev.dev); pm_runtime_put_noidle(cdd->ddev.dev); ... Thanks, Sekhar -- 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
* Sekhar Nori <nsekhar@ti.com> [161115 00:35]: > On Saturday 12 November 2016 12:58 AM, Tony Lindgren wrote: > > If we return early on pm_runtime_get() error, we need to also call > > pm_runtime_put_noidle() as pointed out in a musb related thread > > by Johan Hovold <johan@kernel.org>. This is to keep the PM runtime > > use counts happy. > > > > Fixes: fdea2d09b997 ("dmaengine: cppi41: Add basic PM runtime support") > > Cc: Johan Hovold <johan@kernel.org> > > Signed-off-by: Tony Lindgren <tony@atomide.com> > > --- > > drivers/dma/cppi41.c | 11 +++++++++-- > > 1 file changed, 9 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/dma/cppi41.c b/drivers/dma/cppi41.c > > --- a/drivers/dma/cppi41.c > > +++ b/drivers/dma/cppi41.c > > @@ -366,8 +366,11 @@ static int cppi41_dma_alloc_chan_resources(struct dma_chan *chan) > > int error; > > > > error = pm_runtime_get_sync(cdd->ddev.dev); > > - if (error < 0) > > + if (error < 0) { > > + pm_runtime_put_noidle(cdd->ddev.dev); > > + > > If pm_runtime_get_sync() fails due to callback error, then > rpm_callback() sets dev.power.runtime_error to an error value which gets > cleared by an explicit call to pm_runtime_set_suspended(). > > This will tell the framework that the status of device is suspended. > Else, the failure will be sticky and on subsequent attempts, > rpm_resume() will keep returning early instead of trying to resume the > device again. > > This is as far as I can gather from code. So, I believe the recovery > path should be: > > if (error < 0) { > pm_runtime_set_suspended(cdd->ddev.dev); > pm_runtime_put_noidle(cdd->ddev.dev); > > ... Well we should test this :) BTW, what other users of cppi41 do we have in addition to musb? I think davinci is or will be using it too? 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 Tue, Nov 15, 2016 at 12:58:17PM -0800, Tony Lindgren wrote: > * Sekhar Nori <nsekhar@ti.com> [161115 00:35]: > > On Saturday 12 November 2016 12:58 AM, Tony Lindgren wrote: > > > If we return early on pm_runtime_get() error, we need to also call > > > pm_runtime_put_noidle() as pointed out in a musb related thread > > > by Johan Hovold <johan@kernel.org>. This is to keep the PM runtime > > > use counts happy. > > > > > > Fixes: fdea2d09b997 ("dmaengine: cppi41: Add basic PM runtime support") > > > Cc: Johan Hovold <johan@kernel.org> > > > Signed-off-by: Tony Lindgren <tony@atomide.com> > > > --- > > > drivers/dma/cppi41.c | 11 +++++++++-- > > > 1 file changed, 9 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/dma/cppi41.c b/drivers/dma/cppi41.c > > > --- a/drivers/dma/cppi41.c > > > +++ b/drivers/dma/cppi41.c > > > @@ -366,8 +366,11 @@ static int cppi41_dma_alloc_chan_resources(struct dma_chan *chan) > > > int error; > > > > > > error = pm_runtime_get_sync(cdd->ddev.dev); > > > - if (error < 0) > > > + if (error < 0) { > > > + pm_runtime_put_noidle(cdd->ddev.dev); > > > + > > > > If pm_runtime_get_sync() fails due to callback error, then > > rpm_callback() sets dev.power.runtime_error to an error value which gets > > cleared by an explicit call to pm_runtime_set_suspended(). > > > > This will tell the framework that the status of device is suspended. > > Else, the failure will be sticky and on subsequent attempts, > > rpm_resume() will keep returning early instead of trying to resume the > > device again. > > > > This is as far as I can gather from code. So, I believe the recovery > > path should be: > > > > if (error < 0) { > > pm_runtime_set_suspended(cdd->ddev.dev); > > pm_runtime_put_noidle(cdd->ddev.dev); > > > > ... > > Well we should test this :) > > BTW, what other users of cppi41 do we have in addition to musb? I think > davinci is or will be using it too? AFAIK, musb on am335x, da8xx, am35x uses cppi41. davinci musb uses cppi, not cppi41. Regards, -Bin. -- 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 Wednesday 16 November 2016 02:28 AM, Tony Lindgren wrote: > * Sekhar Nori <nsekhar@ti.com> [161115 00:35]: >> If pm_runtime_get_sync() fails due to callback error, then >> rpm_callback() sets dev.power.runtime_error to an error value which gets >> cleared by an explicit call to pm_runtime_set_suspended(). >> >> This will tell the framework that the status of device is suspended. >> Else, the failure will be sticky and on subsequent attempts, >> rpm_resume() will keep returning early instead of trying to resume the >> device again. >> >> This is as far as I can gather from code. So, I believe the recovery >> path should be: >> >> if (error < 0) { >> pm_runtime_set_suspended(cdd->ddev.dev); >> pm_runtime_put_noidle(cdd->ddev.dev); >> >> ... > > Well we should test this :) Yes, right! Was this patch created to fix an error in practice or just based on code review? > BTW, what other users of cppi41 do we have in addition to musb? I think > davinci is or will be using it too? The list Bin provided seems accurate. Thanks, Sekhar -- 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
* Sekhar Nori <nsekhar@ti.com> [161115 22:25]: > On Wednesday 16 November 2016 02:28 AM, Tony Lindgren wrote: > > * Sekhar Nori <nsekhar@ti.com> [161115 00:35]: > >> If pm_runtime_get_sync() fails due to callback error, then > >> rpm_callback() sets dev.power.runtime_error to an error value which gets > >> cleared by an explicit call to pm_runtime_set_suspended(). > >> > >> This will tell the framework that the status of device is suspended. > >> Else, the failure will be sticky and on subsequent attempts, > >> rpm_resume() will keep returning early instead of trying to resume the > >> device again. > >> > >> This is as far as I can gather from code. So, I believe the recovery > >> path should be: > >> > >> if (error < 0) { > >> pm_runtime_set_suspended(cdd->ddev.dev); > >> pm_runtime_put_noidle(cdd->ddev.dev); > >> > >> ... > > > > Well we should test this :) > > Yes, right! Was this patch created to fix an error in practice or just > based on code review? Based on code review for related musb fixes. I'll test the above today at some point. > > BTW, what other users of cppi41 do we have in addition to musb? I think > > davinci is or will be using it too? > > The list Bin provided seems accurate. OK so it's used by musb only with no other hardware blocks using cppi41? 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
* Tony Lindgren <tony@atomide.com> [161116 06:55]: > * Sekhar Nori <nsekhar@ti.com> [161115 22:25]: > > On Wednesday 16 November 2016 02:28 AM, Tony Lindgren wrote: > > > * Sekhar Nori <nsekhar@ti.com> [161115 00:35]: > > >> If pm_runtime_get_sync() fails due to callback error, then > > >> rpm_callback() sets dev.power.runtime_error to an error value which gets > > >> cleared by an explicit call to pm_runtime_set_suspended(). > > >> > > >> This will tell the framework that the status of device is suspended. > > >> Else, the failure will be sticky and on subsequent attempts, > > >> rpm_resume() will keep returning early instead of trying to resume the > > >> device again. > > >> > > >> This is as far as I can gather from code. So, I believe the recovery > > >> path should be: > > >> > > >> if (error < 0) { > > >> pm_runtime_set_suspended(cdd->ddev.dev); > > >> pm_runtime_put_noidle(cdd->ddev.dev); > > >> > > >> ... > > > > > > Well we should test this :) > > > > Yes, right! Was this patch created to fix an error in practice or just > > based on code review? > > Based on code review for related musb fixes. I'll test the above today > at some point. OK so adding pm_runtime_set_suspended() allows retries, but it also seems dangerous as it clears dev.power.runtime_error. I think we're better of not having cppi41 work at all on errors which now happens. Otherwise some errors could just get ignored and we may even risk corrupting data. 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 @@ -366,8 +366,11 @@ static int cppi41_dma_alloc_chan_resources(struct dma_chan *chan) int error; error = pm_runtime_get_sync(cdd->ddev.dev); - if (error < 0) + if (error < 0) { + pm_runtime_put_noidle(cdd->ddev.dev); + return error; + } dma_cookie_init(chan); dma_async_tx_descriptor_init(&c->txd, chan); @@ -389,8 +392,11 @@ static void cppi41_dma_free_chan_resources(struct dma_chan *chan) int error; error = pm_runtime_get_sync(cdd->ddev.dev); - if (error < 0) + if (error < 0) { + pm_runtime_put_noidle(cdd->ddev.dev); + return; + } WARN_ON(!list_empty(&cdd->pending)); @@ -466,6 +472,7 @@ static void cppi41_dma_issue_pending(struct dma_chan *chan) error = pm_runtime_get(cdd->ddev.dev); if ((error != -EINPROGRESS) && error < 0) { + pm_runtime_put_noidle(cdd->ddev.dev); dev_err(cdd->ddev.dev, "Failed to pm_runtime_get: %i\n", error);
If we return early on pm_runtime_get() error, we need to also call pm_runtime_put_noidle() as pointed out in a musb related thread by Johan Hovold <johan@kernel.org>. This is to keep the PM runtime use counts happy. Fixes: fdea2d09b997 ("dmaengine: cppi41: Add basic PM runtime support") Cc: Johan Hovold <johan@kernel.org> Signed-off-by: Tony Lindgren <tony@atomide.com> --- drivers/dma/cppi41.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)