[PATCH 3/5] net/usb/r8152: modify the tx flow

[Hayes Wang]

Let rtl8152_start_xmit() to queue packet only, and tx_bottom() would
send all of the packets. This simplify the code and make sure all the
packet would be sent by the original order.

Support stopping and waking tx queue. The maximum tx queue length
is 60.

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

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 815d890..a711025 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -276,6 +276,8 @@ enum rtl_register_content {

#define INTR_LINK 0x0004

+#define TX_QLEN 60
+
#define RTL8152_REQT_READ 0xc0
#define RTL8152_REQT_WRITE 0x40
#define RTL8152_REQ_GET_REGS 0x05
@@ -1174,6 +1176,9 @@ static int r8152_tx_agg_fill(struct r8152 *tp, struct tx_agg *agg)
remain = rx_buf_sz - (int)(tx_agg_align(tx_data) - agg->head);
}

+ if (netif_queue_stopped(tp->netdev))
+ netif_wake_queue(tp->netdev);
+
usb_fill_bulk_urb(agg->urb, tp->udev, usb_sndbulkpipe(tp->udev, 2),
agg->head, (int)(tx_data - (u8 *)agg->head),
(usb_complete_t)write_bulk_callback, agg);
@@ -1389,53 +1394,16 @@ static netdev_tx_t rtl8152_start_xmit(struct sk_buff *skb,
struct net_device *netdev)
{
struct r8152 *tp = netdev_priv(netdev);
- struct net_device_stats *stats = rtl8152_get_stats(netdev);
- unsigned long flags;
- struct tx_agg *agg = NULL;
- struct tx_desc *tx_desc;
- unsigned int len;
- u8 *tx_data;
- int res;

skb_tx_timestamp(skb);

- /* If tx_queue is not empty, it means at least one previous packt */
- /* is waiting for sending. Don't send current one before it. */
- if (skb_queue_empty(&tp->tx_queue))
- agg = r8152_get_tx_agg(tp);
-
- if (!agg) {
- skb_queue_tail(&tp->tx_queue, skb);
- return NETDEV_TX_OK;
- }
+ skb_queue_tail(&tp->tx_queue, skb);

- tx_desc = (struct tx_desc *)agg->head;
- tx_data = agg->head + sizeof(*tx_desc);
- agg->skb_num = agg->skb_len = 0;
+ if (skb_queue_len(&tp->tx_queue) > TX_QLEN)
+ netif_stop_queue(netdev);

- len = skb->len;
- r8152_tx_csum(tp, tx_desc, skb);
- memcpy(tx_data, skb->data, len);
- dev_kfree_skb_any(skb);
- agg->skb_num++;
- agg->skb_len += len;
- usb_fill_bulk_urb(agg->urb, tp->udev, usb_sndbulkpipe(tp->udev, 2),
- agg->head, len + sizeof(*tx_desc),
- (usb_complete_t)write_bulk_callback, agg);
- res = usb_submit_urb(agg->urb, GFP_ATOMIC);
- if (res) {
- /* Can we get/handle EPIPE here? */
- if (res == -ENODEV) {
- netif_device_detach(tp->netdev);
- } else {
- netif_warn(tp, tx_err, netdev,
- "failed tx_urb %d\n", res);
- stats->tx_dropped++;
- spin_lock_irqsave(&tp->tx_lock, flags);
- list_add_tail(&agg->list, &tp->tx_free);
- spin_unlock_irqrestore(&tp->tx_lock, flags);
- }
- }
+ if (!list_empty(&tp->tx_free))
+ tasklet_schedule(&tp->tl);

return NETDEV_TX_OK;
}
--
1.8.3.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[Hayes Wang]

Correct some declaration.

Signed-off-by: Hayes Wang <haye...@realtek.com>
---

