Thread (6 messages) 6 messages, 2 authors, 2022-02-04

Re: [PATCH] net: usb: pegasus: Request Ethernet FCS from hardware

From: Petko Manolov <petkan@nucleusys.com>
Date: 2021-12-26 16:25:08
Also in: linux-usb

On 21-12-26 14:25:02, Matthias-Christian Ott wrote:
quoted hunk ↗ jump to hunk
Commit 1a8deec09d12 ("pegasus: fixes reported packet length") tried to
configure the hardware to not include the FCS/CRC of Ethernet frames.
Unfortunately, this does not work with the D-Link DSB-650TX (USB IDs
2001:4002 and 2001:400b): the transferred "packets" (in the terminology
of the hardware) still contain 4 additional octets. For IP packets in
Ethernet this is not a problem as IP packets contain their own lengths
fields but other protocols also see Ethernet frames that include the FCS
in the payload which might be a problem for some protocols.

I was not able to open the D-Link DSB-650TX as the case is a very tight
press fit and opening it would likely destroy it. However, according to
the source code the earlier revision of the D-Link DSB-650TX (USB ID
2001:4002) is a Pegasus (possibly AN986) and not Pegasus II (AN8511)
device. I also tried it with the later revision of the D-Link DSB-650TX
(USB ID 2001:400b) which is a Pegasus II device according to the source
code and had the same results. Therefore, I'm not sure whether the RXCS
(rx_crc_sent) field of the EC0 (Ethernet control_0) register has any
effect or in which revision of the hardware it is usable and has an
effect. As a result, it seems best to me to revert commit
1a8deec09d12 ("pegasus: fixes reported packet length") and to set the
RXCS (rx_crc_sent) field of the EC0 (Ethernet control_0) register so
that the FCS/CRC is always included.

Fixes: 1a8deec09d12 ("pegasus: fixes reported packet length")
Signed-off-by: Matthias-Christian Ott <redacted>
---
 drivers/net/usb/pegasus.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c
index c4cd40b090fd..140d11ae6688 100644
--- a/drivers/net/usb/pegasus.c
+++ b/drivers/net/usb/pegasus.c
@@ -422,7 +422,13 @@ static int enable_net_traffic(struct net_device *dev, struct usb_device *usb)
 	ret = read_mii_word(pegasus, pegasus->phy, MII_LPA, &linkpart);
 	if (ret < 0)
 		goto fail;
-	data[0] = 0xc8; /* TX & RX enable, append status, no CRC */
+	/* At least two hardware revisions of the D-Link DSB-650TX (USB IDs
+	 * 2001:4002 and 2001:400b) include the Ethernet FCS in the packets,
+	 * even if RXCS is set to 0 in the EC0 register and the hardware is
+	 * instructed to not include the Ethernet FCS in the packet.Therefore,
+	 * it seems best to set RXCS to 1 and later ignore the Ethernet FCS.
+	 */
+	data[0] = 0xc9; /* TX & RX enable, append status, CRC */
 	data[1] = 0;
 	if (linkpart & (ADVERTISE_100FULL | ADVERTISE_10FULL))
 		data[1] |= 0x20;	/* set full duplex */
@@ -513,6 +519,13 @@ static void read_bulk_callback(struct urb *urb)
 		pkt_len = buf[count - 3] << 8;
 		pkt_len += buf[count - 4];
 		pkt_len &= 0xfff;
+		/* The FCS at the end of the packet is ignored. So subtract
+		 * its length to ignore it.
+		 */
+		pkt_len -= ETH_FCS_LEN;
+		/* Subtract the length of the received status at the end of the
+		 * packet as it is not part of the Ethernet frame.
+		 */
 		pkt_len -= 4;
 	}
Nice catch.  However, changing these constants for all devices isn't such a good
idea.  I'd rather use vendor and device IDs to distinguish these two cases in
the above code.


cheers,
Petko
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help