| Message ID | 0835B3720019904CB8F7AA43166CEEB2ECE429@RTITMBSV03.realtek.com.tw |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
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? -- 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
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 -- 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
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? -- 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
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 -- 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
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? -- 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
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 -- 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
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. -- 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
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. -- 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
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 -- 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
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;