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

[Bjørn Mork]

Post by 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().
---
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_interfa=

ce *intf, pm_message_t message)

Post by Hayes Wang
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=C3=B8rn

[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 <***@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)

[David Miller]

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

Post by 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.

Applied.

[Oliver Neukum]

Post by 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().

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-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Hayes Wang]

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-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html