dma: cpp41: Fix handling of error path

PATCH Applied Tony Lindgren 5 participants 13 messages Nov 11, 2016 – Nov 16, 2016 6 messages not in the archive dmaengine

lore

Share & tools

lore

Tony Lindgren

author

2016-11-11 19:28 UTC #0 lore raw

PATCH 1/1 dma: cpp41: Fix handling of error path

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 <[email protected]>. This is to keep the PM runtime use counts happy.

Fixes fdea2d09b997 ("dmaengine: cppi41: Add basic PM runtime support")

Cc Johan Hovold <[email protected]>

Signed-off-by Tony Lindgren <[email protected]>

---
 drivers/dma/cppi41.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

drivers/dma/cppi41.c +9 -2

@@ -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);

Signature · 5 lines

2.10.2
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 20161111192852.25399-1-tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org

Vinod Koul

Applied

2016-11-14 08:31 UTC #2 lore raw

Quoted 4 lines · Tony Lindgren

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 <[email protected]>. This is to keep the PM runtime use counts happy.

Applied, thanks

Signature · 5 lines

~Vinod
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Johan Hovold

2016-11-14 14:34 UTC #3 lore raw

In reply to a message not in the archive

Quoted 13 lines · Tony Lindgren

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 <[email protected]>. This is to keep the PM runtime use counts happy.
Fixes fdea2d09b997 ("dmaengine: cppi41: Add basic PM runtime support")
Cc Johan Hovold <[email protected]>
Signed-off-by Tony Lindgren <[email protected]>
---
 drivers/dma/cppi41.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

drivers/dma/cppi41.c static void cppi41_dma_issue_pending(struct dma_chan *chan) · line 477

Quoted 6 lines

drivers/dma/cppi41.c +1 -0

@@ -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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Johan Hovold

2016-11-14 14:41 UTC #4 lore raw

Quoted 15 lines · Johan Hovold

Quoted 14 lines · Tony Lindgren
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 <[email protected]>. This is to keep the PM runtime use counts happy.
Fixes fdea2d09b997 ("dmaengine: cppi41: Add basic PM runtime support")
Cc Johan Hovold <[email protected]>
Signed-off-by Tony Lindgren <[email protected]>
---
 drivers/dma/cppi41.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)
(quoted diff) +0 -0
@@ -466,6 +472,7 @@ static void cppi41_dma_issue_pending(struct dma_chan *chan)
 

Quoted 9 lines

drivers/dma/cppi41.c static void cppi41_dma_issue_pending(struct dma_chan *chan) · line 477
Quoted 5 lines
drivers/dma/cppi41.c +1 -0
 	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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Tony Lindgren

author

2016-11-14 14:47 UTC #5 lore raw

In reply to Johan Hovold

Hi,

Quoted 15 lines · Johan Hovold

Quoted 14 lines · Tony Lindgren
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 <[email protected]>. This is to keep the PM runtime use counts happy.
Fixes fdea2d09b997 ("dmaengine: cppi41: Add basic PM runtime support")
Cc Johan Hovold <[email protected]>
Signed-off-by Tony Lindgren <[email protected]>
---
 drivers/dma/cppi41.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)
(quoted diff) +0 -0
@@ -466,6 +472,7 @@ static void cppi41_dma_issue_pending(struct dma_chan *chan)
 

Quoted 9 lines

drivers/dma/cppi41.c static void cppi41_dma_issue_pending(struct dma_chan *chan) · line 477
Quoted 5 lines
drivers/dma/cppi41.c +1 -0
 	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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): [email protected]

Johan Hovold

2016-11-14 14:59 UTC #7 lore raw

Quoted 28 lines · Tony Lindgren

Hi,
Quoted 15 lines · Johan Hovold
Quoted 14 lines · Tony Lindgren
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 <[email protected]>. This is to keep the PM runtime use counts happy.
Fixes fdea2d09b997 ("dmaengine: cppi41: Add basic PM runtime support")
Cc Johan Hovold <[email protected]>
Signed-off-by Tony Lindgren <[email protected]>
---
 drivers/dma/cppi41.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)
(quoted diff) +0 -0
@@ -466,6 +472,7 @@ static void cppi41_dma_issue_pending(struct dma_chan *chan)
 
Quoted 9 lines
drivers/dma/cppi41.c static void cppi41_dma_issue_pending(struct dma_chan *chan) · line 477
Quoted 5 lines
drivers/dma/cppi41.c +1 -0
 	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.

Quoted 2 lines

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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Tony Lindgren

author

2016-11-14 15:07 UTC #8 lore raw

