[PATCH net-next 0/2] r8152: adjust rx functions

[PATCH net-next 0/2] r8152: adjust rx functions

[Hayes Wang]

Adjust some flows and codes which are relative to r8152_submit_rx()
and rtl_start_rx().

Hayes Wang (2):
  r8152: adjust r8152_submit_rx
  r8152: adjust rtl_start_rx

 drivers/net/usb/r8152.c | 51 +++++++++++++++++++++++++++----------------------
 1 file changed, 28 insertions(+), 23 deletions(-)

-- 
1.9.3

[PATCH net-next 1/2] r8152: adjust r8152_submit_rx

[Hayes Wang]

The behavior of handling the returned status from r8152_submit_rx()
is almost same, so let r8152_submit_rx() deal with the error
directly. This could avoid the duplicate code.

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

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 66b139a..ad62994 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -1032,7 +1032,6 @@ static void read_bulk_callback(struct urb *urb)
 	int status = urb->status;
 	struct rx_agg *agg;
 	struct r8152 *tp;
-	int result;
 
 	agg = urb->context;
 	if (!agg)
@@ -1083,16 +1082,7 @@ static void read_bulk_callback(struct urb *urb)
 		break;
 	}
 
-	result = r8152_submit_rx(tp, agg, GFP_ATOMIC);
-	if (result == -ENODEV) {
-		set_bit(RTL8152_UNPLUG, &tp->flags);
-		netif_device_detach(tp->netdev);
-	} else if (result) {
-		spin_lock(&tp->rx_lock);
-		list_add_tail(&agg->list, &tp->rx_done);
-		spin_unlock(&tp->rx_lock);
-		tasklet_schedule(&tp->tl);
-	}
+	r8152_submit_rx(tp, agg, GFP_ATOMIC);
 }
 
 static void write_bulk_callback(struct urb *urb)
@@ -1681,7 +1671,6 @@ static void rx_bottom(struct r8152 *tp)
 		int len_used = 0;
 		struct urb *urb;
 		u8 *rx_data;
-		int ret;
 
 		list_del_init(cursor);
 
@@ -1734,13 +1723,7 @@ find_next_rx:
 		}
 
 submit:
-		ret = r8152_submit_rx(tp, agg, GFP_ATOMIC);
-		if (ret && ret != -ENODEV) {
-			spin_lock_irqsave(&tp->rx_lock, flags);
-			list_add_tail(&agg->list, &tp->rx_done);
-			spin_unlock_irqrestore(&tp->rx_lock, flags);
-			tasklet_schedule(&tp->tl);
-		}
+		r8152_submit_rx(tp, agg, GFP_ATOMIC);
 	}
 }
 
@@ -1805,11 +1788,27 @@ static void bottom_half(unsigned long data)
 static
 int r8152_submit_rx(struct r8152 *tp, struct rx_agg *agg, gfp_t mem_flags)
 {
+	int ret = 0;
+
 	usb_fill_bulk_urb(agg->urb, tp->udev, usb_rcvbulkpipe(tp->udev, 1),
 			  agg->head, agg_buf_sz,
 			  (usb_complete_t)read_bulk_callback, agg);
 
-	return usb_submit_urb(agg->urb, mem_flags);
+	ret = usb_submit_urb(agg->urb, mem_flags);
+
+	if (ret == -ENODEV) {
+		set_bit(RTL8152_UNPLUG, &tp->flags);
+		netif_device_detach(tp->netdev);
+	} else if (ret) {
+		unsigned long flags;
+
+		spin_lock_irqsave(&tp->rx_lock, flags);
+		list_add_tail(&agg->list, &tp->rx_done);
+		spin_unlock_irqrestore(&tp->rx_lock, flags);
+		tasklet_schedule(&tp->tl);
+	}
+
+	return ret;
 }
 
 static void rtl_drop_queued_tx(struct r8152 *tp)
-- 
1.9.3

[PATCH net-next 2/2] r8152: adjust rtl_start_rx

[Hayes Wang]

