[PATCH v2 0/2] dmaengine: cppi41: PM runtime fixes

[PATCH v2 0/2] dmaengine: cppi41: PM runtime fixes

[Alexandre Bailon]

I have tried to play music on usb headset on BeagleBone Black
but it doesn't work.
This series intend to fix it.

[   12.476441] usb 1-1: new full-speed USB device number 2 using musb-hdrc
[   12.896297] usb 1-1: New USB device found, idVendor=047f, idProduct=c035
[   12.903413] usb 1-1: New USB device strings: Mfr=1, Product=2, SerialNumber=3
[   12.911144] usb 1-1: Product: Plantronics C520
[   12.915848] usb 1-1: Manufacturer: Plantronics
[   12.920639] usb 1-1: SerialNumber: D6E3CB79C4662A4BA9D79F167CF83EC1
[   13.938461] usbcore: registered new interface driver usbhid
[   13.944322] usbhid: USB HID core driver
[   14.020990] usb 1-1: Warning! Unlikely big volume range (=8192), cval->res is probably wrong.
[   14.030018] usb 1-1: [11] FU [Sidetone Playback Volume] ch = 1, val = 0/8192/1
[   14.061893] usbcore: registered new interface driver snd-usb-audio
[   18.466596] cppi41-dma-engine 47400000.dma-controller: cppi41_irq pm runtime get: -115
[   18.576263] ------------[ cut here ]------------
[   18.581247] WARNING: CPU: 0 PID: 14 at drivers/dma/cppi41.c:1154 cppi41_runtime_suspend+0x28/0x34 [cppi41]
[   18.591577] Modules linked in: snd_usb_audio snd_usbmidi_lib snd_rawmidi usbhid snd_hwdep evdev musb_dsps musb_hdrc usbcore phy_am335x cppi41 udc_core usb_common phy_generic phy_am335x_control snd_soc_simple_card snd_soc_simple_card_utils snd_soc_davinci_mcasp snd_soc_omap snd_soc_edma snd_soc_core omap_rng snd_pcm_dmaengine rng_core snd_pcm snd_timer snd soundcore at24 rtc_omap musb_am335x omap_wdt tps65218_pwrbutton cpufreq_dt nvmem_core leds_gpio thermal_sys led_class hwmon
[   18.636684] CPU: 0 PID: 14 Comm: kworker/0:1 Not tainted 4.10.0-rc2-next-20170103-00020-g0f64df3-dirty #202
[   18.646944] Hardware name: Generic AM33XX (Flattened Device Tree)
[   18.653401] Workqueue: pm pm_runtime_work
[   18.657694] [<c010f484>] (unwind_backtrace) from [<c010b5dc>] (show_stack+0x10/0x14)
[   18.665873] [<c010b5dc>] (show_stack) from [<c04933d4>] (dump_stack+0xa0/0xd8)
[   18.673519] [<c04933d4>] (dump_stack) from [<c013692c>] (__warn+0xd4/0xfc)
[   18.680789] [<c013692c>] (__warn) from [<c0136974>] (warn_slowpath_null+0x20/0x28)
[   18.688804] [<c0136974>] (warn_slowpath_null) from [<bf279134>] (cppi41_runtime_suspend+0x28/0x34 [cppi41])
[   18.699218] [<bf279134>] (cppi41_runtime_suspend [cppi41]) from [<c0554b9c>] (pm_generic_runtime_suspend+0x2c/0x40)
[   18.710225] [<c0554b9c>] (pm_generic_runtime_suspend) from [<c0558044>] (__rpm_callback+0x44/0x1ec)
[   18.719769] [<c0558044>] (__rpm_callback) from [<c0558214>] (rpm_callback+0x28/0x88)
[   18.727941] [<c0558214>] (rpm_callback) from [<c0556c20>] (rpm_suspend+0xf0/0x638)
[   18.735931] [<c0556c20>] (rpm_suspend) from [<c05587ec>] (pm_runtime_work+0x7c/0x98)
[   18.744119] [<c05587ec>] (pm_runtime_work) from [<c0153cd8>] (process_one_work+0x1e0/0x6d8)
[   18.752936] [<c0153cd8>] (process_one_work) from [<c015436c>] (worker_thread+0x164/0x48c)
[   18.761574] [<c015436c>] (worker_thread) from [<c015a5b4>] (kthread+0xf8/0x134)
[   18.769299] [<c015a5b4>] (kthread) from [<c0107890>] (ret_from_fork+0x14/0x24)
[   18.777153] ---[ end trace fc3a8cc6a95cb86f ]---
[   28.650884] ------------[ cut here ]------------
[   28.655867] WARNING: CPU: 0 PID: 14 at drivers/dma/cppi41.c:452 push_desc_queue+0x94/0x9c [cppi41]
[   28.665304] Modules linked in: snd_usb_audio snd_usbmidi_lib snd_rawmidi usbhid snd_hwdep evdev musb_dsps musb_hdrc usbcore phy_am335x cppi41 udc_core usb_common phy_generic phy_am335x_control snd_soc_simple_card snd_soc_simple_card_utils snd_soc_davinci_mcasp snd_soc_omap snd_soc_edma snd_soc_core omap_rng snd_pcm_dmaengine rng_core snd_pcm snd_timer snd soundcore at24 rtc_omap musb_am335x omap_wdt tps65218_pwrbutton cpufreq_dt nvmem_core leds_gpio thermal_sys led_class hwmon
[   28.710225] CPU: 0 PID: 14 Comm: kworker/0:1 Tainted: G        W       4.10.0-rc2-next-20170103-00020-g0f64df3-dirty #202
[   28.721757] Hardware name: Generic AM33XX (Flattened Device Tree)
[   28.728213] Workqueue: pm pm_runtime_work
[   28.732507] [<c010f484>] (unwind_backtrace) from [<c010b5dc>] (show_stack+0x10/0x14)
[   28.740687] [<c010b5dc>] (show_stack) from [<c04933d4>] (dump_stack+0xa0/0xd8)
[   28.748333] [<c04933d4>] (dump_stack) from [<c013692c>] (__warn+0xd4/0xfc)
[   28.755603] [<c013692c>] (__warn) from [<c0136974>] (warn_slowpath_null+0x20/0x28)
[   28.763617] [<c0136974>] (warn_slowpath_null) from [<bf278250>] (push_desc_queue+0x94/0x9c [cppi41])
[   28.773440] [<bf278250>] (push_desc_queue [cppi41]) from [<bf27857c>] (cppi41_runtime_resume+0x44/0x88 [cppi41])
[   28.784197] [<bf27857c>] (cppi41_runtime_resume [cppi41]) from [<c0554bdc>] (pm_generic_runtime_resume+0x2c/0x40)
[   28.795024] [<c0554bdc>] (pm_generic_runtime_resume) from [<c0558044>] (__rpm_callback+0x44/0x1ec)
[   28.804476] [<c0558044>] (__rpm_callback) from [<c0558214>] (rpm_callback+0x28/0x88)
[   28.812650] [<c0558214>] (rpm_callback) from [<c0557a68>] (rpm_resume+0x3d4/0x6e8)
[   28.820641] [<c0557a68>] (rpm_resume) from [<c05587d8>] (pm_runtime_work+0x68/0x98)
[   28.828738] [<c05587d8>] (pm_runtime_work) from [<c0153cd8>] (process_one_work+0x1e0/0x6d8)
[   28.837558] [<c0153cd8>] (process_one_work) from [<c015436c>] (worker_thread+0x164/0x48c)
[   28.846196] [<c015436c>] (worker_thread) from [<c015a5b4>] (kthread+0xf8/0x134)
[   28.853924] [<c015a5b4>] (kthread) from [<c0107890>] (ret_from_fork+0x14/0x24)
[   28.861537] ---[ end trace fc3a8cc6a95cb870 ]---
[   28.866456] Unable to handle kernel NULL pointer dereference at virtual address 00000104
[   28.874979] pgd = c0004000
[   28.877841] [00000104] *pgd=00000000
[   28.881647] Internal error: Oops: 817 [#1] SMP ARM
[   28.886706] Modules linked in: snd_usb_audio snd_usbmidi_lib snd_rawmidi usbhid snd_hwdep evdev musb_dsps musb_hdrc usbcore phy_am335x cppi41 udc_core usb_common phy_generic phy_am335x_control snd_soc_simple_card snd_soc_simple_card_utils snd_soc_davinci_mcasp snd_soc_omap snd_soc_edma snd_soc_core omap_rng snd_pcm_dmaengine rng_core snd_pcm snd_timer snd soundcore at24 rtc_omap musb_am335x omap_wdt tps65218_pwrbutton cpufreq_dt nvmem_core leds_gpio thermal_sys led_class hwmon
[   28.931576] CPU: 0 PID: 14 Comm: kworker/0:1 Tainted: G        W       4.10.0-rc2-next-20170103-00020-g0f64df3-dirty #202
[   28.943107] Hardware name: Generic AM33XX (Flattened Device Tree)
[   28.949543] Workqueue: pm pm_runtime_work
[   28.953791] task: de1321c0 task.stack: de176000
[   28.958593] PC is at cppi41_runtime_resume+0x50/0x88 [cppi41]
[   28.964661] LR is at warn_slowpath_null+0x20/0x28
[   28.969632] pc : [<bf278588>]    lr : [<c0136974>]    psr: 60070093
[   28.969632] sp : de177e50  ip : 00000000  fp : dd0cf510
[   28.981717] r10: de7d2b00  r9 : a0070013  r8 : 00000100
[   28.987229] r7 : 00000200  r6 : de7d2af8  r5 : 00000078  r4 : dd008540
[   28.994109] r3 : 00000078  r2 : 00000200  r1 : 00000100  r0 : 00000000
[   29.000994] Flags: nZCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment none
[   29.008606] Control: 10c5387d  Table: 9d488019  DAC: 00000051
[   29.014665] Process kworker/0:1 (pid: 14, stack limit = 0xde176218)
[   29.021271] Stack: (0xde177e50 to 0xde178000)
[   29.025881] 7e40:                                     dd0cf410 de196810 dd0cf4b4 c0c04900
[   29.034513] 7e60: 00000002 c0554bb0 00000000 c0554bdc de1321c0 c0558044 00000001 dd0cf4b4
[   29.043143] 7e80: de1968b4 dd0cf410 de196810 c0554bb0 c0c04900 00000002 c01874ac 00000000
[   29.051774] 7ea0: dd0cf510 c0558214 00000000 dd0cf410 de196810 c0557a68 c0558784 df946a40
[   29.060405] 7ec0: df94da00 c0cc1970 00000001 de177f28 de1321c0 dd0cf4b4 dd0cf554 df946a40
[   29.069037] 7ee0: df94da00 c0cc1970 00000001 de177f28 c0c0792c c05587d8 c0558770 de14b880
[   29.077667] 7f00: dd0cf554 c0153cd8 00000001 00000000 c0153c18 c0c0792c 4d3c9129 df946a40
[   29.086297] 7f20: 00000000 00000000 c1499dbc c0e013dc 00000000 c09e5d9c de1321c0 de14b880
[   29.094928] 7f40: df946a40 df946a74 c0c04900 c0c4dc86 00000008 de14b898 df946a40 c015436c
[   29.103559] 7f60: de1321c0 de14b580 00000000 de14b580 00000000 de158040 de14b5b8 de093e98
[   29.112190] 7f80: c0154208 de14b880 00000000 c015a5b4 de158040 c015a4bc 00000000 00000000
[   29.120819] 7fa0: 00000000 00000000 00000000 c0107890 00000000 00000000 00000000 00000000
[   29.129448] 7fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[   29.138078] 7fe0: 00000000 00000000 00000000 00000000 00000013 00000000 b392d675 d669cbdc
[   29.146731] [<bf278588>] (cppi41_runtime_resume [cppi41]) from [<c0554bdc>] (pm_generic_runtime_resume+0x2c/0x40)
[   29.157555] [<c0554bdc>] (pm_generic_runtime_resume) from [<c0558044>] (__rpm_callback+0x44/0x1ec)
[   29.167006] [<c0558044>] (__rpm_callback) from [<c0558214>] (rpm_callback+0x28/0x88)
[   29.175178] [<c0558214>] (rpm_callback) from [<c0557a68>] (rpm_resume+0x3d4/0x6e8)
[   29.183168] [<c0557a68>] (rpm_resume) from [<c05587d8>] (pm_runtime_work+0x68/0x98)
[   29.191255] [<c05587d8>] (pm_runtime_work) from [<c0153cd8>] (process_one_work+0x1e0/0x6d8)
[   29.200071] [<c0153cd8>] (process_one_work) from [<c015436c>] (worker_thread+0x164/0x48c)
[   29.208703] [<c015436c>] (worker_thread) from [<c015a5b4>] (kthread+0xf8/0x134)
[   29.216422] [<c015a5b4>] (kthread) from [<c0107890>] (ret_from_fork+0x14/0x24)
[   29.224051] Code: ebffff0f e594208c e5941088 e1a03005 (e5812004) 
[   29.230492] ---[ end trace fc3a8cc6a95cb871 ]---

Note that with this series, I'm able to play music again but
I have a lot of underruns. These underruns doesn't happens when
the dma is not enabled.

Changes in v2:
 - remove the condition and use list_empty to set the variable "active"

Alexandre Bailon (2):
  dmaengine: cppi41: Fix list not empty warning on runtime suspend
  dmaengine: cppi41: Ignore EINPROGRESS for PM runtime in interrupt
    handler

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

-- 
2.10.2

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 1/2] dmaengine: cppi41: Fix list not empty warning on runtime suspend