Quoted 32 lines · Johan Hovold

Quoted 28 lines · Tony Lindgren
Hi,
Quoted 15 lines · Johan Hovold
Quoted 14 lines · Tony Lindgren
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 <[email protected]>. This is to keep the PM runtime use counts happy.
Fixes fdea2d09b997 ("dmaengine: cppi41: Add basic PM runtime support")
Cc Johan Hovold <[email protected]>
Signed-off-by Tony Lindgren <[email protected]>
---
 drivers/dma/cppi41.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)
(quoted diff) +0 -0
@@ -466,6 +472,7 @@ static void cppi41_dma_issue_pending(struct dma_chan *chan)
 
Quoted 9 lines
drivers/dma/cppi41.c static void cppi41_dma_issue_pending(struct dma_chan *chan) · line 477
Quoted 5 lines
drivers/dma/cppi41.c +1 -0
 	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.
Quoted 2 lines
There is already an early return in cppi41_dma_issue_pending() on error.

Quoted 7 lines

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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Sekhar Nori

2016-11-15 08:35 UTC #9 lore raw

In reply to a message not in the archive

Quoted 16 lines · Tony Lindgren

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 <[email protected]>. This is to keep the PM runtime use counts happy.
Fixes fdea2d09b997 ("dmaengine: cppi41: Add basic PM runtime support")
Cc Johan Hovold <[email protected]>
Signed-off-by Tony Lindgren <[email protected]>
---
 drivers/dma/cppi41.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

drivers/dma/cppi41.c static int cppi41_dma_alloc_chan_resources(struct dma_chan *chan) · line 371

Quoted 7 lines

drivers/dma/cppi41.c +3 -1

@@ -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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 8d1e380a-ec75-3893-ea50-65e1526d58ea-l0cyMroinI0@public.gmane.org

Tony Lindgren

author

2016-11-15 20:58 UTC #11 lore raw

Quoted 34 lines · Sekhar Nori

Quoted 16 lines · Tony Lindgren
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 <[email protected]>. This is to keep the PM runtime use counts happy.
Fixes fdea2d09b997 ("dmaengine: cppi41: Add basic PM runtime support")
Cc Johan Hovold <[email protected]>
Signed-off-by Tony Lindgren <[email protected]>
---
 drivers/dma/cppi41.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)
drivers/dma/cppi41.c static int cppi41_dma_alloc_chan_resources(struct dma_chan *chan) · line 371
Quoted 7 lines
drivers/dma/cppi41.c +3 -1
@@ -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.

Quoted 8 lines

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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): [email protected]

Bin Liu

2016-11-15 21:27 UTC #13 lore raw

Quoted 42 lines · Tony Lindgren

Quoted 41 lines · Sekhar Nori
Quoted 16 lines · Tony Lindgren
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 <[email protected]>. This is to keep the PM runtime use counts happy.
Fixes fdea2d09b997 ("dmaengine: cppi41: Add basic PM runtime support")
Cc Johan Hovold <[email protected]>
Signed-off-by Tony Lindgren <[email protected]>
---
 drivers/dma/cppi41.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)
drivers/dma/cppi41.c static int cppi41_dma_alloc_chan_resources(struct dma_chan *chan) · line 371
Quoted 7 lines
drivers/dma/cppi41.c +3 -1
@@ -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);

Quoted 6 lines

Quoted 1 line
		...
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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Sekhar Nori

2016-11-16 06:25 UTC #14 lore raw

In reply to a message not in the archive

Quoted 13 lines · Tony Lindgren

Quoted 12 lines · Sekhar Nori
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:

Quoted 7 lines

Quoted 5 lines

	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?

Quoted 2 lines

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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 91726bb9-4919-22a4-174b-9c89e1001421-l0cyMroinI0@public.gmane.org

Tony Lindgren

author

2016-11-16 14:54 UTC #16 lore raw

Quoted 18 lines · Sekhar Nori

Quoted 17 lines · Tony Lindgren
Quoted 16 lines · Sekhar Nori
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);

Quoted 6 lines

Quoted 3 lines
Quoted 1 line
		...
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.

Quoted 4 lines

Quoted 2 lines
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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): [email protected]

Tony Lindgren

author

2016-11-16 17:11 UTC #18 lore raw

Quoted 21 lines · Tony Lindgren

Quoted 20 lines · Sekhar Nori
Quoted 19 lines · Tony Lindgren
Quoted 17 lines · Sekhar Nori
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);

		...

Quoted 7 lines

Quoted 4 lines
Quoted 1 line
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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html