Submit all the rx buffers, even though a error occurs. Otherwise
the buffers which are not submitted would be lost until next
rtl_start_rx() is called. Besides, the fail buffer could be
re-submitted later.

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

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index ad62994..5e0386f 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -1993,10 +1993,16 @@ static int rtl_start_rx(struct r8152 *tp)
 
 	INIT_LIST_HEAD(&tp->rx_done);
 	for (i = 0; i < RTL8152_MAX_RX; i++) {
+		int rr;
+
 		INIT_LIST_HEAD(&tp->rx_info[i].list);
-		ret = r8152_submit_rx(tp, &tp->rx_info[i], GFP_KERNEL);
-		if (ret)
-			break;
+
+		rr = r8152_submit_rx(tp, &tp->rx_info[i], GFP_KERNEL);
+		if (rr)
+			netif_err(tp, rx_err, tp->netdev,
+				  "Couldn't submit rx[%d], ret = %d\n", i, rr);
+		if (!ret)
+			ret = rr;
 	}
 
 	return ret;
-- 
1.9.3

Re: [PATCH net-next 2/2] r8152: adjust rtl_start_rx

[David Miller]

From: Hayes Wang <hayeswang@realtek.com>
Date: Fri, 7 Nov 2014 17:55:11 +0800

> Submit all the rx buffers, even though a error occurs. Otherwise
> the buffers which are not submitted would be lost until next
> rtl_start_rx() is called. Besides, the fail buffer could be
> re-submitted later.
> 
> Signed-off-by: Hayes Wang <hayeswang@realtek.com>

Does this even work?

If you leave a hole in the ring, the device is going to stop there
anyways.

So better to replenish the next time you call into this function
rather than leaving gaps in your receive ring.

RE: [PATCH net-next 2/2] r8152: adjust rtl_start_rx

[Hayes Wang]

 David Miller [mailto:davem@davemloft.net] 
> Sent: Saturday, November 08, 2014 12:35 AM
[...]
> Does this even work?
> 
> If you leave a hole in the ring, the device is going to stop there
> anyways.

Excuse me. I don't sure I understand your meaning clearly.

The behavior is different for PCI(e) and USB ethernet device.
The PCI nic could know the ring buffer by certain way, so
the device could fill the data into the buffer one by one
automatically. However, for usb nic, the driver has to
indicate (i.e. submit) each buffer for each data. The device
doesn't know what is the next buffer by itself. That is,
the driver determines the order by which the data would be
filled.

Therefore, when I try to submit 10 rx buffers and some of
them fail, I could get the data depending on the order of
the successful ones. Besides, the driver has to submit the
buffer for next data continually, so the previous unsuccessful
ones could be tried again for the same time.
 
Best Regards,
Hayes

Re: [PATCH net-next 2/2] r8152: adjust rtl_start_rx

[David Miller]

From: Hayes Wang <hayeswang@realtek.com>
Date: Mon, 10 Nov 2014 03:29:27 +0000

> The behavior is different for PCI(e) and USB ethernet device.
> The PCI nic could know the ring buffer by certain way, so
> the device could fill the data into the buffer one by one
> automatically. However, for usb nic, the driver has to
> indicate (i.e. submit) each buffer for each data. The device
> doesn't know what is the next buffer by itself. That is,
> the driver determines the order by which the data would be
> filled.
> 
> Therefore, when I try to submit 10 rx buffers and some of
> them fail, I could get the data depending on the order of
> the successful ones. Besides, the driver has to submit the
> buffer for next data continually, so the previous unsuccessful
> ones could be tried again for the same time.

Ok, but if we are looping here in rtl_start_rx() and r8152_submit_rx()
fails due to a memory allocation failure, there is nothing which is
going to make such a memory allocation succeed in the next iteration
of the loop.

Unless you can prove that often it can succeed after an initial
failure, this is just wasted work and in fact making it take longer
for the system to reclaim memory when under pressure because these
extra iterations are completely wasted cpu work.

RE: [PATCH net-next 2/2] r8152: adjust rtl_start_rx

[Hayes Wang]

David Miller [mailto:davem@davemloft.net] 
> Sent: Wednesday, November 12, 2014 10:51 AM
[...]
> Ok, but if we are looping here in rtl_start_rx() and r8152_submit_rx()
> fails due to a memory allocation failure, there is nothing which is
> going to make such a memory allocation succeed in the next iteration
> of the loop.
> 
> Unless you can prove that often it can succeed after an initial
> failure, this is just wasted work and in fact making it take longer
> for the system to reclaim memory when under pressure because these
> extra iterations are completely wasted cpu work.

