Re: [PATCH] dma: cpp41: Fix handling of error path
* Tony Lindgren [161116 06:55]:
> * Sekhar Nori [161115 22:25]:
> > On Wednesday 16 November 2016 02:28 AM, Tony Lindgren wrote:
> > > * Sekhar Nori [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 linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] dma: cpp41: Fix handling of error path
* Sekhar Nori [161115 22:25]:
> On Wednesday 16 November 2016 02:28 AM, Tony Lindgren wrote:
> > * Sekhar Nori [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 linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] dma: cpp41: Fix handling of error path
On Wednesday 16 November 2016 02:28 AM, Tony Lindgren wrote:
> * Sekhar Nori [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 linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] dma: cpp41: Fix handling of error path
On Tue, Nov 15, 2016 at 12:58:17PM -0800, Tony Lindgren wrote:
> * Sekhar Nori [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 . This is to keep the PM runtime
> > > use counts happy.
> > >
> > > Fixes: fdea2d09b997 ("dmaengine: cppi41: Add basic PM runtime support")
> > > Cc: Johan Hovold
> > > Signed-off-by: Tony Lindgren
> > > ---
> > > 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 linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] dma: cpp41: Fix handling of error path
* Sekhar Nori [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 . This is to keep the PM runtime
> > use counts happy.
> >
> > Fixes: fdea2d09b997 ("dmaengine: cppi41: Add basic PM runtime support")
> > Cc: Johan Hovold
> > Signed-off-by: Tony Lindgren
> > ---
> > 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 linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] dma: cpp41: Fix handling of error path
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 . This is to keep the PM runtime
> use counts happy.
>
> Fixes: fdea2d09b997 ("dmaengine: cppi41: Add basic PM runtime support")
> Cc: Johan Hovold
> Signed-off-by: Tony Lindgren
> ---
> 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 linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] dma: cpp41: Fix handling of error path
* Johan Hovold [161114 06:59]:
> On Mon, Nov 14, 2016 at 06:47:31AM -0800, Tony Lindgren wrote:
> > Hi,
> >
> > * Johan Hovold [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 . This is to keep the PM runtime
> > > > use counts happy.
> > > >
> > > > Fixes: fdea2d09b997 ("dmaengine: cppi41: Add basic PM runtime support")
> > > > Cc: Johan Hovold
> > > > Signed-off-by: Tony Lindgren
> > > > ---
> > > > 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 linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] dma: cpp41: Fix handling of error path
On Mon, Nov 14, 2016 at 06:47:31AM -0800, Tony Lindgren wrote:
> Hi,
>
> * Johan Hovold [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 . This is to keep the PM runtime
> > > use counts happy.
> > >
> > > Fixes: fdea2d09b997 ("dmaengine: cppi41: Add basic PM runtime support")
> > > Cc: Johan Hovold
> > > Signed-off-by: Tony Lindgren
> > > ---
> > > 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 linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] dma: cpp41: Fix handling of error path
Hi,
* Johan Hovold [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 . This is to keep the PM runtime
> > use counts happy.
> >
> > Fixes: fdea2d09b997 ("dmaengine: cppi41: Add basic PM runtime support")
> > Cc: Johan Hovold
> > Signed-off-by: Tony Lindgren
> > ---
> > 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 linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] dma: cpp41: Fix handling of error path
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 . This is to keep the PM runtime
> > use counts happy.
> >
> > Fixes: fdea2d09b997 ("dmaengine: cppi41: Add basic PM runtime support")
> > Cc: Johan Hovold
> > Signed-off-by: Tony Lindgren
> > ---
> > 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 linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] dma: cpp41: Fix handling of error path
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 . This is to keep the PM runtime
> use counts happy.
>
> Fixes: fdea2d09b997 ("dmaengine: cppi41: Add basic PM runtime support")
> Cc: Johan Hovold
> Signed-off-by: Tony Lindgren
> ---
> 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 linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] dma: cpp41: Fix handling of error path
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 . This is to keep the PM runtime > use counts happy. Applied, thanks -- ~Vinod -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
