[PATCH net-next 0/7] r8152: adjust the code

[PATCH net-next 0/7] r8152: adjust the code

[Hayes Wang]

Code adjustment.

Hayes Wang (7):
  r8152: adjust rx_bottom
  r8152: adjust lpm timer
  r8152: check linking status with netif_carrier_ok
  r8152: check RTL8152_UNPLUG for rtl8152_close
  r8152: adjust the link feed for hw_features
  r8152: replace get_protocol with vlan_get_protocol
  r8152: use BIT macro

 drivers/net/usb/r8152.c | 99 +++++++++++++++++++++++--------------------------
 1 file changed, 46 insertions(+), 53 deletions(-)

-- 
2.1.0

[PATCH net-next 1/7] r8152: adjust rx_bottom

[Hayes Wang]

If a error occurs when submitting rx, skip the remaining submissions
and try to submit them again next time.

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

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 2e22442..78a8917 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -1655,7 +1655,7 @@ static int rx_bottom(struct r8152 *tp, int budget)
 {
 	unsigned long flags;
 	struct list_head *cursor, *next, rx_queue;
-	int work_done = 0;
+	int ret = 0, work_done = 0;
 
 	if (!skb_queue_empty(&tp->rx_queue)) {
 		while (work_done < budget) {
@@ -1746,7 +1746,18 @@ find_next_rx:
 		}
 
 submit:
-		r8152_submit_rx(tp, agg, GFP_ATOMIC);
+		if (!ret) {
+			ret = r8152_submit_rx(tp, agg, GFP_ATOMIC);
+		} else {
+			urb->actual_length = 0;
+			list_add_tail(&agg->list, next);
+		}
+	}
+
+	if (!list_empty(&rx_queue)) {
+		spin_lock_irqsave(&tp->rx_lock, flags);
+		list_splice_tail(&rx_queue, &tp->rx_done);
+		spin_unlock_irqrestore(&tp->rx_lock, flags);
 	}
 
 out1:
-- 
2.1.0

[PATCH net-next 2/7] r8152: adjust lpm timer

[Hayes Wang]

Set LPM timer to 500us, except for RTL_VER_04 which doesn't link at
USB 3.0.

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

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 78a8917..3ee9d06 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -3274,10 +3274,10 @@ static void r8153_init(struct r8152 *tp)
 
 	ocp_data = ocp_read_byte(tp, MCU_TYPE_USB, USB_LPM_CTRL);
 	ocp_data &= ~LPM_TIMER_MASK;
-	if (tp->udev->speed == USB_SPEED_SUPER)
-		ocp_data |= LPM_TIMER_500US;
-	else
+	if (tp->version == RTL_VER_04 && tp->udev->speed != USB_SPEED_SUPER)
 		ocp_data |= LPM_TIMER_500MS;
+	else
+		ocp_data |= LPM_TIMER_500US;
 	ocp_write_byte(tp, MCU_TYPE_USB, USB_LPM_CTRL, ocp_data);
 
 	ocp_data = ocp_read_word(tp, MCU_TYPE_USB, USB_AFE_CTRL2);
-- 
2.1.0

[PATCH net-next 3/7] r8152: check linking status with netif_carrier_ok

[Hayes Wang]

Replace (tp->speed & LINK_STATUS) with netif_carrier_ok().

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

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 3ee9d06..21f853f 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -581,7 +581,6 @@ struct r8152 {
 	u16 ocp_base;
 	u8 *intr_buff;
 	u8 version;
-	u8 speed;
 };
 
 enum rtl_version {
@@ -1169,12 +1168,12 @@ static void intr_callback(struct urb *urb)
 
 	d = urb->transfer_buffer;
 	if (INTR_LINK & __le16_to_cpu(d[0])) {
-		if (!(tp->speed & LINK_STATUS)) {
+		if (!netif_carrier_ok(tp->netdev)) {
 			set_bit(RTL8152_LINK_CHG, &tp->flags);
 			schedule_delayed_work(&tp->schedule, 0);
 		}
 	} else {
-		if (tp->speed & LINK_STATUS) {
+		if (netif_carrier_ok(tp->netdev)) {
 			set_bit(RTL8152_LINK_CHG, &tp->flags);
 			schedule_delayed_work(&tp->schedule, 0);
 		}
@@ -1906,7 +1905,7 @@ static void rtl8152_set_rx_mode(struct net_device *netdev)
 {
 	struct r8152 *tp = netdev_priv(netdev);
 
-	if (tp->speed & LINK_STATUS) {
+	if (netif_carrier_ok(netdev)) {
 		set_bit(RTL8152_SET_RX_MODE, &tp->flags);
 		schedule_delayed_work(&tp->schedule, 0);
 	}
@@ -2936,21 +2935,20 @@ static void set_carrier(struct r8152 *tp)
 	speed = rtl8152_get_speed(tp);
 
 	if (speed & LINK_STATUS) {
-		if (!(tp->speed & LINK_STATUS)) {
+		if (!netif_carrier_ok(netdev)) {
 			tp->rtl_ops.enable(tp);
 			set_bit(RTL8152_SET_RX_MODE, &tp->flags);
 			netif_carrier_on(netdev);
 			rtl_start_rx(tp);
 		}
 	} else {
-		if (tp->speed & LINK_STATUS) {
+		if (netif_carrier_ok(netdev)) {
 			netif_carrier_off(netdev);
 			napi_disable(&tp->napi);
 			tp->rtl_ops.disable(tp);
 			napi_enable(&tp->napi);
 		}
 	}
-	tp->speed = speed;
 }
 
 static void rtl_work_func_t(struct work_struct *work)
@@ -2982,7 +2980,7 @@ static void rtl_work_func_t(struct work_struct *work)
 
 	/* don't schedule napi before linking */
 	if (test_bit(SCHEDULE_NAPI, &tp->flags) &&
-	    (tp->speed & LINK_STATUS)) {
+	    netif_carrier_ok(tp->netdev)) {
 		clear_bit(SCHEDULE_NAPI, &tp->flags);
 		napi_schedule(&tp->napi);
 	}
@@ -3005,8 +3003,7 @@ static int rtl8152_open(struct net_device *netdev)
 	if (res)
 		goto out;
 
-	/* set speed to 0 to avoid autoresume try to submit rx */
-	tp->speed = 0;
+	netif_carrier_off(netdev);
 
 	res = usb_autopm_get_interface(tp->intf);
 	if (res < 0) {
@@ -3023,7 +3020,7 @@ static int rtl8152_open(struct net_device *netdev)
 		cancel_delayed_work_sync(&tp->schedule);
 
 		/* disable the tx/rx, if the workqueue has enabled them. */
-		if (tp->speed & LINK_STATUS)
+		if (netif_carrier_ok(netdev))
 			tp->rtl_ops.disable(tp);
 	}
 
@@ -3032,7 +3029,6 @@ static int rtl8152_open(struct net_device *netdev)
 	rtl8152_set_speed(tp, AUTONEG_ENABLE,
 			  tp->mii.supports_gmii ? SPEED_1000 : SPEED_100,
 			  DUPLEX_FULL);
-	tp->speed = 0;
 	netif_carrier_off(netdev);
 	netif_start_queue(netdev);
 	set_bit(WORK_ENABLE, &tp->flags);
@@ -3358,7 +3354,7 @@ static int rtl8152_resume(struct usb_interface *intf)
 			rtl_runtime_suspend_enable(tp, false);
 			clear_bit(SELECTIVE_SUSPEND, &tp->flags);
 			set_bit(WORK_ENABLE, &tp->flags);
-			if (tp->speed & LINK_STATUS)
+			if (netif_carrier_ok(tp->netdev))
 				rtl_start_rx(tp);
 		} else {
 			tp->rtl_ops.up(tp);
@@ -3366,7 +3362,6 @@ static int rtl8152_resume(struct usb_interface *intf)
 					  tp->mii.supports_gmii ?
 					  SPEED_1000 : SPEED_100,
 					  DUPLEX_FULL);
-			tp->speed = 0;
 			netif_carrier_off(tp->netdev);
 			set_bit(WORK_ENABLE, &tp->flags);
 		}
-- 
2.1.0

[PATCH net-next 4/7] r8152: check RTL8152_UNPLUG for rtl8152_close

[Hayes Wang]

It is unnecessary to accress the hw register if the device is unplugged.

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

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 21f853f..9be642e 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -3064,7 +3064,7 @@ static int rtl8152_close(struct net_device *netdev)
 	netif_stop_queue(netdev);
 
 	res = usb_autopm_get_interface(tp->intf);
-	if (res < 0) {
+	if (res < 0 || test_bit(RTL8152_UNPLUG, &tp->flags)) {
 		rtl_drop_queued_tx(tp);
 		rtl_stop_rx(tp);
 	} else {
-- 
2.1.0

[PATCH net-next 5/7] r8152: adjust the link feed for hw_features

[Hayes Wang]

Keep NETIF_F_HW_VLAN_CTAG_RX and NETIF_F_HW_VLAN_CTAG_TX at the
same line.

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

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 9be642e..f66ffbd 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -3926,8 +3926,7 @@ static int rtl8152_probe(struct usb_interface *intf,
 	netdev->hw_features = NETIF_F_RXCSUM | NETIF_F_IP_CSUM | NETIF_F_SG |
 			      NETIF_F_TSO | NETIF_F_FRAGLIST |
 			      NETIF_F_IPV6_CSUM | NETIF_F_TSO6 |
-			      NETIF_F_HW_VLAN_CTAG_RX |
-			      NETIF_F_HW_VLAN_CTAG_TX;
+			      NETIF_F_HW_VLAN_CTAG_RX | NETIF_F_HW_VLAN_CTAG_TX;
 	netdev->vlan_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_TSO |
 				NETIF_F_HIGHDMA | NETIF_F_FRAGLIST |
 				NETIF_F_IPV6_CSUM | NETIF_F_TSO6;
-- 
2.1.0

[PATCH net-next 6/7] r8152: replace get_protocol with vlan_get_protocol

[Hayes Wang]

vlan_get_protocol() has been defined and use it to replace
get_protocol().

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

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index f66ffbd..46440ed 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -1342,18 +1342,6 @@ static struct tx_agg *r8152_get_tx_agg(struct r8152 *tp)
 	return agg;
 }
 
-static inline __be16 get_protocol(struct sk_buff *skb)
-{
-	__be16 protocol;
-
-	if (skb->protocol == htons(ETH_P_8021Q))
-		protocol = vlan_eth_hdr(skb)->h_vlan_encapsulated_proto;
-	else
-		protocol = skb->protocol;
-
-	return protocol;
-}
-
 /* r8152_csum_workaround()
  * The hw limites the value the transport offset. When the offset is out of the
  * range, calculate the checksum by sw.
@@ -1459,7 +1447,7 @@ static int r8152_tx_csum(struct r8152 *tp, struct tx_desc *desc,
 			goto unavailable;
 		}
 
-		switch (get_protocol(skb)) {
+		switch (vlan_get_protocol(skb)) {
 		case htons(ETH_P_IP):
 			opts1 |= GTSENDV4;
 			break;
@@ -1490,7 +1478,7 @@ static int r8152_tx_csum(struct r8152 *tp, struct tx_desc *desc,
 			goto unavailable;
 		}
 
-		switch (get_protocol(skb)) {
+		switch (vlan_get_protocol(skb)) {
 		case htons(ETH_P_IP):
 			opts2 |= IPV4_CS;
 			ip_protocol = ip_hdr(skb)->protocol;
-- 
2.1.0

[PATCH net-next 7/7] r8152: use BIT macro

[Hayes Wang]

Use BIT macro to replace (1 << bits).

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

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 46440ed..c3a0224 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -489,16 +489,16 @@ struct rx_desc {
 #define RX_LEN_MASK			0x7fff
 
 	__le32 opts2;
-#define RD_UDP_CS			(1 << 23)
-#define RD_TCP_CS			(1 << 22)
-#define RD_IPV6_CS			(1 << 20)
-#define RD_IPV4_CS			(1 << 19)
+#define RD_UDP_CS			BIT(23)
+#define RD_TCP_CS			BIT(22)
+#define RD_IPV6_CS			BIT(20)
+#define RD_IPV4_CS			BIT(19)
 
 	__le32 opts3;
-#define IPF				(1 << 23) /* IP checksum fail */
-#define UDPF				(1 << 22) /* UDP checksum fail */
-#define TCPF				(1 << 21) /* TCP checksum fail */
-#define RX_VLAN_TAG			(1 << 16)
+#define IPF				BIT(23) /* IP checksum fail */
+#define UDPF				BIT(22) /* UDP checksum fail */
+#define TCPF				BIT(21) /* TCP checksum fail */
+#define RX_VLAN_TAG			BIT(16)
 
 	__le32 opts4;
 	__le32 opts5;
@@ -507,24 +507,24 @@ struct rx_desc {
 
 struct tx_desc {
 	__le32 opts1;
-#define TX_FS			(1 << 31) /* First segment of a packet */
-#define TX_LS			(1 << 30) /* Final segment of a packet */
-#define GTSENDV4		(1 << 28)
-#define GTSENDV6		(1 << 27)
+#define TX_FS			BIT(31) /* First segment of a packet */
+#define TX_LS			BIT(30) /* Final segment of a packet */
+#define GTSENDV4		BIT(28)
+#define GTSENDV6		BIT(27)
 #define GTTCPHO_SHIFT		18
 #define GTTCPHO_MAX		0x7fU
 #define TX_LEN_MAX		0x3ffffU
 
 	__le32 opts2;
-#define UDP_CS			(1 << 31) /* Calculate UDP/IP checksum */
-#define TCP_CS			(1 << 30) /* Calculate TCP/IP checksum */
-#define IPV4_CS			(1 << 29) /* Calculate IPv4 checksum */
-#define IPV6_CS			(1 << 28) /* Calculate IPv6 checksum */
+#define UDP_CS			BIT(31) /* Calculate UDP/IP checksum */
+#define TCP_CS			BIT(30) /* Calculate TCP/IP checksum */
+#define IPV4_CS			BIT(29) /* Calculate IPv4 checksum */
+#define IPV6_CS			BIT(28) /* Calculate IPv6 checksum */
 #define MSS_SHIFT		17
 #define MSS_MAX			0x7ffU
 #define TCPHO_SHIFT		17
 #define TCPHO_MAX		0x7ffU
-#define TX_VLAN_TAG			(1 << 16)
+#define TX_VLAN_TAG		BIT(16)
 };
 
 struct r8152;
-- 
2.1.0

Re: [PATCH net-next 1/7] r8152: adjust rx_bottom

[Scott Feldman]

On Sun, Jan 18, 2015 at 11:13 PM, Hayes Wang <hayeswang@realtek.com> wrote:
> If a error occurs when submitting rx, skip the remaining submissions
> and try to submit them again next time.
>
> Signed-off-by: Hayes Wang <hayeswang@realtek.com>
> ---
>  drivers/net/usb/r8152.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index 2e22442..78a8917 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c
> @@ -1655,7 +1655,7 @@ static int rx_bottom(struct r8152 *tp, int budget)
>  {
>         unsigned long flags;
>         struct list_head *cursor, *next, rx_queue;
> -       int work_done = 0;
> +       int ret = 0, work_done = 0;
>
>         if (!skb_queue_empty(&tp->rx_queue)) {
>                 while (work_done < budget) {
> @@ -1746,7 +1746,18 @@ find_next_rx:
>                 }
>
>  submit:
> -               r8152_submit_rx(tp, agg, GFP_ATOMIC);
> +               if (!ret) {
> +                       ret = r8152_submit_rx(tp, agg, GFP_ATOMIC);
> +               } else {
> +                       urb->actual_length = 0;
> +                       list_add_tail(&agg->list, next);

Do you need a spin_lock_irqsave(&tp->rx_lock, flags) around this?

> +               }
> +       }
> +
> +       if (!list_empty(&rx_queue)) {
> +               spin_lock_irqsave(&tp->rx_lock, flags);
> +               list_splice_tail(&rx_queue, &tp->rx_done);
> +               spin_unlock_irqrestore(&tp->rx_lock, flags);
>         }
>
>  out1:
> --
> 2.1.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net-next 1/7] r8152: adjust rx_bottom

[David Miller]

From: Scott Feldman <sfeldma@gmail.com>
Date: Mon, 19 Jan 2015 00:03:42 -0800

> On Sun, Jan 18, 2015 at 11:13 PM, Hayes Wang <hayeswang@realtek.com> wrote:
>> @@ -1746,7 +1746,18 @@ find_next_rx:
>>                 }
>>
>>  submit:
>> -               r8152_submit_rx(tp, agg, GFP_ATOMIC);
>> +               if (!ret) {
>> +                       ret = r8152_submit_rx(tp, agg, GFP_ATOMIC);
>> +               } else {
>> +                       urb->actual_length = 0;
>> +                       list_add_tail(&agg->list, next);
> 
> Do you need a spin_lock_irqsave(&tp->rx_lock, flags) around this?

Indeed, and rtl_start_rx() seems to also access agg->list without
proper locking.

Re: [PATCH net-next 1/7] r8152: adjust rx_bottom

[David Miller]

From: Scott Feldman <sfeldma-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Date: Mon, 19 Jan 2015 00:03:42 -0800

> On Sun, Jan 18, 2015 at 11:13 PM, Hayes Wang <hayeswang-Rasf1IRRPZFBDgjK7y7TUQ@public.gmane.org> wrote:
>> @@ -1746,7 +1746,18 @@ find_next_rx:
>>                 }
>>
>>  submit:
>> -               r8152_submit_rx(tp, agg, GFP_ATOMIC);
>> +               if (!ret) {
>> +                       ret = r8152_submit_rx(tp, agg, GFP_ATOMIC);
>> +               } else {
>> +                       urb->actual_length = 0;
>> +                       list_add_tail(&agg->list, next);
> 
> Do you need a spin_lock_irqsave(&tp->rx_lock, flags) around this?

Indeed, and rtl_start_rx() seems to also access agg->list without
proper locking.
--
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-next 1/7] r8152: adjust rx_bottom

[Hayes Wang]

 David Miller [mailto:davem@davemloft.net] 
> Sent: Tuesday, January 20, 2015 5:14 AM
[...]
> >> -               r8152_submit_rx(tp, agg, GFP_ATOMIC);
> >> +               if (!ret) {
> >> +                       ret = r8152_submit_rx(tp, agg, GFP_ATOMIC);
> >> +               } else {
> >> +                       urb->actual_length = 0;
> >> +                       list_add_tail(&agg->list, next);
> > 
> > Do you need a spin_lock_irqsave(&tp->rx_lock, flags) around this?
> 
> Indeed, and rtl_start_rx() seems to also access agg->list without
> proper locking.

It is unnecessary because I deal with them in a local list_head. My steps are
   1. Move the whole list from tp->rx_done to local rx_queue. (with spin lock)
   2. dequeue/enqueue the lists in rx_queue.
   3. Move the lists in rx_queue to tp->rx_done if it is necessary. (spin lock)
For step 2, it wouldn't have race, because the list_head is local and no other
function would change it. Therefore, I don't think it needs the spin lock.

The rtl_start_rx() also uses the similar way.
 
Best Regards,
Hayes

Re: [PATCH net-next 1/7] r8152: adjust rx_bottom

[David Miller]

From: Hayes Wang <hayeswang@realtek.com>
Date: Tue, 20 Jan 2015 02:48:50 +0000

>> >> +                       urb->actual_length = 0;
>> >> +                       list_add_tail(&agg->list, next);
>> > 
>> > Do you need a spin_lock_irqsave(&tp->rx_lock, flags) around this?
>> 
>> Indeed, and rtl_start_rx() seems to also access agg->list without
>> proper locking.
> 
> It is unnecessary because I deal with them in a local list_head. My steps are
>    1. Move the whole list from tp->rx_done to local rx_queue. (with spin lock)
>    2. dequeue/enqueue the lists in rx_queue.
>    3. Move the lists in rx_queue to tp->rx_done if it is necessary. (spin lock)
> For step 2, it wouldn't have race, because the list_head is local and no other
> function would change it. Therefore, I don't think it needs the spin lock.
> 
> The rtl_start_rx() also uses the similar way.

agg->list is not local, you have to use a spinlock to protect
modifications to it, some other sites which modify agg->list do take
the lock properly.

You cannot modify a list like agg->list without proper locking.

RE: [PATCH net-next 1/7] r8152: adjust rx_bottom

[Hayes Wang]

 David Miller [mailto:davem@davemloft.net] 
> Sent: Tuesday, January 20, 2015 10:52 AM
[...]
> agg->list is not local, you have to use a spinlock to protect
> modifications to it, some other sites which modify agg->list do take
> the lock properly.
> 
> You cannot modify a list like agg->list without proper locking.

Excuse me. I don't understand.

Before step1
   tp_rx_done->listA->listB->listC->listD->...
   rx_queue->
Because the other function would chage tp->rx_done,
I need move the lists with spin lock.

After step1
   tp_rx_done->
   rx_queue->listA->listB->listC->listD->...

Now I dequeue one of the lists from the list_head and
deal with it.
   tp_rx_done->
   rx_queue->listA->listC->listD->...
                    listB

Then, if I want to put it back to rx_queue, I have to
use spin lock. Why? No other function would chage
rx_queue and the items in it.
 
Best Regards,
Hayes

Re: [PATCH net-next 1/7] r8152: adjust rx_bottom

[David Miller]

From: Hayes Wang <hayeswang@realtek.com>
Date: Tue, 20 Jan 2015 03:24:52 +0000

>  David Miller [mailto:davem@davemloft.net] 
>> Sent: Tuesday, January 20, 2015 10:52 AM
> [...]
>> agg->list is not local, you have to use a spinlock to protect
>> modifications to it, some other sites which modify agg->list do take
>> the lock properly.
>> 
>> You cannot modify a list like agg->list without proper locking.
> 
> Excuse me. I don't understand.
> 
> Before step1
>    tp_rx_done->listA->listB->listC->listD->...
>    rx_queue->
> Because the other function would chage tp->rx_done,
> I need move the lists with spin lock.
> 
> After step1
>    tp_rx_done->
>    rx_queue->listA->listB->listC->listD->...
> 
> Now I dequeue one of the lists from the list_head and
> deal with it.
>    tp_rx_done->
>    rx_queue->listA->listC->listD->...
>                     listB
> 
> Then, if I want to put it back to rx_queue, I have to
> use spin lock. Why? No other function would chage
> rx_queue and the items in it.

What keeps rtl_start_rx() from running in parallel with
r8152_submit_rx(), or any other accessor of the RX agg->list?

You also keep using different terminology from me when
discussing what lists do or do not need protection, and that
is going to make it difficult for anyone to follow our
conversation at all.

We're talking specifically about RX agg->list objects and
whether access to them need synchronization or not.

RE: [PATCH net-next 1/7] r8152: adjust rx_bottom

[Hayes Wang]

 David Miller [mailto:davem@davemloft.net] 
> Sent: Sunday, January 25, 2015 2:44 PM
[...]
> What keeps rtl_start_rx() from running in parallel with
> r8152_submit_rx(), or any other accessor of the RX agg->list?

Forgive my poor English. I would try to describe them clearly.
The steps about the rx agg->list would be
1. carrier on or autoresume occurs.
2. Call rtl_start_rx().
3. Rx agg->list flows between device and tp->rx_done.
4. carrier off or autosuspend occurs.
5. call rtl_stop_rx().

The rtl_start_rx() would only be called when the linking
status is changed from off to on or the auto resume occurs.
And rtl_start_rx() would reinitialize the tp->rx_done and
all of the rx agg->list. After step 2, the rx agg->list
would flow between the usb host controller and the driver.
If r8152_submit_rx() is success, the driver wouldn't own the
rx agg->list until it is returned from the usb host controller.
If r8152_submit_rx() is fail, the driver would still own the
rx agg->list, and queue it to the tp->rx_done with spin lock
for next try.

If the status stays in step 3, only the rx_bottom() would submit
the rx agg. The rtl_start_rx() wouldn't be called suddenly,
unless the linking down or auto suspend occur first and linking
on or auto resume occur again. If linking down or auto suspend
occur, rtl_stop_rx() would be called (step 5). After this step,
rx_bottom() wouldn't submit rx, and all rx agg->list would stop
flowing. That is, the tp->rx_done and all rx agg->list wouldn't
be changed until the next rtl_start_rx() is called.

Therefore, the flow for each rx agg->list would be
a. submittd by rtl_start_rx().
b. goto step c if success, otherwise goto step d.
c. completed by usb host controller.
d. queued to tp->rx_done with spin lock.
e. dequeue from tp->rx_done with spin lock by rx_botoom().
f. goto step i if link down, otherwise goto step g.
g. submitted by rx_botoom().
h. goto step b.
i. goto step a if link on.

And the patch change the step g to g1.
g1. submitted by rx_botoom() if (!ret), otherwise goto step d.

Best Regards,
Hayes
> 
> You also keep using different terminology from me when
> discussing what lists do or do not need protection, and that
> is going to make it difficult for anyone to follow our
> conversation at all.
> 
> We're talking specifically about RX agg->list objects and
> whether access to them need synchronization or not.

RE: [PATCH net-next 1/7] r8152: adjust rx_bottom

[Hayes Wang]

 David Miller [mailto:davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org] 
> Sent: Sunday, January 25, 2015 2:44 PM
[...]
> What keeps rtl_start_rx() from running in parallel with
> r8152_submit_rx(), or any other accessor of the RX agg->list?

Forgive my poor English. I would try to describe them clearly.
The steps about the rx agg->list would be
1. carrier on or autoresume occurs.
2. Call rtl_start_rx().
3. Rx agg->list flows between device and tp->rx_done.
4. carrier off or autosuspend occurs.
5. call rtl_stop_rx().

The rtl_start_rx() would only be called when the linking
status is changed from off to on or the auto resume occurs.
And rtl_start_rx() would reinitialize the tp->rx_done and
all of the rx agg->list. After step 2, the rx agg->list
would flow between the usb host controller and the driver.
If r8152_submit_rx() is success, the driver wouldn't own the
rx agg->list until it is returned from the usb host controller.
If r8152_submit_rx() is fail, the driver would still own the
rx agg->list, and queue it to the tp->rx_done with spin lock
for next try.

If the status stays in step 3, only the rx_bottom() would submit
the rx agg. The rtl_start_rx() wouldn't be called suddenly,
unless the linking down or auto suspend occur first and linking
on or auto resume occur again. If linking down or auto suspend
occur, rtl_stop_rx() would be called (step 5). After this step,
rx_bottom() wouldn't submit rx, and all rx agg->list would stop
flowing. That is, the tp->rx_done and all rx agg->list wouldn't
be changed until the next rtl_start_rx() is called.

Therefore, the flow for each rx agg->list would be
a. submittd by rtl_start_rx().
b. goto step c if success, otherwise goto step d.
c. completed by usb host controller.
d. queued to tp->rx_done with spin lock.
e. dequeue from tp->rx_done with spin lock by rx_botoom().
f. goto step i if link down, otherwise goto step g.
g. submitted by rx_botoom().
h. goto step b.
i. goto step a if link on.

And the patch change the step g to g1.
g1. submitted by rx_botoom() if (!ret), otherwise goto step d.

Best Regards,
Hayes
> 
> You also keep using different terminology from me when
> discussing what lists do or do not need protection, and that
> is going to make it difficult for anyone to follow our
> conversation at all.
> 
> We're talking specifically about RX agg->list objects and
> whether access to them need synchronization or not.
--
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-next 1/7] r8152: adjust rx_bottom

[Scott Feldman]

On Mon, Jan 19, 2015 at 6:48 PM, Hayes Wang <hayeswang@realtek.com> wrote:
>  David Miller [mailto:davem@davemloft.net]
>> Sent: Tuesday, January 20, 2015 5:14 AM
> [...]
>> >> -               r8152_submit_rx(tp, agg, GFP_ATOMIC);
>> >> +               if (!ret) {
>> >> +                       ret = r8152_submit_rx(tp, agg, GFP_ATOMIC);
>> >> +               } else {
>> >> +                       urb->actual_length = 0;
>> >> +                       list_add_tail(&agg->list, next);
>> >
>> > Do you need a spin_lock_irqsave(&tp->rx_lock, flags) around this?
>>
>> Indeed, and rtl_start_rx() seems to also access agg->list without
>> proper locking.
>
> It is unnecessary because I deal with them in a local list_head. My steps are
>    1. Move the whole list from tp->rx_done to local rx_queue. (with spin lock)
>    2. dequeue/enqueue the lists in rx_queue.
>    3. Move the lists in rx_queue to tp->rx_done if it is necessary. (spin lock)
> For step 2, it wouldn't have race, because the list_head is local and no other
> function would change it. Therefore, I don't think it needs the spin lock.

Sorry guys, I think I made a mistake in my review and caused some
confusion/grief.

My mistake was getting the params to list_add_tail() backwards.  It's
list_add_tail(entry, head).  I saw list_add_tail(&agg->list, next) and
was fooled into thinking agg->list was the list getting appended with
the entry 'next'.  It's the opposite.  Duh.  So locking isn't needed
because the list is indeed local.

-scott

Re: [PATCH net-next 1/7] r8152: adjust rx_bottom

[Scott Feldman]

On Mon, Jan 19, 2015 at 6:48 PM, Hayes Wang <hayeswang-Rasf1IRRPZFBDgjK7y7TUQ@public.gmane.org> wrote:
>  David Miller [mailto:davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org]
>> Sent: Tuesday, January 20, 2015 5:14 AM
> [...]
>> >> -               r8152_submit_rx(tp, agg, GFP_ATOMIC);
>> >> +               if (!ret) {
>> >> +                       ret = r8152_submit_rx(tp, agg, GFP_ATOMIC);
>> >> +               } else {
>> >> +                       urb->actual_length = 0;
>> >> +                       list_add_tail(&agg->list, next);
>> >
>> > Do you need a spin_lock_irqsave(&tp->rx_lock, flags) around this?
>>
>> Indeed, and rtl_start_rx() seems to also access agg->list without
>> proper locking.
>
> It is unnecessary because I deal with them in a local list_head. My steps are
>    1. Move the whole list from tp->rx_done to local rx_queue. (with spin lock)
>    2. dequeue/enqueue the lists in rx_queue.
>    3. Move the lists in rx_queue to tp->rx_done if it is necessary. (spin lock)
> For step 2, it wouldn't have race, because the list_head is local and no other
> function would change it. Therefore, I don't think it needs the spin lock.

Sorry guys, I think I made a mistake in my review and caused some
confusion/grief.

My mistake was getting the params to list_add_tail() backwards.  It's
list_add_tail(entry, head).  I saw list_add_tail(&agg->list, next) and
was fooled into thinking agg->list was the list getting appended with
the entry 'next'.  It's the opposite.  Duh.  So locking isn't needed
because the list is indeed local.

-scott
--
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-next 1/7] r8152: adjust rx_bottom

[Hayes Wang]

>  David Miller [mailto:davem@davemloft.net] 
> > Sent: Sunday, January 25, 2015 2:44 PM
> > What keeps rtl_start_rx() from running in parallel with
> > r8152_submit_rx(), or any other accessor of the RX agg->list?
> 
> Forgive my poor English. I would try to describe them clearly.
> The steps about the rx agg->list would be
> 1. carrier on or autoresume occurs.
> 2. Call rtl_start_rx().
> 3. Rx agg->list flows between device and tp->rx_done.
> 4. carrier off or autosuspend occurs.
> 5. call rtl_stop_rx().
> 
> The rtl_start_rx() would only be called when the linking
> status is changed from off to on or the auto resume occurs.
> And rtl_start_rx() would reinitialize the tp->rx_done and
> all of the rx agg->list. After step 2, the rx agg->list
> would flow between the usb host controller and the driver.
> If r8152_submit_rx() is success, the driver wouldn't own the
> rx agg->list until it is returned from the usb host controller.
> If r8152_submit_rx() is fail, the driver would still own the
> rx agg->list, and queue it to the tp->rx_done with spin lock
> for next try.
> 
> If the status stays in step 3, only the rx_bottom() would submit
> the rx agg. The rtl_start_rx() wouldn't be called suddenly,
> unless the linking down or auto suspend occur first and linking
> on or auto resume occur again. If linking down or auto suspend
> occur, rtl_stop_rx() would be called (step 5). After this step,
> rx_bottom() wouldn't submit rx, and all rx agg->list would stop
> flowing. That is, the tp->rx_done and all rx agg->list wouldn't
> be changed until the next rtl_start_rx() is called.
> 
> Therefore, the flow for each rx agg->list would be
> a. submittd by rtl_start_rx().
> b. goto step c if success, otherwise goto step d.
> c. completed by usb host controller.
> d. queued to tp->rx_done with spin lock.
> e. dequeue from tp->rx_done with spin lock by rx_botoom().
> f. goto step i if link down, otherwise goto step g.
> g. submitted by rx_botoom().
> h. goto step b.
> i. goto step a if link on.
> 
> And the patch change the step g to g1.
> g1. submitted by rx_botoom() if (!ret), otherwise goto step d.

Excuse me. Any other question or suggestion for this patch?
 
Best Regards,
Hayes

[PATCH net-next v2 0/7] r8152: adjust the code

[Hayes Wang]

V2:
Correct the subject of patch #5. Replace "link feed" with "line feed".

v1:
Code adjustment.

Hayes Wang (7):
  r8152: adjust rx_bottom
  r8152: adjust lpm timer
  r8152: check linking status with netif_carrier_ok
  r8152: check RTL8152_UNPLUG for rtl8152_close
  r8152: adjust the line feed for hw_features
  r8152: replace get_protocol with vlan_get_protocol
  r8152: use BIT macro

 drivers/net/usb/r8152.c | 99 +++++++++++++++++++++++--------------------------
 1 file changed, 46 insertions(+), 53 deletions(-)

-- 
2.1.0

[PATCH net-next v2 1/7] r8152: adjust rx_bottom

[Hayes Wang]

If a error occurs when submitting rx, skip the remaining submissions
and try to submit them again next time.

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

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index b74a272..41a1cbc 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -1643,7 +1643,7 @@ static int rx_bottom(struct r8152 *tp, int budget)
 {
 	unsigned long flags;
 	struct list_head *cursor, *next, rx_queue;
-	int work_done = 0;
+	int ret = 0, work_done = 0;
 
 	if (!skb_queue_empty(&tp->rx_queue)) {
 		while (work_done < budget) {
@@ -1734,7 +1734,18 @@ find_next_rx:
 		}
 
 submit:
-		r8152_submit_rx(tp, agg, GFP_ATOMIC);
+		if (!ret) {
+			ret = r8152_submit_rx(tp, agg, GFP_ATOMIC);
+		} else {
+			urb->actual_length = 0;
+			list_add_tail(&agg->list, next);
+		}
+	}
+
+	if (!list_empty(&rx_queue)) {
+		spin_lock_irqsave(&tp->rx_lock, flags);
+		list_splice_tail(&rx_queue, &tp->rx_done);
+		spin_unlock_irqrestore(&tp->rx_lock, flags);
 	}
 
 out1:
-- 
2.1.0

[PATCH net-next v2 2/7] r8152: adjust lpm timer

[Hayes Wang]

Set LPM timer to 500us, except for RTL_VER_04 which doesn't link at
USB 3.0.

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

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 41a1cbc..ff122fa 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -3256,10 +3256,10 @@ static void r8153_init(struct r8152 *tp)
 
 	ocp_data = ocp_read_byte(tp, MCU_TYPE_USB, USB_LPM_CTRL);
 	ocp_data &= ~LPM_TIMER_MASK;
-	if (tp->udev->speed == USB_SPEED_SUPER)
-		ocp_data |= LPM_TIMER_500US;
-	else
+	if (tp->version == RTL_VER_04 && tp->udev->speed != USB_SPEED_SUPER)
 		ocp_data |= LPM_TIMER_500MS;
+	else
+		ocp_data |= LPM_TIMER_500US;
 	ocp_write_byte(tp, MCU_TYPE_USB, USB_LPM_CTRL, ocp_data);
 
 	ocp_data = ocp_read_word(tp, MCU_TYPE_USB, USB_AFE_CTRL2);
-- 
2.1.0

[PATCH net-next v2 3/7] r8152: check linking status with netif_carrier_ok

[Hayes Wang]

Replace (tp->speed & LINK_STATUS) with netif_carrier_ok().

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

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index ff122fa..6667809 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -581,7 +581,6 @@ struct r8152 {
 	u16 ocp_base;
 	u8 *intr_buff;
 	u8 version;
-	u8 speed;
 };
 
 enum rtl_version {
@@ -1157,12 +1156,12 @@ static void intr_callback(struct urb *urb)
 
 	d = urb->transfer_buffer;
 	if (INTR_LINK & __le16_to_cpu(d[0])) {
-		if (!(tp->speed & LINK_STATUS)) {
+		if (!netif_carrier_ok(tp->netdev)) {
 			set_bit(RTL8152_LINK_CHG, &tp->flags);
 			schedule_delayed_work(&tp->schedule, 0);
 		}
 	} else {
-		if (tp->speed & LINK_STATUS) {
+		if (netif_carrier_ok(tp->netdev)) {
 			set_bit(RTL8152_LINK_CHG, &tp->flags);
 			schedule_delayed_work(&tp->schedule, 0);
 		}
@@ -1894,7 +1893,7 @@ static void rtl8152_set_rx_mode(struct net_device *netdev)
 {
 	struct r8152 *tp = netdev_priv(netdev);
 
-	if (tp->speed & LINK_STATUS) {
+	if (netif_carrier_ok(netdev)) {
 		set_bit(RTL8152_SET_RX_MODE, &tp->flags);
 		schedule_delayed_work(&tp->schedule, 0);
 	}
@@ -2918,21 +2917,20 @@ static void set_carrier(struct r8152 *tp)
 	speed = rtl8152_get_speed(tp);
 
 	if (speed & LINK_STATUS) {
-		if (!(tp->speed & LINK_STATUS)) {
+		if (!netif_carrier_ok(netdev)) {
 			tp->rtl_ops.enable(tp);
 			set_bit(RTL8152_SET_RX_MODE, &tp->flags);
 			netif_carrier_on(netdev);
 			rtl_start_rx(tp);
 		}
 	} else {
-		if (tp->speed & LINK_STATUS) {
+		if (netif_carrier_ok(netdev)) {
 			netif_carrier_off(netdev);
 			napi_disable(&tp->napi);
 			tp->rtl_ops.disable(tp);
 			napi_enable(&tp->napi);
 		}
 	}
-	tp->speed = speed;
 }
 
 static void rtl_work_func_t(struct work_struct *work)
@@ -2964,7 +2962,7 @@ static void rtl_work_func_t(struct work_struct *work)
 
 	/* don't schedule napi before linking */
 	if (test_bit(SCHEDULE_NAPI, &tp->flags) &&
-	    (tp->speed & LINK_STATUS)) {
+	    netif_carrier_ok(tp->netdev)) {
 		clear_bit(SCHEDULE_NAPI, &tp->flags);
 		napi_schedule(&tp->napi);
 	}
@@ -2987,8 +2985,7 @@ static int rtl8152_open(struct net_device *netdev)
 	if (res)
 		goto out;
 
-	/* set speed to 0 to avoid autoresume try to submit rx */
-	tp->speed = 0;
+	netif_carrier_off(netdev);
 
 	res = usb_autopm_get_interface(tp->intf);
 	if (res < 0) {
@@ -3005,7 +3002,7 @@ static int rtl8152_open(struct net_device *netdev)
 		cancel_delayed_work_sync(&tp->schedule);
 
 		/* disable the tx/rx, if the workqueue has enabled them. */
-		if (tp->speed & LINK_STATUS)
+		if (netif_carrier_ok(netdev))
 			tp->rtl_ops.disable(tp);
 	}
 
@@ -3014,7 +3011,6 @@ static int rtl8152_open(struct net_device *netdev)
 	rtl8152_set_speed(tp, AUTONEG_ENABLE,
 			  tp->mii.supports_gmii ? SPEED_1000 : SPEED_100,
 			  DUPLEX_FULL);
-	tp->speed = 0;
 	netif_carrier_off(netdev);
 	netif_start_queue(netdev);
 	set_bit(WORK_ENABLE, &tp->flags);
@@ -3340,7 +3336,7 @@ static int rtl8152_resume(struct usb_interface *intf)
 			rtl_runtime_suspend_enable(tp, false);
 			clear_bit(SELECTIVE_SUSPEND, &tp->flags);
 			set_bit(WORK_ENABLE, &tp->flags);
-			if (tp->speed & LINK_STATUS)
+			if (netif_carrier_ok(tp->netdev))
 				rtl_start_rx(tp);
 		} else {
 			tp->rtl_ops.up(tp);
@@ -3348,7 +3344,6 @@ static int rtl8152_resume(struct usb_interface *intf)
 					  tp->mii.supports_gmii ?
 					  SPEED_1000 : SPEED_100,
 					  DUPLEX_FULL);
-			tp->speed = 0;
 			netif_carrier_off(tp->netdev);
 			set_bit(WORK_ENABLE, &tp->flags);
 		}
-- 
2.1.0

[PATCH net-next v2 4/7] r8152: check RTL8152_UNPLUG for rtl8152_close

[Hayes Wang]

It is unnecessary to accress the hw register if the device is unplugged.

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

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 6667809..aade7b5 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -3046,7 +3046,7 @@ static int rtl8152_close(struct net_device *netdev)
 	netif_stop_queue(netdev);
 
 	res = usb_autopm_get_interface(tp->intf);
-	if (res < 0) {
+	if (res < 0 || test_bit(RTL8152_UNPLUG, &tp->flags)) {
 		rtl_drop_queued_tx(tp);
 		rtl_stop_rx(tp);
 	} else {
-- 
2.1.0

[PATCH net-next v2 5/7] r8152: adjust the line feed for hw_features

[Hayes Wang]

Keep NETIF_F_HW_VLAN_CTAG_RX and NETIF_F_HW_VLAN_CTAG_TX at the
same line.

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

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index aade7b5..8bdd477 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -3908,8 +3908,7 @@ static int rtl8152_probe(struct usb_interface *intf,
 	netdev->hw_features = NETIF_F_RXCSUM | NETIF_F_IP_CSUM | NETIF_F_SG |
 			      NETIF_F_TSO | NETIF_F_FRAGLIST |
 			      NETIF_F_IPV6_CSUM | NETIF_F_TSO6 |
-			      NETIF_F_HW_VLAN_CTAG_RX |
-			      NETIF_F_HW_VLAN_CTAG_TX;
+			      NETIF_F_HW_VLAN_CTAG_RX | NETIF_F_HW_VLAN_CTAG_TX;
 	netdev->vlan_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_TSO |
 				NETIF_F_HIGHDMA | NETIF_F_FRAGLIST |
 				NETIF_F_IPV6_CSUM | NETIF_F_TSO6;
-- 
2.1.0

[PATCH net-next v2 6/7] r8152: replace get_protocol with vlan_get_protocol

[Hayes Wang]

vlan_get_protocol() has been defined and use it to replace
get_protocol().

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

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 8bdd477..24e6aef9 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -1330,18 +1330,6 @@ static struct tx_agg *r8152_get_tx_agg(struct r8152 *tp)
 	return agg;
 }
 
-static inline __be16 get_protocol(struct sk_buff *skb)
-{
-	__be16 protocol;
-
-	if (skb->protocol == htons(ETH_P_8021Q))
-		protocol = vlan_eth_hdr(skb)->h_vlan_encapsulated_proto;
-	else
-		protocol = skb->protocol;
-
-	return protocol;
-}
-
 /* r8152_csum_workaround()
  * The hw limites the value the transport offset. When the offset is out of the
  * range, calculate the checksum by sw.
@@ -1447,7 +1435,7 @@ static int r8152_tx_csum(struct r8152 *tp, struct tx_desc *desc,
 			goto unavailable;
 		}
 
-		switch (get_protocol(skb)) {
+		switch (vlan_get_protocol(skb)) {
 		case htons(ETH_P_IP):
 			opts1 |= GTSENDV4;
 			break;
@@ -1478,7 +1466,7 @@ static int r8152_tx_csum(struct r8152 *tp, struct tx_desc *desc,
 			goto unavailable;
 		}
 
-		switch (get_protocol(skb)) {
+		switch (vlan_get_protocol(skb)) {
 		case htons(ETH_P_IP):
 			opts2 |= IPV4_CS;
 			ip_protocol = ip_hdr(skb)->protocol;
-- 
2.1.0

[PATCH net-next v2 7/7] r8152: use BIT macro

[Hayes Wang]

Use BIT macro to replace (1 << bits).

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

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 24e6aef9..5980ac6 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -489,16 +489,16 @@ struct rx_desc {
 #define RX_LEN_MASK			0x7fff
 
 	__le32 opts2;
-#define RD_UDP_CS			(1 << 23)
-#define RD_TCP_CS			(1 << 22)
-#define RD_IPV6_CS			(1 << 20)
-#define RD_IPV4_CS			(1 << 19)
+#define RD_UDP_CS			BIT(23)
+#define RD_TCP_CS			BIT(22)
+#define RD_IPV6_CS			BIT(20)
+#define RD_IPV4_CS			BIT(19)
 
 	__le32 opts3;
-#define IPF				(1 << 23) /* IP checksum fail */
-#define UDPF				(1 << 22) /* UDP checksum fail */
-#define TCPF				(1 << 21) /* TCP checksum fail */
-#define RX_VLAN_TAG			(1 << 16)
+#define IPF				BIT(23) /* IP checksum fail */
+#define UDPF				BIT(22) /* UDP checksum fail */
+#define TCPF				BIT(21) /* TCP checksum fail */
+#define RX_VLAN_TAG			BIT(16)
 
 	__le32 opts4;
 	__le32 opts5;
@@ -507,24 +507,24 @@ struct rx_desc {
 
 struct tx_desc {
 	__le32 opts1;
-#define TX_FS			(1 << 31) /* First segment of a packet */
-#define TX_LS			(1 << 30) /* Final segment of a packet */
-#define GTSENDV4		(1 << 28)
-#define GTSENDV6		(1 << 27)
+#define TX_FS			BIT(31) /* First segment of a packet */
+#define TX_LS			BIT(30) /* Final segment of a packet */
+#define GTSENDV4		BIT(28)
+#define GTSENDV6		BIT(27)
 #define GTTCPHO_SHIFT		18
 #define GTTCPHO_MAX		0x7fU
 #define TX_LEN_MAX		0x3ffffU
 
 	__le32 opts2;
-#define UDP_CS			(1 << 31) /* Calculate UDP/IP checksum */
-#define TCP_CS			(1 << 30) /* Calculate TCP/IP checksum */
-#define IPV4_CS			(1 << 29) /* Calculate IPv4 checksum */
-#define IPV6_CS			(1 << 28) /* Calculate IPv6 checksum */
+#define UDP_CS			BIT(31) /* Calculate UDP/IP checksum */
+#define TCP_CS			BIT(30) /* Calculate TCP/IP checksum */
+#define IPV4_CS			BIT(29) /* Calculate IPv4 checksum */
+#define IPV6_CS			BIT(28) /* Calculate IPv6 checksum */
 #define MSS_SHIFT		17
 #define MSS_MAX			0x7ffU
 #define TCPHO_SHIFT		17
 #define TCPHO_MAX		0x7ffU
-#define TX_VLAN_TAG			(1 << 16)
+#define TX_VLAN_TAG		BIT(16)
 };
 
 struct r8152;
-- 
2.1.0

Re: [PATCH net-next v2 0/7] r8152: adjust the code

[David Miller]

From: Hayes Wang <hayeswang@realtek.com>
Date: Fri, 6 Feb 2015 11:30:44 +0800

> V2:
> Correct the subject of patch #5. Replace "link feed" with "line feed".
> 
> v1:
> Code adjustment.

Series applied, thanks.