How about that when a error occurs, add the remaining rx
to the list without submission? Then, the remianing rx
could be re-submitted later, and the rtl_start_rx() could
be completed as soon as possible.

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 0a30fd3..3273e3d 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -1991,14 +1991,35 @@ static void rxdy_gated_en(struct r8152 *tp, bool enable)
 
 static int rtl_start_rx(struct r8152 *tp)
 {
+	struct list_head rx_queue;
 	int i, ret = 0;
 
 	INIT_LIST_HEAD(&tp->rx_done);
 	for (i = 0; i < RTL8152_MAX_RX; i++) {
 		INIT_LIST_HEAD(&tp->rx_info[i].list);
 		ret = r8152_submit_rx(tp, &tp->rx_info[i], GFP_KERNEL);
-		if (ret)
+		if (ret) {
+			i++;
 			break;
+		}
+	}
+
+	INIT_LIST_HEAD(&rx_queue);
+	for (; i < RTL8152_MAX_RX; i++) {
+		struct rx_agg *agg = &tp->rx_info[i];
+		struct urb *urb = agg->urb;
+
+		INIT_LIST_HEAD(&agg->list);
+		urb->actual_length = 0;
+		list_add_tail(&agg->list, &rx_queue);
+	}
+
+	if (!list_empty(&rx_queue)) {
+		unsigned long flags;
+
+		spin_lock_irqsave(&tp->rx_lock, flags);
+		list_splice_tail(&rx_queue, &tp->rx_done);
+		spin_unlock_irqrestore(&tp->rx_lock, flags);
 	}
 
 	return ret;
 
Best Regards,
Hayes

Re: [PATCH net-next 2/2] r8152: adjust rtl_start_rx

[David Miller]

From: Hayes Wang <hayeswang@realtek.com>
Date: Wed, 12 Nov 2014 05:07:40 +0000

> How about that when a error occurs, add the remaining rx
> to the list without submission? Then, the remianing rx
> could be re-submitted later, and the rtl_start_rx() could
> be completed as soon as possible.

I really want to know why you are spending so much effort on this.

Is there a real situation that happened very often, which you
diagnosed in detail, and therefore you want to address?

RE: [PATCH net-next 2/2] r8152: adjust rtl_start_rx

[Hayes Wang]

David Miller [mailto:davem@davemloft.net] 
> Sent: Wednesday, November 12, 2014 1:13 PM
[...]
> I really want to know why you are spending so much effort on this.
> 
> Is there a real situation that happened very often, which you
> diagnosed in detail, and therefore you want to address?

No. I just consider the possible situation and want to
make the driver better. If you think this is unnecessary,
I would remove it.
 
Best Regards,
Hayes

Re: [PATCH net-next 2/2] r8152: adjust rtl_start_rx

[David Miller]

From: Hayes Wang <hayeswang@realtek.com>
Date: Wed, 12 Nov 2014 05:23:03 +0000

> David Miller [mailto:davem@davemloft.net] 
>> Sent: Wednesday, November 12, 2014 1:13 PM
> [...]
>> I really want to know why you are spending so much effort on this.
>> 
>> Is there a real situation that happened very often, which you
>> diagnosed in detail, and therefore you want to address?
> 
> No. I just consider the possible situation and want to
> make the driver better. If you think this is unnecessary,
> I would remove it.

What do other USB network drivers do in similar situations?

RE: [PATCH net-next 2/2] r8152: adjust rtl_start_rx

[Hayes Wang]

David Miller [mailto:davem@davemloft.net] 
> Sent: Wednesday, November 12, 2014 1:44 PM
[...]
> What do other USB network drivers do in similar situations?

According to the usbnet.c, it would make sure to submit the
number of min(10, RX_QLEN(dev)) rx buffers. If there are
not enough rx buffers, it schedule a tasklet for next try.

The brief flow is as following.
1. Call open().
   - schedule a tasklet.
2. Tasklet is called.
   if (dev->rxq.qlen < RX_QLEN(dev)) {
	   - submit rx buffers util the number of
	     min(10, RX_QLEN(dev)). If the error
	     occurs, break the loop.
	   - If the dev->rxq.qlen < RX_QLEN(dev),
	     schedule the tasklet.
   }

Best Regards,
Hayes

Re: [PATCH net-next 2/2] r8152: adjust rtl_start_rx

[David Miller]

From: Hayes Wang <hayeswang@realtek.com>
Date: Wed, 12 Nov 2014 06:29:46 +0000

> David Miller [mailto:davem@davemloft.net] 
>> Sent: Wednesday, November 12, 2014 1:44 PM
> [...]
>> What do other USB network drivers do in similar situations?
> 
> According to the usbnet.c, it would make sure to submit the
> number of min(10, RX_QLEN(dev)) rx buffers. If there are
> not enough rx buffers, it schedule a tasklet for next try.
> 
> The brief flow is as following.
> 1. Call open().
>    - schedule a tasklet.
> 2. Tasklet is called.
>    if (dev->rxq.qlen < RX_QLEN(dev)) {
> 	   - submit rx buffers util the number of
> 	     min(10, RX_QLEN(dev)). If the error
> 	     occurs, break the loop.
> 	   - If the dev->rxq.qlen < RX_QLEN(dev),
> 	     schedule the tasklet.
>    }

That sounds like a better recovery model, why don't you mimick it?

RE: [PATCH net-next 2/2] r8152: adjust rtl_start_rx

[Hayes Wang]

David Miller [mailto:davem@davemloft.net] 
> Sent: Thursday, November 13, 2014 3:50 AM
[...]
> > According to the usbnet.c, it would make sure to submit the
> > number of min(10, RX_QLEN(dev)) rx buffers. If there are
> > not enough rx buffers, it schedule a tasklet for next try.
> > 
> > The brief flow is as following.
> > 1. Call open().
> >    - schedule a tasklet.
> > 2. Tasklet is called.
> >    if (dev->rxq.qlen < RX_QLEN(dev)) {
> > 	   - submit rx buffers util the number of
> > 	     min(10, RX_QLEN(dev)). If the error
> > 	     occurs, break the loop.
> > 	   - If the dev->rxq.qlen < RX_QLEN(dev),
> > 	     schedule the tasklet.
> >    }
> 
> That sounds like a better recovery model, why don't you mimick it?

My last method which I mentioned yesterday is similar to
this one. The difference is that I would re-use the rx
buffers, so I have to add them to the list for re-submitting,
not alwayes allocate new one.

Although one rx buffer could contain many packets, I don't
think the whole size of the rx buffer is alwayes used.
Therefore, I re-use the rx buffers to avoid allocating
the 16K bytes rx buffer alwayes. This also makes sure that
I always have the buffers to submit without allocating new
one.

If you could accept this, I would modify this patch by
this way.
 
Best Regards,
Hayes

Re: [PATCH net-next 2/2] r8152: adjust rtl_start_rx

[David Miller]

From: Hayes Wang <hayeswang@realtek.com>
Date: Thu, 13 Nov 2014 02:31:14 +0000

> My last method which I mentioned yesterday is similar to
> this one. The difference is that I would re-use the rx
> buffers, so I have to add them to the list for re-submitting,
> not alwayes allocate new one.
> 
> Although one rx buffer could contain many packets, I don't
> think the whole size of the rx buffer is alwayes used.
> Therefore, I re-use the rx buffers to avoid allocating
> the 16K bytes rx buffer alwayes. This also makes sure that
> I always have the buffers to submit without allocating new
> one.
> 
> If you could accept this, I would modify this patch by
> this way.

I'll reread your original patch and think some more about this.

Thanks.

Re: [PATCH net-next 2/2] r8152: adjust rtl_start_rx

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Wed, 12 Nov 2014 22:31:46 -0500 (EST)

> From: Hayes Wang <hayeswang@realtek.com>
> Date: Thu, 13 Nov 2014 02:31:14 +0000
> 
>> My last method which I mentioned yesterday is similar to
>> this one. The difference is that I would re-use the rx
>> buffers, so I have to add them to the list for re-submitting,
>> not alwayes allocate new one.
>> 
>> Although one rx buffer could contain many packets, I don't
>> think the whole size of the rx buffer is alwayes used.
>> Therefore, I re-use the rx buffers to avoid allocating
>> the 16K bytes rx buffer alwayes. This also makes sure that
>> I always have the buffers to submit without allocating new
>> one.
>> 
>> If you could accept this, I would modify this patch by
>> this way.
> 
> I'll reread your original patch and think some more about this.

What if even the first r8152_submit_rx() fails?  What ever will cause
any of these retries to trigger at all?

Second, why does your patch increment 'i' with 'i++;' in the error
break path?  You should mark the first failed entry as unallocated
with actual_length == 0 and place it on the rx_done queue.

RE: [PATCH net-next 2/2] r8152: adjust rtl_start_rx

[Hayes Wang]

David Miller [mailto:davem@davemloft.net] 
> Sent: Friday, November 14, 2014 5:23 AM
[...]
> What if even the first r8152_submit_rx() fails?  What ever will cause
> any of these retries to trigger at all?

According to the patch #1 "adjust r8152_submit_rx", the
r8152_submit_rx() would add the rx to the list and schedule
the tasklet, when the error occurs. Each time the tasklet is
called, the rx_bottom() would deal with all the rx in the
list. If the actual_length isn't vaild, the rx buffer would be
submitted directly. By this way, the retries would be done.
That is, the retries would be triggered when the tasklet
is called. Therefore, any tx, rx, and tasklet scheduling
would result in the retries.

> Second, why does your patch increment 'i' with 'i++;' in the error
> break path?  You should mark the first failed entry as unallocated
> with actual_length == 0 and place it on the rx_done queue.

Because the r8152_submit_rx() would add the failed rx to
the list, I only have to deal with the remaining ones. That
is why I increase the "i", otherwise the failed one would
be added twice.

I remember the usb_submit_urb() would set actual_length
to 0, so I skip the step. I would check it again.
 
Best Regards,
Hayes

RE: [PATCH net-next 2/2] r8152: adjust rtl_start_rx

[Hayes Wang]

David Miller [mailto:davem@davemloft.net] 
> Sent: Saturday, November 08, 2014 12:35 AM
[...]
> Does this even work?
> 
> If you leave a hole in the ring, the device is going to stop there
> anyways.
> 
> So better to replenish the next time you call into this function
> rather than leaving gaps in your receive ring.

Excuse me. Is this still unacceptable?
Should I remove this patch for keeping the original flow?
 
Best Regards,
Hayes

Re: [PATCH net-next 2/2] r8152: adjust rtl_start_rx

[David Miller]

From: Hayes Wang <hayeswang@realtek.com>
Date: Wed, 12 Nov 2014 01:45:08 +0000

> David Miller [mailto:davem@davemloft.net] 
>> Sent: Saturday, November 08, 2014 12:35 AM
> [...]
>> Does this even work?
>> 
>> If you leave a hole in the ring, the device is going to stop there
>> anyways.
>> 
>> So better to replenish the next time you call into this function
>> rather than leaving gaps in your receive ring.
> 
> Excuse me. Is this still unacceptable?
> Should I remove this patch for keeping the original flow?

I haven't had time to process your original reply, please be patient.

RE: [PATCH net-next 2/2] r8152: adjust rtl_start_rx

[Hayes Wang]

David Miller [mailto:davem@davemloft.net] 
> Sent: Wednesday, November 12, 2014 10:20 AM
[...]
> I haven't had time to process your original reply, please be patient.

I am sorry for bothering you, and thanks for your reply.
 
Best Regards,
Hayes

[PATCH net-next v2 0/2] r8152: adjust rx functions

[Hayes Wang]

v2:
For patch #1, set actual_length to 0 before adding the rx to the
list, when a error occurs.

For patch #2, change the flow. Stop submitting the rx if a error
occurs, and add the remaining rx to the list for submitting later.

v1:
Adjust some flows and codes which are relative to r8152_submit_rx()
and rtl_start_rx().

Hayes Wang (2):
  r8152: adjust r8152_submit_rx
  r8152: adjust rtl_start_rx

 drivers/net/usb/r8152.c | 60 ++++++++++++++++++++++++++++++++-----------------
 1 file changed, 40 insertions(+), 20 deletions(-)

-- 
1.9.3

[PATCH net-next v2 1/2] r8152: adjust r8152_submit_rx

[Hayes Wang]

The behavior of handling the returned status from r8152_submit_rx()
is almost same, so let r8152_submit_rx() deal with the error
directly. This could avoid the duplicate code.

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

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 0a30fd3..df0868b 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -1032,7 +1032,6 @@ static void read_bulk_callback(struct urb *urb)
 	int status = urb->status;
 	struct rx_agg *agg;
 	struct r8152 *tp;
-	int result;
 
 	agg = urb->context;
 	if (!agg)
@@ -1083,16 +1082,7 @@ static void read_bulk_callback(struct urb *urb)
 		break;
 	}
 
