[PATCH net] r8152: use cancel_delayed_work for runtime suspend

[PATCH net] r8152: use cancel_delayed_work for runtime suspend

[Hayes Wang]

It would cause dead lock for runtime suspend, when the workqueue
is running and runtime suspend occurs before the workqueue wakes
up the device. The rtl8152_suspend() waits the workqueue to finish
because of calling cancel_delayed_work_sync(). The workqueue waits
the suspend function to finish for waking up the device because of
calling usb_autopm_get_interface().

Signed-off-by: Hayes Wang <hayeswang@realtek.com>
---
 drivers/net/usb/r8152.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 864159e..7d4e55a 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -3200,12 +3200,13 @@ static int rtl8152_suspend(struct usb_interface *intf, pm_message_t message)
 	if (netif_running(tp->netdev)) {
 		clear_bit(WORK_ENABLE, &tp->flags);
 		usb_kill_urb(tp->intr_urb);
-		cancel_delayed_work_sync(&tp->schedule);
 		tasklet_disable(&tp->tl);
 		if (test_bit(SELECTIVE_SUSPEND, &tp->flags)) {
+			cancel_delayed_work(&tp->schedule);
 			rtl_stop_rx(tp);
 			rtl_runtime_suspend_enable(tp, true);
 		} else {
+			cancel_delayed_work_sync(&tp->schedule);
 			tp->rtl_ops.down(tp);
 		}
 		tasklet_enable(&tp->tl);
-- 
1.9.3

Re: [PATCH net] r8152: use cancel_delayed_work for runtime suspend

[Bjørn Mork]

Hayes Wang <hayeswang@realtek.com> writes:

> It would cause dead lock for runtime suspend, when the workqueue
> is running and runtime suspend occurs before the workqueue wakes
> up the device. The rtl8152_suspend() waits the workqueue to finish
> because of calling cancel_delayed_work_sync(). The workqueue waits
> the suspend function to finish for waking up the device because of
> calling usb_autopm_get_interface().
>
> Signed-off-by: Hayes Wang <hayeswang@realtek.com>
> ---
>  drivers/net/usb/r8152.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index 864159e..7d4e55a 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c
> @@ -3200,12 +3200,13 @@ static int rtl8152_suspend(struct usb_interface *intf, pm_message_t message)
>  	if (netif_running(tp->netdev)) {
>  		clear_bit(WORK_ENABLE, &tp->flags);
>  		usb_kill_urb(tp->intr_urb);
> -		cancel_delayed_work_sync(&tp->schedule);
>  		tasklet_disable(&tp->tl);
>  		if (test_bit(SELECTIVE_SUSPEND, &tp->flags)) {
> +			cancel_delayed_work(&tp->schedule);
>  			rtl_stop_rx(tp);
>  			rtl_runtime_suspend_enable(tp, true);
>  		} else {
> +			cancel_delayed_work_sync(&tp->schedule);
>  			tp->rtl_ops.down(tp);
>  		}
>  		tasklet_enable(&tp->tl);

This looks strange to me. The delayed work will cause an immediate
resume due to the usb_autopm_get_interface() it starts with.  Wouldn't
it be better to just prevent runtime suspending by returning -EBUSY if
there is any delayed work scheduled?


Bjørn

[PATCH net] r8152: return -EBUSY for runtime suspend

[Hayes Wang]

Remove calling cancel_delayed_work_sync() for runtime suspend,
because it would cause dead lock. Instead, return -EBUSY to
avoid the device enters suspending if the net is running and
the delayed work is pending or running. The delayed work would
try to wake up the device later, so the suspending is not
necessary.

Signed-off-by: Hayes Wang <hayeswang@realtek.com>
---
 drivers/net/usb/r8152.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 864159e..e3d84c3 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -3189,31 +3189,39 @@ static void r8153_init(struct r8152 *tp)
 static int rtl8152_suspend(struct usb_interface *intf, pm_message_t message)
 {
 	struct r8152 *tp = usb_get_intfdata(intf);
+	struct net_device *netdev = tp->netdev;
+	int ret = 0;
 
 	mutex_lock(&tp->control);
 
-	if (PMSG_IS_AUTO(message))
+	if (PMSG_IS_AUTO(message)) {
+		if (netif_running(netdev) && work_busy(&tp->schedule.work)) {
+			ret = -EBUSY;
+			goto out1;
+		}
+
 		set_bit(SELECTIVE_SUSPEND, &tp->flags);
-	else
-		netif_device_detach(tp->netdev);
+	} else {
+		netif_device_detach(netdev);
+	}
 
-	if (netif_running(tp->netdev)) {
+	if (netif_running(netdev)) {
 		clear_bit(WORK_ENABLE, &tp->flags);
 		usb_kill_urb(tp->intr_urb);
-		cancel_delayed_work_sync(&tp->schedule);
 		tasklet_disable(&tp->tl);
 		if (test_bit(SELECTIVE_SUSPEND, &tp->flags)) {
 			rtl_stop_rx(tp);
 			rtl_runtime_suspend_enable(tp, true);
 		} else {
+			cancel_delayed_work_sync(&tp->schedule);
 			tp->rtl_ops.down(tp);
 		}
 		tasklet_enable(&tp->tl);
 	}
-
+out1:
 	mutex_unlock(&tp->control);
 
-	return 0;
+	return ret;
 }
 
 static int rtl8152_resume(struct usb_interface *intf)
-- 
1.9.3

Re: [PATCH net] r8152: return -EBUSY for runtime suspend

[David Miller]

From: Hayes Wang <hayeswang@realtek.com>
Date: Fri, 17 Oct 2014 16:55:08 +0800

> Remove calling cancel_delayed_work_sync() for runtime suspend,
> because it would cause dead lock. Instead, return -EBUSY to
> avoid the device enters suspending if the net is running and
> the delayed work is pending or running. The delayed work would
> try to wake up the device later, so the suspending is not
> necessary.
> 
> Signed-off-by: Hayes Wang <hayeswang@realtek.com>

Applied.

Re: [PATCH net] r8152: use cancel_delayed_work for runtime suspend

[Oliver Neukum]

On Fri, 2014-10-17 at 13:55 +0800, Hayes Wang wrote:
> It would cause dead lock for runtime suspend, when the workqueue
> is running and runtime suspend occurs before the workqueue wakes
> up the device. The rtl8152_suspend() waits the workqueue to finish
> because of calling cancel_delayed_work_sync(). The workqueue waits
> the suspend function to finish for waking up the device because of
> calling usb_autopm_get_interface().

The diagnosis is good, the fix is not good. It opens a race
during which the queued work can touch a suspended device.

	Regards
		Oliver


--
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 net] r8152: use cancel_delayed_work for runtime suspend

[Hayes Wang]

 Oliver Neukum [mailto:oliver-GvhC2dPhHPQdnm+yROfE0A@public.gmane.org] 
> Sent: Sunday, October 19, 2014 3:48 AM
[...]
> The diagnosis is good, the fix is not good. It opens a race
> during which the queued work can touch a suspended device.

The delayed work would wake up the device by
calling usb_autopm_get_interface() before
accessing the device. Besides, there is a mutex
to avoid the race.
 
Best Regards,
Hayes
--
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