[Alexandre Bailon]

Sometime, a transfer may not be queued due to a race between runtime pm
and cppi41_dma_issue_pending().
Sometime, cppi41_runtime_resume() may be interrupted right before to
update device PM state to RESUMED.
When it happens, if a new dma transfer is issued, because the device is not
in active state, the descriptor will be added to the pendding list.
But because the descriptors in the pendding list are only queued to cppi41
on runtime resume, the descriptor will not be queued.
On runtime suspend, the list is not empty, which is causing a warning.
Queue the descriptor if the device is active or resuming.

Signed-off-by: Alexandre Bailon <abailon-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
---
 drivers/dma/cppi41.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/cppi41.c b/drivers/dma/cppi41.c
index d5ba43a..025fee4 100644
--- a/drivers/dma/cppi41.c
+++ b/drivers/dma/cppi41.c
@@ -471,6 +471,8 @@ static void cppi41_dma_issue_pending(struct dma_chan *chan)
 {
 	struct cppi41_channel *c = to_cpp41_chan(chan);
 	struct cppi41_dd *cdd = c->cdd;
+	unsigned long flags;
+	bool active;
 	int error;
 
 	error = pm_runtime_get(cdd->ddev.dev);
@@ -482,7 +484,21 @@ static void cppi41_dma_issue_pending(struct dma_chan *chan)
 		return;
 	}
 