-	result = r8152_submit_rx(tp, agg, GFP_ATOMIC);
-	if (result == -ENODEV) {
-		set_bit(RTL8152_UNPLUG, &tp->flags);
-		netif_device_detach(tp->netdev);
-	} else if (result) {
-		spin_lock(&tp->rx_lock);
-		list_add_tail(&agg->list, &tp->rx_done);
-		spin_unlock(&tp->rx_lock);
-		tasklet_schedule(&tp->tl);
-	}
+	r8152_submit_rx(tp, agg, GFP_ATOMIC);
 }
 
 static void write_bulk_callback(struct urb *urb)
@@ -1680,7 +1670,6 @@ static void rx_bottom(struct r8152 *tp)
 		int len_used = 0;
 		struct urb *urb;
 		u8 *rx_data;
-		int ret;
 
 		list_del_init(cursor);
 
@@ -1733,13 +1722,7 @@ find_next_rx:
 		}
 
 submit:
-		ret = r8152_submit_rx(tp, agg, GFP_ATOMIC);
-		if (ret && ret != -ENODEV) {
-			spin_lock_irqsave(&tp->rx_lock, flags);
-			list_add_tail(&agg->list, &tp->rx_done);
-			spin_unlock_irqrestore(&tp->rx_lock, flags);
-			tasklet_schedule(&tp->tl);
-		}
+		r8152_submit_rx(tp, agg, GFP_ATOMIC);
 	}
 }
 
