RE: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable

[Hayes Wang]

Mark Lord [mailto:mlord@xxxxxxxxx]
> 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