-	if (likely(pm_runtime_active(cdd->ddev.dev)))
+	active = pm_runtime_active(cdd->ddev.dev);
+	if (!active) {
+		/*
+		 * Runtime resume may be interrupted before runtime_status
+		 * has been updated. Test if device has resumed.
+		 */
+		if (error == -EINPROGRESS) {
+			spin_lock_irqsave(&cdd->lock, flags);
+			if (list_empty(&cdd->pending))
+				active = true;
+			spin_unlock_irqrestore(&cdd->lock, flags);
+		}
+	}
+
+	if (likely(active))
 		push_desc_queue(c);
 	else
 		pending_desc(c);
-- 
2.10.2

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/2] dmaengine: cppi41: Ignore EINPROGRESS for PM runtime in interrupt handler

[Alexandre Bailon]

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-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
---
 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 linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/2] dmaengine: cppi41: Fix list not empty warning on runtime suspend

[Sergei Shtylyov]

Hello!

On 01/09/2017 08:03 PM, Alexandre Bailon wrote:

> Sometime, a transfer may not be queued due to a race between runtime pm
> and cppi41_dma_issue_pending().
> Sometime, cppi41_runtime_resume() may be interrupted right before to
> update device PM state to RESUMED.
> When it happens, if a new dma transfer is issued, because the device is not
> in active state, the descriptor will be added to the pendding list.

    Pending.

> But because the descriptors in the pendding list are only queued to cppi41

    Likewise.

> on runtime resume, the descriptor will not be queued.
> On runtime suspend, the list is not empty, which is causing a warning.
> Queue the descriptor if the device is active or resuming.
>
> Signed-off-by: Alexandre Bailon <abailon-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
[...]

MBR, Sergei

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/2] dmaengine: cppi41: Fix list not empty warning on runtime suspend

[Grygorii Strashko]

On 01/09/2017 11:03 AM, Alexandre Bailon wrote:
> Sometime, a transfer may not be queued due to a race between runtime pm
> and cppi41_dma_issue_pending().
> Sometime, cppi41_runtime_resume() may be interrupted right before to
> update device PM state to RESUMED.
> When it happens, if a new dma transfer is issued, because the device is not
> in active state, the descriptor will be added to the pendding list.
> But because the descriptors in the pendding list are only queued to cppi41
> on runtime resume, the descriptor will not be queued.
> On runtime suspend, the list is not empty, which is causing a warning.
> Queue the descriptor if the device is active or resuming.
> 
> Signed-off-by: Alexandre Bailon <abailon-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
> ---
>  drivers/dma/cppi41.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/dma/cppi41.c b/drivers/dma/cppi41.c
> index d5ba43a..025fee4 100644
> --- a/drivers/dma/cppi41.c
> +++ b/drivers/dma/cppi41.c
> @@ -471,6 +471,8 @@ static void cppi41_dma_issue_pending(struct dma_chan *chan)
>  {
>  	struct cppi41_channel *c = to_cpp41_chan(chan);
>  	struct cppi41_dd *cdd = c->cdd;
> +	unsigned long flags;
> +	bool active;
>  	int error;
>  
>  	error = pm_runtime_get(cdd->ddev.dev);
> @@ -482,7 +484,21 @@ static void cppi41_dma_issue_pending(struct dma_chan *chan)
>  		return;
>  	}
>  
> -	if (likely(pm_runtime_active(cdd->ddev.dev)))
> +	active = pm_runtime_active(cdd->ddev.dev);
> +	if (!active) {

just curious, what does prevent from using pm_runtime_get_sync() here and in
cppi41_dma_issue_pending()?

> +		/*
> +		 * Runtime resume may be interrupted before runtime_status
> +		 * has been updated. Test if device has resumed.
> +		 */
> +		if (error == -EINPROGRESS) {
> +			spin_lock_irqsave(&cdd->lock, flags);
> +			if (list_empty(&cdd->pending))
> +				active = true;
> +			spin_unlock_irqrestore(&cdd->lock, flags);
> +		}
> +	}
> +
> +	if (likely(active))
>  		push_desc_queue(c);
>  	else
>  		pending_desc(c);
> 