drivers/net/usb/r8152.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index a711025..90bc105 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -309,22 +309,22 @@ enum rtl8152_flags {
#define MCU_TYPE_USB 0x0000

struct rx_desc {
- u32 opts1;
+ __le32 opts1;
#define RX_LEN_MASK 0x7fff
- u32 opts2;
- u32 opts3;
- u32 opts4;
- u32 opts5;
- u32 opts6;
+ __le32 opts2;
+ __le32 opts3;
+ __le32 opts4;
+ __le32 opts5;
+ __le32 opts6;
};

struct tx_desc {
- u32 opts1;
+ __le32 opts1;
#define TX_FS (1 << 31) /* First segment of a packet */
#define TX_LS (1 << 30) /* Final segment of a packet */
#define TX_LEN_MASK 0x3ffff

- u32 opts2;
+ __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 */
@@ -878,7 +878,7 @@ static void write_bulk_callback(struct urb *urb)
static void intr_callback(struct urb *urb)
{
struct r8152 *tp;
- __u16 *d;
+ __le16 *d;
int status = urb->status;
int res;

[Hayes Wang]

-Remove rtl8152_get_stats()
-Fix the wrong name
-Something else

Signed-off-by: Hayes Wang <haye...@realtek.com>
---

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

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 90bc105..d252ff6 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -218,7 +218,7 @@
#define POWER_CUT 0x0100

/* USB_PM_CTRL_STATUS */
-#define RWSUME_INDICATE 0x0001
+#define RESUME_INDICATE 0x0001

/* USB_USB_CTRL */
#define RX_AGG_DISABLE 0x0010
@@ -376,7 +376,8 @@ struct r8152 {
enum rtl_version {
RTL_VER_UNKNOWN = 0,
RTL_VER_01,
- RTL_VER_02
+ RTL_VER_02,
+ RTL_VER_INVALLID = -1
};

/* Maximum number of multicast addresses to filter (vs. Rx-all-multicast).
@@ -422,14 +423,15 @@ int set_registers(struct r8152 *tp, u16 value, u16 index, u16 size, void *data)
value, index, tmp, size, 500);

kfree(tmp);
+
return ret;
}

static int generic_ocp_read(struct r8152 *tp, u16 index, u16 size,
void *data, u16 type)
{
- u16 limit = 64;
- int ret = 0;
+ u16 limit = 64;
+ int ret = 0;

if (test_bit(RTL8152_UNPLUG, &tp->flags))
return -ENODEV;
@@ -468,9 +470,9 @@ static int generic_ocp_read(struct r8152 *tp, u16 index, u16 size,
static int generic_ocp_write(struct r8152 *tp, u16 index, u16 byteen,
u16 size, void *data, u16 type)
{
- int ret;
- u16 byteen_start, byteen_end, byen;
- u16 limit = 512;
+ int ret;
+ u16 byteen_start, byteen_end, byen;
+ u16 limit = 512;

if (test_bit(RTL8152_UNPLUG, &tp->flags))
return -ENODEV;
@@ -763,11 +765,6 @@ static int rtl8152_set_mac_address(struct net_device *netdev, void *p)
return 0;
}

-static struct net_device_stats *rtl8152_get_stats(struct net_device *dev)
-{
- return &dev->stats;
-}
-
static void read_bulk_callback(struct urb *urb)
{
struct net_device *netdev;
@@ -836,6 +833,7 @@ static void read_bulk_callback(struct urb *urb)

static void write_bulk_callback(struct urb *urb)

{
struct net_device_stats *stats;
+ struct net_device *netdev;
unsigned long flags;
struct tx_agg *agg;
struct r8152 *tp;
@@ -849,7 +847,8 @@ static void write_bulk_callback(struct urb *urb)
if (!tp)
return;

- stats = rtl8152_get_stats(tp->netdev);
+ netdev = tp->netdev;
+ stats = &netdev->stats;
if (status) {
pr_warn_ratelimited("Tx status %d\n", status);
stats->tx_errors += agg->skb_num;
@@ -862,7 +861,7 @@ static void write_bulk_callback(struct urb *urb)
list_add_tail(&agg->list, &tp->tx_free);
spin_unlock_irqrestore(&tp->tx_lock, flags);

- if (!netif_carrier_ok(tp->netdev))
+ if (!netif_carrier_ok(netdev))
return;

if (!test_bit(WORK_ENABLE, &tp->flags))
@@ -1214,7 +1213,7 @@ static void rx_bottom(struct r8152 *tp)

while (urb->actual_length > len_used) {
struct net_device *netdev = tp->netdev;
- struct net_device_stats *stats;
+ struct net_device_stats *stats = &netdev->stats;
unsigned pkt_len;
struct sk_buff *skb;

@@ -1226,8 +1225,6 @@ static void rx_bottom(struct r8152 *tp)
if (urb->actual_length < len_used)
break;

- stats = rtl8152_get_stats(netdev);
-
pkt_len -= 4; /* CRC */
rx_data += sizeof(struct rx_desc);

@@ -1281,7 +1278,7 @@ static void tx_bottom(struct r8152 *tp)
unsigned long flags;

netdev = tp->netdev;
- stats = rtl8152_get_stats(netdev);
+ stats = &netdev->stats;

if (res == -ENODEV) {
netif_device_detach(netdev);
@@ -1476,7 +1473,7 @@ static int rtl8152_enable(struct r8152 *tp)

static void rtl8152_disable(struct r8152 *tp)
{
- struct net_device_stats *stats = rtl8152_get_stats(tp->netdev);
+ struct net_device_stats *stats = &tp->netdev->stats;
struct sk_buff *skb;
u32 ocp_data;
int i;
@@ -1600,8 +1597,8 @@ static void r8152b_exit_oob(struct r8152 *tp)

static void r8152b_enter_oob(struct r8152 *tp)
{
- u32 ocp_data;
- int i;
+ u32 ocp_data;
+ int i;

ocp_data = ocp_read_byte(tp, MCU_TYPE_PLA, PLA_OOB_CTRL);
ocp_data &= ~NOW_IS_OOB;
@@ -1722,7 +1719,6 @@ static int rtl8152_set_speed(struct r8152 *tp, u8 autoneg, u16 speed, u8 duplex)
r8152_mdio_write(tp, MII_BMCR, bmcr);

out:
-
return ret;
}

@@ -1840,7 +1836,7 @@ static void rtl_clear_bp(struct r8152 *tp)

static void r8152b_enable_eee(struct r8152 *tp)
{
- u32 ocp_data;
+ u32 ocp_data;

ocp_data = ocp_read_word(tp, MCU_TYPE_PLA, PLA_EEE_CR);
ocp_data |= EEE_RX_EN | EEE_TX_EN;
@@ -1896,7 +1892,7 @@ static void r8152b_init(struct r8152 *tp)
ocp_write_word(tp, MCU_TYPE_USB, USB_UPS_CTRL, ocp_data);

ocp_data = ocp_read_word(tp, MCU_TYPE_USB, USB_PM_CTRL_STATUS);
- ocp_data &= ~RWSUME_INDICATE;
+ ocp_data &= ~RESUME_INDICATE;
ocp_write_word(tp, MCU_TYPE_USB, USB_PM_CTRL_STATUS, ocp_data);

r8152b_exit_oob(tp);
@@ -2148,7 +2144,7 @@ static void rtl8152_unload(struct r8152 *tp)
}

ocp_data = ocp_read_word(tp, MCU_TYPE_USB, USB_PM_CTRL_STATUS);
- ocp_data &= ~RWSUME_INDICATE;
+ ocp_data &= ~RESUME_INDICATE;
ocp_write_word(tp, MCU_TYPE_USB, USB_PM_CTRL_STATUS, ocp_data);

[Hayes Wang]

The tx/rx would access the memory which is out of the desired range.
Modify the method of checking the end of the memory to avoid it.

Signed-off-by: Hayes Wang <haye...@realtek.com>
---

drivers/net/usb/r8152.c | 30 +++++++++++++++++-------------
1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index f3fce41..5dbfe50 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -24,7 +24,7 @@
#include <linux/ipv6.h>

/* Version Information */
-#define DRIVER_VERSION "v1.01.0 (2013/08/12)"
+#define DRIVER_VERSION "v1.02.0 (2013/10/28)"
#define DRIVER_AUTHOR "Realtek linux nic maintainers <nic_...@realtek.com>"
#define DRIVER_DESC "Realtek RTL8152 Based USB 2.0 Ethernet Adapters"
#define MODULENAME "r8152"
@@ -1136,14 +1136,14 @@ r8152_tx_csum(struct r8152 *tp, struct tx_desc *desc, struct sk_buff *skb)

static int r8152_tx_agg_fill(struct r8152 *tp, struct tx_agg *agg)

{
- u32 remain;
+ int remain;
u8 *tx_data;

tx_data = agg->head;

agg->skb_num = agg->skb_len = 0;

- remain = rx_buf_sz - sizeof(struct tx_desc);
+ remain = rx_buf_sz;

- while (remain >= ETH_ZLEN) {
+ while (remain >= ETH_ZLEN + sizeof(struct tx_desc)) {
struct tx_desc *tx_desc;
struct sk_buff *skb;
unsigned int len;
@@ -1152,12 +1152,14 @@ static int r8152_tx_agg_fill(struct r8152 *tp, struct tx_agg *agg)
if (!skb)
break;

+ remain -= sizeof(*tx_desc);
len = skb->len;
if (remain < len) {
skb_queue_head(&tp->tx_queue, skb);
break;
}

+ tx_data = tx_agg_align(tx_data);
tx_desc = (struct tx_desc *)tx_data;
tx_data += sizeof(*tx_desc);

@@ -1167,9 +1169,8 @@ static int r8152_tx_agg_fill(struct r8152 *tp, struct tx_agg *agg)
agg->skb_len += len;
dev_kfree_skb_any(skb);

- tx_data = tx_agg_align(tx_data + len);
- remain = rx_buf_sz - sizeof(*tx_desc) -
- (u32)((void *)tx_data - agg->head);
+ tx_data += len;
+ remain = rx_buf_sz - (int)(tx_agg_align(tx_data) - agg->head);

}

usb_fill_bulk_urb(agg->urb, tp->udev, usb_sndbulkpipe(tp->udev, 2),

@@ -1188,7 +1189,6 @@ static void rx_bottom(struct r8152 *tp)
list_for_each_safe(cursor, next, &tp->rx_done) {
struct rx_desc *rx_desc;
struct rx_agg *agg;
- unsigned pkt_len;
int len_used = 0;
struct urb *urb;
u8 *rx_data;
@@ -1204,17 +1204,22 @@ static void rx_bottom(struct r8152 *tp)

rx_desc = agg->head;
rx_data = agg->head;
- pkt_len = le32_to_cpu(rx_desc->opts1) & RX_LEN_MASK;
- len_used += sizeof(struct rx_desc) + pkt_len;
+ len_used += sizeof(struct rx_desc);

- while (urb->actual_length >= len_used) {
+ while (urb->actual_length > len_used) {

struct net_device *netdev = tp->netdev;

struct net_device_stats *stats;
+ unsigned pkt_len;
struct sk_buff *skb;

+ pkt_len = le32_to_cpu(rx_desc->opts1) & RX_LEN_MASK;
if (pkt_len < ETH_ZLEN)
break;

+ len_used += pkt_len;
+ if (urb->actual_length < len_used)
+ break;
+
stats = rtl8152_get_stats(netdev);

pkt_len -= 4; /* CRC */

@@ -1234,9 +1239,8 @@ static void rx_bottom(struct r8152 *tp)

rx_data = rx_agg_align(rx_data + pkt_len + 4);
rx_desc = (struct rx_desc *)rx_data;
- pkt_len = le32_to_cpu(rx_desc->opts1) & RX_LEN_MASK;
len_used = (int)(rx_data - (u8 *)agg->head);
- len_used += sizeof(struct rx_desc) + pkt_len;
+ len_used += sizeof(struct rx_desc);
}

submit:

[Hayes Wang]

Clear the checksum setting before checking the necessary of hardware
checksum.

Signed-off-by: Hayes Wang <haye...@realtek.com>
---

drivers/net/usb/r8152.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 5dbfe50..815d890 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -1094,6 +1094,7 @@ r8152_tx_csum(struct r8152 *tp, struct tx_desc *desc, struct sk_buff *skb)
memset(desc, 0, sizeof(*desc));

desc->opts1 = cpu_to_le32((skb->len & TX_LEN_MASK) | TX_FS | TX_LS);
+ desc->opts2 = 0;

if (skb->ip_summed == CHECKSUM_PARTIAL) {
__be16 protocol;

[Hayes Wang]

These could fix some driver issues.

Hayes Wang (5):
net/usb/r8152: fix tx/rx memory overflow
net/usb/r8152: make sure the tx checksum setting is correct
net/usb/r8152: modify the tx flow
net/usb/r8152: fix incorrect type in assignment
net/usb/r8152: code adjust

drivers/net/usb/r8152.c | 145 +++++++++++++++++++-----------------------------
1 file changed, 57 insertions(+), 88 deletions(-)

[David Miller]

From: Hayes Wang <haye...@realtek.com>
Date: Mon, 28 Oct 2013 19:58:09 +0800

> These could fix some driver issues.
>
> Hayes Wang (5):
> net/usb/r8152: fix tx/rx memory overflow
> net/usb/r8152: make sure the tx checksum setting is correct
> net/usb/r8152: modify the tx flow
> net/usb/r8152: fix incorrect type in assignment
> net/usb/r8152: code adjust

These are not bug fixes, some of them are just cleanups.

It is inappropriate to mix real bug fixes and non-bug fixes into
a patch series.

You must send purely the bug fixes for 'net' tree, and then later
the code cleanups and other changes targetting the 'net-next' tree.

Also, from patch #5:

====================
-Something else
====================

That is completely unacceptable. You must say what changes you are
making, the above says nothing to me nor the person reading your
commit messages in the future.

In general, your commit messages are poorly written in that they
contain not enough information for the person trying to understand
your patches at all.

[Hayes Wang]

These could fix some driver issues.

Hayes Wang (3):
r8152: fix tx/rx memory overflow
r8152: modify the tx flow

r8152: fix incorrect type in assignment

drivers/net/usb/r8152.c | 100 +++++++++++++++++-------------------------------
1 file changed, 36 insertions(+), 64 deletions(-)

--
1.8.3.1

[Hayes Wang]

Remove the code for sending the packet in the rtl8152_start_xmit().
Let rtl8152_start_xmit() to queue the packet only, and schedule a
tasklet to send the queued packets. This simplify the code and make

sure all the packet would be sent by the original order.

Signed-off-by: Hayes Wang <haye...@realtek.com>
---

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

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 5dbfe50..763234d 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -1388,53 +1388,13 @@ static netdev_tx_t rtl8152_start_xmit(struct sk_buff *skb,

struct net_device *netdev)
{
struct r8152 *tp = netdev_priv(netdev);
- struct net_device_stats *stats = rtl8152_get_stats(netdev);
- unsigned long flags;
- struct tx_agg *agg = NULL;
- struct tx_desc *tx_desc;
- unsigned int len;
- u8 *tx_data;
- int res;

skb_tx_timestamp(skb);

- /* If tx_queue is not empty, it means at least one previous packt */
- /* is waiting for sending. Don't send current one before it. */
- if (skb_queue_empty(&tp->tx_queue))
- agg = r8152_get_tx_agg(tp);
-
- if (!agg) {
- skb_queue_tail(&tp->tx_queue, skb);
- return NETDEV_TX_OK;
- }

-

- tx_desc = (struct tx_desc *)agg->head;
- tx_data = agg->head + sizeof(*tx_desc);

- agg->skb_num = agg->skb_len = 0;
+ skb_queue_tail(&tp->tx_queue, skb);

[Hayes Wang]

The tx/rx would access the memory which is out of the desired range.
Modify the method of checking the end of the memory to avoid it.

For r8152_tx_agg_fill(), the variable remain may become negative.
However, the declaration is unsigned, so the while loop wouldn't
break when reach the end of the desied memory. Although change the
declaration from unsigned to signed is enough to fix it, I also
modify the checking method for safe. Replace

remain = rx_buf_sz - sizeof(*tx_desc) -

(u32)((void *)tx_data - agg->head);

with

remain = rx_buf_sz - (int)(tx_agg_align(tx_data) - agg->head);

to make sure the variable remain is always positive. Then, the
overflow wouldn't happen.

For rx_bottom(), the rx_desc should not be used to calculate the
packet length before making sure the rx_desc is in the desired range.
Change the checking to two parts. First, check the descriptor is in
the memory. The other, using the descriptor to find out the packet
length and check if the packet is in the memory.

Signed-off-by: Hayes Wang <haye...@realtek.com>
---

drivers/net/usb/r8152.c | 30 +++++++++++++++++-------------
1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index f3fce41..5dbfe50 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c

@@ -24,7 +24,7 @@
#include <linux/ipv6.h>

/* Version Information */
-#define DRIVER_VERSION "v1.01.0 (2013/08/12)"
+#define DRIVER_VERSION "v1.02.0 (2013/10/28)"
#define DRIVER_AUTHOR "Realtek linux nic maintainers <nic_...@realtek.com>"
#define DRIVER_DESC "Realtek RTL8152 Based USB 2.0 Ethernet Adapters"
#define MODULENAME "r8152"
@@ -1136,14 +1136,14 @@ r8152_tx_csum(struct r8152 *tp, struct tx_desc *desc, struct sk_buff *skb)

static int r8152_tx_agg_fill(struct r8152 *tp, struct tx_agg *agg)
{
- u32 remain;
+ int remain;
u8 *tx_data;

tx_data = agg->head;

agg->skb_num = agg->skb_len = 0;

usb_fill_bulk_urb(agg->urb, tp->udev, usb_sndbulkpipe(tp->udev, 2),

[Hayes Wang]

I have update the commit messages. I hope they are clear enough.

I remove the stop/wake tx queue from the second patch first, until
I find a way to determine which value is suitable for TX_QLEN.

Hayes Wang (3):
r8152: fix tx/rx memory overflow
r8152: modify the tx flow
r8152: fix incorrect type in assignment

drivers/net/usb/r8152.c | 94 +++++++++++++++----------------------------------
1 file changed, 29 insertions(+), 65 deletions(-)

[Hayes Wang]

The data from the hardware should be little endian. Correct the
declaration.

Signed-off-by: Hayes Wang <haye...@realtek.com>
---

drivers/net/usb/r8152.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 763234d..a77bdb8 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -307,22 +307,22 @@ enum rtl8152_flags {

@@ -876,7 +876,7 @@ static void write_bulk_callback(struct urb *urb)

static void intr_callback(struct urb *urb)
{
struct r8152 *tp;
- __u16 *d;
+ __le16 *d;
int status = urb->status;
int res;

[David Miller]

From: Hayes Wang <haye...@realtek.com>
Date: Wed, 30 Oct 2013 15:13:39 +0800

> Remove the code for sending the packet in the rtl8152_start_xmit().
> Let rtl8152_start_xmit() to queue the packet only, and schedule a
> tasklet to send the queued packets. This simplify the code and make
> sure all the packet would be sent by the original order.
>
> Signed-off-by: Hayes Wang <haye...@realtek.com>

Basically, your driver will now queue up to 1,000 packets onto
this tx_queue list, because that is what tx_queue_len will be
for alloc_etherdev() allocated network devices.

In my previous reply to you about this patch, I asked you to
quantify and study the effects of using a limit of 60. I said
that 60 might be too large.

You've responded by removing the limit completely, which is exactly
the opposite of what I've asked you to do. Why did you do this?

This patch series is still not in a state where I can apply it,
sorry.

[hayeswang]

From: David Miller [mailto:da...@davemloft.net]
Sent: Thursday, October 31, 2013 5:05 AM

>
> From: Hayes Wang <haye...@realtek.com>
> Date: Wed, 30 Oct 2013 15:13:39 +0800
[...]
> Basically, your driver will now queue up to 1,000 packets onto
> this tx_queue list, because that is what tx_queue_len will be
> for alloc_etherdev() allocated network devices.
>
> In my previous reply to you about this patch, I asked you to
> quantify and study the effects of using a limit of 60. I said
> that 60 might be too large.
>
> You've responded by removing the limit completely, which is exactly
> the opposite of what I've asked you to do. Why did you do this?

Excuse me. My question is that the original code doesn't stop the tx queue
either, so I don't understand why it is necessary for this patch.

I don't say I wouldn't find the suitable value for the tx queue length.
I feel I need some time to think how to find the reasonable value. And
I don't hope it influences the submission of the other patches, so I
remove it first. Or, may I submit the other two patches first?

[David Miller]

From: hayeswang <haye...@realtek.com>
Date: Thu, 31 Oct 2013 13:52:38 +0800

> From: David Miller [mailto:da...@davemloft.net]
> Sent: Thursday, October 31, 2013 5:05 AM
>>
>> From: Hayes Wang <haye...@realtek.com>
>> Date: Wed, 30 Oct 2013 15:13:39 +0800
> [...]
>> Basically, your driver will now queue up to 1,000 packets onto
>> this tx_queue list, because that is what tx_queue_len will be
>> for alloc_etherdev() allocated network devices.
>>
>> In my previous reply to you about this patch, I asked you to
>> quantify and study the effects of using a limit of 60. I said
>> that 60 might be too large.
>>
>> You've responded by removing the limit completely, which is exactly
>> the opposite of what I've asked you to do. Why did you do this?
>
> Excuse me. My question is that the original code doesn't stop the tx queue
> either, so I don't understand why it is necessary for this patch.
>
> I don't say I wouldn't find the suitable value for the tx queue length.
> I feel I need some time to think how to find the reasonable value. And
> I don't hope it influences the submission of the other patches, so I
> remove it first. Or, may I submit the other two patches first?

The more TX work you push into the workqueue handler, the longer the
latency for releasing the SKB and releasing all the queues that are
waiting for release of that packet.

Do you know that sockets, queueing discplines, etc. all rely upon
there being a timely release of SKBs once they are successfully
transmitted? It must happen at the earliest moment possible that
can be reasonable obtained.

[hayeswang]

David Miller [mailto:da...@davemloft.net]
> Sent: Tuesday, November 05, 2013 4:53 AM
> To: Hayeswang
> Cc: net...@vger.kernel.org; nic_swsd;
> linux-...@vger.kernel.org; linu...@vger.kernel.org
> Subject: Re: [PATCH net v2 2/3] r8152: modify the tx flow
[...]

> The more TX work you push into the workqueue handler, the longer the
> latency for releasing the SKB and releasing all the queues that are
> waiting for release of that packet.
>
> Do you know that sockets, queueing discplines, etc. all rely upon
> there being a timely release of SKBs once they are successfully
> transmitted? It must happen at the earliest moment possible that
> can be reasonable obtained.

Thanks for your answer. I would resend the patches after finishing the
setting of the queue length.