[PATCH net-next 0/2] r8152: adjust runtime suspend/resume

STALE3411d SUPERSEDED

Revision v1 of 2 in this series.

11 messages, 3 authors, 2017-06-16 · open the first message on its own page

[PATCH net-next 0/2] r8152: adjust runtime suspend/resume

From: Hayes Wang <hidden>
Date: 2017-06-12 08:21:37

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

[PATCH net-next 1/2] r8152: split rtl8152_resume function

From: Hayes Wang <hidden>
Date: 2017-06-12 08:21:42

Split rtl8152_resume() into rtl8152_runtime_resume() and
rtl8152_system_resume().

Signed-off-by: Hayes Wang <redacted>
---
 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

--
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 net-next 2/2] r8152: move calling delay_autosuspend function

From: Hayes Wang <hidden>
Date: 2017-06-12 08:21:47

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 <redacted>
---
 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

Re: [PATCH net-next 1/2] r8152: split rtl8152_resume function

From: Oliver Neukum <oneukum@suse.com>
Date: 2017-06-12 12:35:25

Am Montag, den 12.06.2017, 16:21 +0800 schrieb Hayes Wang:

quoted hunk

Split rtl8152_resume() into rtl8152_runtime_resume() and
rtl8152_system_resume().

Signed-off-by: Hayes Wang <redacted>
---
 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

RE: [PATCH net-next 1/2] r8152: split rtl8152_resume function

From: Hayes Wang <hidden>
Date: 2017-06-13 02:27:33

Oliver Neukum [mailto:oneukum@suse.com]

Sent: Monday, June 12, 2017 8:33 PM

[...]

quoted

+               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

[PATCH net-next v2 0/2] r8152: adjust runtime suspend/resume

From: Hayes Wang <hidden>
Date: 2017-06-13 07:14:51

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

[PATCH net-next v2 1/2] r8152: split rtl8152_resume function

From: Hayes Wang <hidden>
Date: 2017-06-13 07:14:56

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 <redacted>
---
 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

[PATCH net-next v2 2/2] r8152: move calling delay_autosuspend function

From: Hayes Wang <hidden>
Date: 2017-06-13 07:15:02

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 <redacted>
---
 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

Re: [PATCH net-next v2 0/2] r8152: adjust runtime suspend/resume

From: David Miller <davem@davemloft.net>
Date: 2017-06-13 17:01:58

From: Hayes Wang <redacted>
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.

RE: [PATCH net-next v2 0/2] r8152: adjust runtime suspend/resume

From: Hayes Wang <hidden>
Date: 2017-06-16 03:29:10

David Miller [mailto:davem@davemloft.net]

Sent: Wednesday, June 14, 2017 1:02 AM

quoted

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

Re: [PATCH net-next v2 0/2] r8152: adjust runtime suspend/resume

From: David Miller <davem@davemloft.net>
Date: 2017-06-16 15:37:51

From: Hayes Wang <redacted>
Date: Fri, 16 Jun 2017 03:29:01 +0000

David Miller [mailto:davem@davemloft.net]

quoted

Sent: Wednesday, June 14, 2017 1:02 AM

quoted

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.

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help