-- 
regards,
-grygorii
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/2] dmaengine: cppi41: Fix list not empty warning on runtime suspend

[Tony Lindgren]

Hi,

* Alexandre Bailon <abailon-rdvid1DuHRBWk0Htik3J/w@public.gmane.org> [170109 09:04]:
> Sometime, a transfer may not be queued due to a race between runtime pm
> and cppi41_dma_issue_pending().
> Sometime, cppi41_runtime_resume() may be interrupted right before to
> update device PM state to RESUMED.
> When it happens, if a new dma transfer is issued, because the device is not
> in active state, the descriptor will be added to the pendding list.
> But because the descriptors in the pendding list are only queued to cppi41
> on runtime resume, the descriptor will not be queued.
> On runtime suspend, the list is not empty, which is causing a warning.
> Queue the descriptor if the device is active or resuming.
> 
> Signed-off-by: Alexandre Bailon <abailon-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
> ---
>  drivers/dma/cppi41.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/dma/cppi41.c b/drivers/dma/cppi41.c
> index d5ba43a..025fee4 100644
> --- a/drivers/dma/cppi41.c
> +++ b/drivers/dma/cppi41.c
> @@ -471,6 +471,8 @@ static void cppi41_dma_issue_pending(struct dma_chan *chan)
>  {
>  	struct cppi41_channel *c = to_cpp41_chan(chan);
>  	struct cppi41_dd *cdd = c->cdd;
> +	unsigned long flags;
> +	bool active;
>  	int error;
>  
>  	error = pm_runtime_get(cdd->ddev.dev);
> @@ -482,7 +484,21 @@ static void cppi41_dma_issue_pending(struct dma_chan *chan)
>  		return;
>  	}
>  
> -	if (likely(pm_runtime_active(cdd->ddev.dev)))
> +	active = pm_runtime_active(cdd->ddev.dev);
> +	if (!active) {
> +		/*
> +		 * Runtime resume may be interrupted before runtime_status
> +		 * has been updated. Test if device has resumed.
> +		 */
> +		if (error == -EINPROGRESS) {
> +			spin_lock_irqsave(&cdd->lock, flags);
> +			if (list_empty(&cdd->pending))
> +				active = true;
> +			spin_unlock_irqrestore(&cdd->lock, flags);
> +		}
> +	}
> +
> +	if (likely(active))
>  		push_desc_queue(c);
>  	else
>  		pending_desc(c);

What guarantees that the PM runtime state is really active few lines later?

A safer approach might be to check the queue for new entries by in
cppi41_runtime_resume() using "while (!list_empty())" instead of
list_for_each_entry(). That releases the spinlock between each entry
and rechecks the list.

And instead of doing WARN_ON(!list_empty(&cdd->pending)) it seems we
should run the queue also on cppi41_runtime_suspend()?

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/2] dmaengine: cppi41: Ignore EINPROGRESS for PM runtime in interrupt handler

[Tony Lindgren]

* Alexandre Bailon <abailon-rdvid1DuHRBWk0Htik3J/w@public.gmane.org> [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-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
> ---
>  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 linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/2] dmaengine: cppi41: Ignore EINPROGRESS for PM runtime in interrupt handler

[Grygorii Strashko]

On 01/09/2017 12:39 PM, Tony Lindgren wrote:
> * Alexandre Bailon <abailon-rdvid1DuHRBWk0Htik3J/w@public.gmane.org> [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-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
>> ---
>>  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

> 

-- 
regards,
-grygorii
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/2] dmaengine: cppi41: Fix list not empty warning on runtime suspend

[Alexandre Bailon]

On 01/09/2017 07:16 PM, Grygorii Strashko wrote:
> 
> 
> On 01/09/2017 11:03 AM, Alexandre Bailon wrote:
>> Sometime, a transfer may not be queued due to a race between runtime pm
>> and cppi41_dma_issue_pending().
>> Sometime, cppi41_runtime_resume() may be interrupted right before to
>> update device PM state to RESUMED.
>> When it happens, if a new dma transfer is issued, because the device is not
>> in active state, the descriptor will be added to the pendding list.
>> But because the descriptors in the pendding list are only queued to cppi41
>> on runtime resume, the descriptor will not be queued.
>> On runtime suspend, the list is not empty, which is causing a warning.
>> Queue the descriptor if the device is active or resuming.
>>
>> Signed-off-by: Alexandre Bailon <abailon-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
>> ---
>>  drivers/dma/cppi41.c | 18 +++++++++++++++++-
>>  1 file changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/dma/cppi41.c b/drivers/dma/cppi41.c
>> index d5ba43a..025fee4 100644
>> --- a/drivers/dma/cppi41.c
>> +++ b/drivers/dma/cppi41.c
>> @@ -471,6 +471,8 @@ static void cppi41_dma_issue_pending(struct dma_chan *chan)
>>  {
>>  	struct cppi41_channel *c = to_cpp41_chan(chan);
>>  	struct cppi41_dd *cdd = c->cdd;
>> +	unsigned long flags;
>> +	bool active;
>>  	int error;
>>  
>>  	error = pm_runtime_get(cdd->ddev.dev);
>> @@ -482,7 +484,21 @@ static void cppi41_dma_issue_pending(struct dma_chan *chan)
>>  		return;
>>  	}
>>  
>> -	if (likely(pm_runtime_active(cdd->ddev.dev)))
>> +	active = pm_runtime_active(cdd->ddev.dev);
>> +	if (!active) {
> 
> just curious, what does prevent from using pm_runtime_get_sync() here and in
> cppi41_dma_issue_pending()?
This function is called from atomic or interrupt context.
> 
>> +		/*
>> +		 * Runtime resume may be interrupted before runtime_status
>> +		 * has been updated. Test if device has resumed.
>> +		 */
>> +		if (error == -EINPROGRESS) {
>> +			spin_lock_irqsave(&cdd->lock, flags);
>> +			if (list_empty(&cdd->pending))
>> +				active = true;
>> +			spin_unlock_irqrestore(&cdd->lock, flags);
>> +		}
>> +	}
>> +
>> +	if (likely(active))
>>  		push_desc_queue(c);
>>  	else
>>  		pending_desc(c);
>>
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/2] dmaengine: cppi41: Ignore EINPROGRESS for PM runtime in interrupt handler