@@ -1806,11 +1789,29 @@ static void bottom_half(unsigned long data)
 static
 int r8152_submit_rx(struct r8152 *tp, struct rx_agg *agg, gfp_t mem_flags)
 {
+	int ret = 0;
+
 	usb_fill_bulk_urb(agg->urb, tp->udev, usb_rcvbulkpipe(tp->udev, 1),
 			  agg->head, agg_buf_sz,
 			  (usb_complete_t)read_bulk_callback, agg);
 
-	return usb_submit_urb(agg->urb, mem_flags);
+	ret = usb_submit_urb(agg->urb, mem_flags);
+
+	if (ret == -ENODEV) {
+		set_bit(RTL8152_UNPLUG, &tp->flags);
+		netif_device_detach(tp->netdev);
+	} else if (ret) {
+		struct urb *urb = agg->urb;
+		unsigned long flags;
+
+		urb->actual_length = 0;
+		spin_lock_irqsave(&tp->rx_lock, flags);
+		list_add_tail(&agg->list, &tp->rx_done);
+		spin_unlock_irqrestore(&tp->rx_lock, flags);
+		tasklet_schedule(&tp->tl);
+	}
+
+	return ret;
 }
 
 static void rtl_drop_queued_tx(struct r8152 *tp)
-- 
1.9.3

