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 <[email protected]>
---
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
Hayes Wang <[email protected]> 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 <[email protected]>
> ---
> 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
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 <[email protected]>
---
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
From: Hayes Wang <[email protected]>
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 <[email protected]>
Applied.
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
Oliver Neukum [mailto:[email protected]]
> 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