[Alexandre Bailon]

On 01/09/2017 08:11 PM, Grygorii Strashko wrote:
> 
> 
> On 01/09/2017 12:39 PM, Tony Lindgren wrote:
>> * Alexandre Bailon <abailon-rdvid1DuHRBWk0Htik3J/w@public.gmane.org> [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-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
>>> ---
>>>  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 linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/2] dmaengine: cppi41: Ignore EINPROGRESS for PM runtime in interrupt handler

[Andy Shevchenko]

On Tue, Jan 10, 2017 at 1:34 PM, Alexandre Bailon <abailon-rdvid1DuHRBWk0Htik3J/w@public.gmane.org> 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.

-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/2] dmaengine: cppi41: Fix list not empty warning on runtime suspend

[Tony Lindgren]

* Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> [170109 10:35]:
> Hi,
> 
> * Alexandre Bailon <abailon-rdvid1DuHRBWk0Htik3J/w@public.gmane.org> [170109 09:04]:
> > Sometime, a transfer may not be queued due to a race between runtime pm
> > and cppi41_dma_issue_pending().
> > Sometime, cppi41_runtime_resume() may be interrupted right before to
> > update device PM state to RESUMED.
> > When it happens, if a new dma transfer is issued, because the device is not
> > in active state, the descriptor will be added to the pendding list.
> > But because the descriptors in the pendding list are only queued to cppi41
> > on runtime resume, the descriptor will not be queued.
> > On runtime suspend, the list is not empty, which is causing a warning.
> > Queue the descriptor if the device is active or resuming.
> > 
> > Signed-off-by: Alexandre Bailon <abailon-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
> > ---
> >  drivers/dma/cppi41.c | 18 +++++++++++++++++-
> >  1 file changed, 17 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/dma/cppi41.c b/drivers/dma/cppi41.c
> > index d5ba43a..025fee4 100644
> > --- a/drivers/dma/cppi41.c
> > +++ b/drivers/dma/cppi41.c
> > @@ -471,6 +471,8 @@ static void cppi41_dma_issue_pending(struct dma_chan *chan)
> >  {
> >  	struct cppi41_channel *c = to_cpp41_chan(chan);
> >  	struct cppi41_dd *cdd = c->cdd;
> > +	unsigned long flags;
> > +	bool active;
> >  	int error;
> >  
> >  	error = pm_runtime_get(cdd->ddev.dev);
> > @@ -482,7 +484,21 @@ static void cppi41_dma_issue_pending(struct dma_chan *chan)
> >  		return;
> >  	}
> >  
> > -	if (likely(pm_runtime_active(cdd->ddev.dev)))
> > +	active = pm_runtime_active(cdd->ddev.dev);
> > +	if (!active) {
> > +		/*
> > +		 * Runtime resume may be interrupted before runtime_status
> > +		 * has been updated. Test if device has resumed.
> > +		 */
> > +		if (error == -EINPROGRESS) {
> > +			spin_lock_irqsave(&cdd->lock, flags);
> > +			if (list_empty(&cdd->pending))
> > +				active = true;
> > +			spin_unlock_irqrestore(&cdd->lock, flags);
> > +		}
> > +	}
> > +
> > +	if (likely(active))
> >  		push_desc_queue(c);
> >  	else
> >  		pending_desc(c);
> 
> What guarantees that the PM runtime state is really active few lines later?
> 
> A safer approach might be to check the queue for new entries by in
> cppi41_runtime_resume() using "while (!list_empty())" instead of
> list_for_each_entry(). That releases the spinlock between each entry
> and rechecks the list.
> 
> And instead of doing WARN_ON(!list_empty(&cdd->pending)) it seems we
> should run the queue also on cppi41_runtime_suspend()?

Hmm so I started seeing these issues too with Linux next just plugging
in USB cable on bbb while configured as a USB peripheral :)

Below is what seems to fix issues for me, not seeing any more warnings
either.

Care to give it a try with your USB headset?

Regards,

Tony

8< --------------------------
diff --git a/drivers/dma/cppi41.c b/drivers/dma/cppi41.c
--- a/drivers/dma/cppi41.c
+++ b/drivers/dma/cppi41.c
@@ -153,6 +153,8 @@ struct cppi41_dd {
 
 	/* context for suspend/resume */
 	unsigned int dma_tdfdq;
+
+	bool active;
 };
 
 #define FIST_COMPLETION_QUEUE	93
@@ -320,7 +322,8 @@ 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) ||
+			    !cdd->active)
 				dev_err(cdd->ddev.dev, "%s pm runtime get: %i\n",
 					__func__, error);
 
@@ -482,7 +485,7 @@ static void cppi41_dma_issue_pending(struct dma_chan *chan)
 		return;
 	}
 
-	if (likely(pm_runtime_active(cdd->ddev.dev)))
+	if (likely(cdd->active))
 		push_desc_queue(c);
 	else
 		pending_desc(c);