Re: [PATCH net-next v2 1/2] r8152: adjust r8152_submit_rx

[Sergei Shtylyov]

Hello.

On 11/19/2014 8:20 AM, Hayes Wang wrote:

> The behavior of handling the returned status from r8152_submit_rx()
> is almost same, so let r8152_submit_rx() deal with the error
> directly. This could avoid the duplicate code.

> Signed-off-by: Hayes Wang <hayeswang@realtek.com>
> ---
>   drivers/net/usb/r8152.c | 41 +++++++++++++++++++++--------------------
>   1 file changed, 21 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index 0a30fd3..df0868b 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c
[...]
> @@ -1806,11 +1789,29 @@ static void bottom_half(unsigned long data)
>   static
>   int r8152_submit_rx(struct r8152 *tp, struct rx_agg *agg, gfp_t mem_flags)
>   {
> +	int ret = 0;

    Initialization not needed.

> +
>   	usb_fill_bulk_urb(agg->urb, tp->udev, usb_rcvbulkpipe(tp->udev, 1),
>   			  agg->head, agg_buf_sz,
>   			  (usb_complete_t)read_bulk_callback, agg);
>
> -	return usb_submit_urb(agg->urb, mem_flags);
> +	ret = usb_submit_urb(agg->urb, mem_flags);
> +

    Empty line not needed here either.

> +	if (ret == -ENODEV) {
> +		set_bit(RTL8152_UNPLUG, &tp->flags);
> +		netif_device_detach(tp->netdev);
> +	} else if (ret) {
> +		struct urb *urb = agg->urb;
> +		unsigned long flags;
> +
> +		urb->actual_length = 0;
> +		spin_lock_irqsave(&tp->rx_lock, flags);
> +		list_add_tail(&agg->list, &tp->rx_done);
> +		spin_unlock_irqrestore(&tp->rx_lock, flags);
> +		tasklet_schedule(&tp->tl);
> +	}
> +
> +	return ret;
>   }
[...]

