Improve the flow about runtime suspend/resume and make the code
easy to read.
Hayes Wang (2):
r8152: split rtl8152_resume function
r8152: move calling delay_autosuspend function
drivers/net/usb/r8152.c | 107 ++++++++++++++++++++++++++++--------------------
1 file changed, 62 insertions(+), 45 deletions(-)
--
2.7.4
Split rtl8152_resume() into rtl8152_runtime_resume() and
rtl8152_system_resume().
Signed-off-by: Hayes Wang <[email protected]>
---
drivers/net/usb/r8152.c | 99 ++++++++++++++++++++++++++++++-------------------
1 file changed, 61 insertions(+), 38 deletions(-)
diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 5a02053..3257955 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -3686,6 +3686,61 @@ static bool delay_autosuspend(struct r8152 *tp)
return false;
}
+static int rtl8152_runtime_resume(struct r8152 *tp)
+{
+ struct net_device *netdev = tp->netdev;
+
+ if (netif_running(netdev) && netdev->flags & IFF_UP) {
+ struct napi_struct *napi = &tp->napi;
+
+ tp->rtl_ops.autosuspend_en(tp, false);
+ napi_disable(napi);
+ set_bit(WORK_ENABLE, &tp->flags);
+
+ if (netif_carrier_ok(netdev)) {
+ if (rtl8152_get_speed(tp) & LINK_STATUS) {
+ rtl_start_rx(tp);
+ } else {
+ netif_carrier_off(netdev);
+ tp->rtl_ops.disable(tp);
+ netif_info(tp, link, netdev, "linking down\n");
+ }
+ }
+
+ napi_enable(napi);
+ clear_bit(SELECTIVE_SUSPEND, &tp->flags);
+ smp_mb__after_atomic();
+
+ if (!list_empty(&tp->rx_done))
+ napi_schedule(&tp->napi);
+
+ usb_submit_urb(tp->intr_urb, GFP_KERNEL);
+ } else {
+ if (netdev->flags & IFF_UP)
+ tp->rtl_ops.autosuspend_en(tp, false);
+
+ clear_bit(SELECTIVE_SUSPEND, &tp->flags);
+ }
+
+ return 0;
+}
+
+static int rtl8152_system_resume(struct r8152 *tp)
+{
+ struct net_device *netdev = tp->netdev;
+
+ netif_device_attach(netdev);
+
+ if (netif_running(netdev) && netdev->flags & IFF_UP) {
+ tp->rtl_ops.up(tp);
+ netif_carrier_off(netdev);
+ set_bit(WORK_ENABLE, &tp->flags);
+ usb_submit_urb(tp->intr_urb, GFP_KERNEL);
+ }
+
+ return 0;
+}
+
static int rtl8152_runtime_suspend(struct r8152 *tp)
{
struct net_device *netdev = tp->netdev;
@@ -3784,50 +3839,18 @@ static int rtl8152_suspend(struct usb_interface *intf, pm_message_t message)
static int rtl8152_resume(struct usb_interface *intf)
{
struct r8152 *tp = usb_get_intfdata(intf);
- struct net_device *netdev = tp->netdev;
+ int ret;
mutex_lock(&tp->control);
- if (!test_bit(SELECTIVE_SUSPEND, &tp->flags))
- netif_device_attach(netdev);
-
- if (netif_running(netdev) && netdev->flags & IFF_UP) {
- if (test_bit(SELECTIVE_SUSPEND, &tp->flags)) {
- struct napi_struct *napi = &tp->napi;
-
- tp->rtl_ops.autosuspend_en(tp, false);
- napi_disable(napi);
- set_bit(WORK_ENABLE, &tp->flags);
- if (netif_carrier_ok(netdev)) {
- if (rtl8152_get_speed(tp) & LINK_STATUS) {
- rtl_start_rx(tp);
- } else {
- netif_carrier_off(netdev);
- tp->rtl_ops.disable(tp);
- netif_info(tp, link, netdev,
- "linking down\n");
- }
- }
- napi_enable(napi);
- clear_bit(SELECTIVE_SUSPEND, &tp->flags);
- smp_mb__after_atomic();
- if (!list_empty(&tp->rx_done))
- napi_schedule(&tp->napi);
- } else {
- tp->rtl_ops.up(tp);
- netif_carrier_off(netdev);
- set_bit(WORK_ENABLE, &tp->flags);
- }
- usb_submit_urb(tp->intr_urb, GFP_KERNEL);
- } else if (test_bit(SELECTIVE_SUSPEND, &tp->flags)) {
- if (netdev->flags & IFF_UP)
- tp->rtl_ops.autosuspend_en(tp, false);
- clear_bit(SELECTIVE_SUSPEND, &tp->flags);
- }
+ if (test_bit(SELECTIVE_SUSPEND, &tp->flags))
+ ret = rtl8152_runtime_resume(tp);
+ else
+ ret = rtl8152_system_resume(tp);
mutex_unlock(&tp->control);
- return 0;
+ return ret;
}
static int rtl8152_reset_resume(struct usb_interface *intf)
--
2.7.4
Move calling delay_autosuspend() in rtl8152_runtime_suspend(). Calling
delay_autosuspend() as late as possible.
The original flows are
1. check if the driver/device is busy now.
2. set wake events.
3. enter runtime suspend.
If the wake event occurs between (1) and (2), the device may miss it. Besides,
to avoid the runtime resume occurs after runtime suspend immediately, move the
checking to the end of rtl8152_runtime_suspend().
Signed-off-by: Hayes Wang <[email protected]>
---
drivers/net/usb/r8152.c | 12 +++++-------
1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 3257955..fb6fa6e 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -3752,13 +3752,6 @@ static int rtl8152_runtime_suspend(struct r8152 *tp)
if (netif_running(netdev) && test_bit(WORK_ENABLE, &tp->flags)) {
u32 rcr = 0;
- if (delay_autosuspend(tp)) {
- clear_bit(SELECTIVE_SUSPEND, &tp->flags);
- smp_mb__after_atomic();
- ret = -EBUSY;
- goto out1;
- }
-
if (netif_carrier_ok(netdev)) {
u32 ocp_data;
@@ -3792,6 +3785,11 @@ static int rtl8152_runtime_suspend(struct r8152 *tp)
ocp_write_dword(tp, MCU_TYPE_PLA, PLA_RCR, rcr);
napi_enable(napi);
}
+
+ if (delay_autosuspend(tp)) {
+ rtl8152_runtime_resume(tp);
+ ret = -EBUSY;
+ }
}
out1:
--
2.7.4
Am Montag, den 12.06.2017, 16:21 +0800 schrieb Hayes Wang:
> Split rtl8152_resume() into rtl8152_runtime_resume() and
> rtl8152_system_resume().
>
> Signed-off-by: Hayes Wang <[email protected]>
> ---
> drivers/net/usb/r8152.c | 99 ++++++++++++++++++++++++++++++-------------------
> 1 file changed, 61 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index 5a02053..3257955 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c
> @@ -3686,6 +3686,61 @@ static bool delay_autosuspend(struct r8152 *tp)
> return false;
> }
>
> +static int rtl8152_runtime_resume(struct r8152 *tp)
> +{
> + struct net_device *netdev = tp->netdev;
> +
> + if (netif_running(netdev) && netdev->flags & IFF_UP) {
> + struct napi_struct *napi = &tp->napi;
> +
> + tp->rtl_ops.autosuspend_en(tp, false);
> + napi_disable(napi);
> + set_bit(WORK_ENABLE, &tp->flags);
> +
> + if (netif_carrier_ok(netdev)) {
> + if (rtl8152_get_speed(tp) & LINK_STATUS) {
> + rtl_start_rx(tp);
> + } else {
> + netif_carrier_off(netdev);
> + tp->rtl_ops.disable(tp);
> + netif_info(tp, link, netdev, "linking down\n");
> + }
> + }
> +
> + napi_enable(napi);
> + clear_bit(SELECTIVE_SUSPEND, &tp->flags);
> + smp_mb__after_atomic();
> +
> + if (!list_empty(&tp->rx_done))
> + napi_schedule(&tp->napi);
> +
> + usb_submit_urb(tp->intr_urb, GFP_KERNEL);
If you ever built a device with included storage, this can deadlock,
as you may want to wake up a device for memory that is needed to wake
up a device. Use GFP_NOIO in resume() and reset_resume(), always.
Regards
Oliver
Oliver Neukum [mailto:[email protected]]
> Sent: Monday, June 12, 2017 8:33 PM
[...]
> > + usb_submit_urb(tp->intr_urb, GFP_KERNEL);
>
> If you ever built a device with included storage, this can deadlock,
> as you may want to wake up a device for memory that is needed to wake
> up a device. Use GFP_NOIO in resume() and reset_resume(), always.
I would change it. Thanks.
Best Regards,
Hayes
v2:
For #1, replace GFP_KERNEL with GFP_NOIO for usb_submit_urb().
v1:
Improve the flow about runtime suspend/resume and make the code
easy to read.
Hayes Wang (2):
r8152: split rtl8152_resume function
r8152: move calling delay_autosuspend function
drivers/net/usb/r8152.c | 107 ++++++++++++++++++++++++++++--------------------
1 file changed, 62 insertions(+), 45 deletions(-)
--
2.7.4
Split rtl8152_resume() into rtl8152_runtime_resume() and
rtl8152_system_resume().
Besides, replace GFP_KERNEL with GFP_NOIO for usb_submit_urb().
Signed-off-by: Hayes Wang <[email protected]>
---
drivers/net/usb/r8152.c | 99 ++++++++++++++++++++++++++++++-------------------
1 file changed, 61 insertions(+), 38 deletions(-)
diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 5a02053..2d238b5 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -3686,6 +3686,61 @@ static bool delay_autosuspend(struct r8152 *tp)
return false;
}
+static int rtl8152_runtime_resume(struct r8152 *tp)
+{
+ struct net_device *netdev = tp->netdev;
+
+ if (netif_running(netdev) && netdev->flags & IFF_UP) {
+ struct napi_struct *napi = &tp->napi;
+
+ tp->rtl_ops.autosuspend_en(tp, false);
+ napi_disable(napi);
+ set_bit(WORK_ENABLE, &tp->flags);
+
+ if (netif_carrier_ok(netdev)) {
+ if (rtl8152_get_speed(tp) & LINK_STATUS) {
+ rtl_start_rx(tp);
+ } else {
+ netif_carrier_off(netdev);
+ tp->rtl_ops.disable(tp);
+ netif_info(tp, link, netdev, "linking down\n");
+ }
+ }
+
+ napi_enable(napi);
+ clear_bit(SELECTIVE_SUSPEND, &tp->flags);
+ smp_mb__after_atomic();
+
+ if (!list_empty(&tp->rx_done))
+ napi_schedule(&tp->napi);
+
+ usb_submit_urb(tp->intr_urb, GFP_NOIO);
+ } else {
+ if (netdev->flags & IFF_UP)
+ tp->rtl_ops.autosuspend_en(tp, false);
+
+ clear_bit(SELECTIVE_SUSPEND, &tp->flags);
+ }
+
+ return 0;
+}
+
+static int rtl8152_system_resume(struct r8152 *tp)
+{
+ struct net_device *netdev = tp->netdev;
+
+ netif_device_attach(netdev);
+
+ if (netif_running(netdev) && netdev->flags & IFF_UP) {
+ tp->rtl_ops.up(tp);
+ netif_carrier_off(netdev);
+ set_bit(WORK_ENABLE, &tp->flags);
+ usb_submit_urb(tp->intr_urb, GFP_NOIO);
+ }
+
+ return 0;
+}
+
static int rtl8152_runtime_suspend(struct r8152 *tp)
{
struct net_device *netdev = tp->netdev;
@@ -3784,50 +3839,18 @@ static int rtl8152_suspend(struct usb_interface *intf, pm_message_t message)
static int rtl8152_resume(struct usb_interface *intf)
{
struct r8152 *tp = usb_get_intfdata(intf);
- struct net_device *netdev = tp->netdev;
+ int ret;
mutex_lock(&tp->control);
- if (!test_bit(SELECTIVE_SUSPEND, &tp->flags))
- netif_device_attach(netdev);
-
- if (netif_running(netdev) && netdev->flags & IFF_UP) {
- if (test_bit(SELECTIVE_SUSPEND, &tp->flags)) {
- struct napi_struct *napi = &tp->napi;
-
- tp->rtl_ops.autosuspend_en(tp, false);
- napi_disable(napi);
- set_bit(WORK_ENABLE, &tp->flags);
- if (netif_carrier_ok(netdev)) {
- if (rtl8152_get_speed(tp) & LINK_STATUS) {
- rtl_start_rx(tp);
- } else {
- netif_carrier_off(netdev);
- tp->rtl_ops.disable(tp);
- netif_info(tp, link, netdev,
- "linking down\n");
- }
- }
- napi_enable(napi);
- clear_bit(SELECTIVE_SUSPEND, &tp->flags);
- smp_mb__after_atomic();
- if (!list_empty(&tp->rx_done))
- napi_schedule(&tp->napi);
- } else {
- tp->rtl_ops.up(tp);
- netif_carrier_off(netdev);
- set_bit(WORK_ENABLE, &tp->flags);
- }
- usb_submit_urb(tp->intr_urb, GFP_KERNEL);
- } else if (test_bit(SELECTIVE_SUSPEND, &tp->flags)) {
- if (netdev->flags & IFF_UP)
- tp->rtl_ops.autosuspend_en(tp, false);
- clear_bit(SELECTIVE_SUSPEND, &tp->flags);
- }
+ if (test_bit(SELECTIVE_SUSPEND, &tp->flags))
+ ret = rtl8152_runtime_resume(tp);
+ else
+ ret = rtl8152_system_resume(tp);
mutex_unlock(&tp->control);
- return 0;
+ return ret;
}
static int rtl8152_reset_resume(struct usb_interface *intf)
--
2.7.4
Move calling delay_autosuspend() in rtl8152_runtime_suspend(). Calling
delay_autosuspend() as late as possible.
The original flows are
1. check if the driver/device is busy now.
2. set wake events.
3. enter runtime suspend.
If the wake event occurs between (1) and (2), the device may miss it. Besides,
to avoid the runtime resume occurs after runtime suspend immediately, move the
checking to the end of rtl8152_runtime_suspend().
Signed-off-by: Hayes Wang <[email protected]>
---
drivers/net/usb/r8152.c | 12 +++++-------
1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 2d238b5..b916418 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -3752,13 +3752,6 @@ static int rtl8152_runtime_suspend(struct r8152 *tp)
if (netif_running(netdev) && test_bit(WORK_ENABLE, &tp->flags)) {
u32 rcr = 0;
- if (delay_autosuspend(tp)) {
- clear_bit(SELECTIVE_SUSPEND, &tp->flags);
- smp_mb__after_atomic();
- ret = -EBUSY;
- goto out1;
- }
-
if (netif_carrier_ok(netdev)) {
u32 ocp_data;
@@ -3792,6 +3785,11 @@ static int rtl8152_runtime_suspend(struct r8152 *tp)
ocp_write_dword(tp, MCU_TYPE_PLA, PLA_RCR, rcr);
napi_enable(napi);
}
+
+ if (delay_autosuspend(tp)) {
+ rtl8152_runtime_resume(tp);
+ ret = -EBUSY;
+ }
}
out1:
--
2.7.4
From: Hayes Wang <[email protected]>
Date: Tue, 13 Jun 2017 15:14:38 +0800
> v2:
> For #1, replace GFP_KERNEL with GFP_NOIO for usb_submit_urb().
>
> v1:
> Improve the flow about runtime suspend/resume and make the code
> easy to read.
Series applied.
David Miller [mailto:[email protected]]
> Sent: Wednesday, June 14, 2017 1:02 AM
> > v2:
> > For #1, replace GFP_KERNEL with GFP_NOIO for usb_submit_urb().
> >
> > v1:
> > Improve the flow about runtime suspend/resume and make the code
> > easy to read.
>
> Series applied.
Excuse me. I don't see these patches in net-next repository. Where could I find them?
Best Regards,
Hayes
From: Hayes Wang <[email protected]>
Date: Fri, 16 Jun 2017 03:29:01 +0000
> David Miller [mailto:[email protected]]
>> Sent: Wednesday, June 14, 2017 1:02 AM
>> > v2:
>> > For #1, replace GFP_KERNEL with GFP_NOIO for usb_submit_urb().
>> >
>> > v1:
>> > Improve the flow about runtime suspend/resume and make the code
>> > easy to read.
>>
>> Series applied.
>
> Excuse me. I don't see these patches in net-next repository. Where could I find them?
Sorry, I don't know how that happened.
It should be there now.
Thanks.