[PATCH net v2] net: usb: pegasus: Do not drop long Ethernet frames

[PATCH net v2] net: usb: pegasus: Do not drop long Ethernet frames

[Matthias-Christian Ott]

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).

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Matthias-Christian Ott <ott@mirix.org>
---
V1 -> V2: Included "Fixes:" tag

 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	*/
 			net->stats.rx_length_errors++;
 		if (rx_status & 0x08)
 			net->stats.rx_crc_errors++;
-- 
2.30.2

Re: [PATCH net v2] net: usb: pegasus: Do not drop long Ethernet frames

[Andrew Lunn]

On Sun, Dec 26, 2021 at 11:12:08PM +0100, Matthias-Christian Ott wrote:
> 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).
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Matthias-Christian Ott <ott@mirix.org>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net v2] net: usb: pegasus: Do not drop long Ethernet frames

[Petko Manolov]

On 21-12-26 23:12:08, Matthias-Christian Ott wrote:
> 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).

2048 is not mentioned anywhere in both, adm8511 and adm8515 documents.  In the
latter I found 1638 as max packet length, but that's not the default.  The
default is 1528 and i don't feel like changing it without further investigation.

Thus, i assume it is safe to change PEGASUS_MTU to 1528 for the moment.  VLAN
frames have 4 additional bytes so we aren't breaking neither pegasus I nor
pegasus II devices.

If you're going to make ver 3 of this change, you might as well modify
PEGASUS_MTU in pegasus.h as a separate patch within the same series.


		Petko


> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Matthias-Christian Ott <ott@mirix.org>
> ---
> V1 -> V2: Included "Fixes:" tag
> 
>  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	*/
>  			net->stats.rx_length_errors++;
>  		if (rx_status & 0x08)
>  			net->stats.rx_crc_errors++;
> -- 
> 2.30.2
> 
> 

Re: [PATCH net v2] net: usb: pegasus: Do not drop long Ethernet frames

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (master)
by David S. Miller <davem@davemloft.net>:

On Sun, 26 Dec 2021 23:12:08 +0100 you wrote:
> 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.
> 
> [...]

Here is the summary with links:
  - [net,v2] net: usb: pegasus: Do not drop long Ethernet frames
    https://git.kernel.org/netdev/net/c/ca506fca461b

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net v2] net: usb: pegasus: Do not drop long Ethernet frames

[Matthias-Christian Ott]

On 27/12/2021 12:11, Petko Manolov wrote:
> On 21-12-26 23:12:08, Matthias-Christian Ott wrote:
>> 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).
> 
> 2048 is not mentioned anywhere in both, adm8511 and adm8515 documents.  In the
> latter I found 1638 as max packet length, but that's not the default.  The
> default is 1528 and i don't feel like changing it without further investigation.

I can't remember where I found the number. I adapted the original bug
report that I wrote months ago for the commit message. I'm assuming that
the number comes from the size of the SRAM transmit buffer/TX FIFO. I
also remember that I did some experiments with the MTU to find out what
the hardware supports. However, I don't remember the results of these
experiments. So treat the 2048 as an unverified and perhaps wrong claim.
I will remove it a subsequent version of the patch.

The ADM8515/X datasheet states that the 2 KiB SRAM TX FIFO can hold 4
Ethernet frames which equals a MTU of 512 Octets and that the 24 KiB
SRAM RX FIFO can hold 16 Ethernet frames which equals a MTU of 1536
Octets. This somewhat contradicts the 1638 Octets from the same
datasheet. So it seems best to me find it out with an experiment.

> Thus, i assume it is safe to change PEGASUS_MTU to 1528 for the moment.  VLAN
> frames have 4 additional bytes so we aren't breaking neither pegasus I nor
> pegasus II devices.

Yes, this also not the subject or intent of the commit. I will remove
the sentence from the paragraph in a subsequent version of the patch.

Kind regards,
Matthias-Christian Ott

Re: [PATCH net v2] net: usb: pegasus: Do not drop long Ethernet frames

[Matthias-Christian Ott]

On 02/01/2022 15:16, Matthias-Christian Ott wrote:
> Yes, this also not the subject or intent of the commit. I will remove
> the sentence from the paragraph in a subsequent version of the patch.

It seems that the patch was already merged. So I was too late to change
it. I suppose we now have to live the commit message. I apologize.
Perhaps the archived discussion about it can somewhat correct it.

Kind regards,
Matthias-Christian Ott