WBR, Sergei

[PATCH net-next v2 2/2] r8152: adjust rtl_start_rx

[Hayes Wang]

If there is a error for r8152_submit_rx(), add the remaining rx
buffers to the list. Then the remaining rx buffers could be
submitted later.

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

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index df0868b..c7acadd 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -2002,6 +2002,25 @@ static int rtl_start_rx(struct r8152 *tp)
 			break;
 	}
 
+	if (ret && ++i < RTL8152_MAX_RX) {
+		struct list_head rx_queue;
+		unsigned long flags;
+
+		INIT_LIST_HEAD(&rx_queue);
+
+		do {
+			struct rx_agg *agg = &tp->rx_info[i++];
+			struct urb *urb = agg->urb;
+
+			urb->actual_length = 0;
+			list_add_tail(&agg->list, &rx_queue);
+		} while (i < RTL8152_MAX_RX);
+
+		spin_lock_irqsave(&tp->rx_lock, flags);
+		list_splice_tail(&rx_queue, &tp->rx_done);
+		spin_unlock_irqrestore(&tp->rx_lock, flags);
+	}
+
 	return ret;
 }
 
-- 
1.9.3

[PATCH net-next v3 0/2] r8152: adjust rx functions

[Hayes Wang]

v3:
For patch #1, remove unnecessary initialization for ret and
unnecessary blank line in r8152_submit_rx().

v2:
For patch #1, set actual_length to 0 before adding the rx to the
list, when a error occurs.

For patch #2, change the flow. Stop submitting the rx if a error
occurs, and add the remaining rx to the list for submitting later.

v1:
Adjust some flows and codes which are relative to r8152_submit_rx()
and rtl_start_rx().

Hayes Wang (2):
  r8152: adjust r8152_submit_rx
  r8152: adjust rtl_start_rx

 drivers/net/usb/r8152.c | 60 ++++++++++++++++++++++++++++++++-----------------
 1 file changed, 40 insertions(+), 20 deletions(-)

--
1.9.3

[PATCH net-next v3 1/2] r8152: adjust r8152_submit_rx

[Hayes Wang]

The behavior of handling the returned status from r8152_submit_rx()
is almost same, so let r8152_submit_rx() deal with the error
directly. This could avoid the duplicate code.

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

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 0a30fd3..3b89229 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -1032,7 +1032,6 @@ static void read_bulk_callback(struct urb *urb)
 	int status = urb->status;
 	struct rx_agg *agg;
 	struct r8152 *tp;
-	int result;
 
 	agg = urb->context;
 	if (!agg)
@@ -1083,16 +1082,7 @@ static void read_bulk_callback(struct urb *urb)
 		break;
 	}
 
-	result = r8152_submit_rx(tp, agg, GFP_ATOMIC);
-	if (result == -ENODEV) {
-		set_bit(RTL8152_UNPLUG, &tp->flags);
-		netif_device_detach(tp->netdev);
-	} else if (result) {
-		spin_lock(&tp->rx_lock);
-		list_add_tail(&agg->list, &tp->rx_done);
-		spin_unlock(&tp->rx_lock);
-		tasklet_schedule(&tp->tl);
-	}
+	r8152_submit_rx(tp, agg, GFP_ATOMIC);
 }
 
 static void write_bulk_callback(struct urb *urb)