@@ -1151,6 +1154,7 @@ static int __maybe_unused cppi41_runtime_suspend(struct device *dev)
 {
 	struct cppi41_dd *cdd = dev_get_drvdata(dev);
 
+	cdd->active = false;
 	WARN_ON(!list_empty(&cdd->pending));
 
 	return 0;
@@ -1159,13 +1163,20 @@ static int __maybe_unused cppi41_runtime_suspend(struct device *dev)
 static int __maybe_unused cppi41_runtime_resume(struct device *dev)
 {
 	struct cppi41_dd *cdd = dev_get_drvdata(dev);
-	struct cppi41_channel *c, *_c;
+	struct cppi41_channel *c;
 	unsigned long flags;
 
+	cdd->active = true;
+
 	spin_lock_irqsave(&cdd->lock, flags);
-	list_for_each_entry_safe(c, _c, &cdd->pending, node) {
-		push_desc_queue(c);
+	while (!list_empty(&cdd->pending)) {
+		c = list_first_entry(&cdd->pending,
+				     struct cppi41_channel,
+				     node);
 		list_del(&c->node);
+		spin_unlock_irqrestore(&cdd->lock, flags);
+		push_desc_queue(c);
+		spin_lock_irqsave(&cdd->lock, flags);
 	}
 	spin_unlock_irqrestore(&cdd->lock, flags);
 
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/2] dmaengine: cppi41: Fix list not empty warning on runtime suspend

[Grygorii Strashko]

On 01/12/2017 11:09 AM, Tony Lindgren wrote:
> * Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> [170109 10:35]:
>> Hi,
>>
>> * Alexandre Bailon <abailon-rdvid1DuHRBWk0Htik3J/w@public.gmane.org> [170109 09:04]:
>>> Sometime, a transfer may not be queued due to a race between runtime pm
>>> and cppi41_dma_issue_pending().
>>> Sometime, cppi41_runtime_resume() may be interrupted right before to
>>> update device PM state to RESUMED.
>>> When it happens, if a new dma transfer is issued, because the device is not
>>> in active state, the descriptor will be added to the pendding list.
>>> But because the descriptors in the pendding list are only queued to cppi41
>>> on runtime resume, the descriptor will not be queued.
>>> On runtime suspend, the list is not empty, which is causing a warning.
>>> Queue the descriptor if the device is active or resuming.
>>>
>>> Signed-off-by: Alexandre Bailon <abailon-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
>>> ---
>>>  drivers/dma/cppi41.c | 18 +++++++++++++++++-
>>>  1 file changed, 17 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/dma/cppi41.c b/drivers/dma/cppi41.c
>>> index d5ba43a..025fee4 100644
>>> --- a/drivers/dma/cppi41.c
>>> +++ b/drivers/dma/cppi41.c
>>> @@ -471,6 +471,8 @@ static void cppi41_dma_issue_pending(struct dma_chan *chan)
>>>  {
>>>  	struct cppi41_channel *c = to_cpp41_chan(chan);
>>>  	struct cppi41_dd *cdd = c->cdd;
>>> +	unsigned long flags;
>>> +	bool active;
>>>  	int error;
>>>  
>>>  	error = pm_runtime_get(cdd->ddev.dev);
>>> @@ -482,7 +484,21 @@ static void cppi41_dma_issue_pending(struct dma_chan *chan)
>>>  		return;
>>>  	}
>>>  
>>> -	if (likely(pm_runtime_active(cdd->ddev.dev)))
>>> +	active = pm_runtime_active(cdd->ddev.dev);
>>> +	if (!active) {
>>> +		/*
>>> +		 * Runtime resume may be interrupted before runtime_status
>>> +		 * has been updated. Test if device has resumed.
>>> +		 */
>>> +		if (error == -EINPROGRESS) {
>>> +			spin_lock_irqsave(&cdd->lock, flags);
>>> +			if (list_empty(&cdd->pending))
>>> +				active = true;
>>> +			spin_unlock_irqrestore(&cdd->lock, flags);
>>> +		}
>>> +	}
>>> +
>>> +	if (likely(active))
>>>  		push_desc_queue(c);
>>>  	else
>>>  		pending_desc(c);
>>
>> What guarantees that the PM runtime state is really active few lines later?
>>
>> A safer approach might be to check the queue for new entries by in
>> cppi41_runtime_resume() using "while (!list_empty())" instead of
>> list_for_each_entry(). That releases the spinlock between each entry
>> and rechecks the list.
>>
>> And instead of doing WARN_ON(!list_empty(&cdd->pending)) it seems we
>> should run the queue also on cppi41_runtime_suspend()?
> 
> Hmm so I started seeing these issues too with Linux next just plugging
> in USB cable on bbb while configured as a USB peripheral :)
> 
> Below is what seems to fix issues for me, not seeing any more warnings
> either.
> 
> Care to give it a try with your USB headset?

This looks more close to what is provided in Documentation\power\runtime_pm.txt
(example at the end of the file),
but as per this document custom locking is required to access .active var.


> 
> 8< --------------------------
> diff --git a/drivers/dma/cppi41.c b/drivers/dma/cppi41.c
> --- a/drivers/dma/cppi41.c
> +++ b/drivers/dma/cppi41.c
> @@ -153,6 +153,8 @@ struct cppi41_dd {
>  
>  	/* context for suspend/resume */
>  	unsigned int dma_tdfdq;
> +
> +	bool active;
>  };
>  
>  #define FIST_COMPLETION_QUEUE	93
> @@ -320,7 +322,8 @@ 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) ||
> +			    !cdd->active)
>  				dev_err(cdd->ddev.dev, "%s pm runtime get: %i\n",
>  					__func__, error);
>  
> @@ -482,7 +485,7 @@ static void cppi41_dma_issue_pending(struct dma_chan *chan)
>  		return;
>  	}
>  
> -	if (likely(pm_runtime_active(cdd->ddev.dev)))
> +	if (likely(cdd->active))
>  		push_desc_queue(c);
>  	else
>  		pending_desc(c);
> @@ -1151,6 +1154,7 @@ static int __maybe_unused cppi41_runtime_suspend(struct device *dev)
>  {
>  	struct cppi41_dd *cdd = dev_get_drvdata(dev);
>  
> +	cdd->active = false;
>  	WARN_ON(!list_empty(&cdd->pending));
>  
>  	return 0;
> @@ -1159,13 +1163,20 @@ static int __maybe_unused cppi41_runtime_suspend(struct device *dev)
>  static int __maybe_unused cppi41_runtime_resume(struct device *dev)
>  {
>  	struct cppi41_dd *cdd = dev_get_drvdata(dev);
> -	struct cppi41_channel *c, *_c;
> +	struct cppi41_channel *c;
>  	unsigned long flags;
>  
> +	cdd->active = true;
> +
>  	spin_lock_irqsave(&cdd->lock, flags);
> -	list_for_each_entry_safe(c, _c, &cdd->pending, node) {
> -		push_desc_queue(c);
> +	while (!list_empty(&cdd->pending)) {
> +		c = list_first_entry(&cdd->pending,
> +				     struct cppi41_channel,
> +				     node);
>  		list_del(&c->node);
> +		spin_unlock_irqrestore(&cdd->lock, flags);
> +		push_desc_queue(c);
> +		spin_lock_irqsave(&cdd->lock, flags);
>  	}
>  	spin_unlock_irqrestore(&cdd->lock, flags);
>  
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 
regards,
-grygorii
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/2] dmaengine: cppi41: Fix list not empty warning on runtime suspend

[Alexandre Bailon]

On 01/12/2017 06:23 PM, Grygorii Strashko wrote:
> 
> 
> On 01/12/2017 11:09 AM, Tony Lindgren wrote:
>> * Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> [170109 10:35]:
>>> Hi,
>>>
>>> * Alexandre Bailon <abailon-rdvid1DuHRBWk0Htik3J/w@public.gmane.org> [170109 09:04]:
>>>> Sometime, a transfer may not be queued due to a race between runtime pm
>>>> and cppi41_dma_issue_pending().
>>>> Sometime, cppi41_runtime_resume() may be interrupted right before to
>>>> update device PM state to RESUMED.
>>>> When it happens, if a new dma transfer is issued, because the device is not
>>>> in active state, the descriptor will be added to the pendding list.
>>>> But because the descriptors in the pendding list are only queued to cppi41
>>>> on runtime resume, the descriptor will not be queued.
>>>> On runtime suspend, the list is not empty, which is causing a warning.
>>>> Queue the descriptor if the device is active or resuming.
>>>>
>>>> Signed-off-by: Alexandre Bailon <abailon-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
>>>> ---
>>>>  drivers/dma/cppi41.c | 18 +++++++++++++++++-
>>>>  1 file changed, 17 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/dma/cppi41.c b/drivers/dma/cppi41.c
>>>> index d5ba43a..025fee4 100644
>>>> --- a/drivers/dma/cppi41.c
>>>> +++ b/drivers/dma/cppi41.c
>>>> @@ -471,6 +471,8 @@ static void cppi41_dma_issue_pending(struct dma_chan *chan)
>>>>  {
>>>>  	struct cppi41_channel *c = to_cpp41_chan(chan);
>>>>  	struct cppi41_dd *cdd = c->cdd;
>>>> +	unsigned long flags;
>>>> +	bool active;
>>>>  	int error;
>>>>  
>>>>  	error = pm_runtime_get(cdd->ddev.dev);
>>>> @@ -482,7 +484,21 @@ static void cppi41_dma_issue_pending(struct dma_chan *chan)
>>>>  		return;
>>>>  	}
>>>>  
>>>> -	if (likely(pm_runtime_active(cdd->ddev.dev)))
>>>> +	active = pm_runtime_active(cdd->ddev.dev);
>>>> +	if (!active) {
>>>> +		/*
>>>> +		 * Runtime resume may be interrupted before runtime_status
>>>> +		 * has been updated. Test if device has resumed.
>>>> +		 */
>>>> +		if (error == -EINPROGRESS) {
>>>> +			spin_lock_irqsave(&cdd->lock, flags);
>>>> +			if (list_empty(&cdd->pending))
>>>> +				active = true;
>>>> +			spin_unlock_irqrestore(&cdd->lock, flags);
>>>> +		}
>>>> +	}
>>>> +
>>>> +	if (likely(active))
>>>>  		push_desc_queue(c);
>>>>  	else
>>>>  		pending_desc(c);
>>>
>>> What guarantees that the PM runtime state is really active few lines later?
>>>
>>> A safer approach might be to check the queue for new entries by in
>>> cppi41_runtime_resume() using "while (!list_empty())" instead of
>>> list_for_each_entry(). That releases the spinlock between each entry
>>> and rechecks the list.
>>>
>>> And instead of doing WARN_ON(!list_empty(&cdd->pending)) it seems we
>>> should run the queue also on cppi41_runtime_suspend()?
>>
>> Hmm so I started seeing these issues too with Linux next just plugging
>> in USB cable on bbb while configured as a USB peripheral :)
>>
>> Below is what seems to fix issues for me, not seeing any more warnings
>> either.
>>
>> Care to give it a try with your USB headset?
This solves the issue but I still have a bad playback quality.
I don't remember if I have spoken about it so I will describe it.
When I play audio (with your patch or mine), the music cut a lot.
The issue go away when the MUSB driver is built in PIO mode only.
Some experimentation I made today let me think it is related to
PM runtime.
I guess I should probably start another thread about that.
> 
> This looks more close to what is provided in Documentation\power\runtime_pm.txt
> (example at the end of the file),
> but as per this document custom locking is required to access .active var.
> 
> 
>>
>> 8< --------------------------
>> diff --git a/drivers/dma/cppi41.c b/drivers/dma/cppi41.c
>> --- a/drivers/dma/cppi41.c
>> +++ b/drivers/dma/cppi41.c
>> @@ -153,6 +153,8 @@ struct cppi41_dd {
>>  
>>  	/* context for suspend/resume */
>>  	unsigned int dma_tdfdq;
>> +
>> +	bool active;
>>  };
>>  
>>  #define FIST_COMPLETION_QUEUE	93
>> @@ -320,7 +322,8 @@ 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) ||
>> +			    !cdd->active)
>>  				dev_err(cdd->ddev.dev, "%s pm runtime get: %i\n",
>>  					__func__, error);
>>  
>> @@ -482,7 +485,7 @@ static void cppi41_dma_issue_pending(struct dma_chan *chan)
>>  		return;
>>  	}
>>  
>> -	if (likely(pm_runtime_active(cdd->ddev.dev)))
>> +	if (likely(cdd->active))
>>  		push_desc_queue(c);
>>  	else
>>  		pending_desc(c);
>> @@ -1151,6 +1154,7 @@ static int __maybe_unused cppi41_runtime_suspend(struct device *dev)
>>  {
>>  	struct cppi41_dd *cdd = dev_get_drvdata(dev);
>>  
>> +	cdd->active = false;
>>  	WARN_ON(!list_empty(&cdd->pending));
>>  
>>  	return 0;
>> @@ -1159,13 +1163,20 @@ static int __maybe_unused cppi41_runtime_suspend(struct device *dev)
>>  static int __maybe_unused cppi41_runtime_resume(struct device *dev)
>>  {
>>  	struct cppi41_dd *cdd = dev_get_drvdata(dev);
>> -	struct cppi41_channel *c, *_c;
>> +	struct cppi41_channel *c;
>>  	unsigned long flags;
>>  
>> +	cdd->active = true;
>> +
>>  	spin_lock_irqsave(&cdd->lock, flags);
>> -	list_for_each_entry_safe(c, _c, &cdd->pending, node) {
>> -		push_desc_queue(c);
>> +	while (!list_empty(&cdd->pending)) {
>> +		c = list_first_entry(&cdd->pending,
>> +				     struct cppi41_channel,
>> +				     node);
>>  		list_del(&c->node);
>> +		spin_unlock_irqrestore(&cdd->lock, flags);
>> +		push_desc_queue(c);
>> +		spin_lock_irqsave(&cdd->lock, flags);
>>  	}
>>  	spin_unlock_irqrestore(&cdd->lock, flags);
>>  
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/2] dmaengine: cppi41: Fix list not empty warning on runtime suspend

[Tony Lindgren]

* Grygorii Strashko <grygorii.strashko-l0cyMroinI0@public.gmane.org> [170112 09:24]:
> On 01/12/2017 11:09 AM, Tony Lindgren wrote:
> > Below is what seems to fix issues for me, not seeing any more warnings
> > either.
> > 
> > Care to give it a try with your USB headset?
> 
> This looks more close to what is provided in Documentation\power\runtime_pm.txt
> (example at the end of the file),
> but as per this document custom locking is required to access .active var.

OK so sounds like we're on the right track then :) Here's a version
using atomic.h.

Regards,

Tony

8< -------------------
diff --git a/drivers/dma/cppi41.c b/drivers/dma/cppi41.c
--- a/drivers/dma/cppi41.c
+++ b/drivers/dma/cppi41.c
@@ -1,3 +1,4 @@
+#include <linux/atomic.h>
 #include <linux/delay.h>
 #include <linux/dmaengine.h>
 #include <linux/dma-mapping.h>
@@ -153,6 +154,8 @@ struct cppi41_dd {
 
 	/* context for suspend/resume */
 	unsigned int dma_tdfdq;
+
+	atomic_t active;
 };
 
 #define FIST_COMPLETION_QUEUE	93
@@ -320,7 +323,8 @@ 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) ||
+			    !atomic_read(&cdd->active))
 				dev_err(cdd->ddev.dev, "%s pm runtime get: %i\n",
 					__func__, error);
 
