On 21-12-26 17:01:24, Matthias-Christian Ott wrote:

On 26/12/2021 16:39, Andrew Lunn wrote:

quoted

On Sun, Dec 26, 2021 at 02:29:30PM +0100, Matthias-Christian Ott wrote:

quoted

The D-Link DSB-650TX (2001:4002) is unable to receive Ethernet frames that
are longer than 1518 octets, for example, Ethernet frames that contain
802.1Q VLAN tags.

The frames are sent to the pegasus driver via USB but the driver discards
them because they have the Long_pkt field set to 1 in the received status
report. The function read_bulk_callback of the pegasus driver treats such
received "packets" (in the terminology of the hardware) as errors but the
field simply does just indicate that the Ethernet frame (MAC destination to
FCS) is longer than 1518 octets.

It seems that in the 1990s there was a distinction between "giant" (> 1518)
and "runt" (< 64) frames and the hardware includes flags to indicate this
distinction. It seems that the purpose of the distinction "giant" frames
was to not allow infinitely long frames due to transmission errors and to
allow hardware to have an upper limit of the frame size. However, the
hardware already has such limit with its 2048 octet receive buffer and,
therefore, Long_pkt is merely a convention and should not be treated as a
receive error.

Actually, the hardware is even able to receive Ethernet frames with 2048
octets which exceeds the claimed limit frame size limit of the driver of
1536 octets (PEGASUS_MTU).

Signed-off-by: Matthias-Christian Ott <redacted>
---
 drivers/net/usb/pegasus.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c
index 140d11ae6688..2582daf23015 100644
--- a/drivers/net/usb/pegasus.c
+++ b/drivers/net/usb/pegasus.c

@@ -499,11 +499,11 @@ static void read_bulk_callback(struct urb *urb)
 		goto goon;
 
 	rx_status = buf[count - 2];
-	if (rx_status & 0x1e) {
+	if (rx_status & 0x1c) {
 		netif_dbg(pegasus, rx_err, net,
 			  "RX packet error %x\n", rx_status);
 		net->stats.rx_errors++;
-		if (rx_status & 0x06)	/* long or runt	*/
+		if (rx_status & 0x04)	/* runt	*/

I've nothing against this patch, but if you are working on the driver, it
would be nice to replace these hex numbers with #defines using BIT, or
FIELD. It will make the code more readable.

Replacing the constants with macros is on my list of things that I want to do.
In this case, I did not do it because I wanted to a have small patch that gets
easily accepted and allows me to figure out the current process to submit
patches after years of inactivity.

To be honest, that's due to the fact the original code was submitted more than
20 years ago, when the driver acceptance criteria were a lot more relaxed than
they are now.  Ideally, these constants should be replaced with human readable
macros, something which i never got around doing.

If you are in the mood, you could send two patch series, one that fixes the
constants and another that fixes the packet size bug.  As is often the case, one
of them may get mainlined right away, while the other is being debated for a
while.


cheers,
Petko