Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756253AbcKWDwm (ORCPT ); Tue, 22 Nov 2016 22:52:42 -0500 Received: from rtits2.realtek.com ([211.75.126.72]:48479 "EHLO rtits2.realtek.com.tw" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754572AbcKWDwk (ORCPT ); Tue, 22 Nov 2016 22:52:40 -0500 Authenticated-By: X-SpamFilter-By: BOX Solutions SpamTrap 5.56 with qID uAN3qUki021318, This message is accepted by code: ctloc85258 From: Hayes Wang To: Mark Lord , "netdev@vger.kernel.org" CC: nic_swsd , "linux-kernel@vger.kernel.org" , "linux-usb@vger.kernel.org" Subject: RE: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable Thread-Topic: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable Thread-Index: AQHSO+t3nHKkZLLsBUqa3CUPsCXz6KDci+AwgAAm4ICAAaIjoP//1LUAgAfJkeA= Date: Wed, 23 Nov 2016 03:52:30 +0000 Message-ID: <0835B3720019904CB8F7AA43166CEEB2010517CE@RTITMBSV03.realtek.com.tw> References: <1394712342-15778-226-Taiwan-albertk@realtek.com> <1394712342-15778-227-Taiwan-albertk@realtek.com> <0835B3720019904CB8F7AA43166CEEB20105013E@RTITMBSV03.realtek.com.tw> <0835B3720019904CB8F7AA43166CEEB201050B7E@RTITMBSV03.realtek.com.tw> In-Reply-To: Accept-Language: zh-TW, en-US Content-Language: zh-TW X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [172.21.177.128] Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by mail.home.local id uAN3qk4w013428 Content-Length: 2880 Lines: 91 Mark Lord [mailto:mlord@pobox.com] > Sent: Friday, November 18, 2016 8:03 PM [..] > How does the RTL8152 know that the limit is 16KB, > rather than some other number? Is this a hardwired number > in the hardware, or is it a parameter that the software > sends to the chip during initialization? It is the limitation of the hardware. > I have a USB analyzer, but it is difficult to figure out how > to program an appropriate trigger point for the capture, > since the problem (with 16KB URBs) takes minutes to hours > or even days to trigger. It is good. Our hw engineers real want it. Maybe you could send a specific packet, and trigger it. You could allocate a skb and fill the data which you prefer, and call skb_queue_tail(&tp->tx_queue, skb); [...] > The first issue is that a packet sometimes begins in one URB, > and completes in the next URB, without an rx_desc at the start > of the second URB. This I have already reported earlier. However, our hw engineer says it wouldn't happen. Our hw always sends rx_desc + packet + padding. The hw wouldn't split it to two or more transmission. That is why I wonder who does it. > But the driver, as written, sometimes accesses bytes outside > of the 16KB URB buffer, because it trusts the non-existent > rx_desc in these cases, and also because it accesses bytes > from the rx_desc without first checking whether there is > sufficient remaining space in the URB to hold an rx_desc. I think I check them. According to the followning code, list_for_each_safe(cursor, next, &rx_queue) { struct rx_desc *rx_desc; struct rx_agg *agg; int len_used = 0; struct urb *urb; u8 *rx_data; ... rx_desc = agg->head; rx_data = agg->head; len_used += sizeof(struct rx_desc); //<-- add the size of next rx_desc while (urb->actual_length > len_used) { struct net_device *netdev = tp->netdev; struct net_device_stats *stats = &netdev->stats; unsigned int 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; pkt_len -= CRC_SIZE; rx_data += sizeof(struct rx_desc); ... find_next_rx: rx_data = rx_agg_align(rx_data + pkt_len + CRC_SIZE); rx_desc = (struct rx_desc *)rx_data; len_used = (int)(rx_data - (u8 *)agg->head); len_used += sizeof(struct rx_desc); //<-- add the size of next rx_desc } submit: ... } The while loop would check if the next rx_desc is inside the urb buffer, because the len_used includes the size of the next rx_desc. Then, in the while loop, the len_used adds the packet size and check with urb->actual_length again. These make sure the rx_desc and the packet are inside the urb buffer. Except the urb->actual_length is more than agg_buf_sz. However, I don't think it would happen. Best Regards, Hayes