@@ -482,7 +486,7 @@ static void cppi41_dma_issue_pending(struct dma_chan *chan)
 		return;
 	}
 
-	if (likely(pm_runtime_active(cdd->ddev.dev)))
+	if (likely(atomic_read(&cdd->active)))
 		push_desc_queue(c);
 	else
 		pending_desc(c);
@@ -1003,6 +1007,7 @@ static int cppi41_dma_probe(struct platform_device *pdev)
 	cdd->ddev.dst_addr_widths = CPPI41_DMA_BUSWIDTHS;
 	cdd->ddev.residue_granularity = DMA_RESIDUE_GRANULARITY_BURST;
 	cdd->ddev.dev = dev;
+	atomic_set(&cdd->active, 0);
 	INIT_LIST_HEAD(&cdd->ddev.channels);
 	cpp41_dma_info.dma_cap = cdd->ddev.cap_mask;
 
@@ -1151,6 +1156,7 @@ static int __maybe_unused cppi41_runtime_suspend(struct device *dev)
 {
 	struct cppi41_dd *cdd = dev_get_drvdata(dev);
 
+	atomic_set(&cdd->active, 0);
 	WARN_ON(!list_empty(&cdd->pending));
 
 	return 0;
@@ -1159,13 +1165,20 @@ static int __maybe_unused cppi41_runtime_suspend(struct device *dev)
 static int __maybe_unused cppi41_runtime_resume(struct device *dev)
 {
 	struct cppi41_dd *cdd = dev_get_drvdata(dev);
-	struct cppi41_channel *c, *_c;
+	struct cppi41_channel *c;
 	unsigned long flags;
 
+	atomic_set(&cdd->active, 1);
+
 	spin_lock_irqsave(&cdd->lock, flags);
-	list_for_each_entry_safe(c, _c, &cdd->pending, node) {
-		push_desc_queue(c);
+	while (!list_empty(&cdd->pending)) {
+		c = list_first_entry(&cdd->pending,
+				     struct cppi41_channel,
+				     node);
 		list_del(&c->node);
+		spin_unlock_irqrestore(&cdd->lock, flags);
+		push_desc_queue(c);
+		spin_lock_irqsave(&cdd->lock, flags);
 	}
 	spin_unlock_irqrestore(&cdd->lock, flags);
 
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/2] dmaengine: cppi41: Fix list not empty warning on runtime suspend

[Tony Lindgren]

* Alexandre Bailon <abailon-rdvid1DuHRBWk0Htik3J/w@public.gmane.org> [170112 09:42]:
> This solves the issue but I still have a bad playback quality.
> I don't remember if I have spoken about it so I will describe it.
> When I play audio (with your patch or mine), the music cut a lot.
> The issue go away when the MUSB driver is built in PIO mode only.
> Some experimentation I made today let me think it is related to
> PM runtime.
> I guess I should probably start another thread about that.

OK interesting. We may be able to fix that just by requesting
PM QoS for the audio device. See a fix for similar issue in thread
"[PATCH v6] ASoC: omap-mcbsp: Add PM QoS support for McBSP to
prevent glitches".

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html