@@ -1680,7 +1670,6 @@ static void rx_bottom(struct r8152 *tp)
 		int len_used = 0;
 		struct urb *urb;
 		u8 *rx_data;
-		int ret;
 
 		list_del_init(cursor);
 
@@ -1733,13 +1722,7 @@ find_next_rx:
 		}
 
 submit:
-		ret = r8152_submit_rx(tp, agg, GFP_ATOMIC);
-		if (ret && ret != -ENODEV) {
-			spin_lock_irqsave(&tp->rx_lock, flags);
-			list_add_tail(&agg->list, &tp->rx_done);
-			spin_unlock_irqrestore(&tp->rx_lock, flags);
-			tasklet_schedule(&tp->tl);
-		}
+		r8152_submit_rx(tp, agg, GFP_ATOMIC);
 	}
 }
 
@@ -1806,11 +1789,28 @@ static void bottom_half(unsigned long data)
 static
 int r8152_submit_rx(struct r8152 *tp, struct rx_agg *agg, gfp_t mem_flags)
 {
+	int ret;
+
 	usb_fill_bulk_urb(agg->urb, tp->udev, usb_rcvbulkpipe(tp->udev, 1),
 			  agg->head, agg_buf_sz,
 			  (usb_complete_t)read_bulk_callback, agg);
 
-	return usb_submit_urb(agg->urb, mem_flags);
+	ret = usb_submit_urb(agg->urb, mem_flags);
+	if (ret == -ENODEV) {
+		set_bit(RTL8152_UNPLUG, &tp->flags);
+		netif_device_detach(tp->netdev);
+	} else if (ret) {
+		struct urb *urb = agg->urb;
+		unsigned long flags;
+
+		urb->actual_length = 0;
+		spin_lock_irqsave(&tp->rx_lock, flags);
+		list_add_tail(&agg->list, &tp->rx_done);
+		spin_unlock_irqrestore(&tp->rx_lock, flags);
+		tasklet_schedule(&tp->tl);
+	}
+
+	return ret;
 }
 
 static void rtl_drop_queued_tx(struct r8152 *tp)
-- 
1.9.3

[PATCH net-next v3 2/2] r8152: adjust rtl_start_rx

[Hayes Wang]

If there is a error for r8152_submit_rx(), add the remaining rx
buffers to the list. Then the remaining rx buffers could be
submitted later.

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

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 3b89229..4a9ece0 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -2001,6 +2001,25 @@ static int rtl_start_rx(struct r8152 *tp)
 			break;
 	}
 
+	if (ret && ++i < RTL8152_MAX_RX) {
+		struct list_head rx_queue;
+		unsigned long flags;
+
+		INIT_LIST_HEAD(&rx_queue);
+
+		do {
+			struct rx_agg *agg = &tp->rx_info[i++];
+			struct urb *urb = agg->urb;
+
+			urb->actual_length = 0;
+			list_add_tail(&agg->list, &rx_queue);
+		} while (i < RTL8152_MAX_RX);
+
+		spin_lock_irqsave(&tp->rx_lock, flags);
+		list_splice_tail(&rx_queue, &tp->rx_done);
+		spin_unlock_irqrestore(&tp->rx_lock, flags);
+	}
+
 	return ret;
 }
 
-- 
1.9.3

Re: [PATCH net-next v3 0/2] r8152: adjust rx functions

[David Miller]

From: Hayes Wang <hayeswang@realtek.com>
Date: Thu, 20 Nov 2014 10:29:04 +0800

> v3:
> For patch #1, remove unnecessary initialization for ret and
> unnecessary blank line in r8152_submit_rx().
> 
> v2:
> For patch #1, set actual_length to 0 before adding the rx to the
> list, when a error occurs.
> 
> For patch #2, change the flow. Stop submitting the rx if a error
> occurs, and add the remaining rx to the list for submitting later.
> 
> v1:
> Adjust some flows and codes which are relative to r8152_submit_rx()
> and rtl_start_rx().

Series applied, thanks.