* [PATCH] net: usb: pegasus: Do not drop long Ethernet frames @ 2021-12-26 13:29 Matthias-Christian Ott 2021-12-26 15:39 ` Andrew Lunn 2021-12-27 9:56 ` Sergei Shtylyov 0 siblings, 2 replies; 8+ messages in thread From: Matthias-Christian Ott @ 2021-12-26 13:29 UTC (permalink / raw) To: Petko Manolov; +Cc: linux-usb, netdev, 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). Signed-off-by: Matthias-Christian Ott <ott@mirix.org> --- 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] net: usb: pegasus: Do not drop long Ethernet frames 2021-12-26 13:29 [PATCH] net: usb: pegasus: Do not drop long Ethernet frames Matthias-Christian Ott @ 2021-12-26 15:39 ` Andrew Lunn 2021-12-26 16:01 ` Matthias-Christian Ott 2021-12-27 9:56 ` Sergei Shtylyov 1 sibling, 1 reply; 8+ messages in thread From: Andrew Lunn @ 2021-12-26 15:39 UTC (permalink / raw) To: Matthias-Christian Ott; +Cc: Petko Manolov, linux-usb, netdev On Sun, Dec 26, 2021 at 02:29:30PM +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). > > Signed-off-by: Matthias-Christian Ott <ott@mirix.org> > --- > 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. Andrew ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] net: usb: pegasus: Do not drop long Ethernet frames 2021-12-26 15:39 ` Andrew Lunn @ 2021-12-26 16:01 ` Matthias-Christian Ott 2021-12-26 16:31 ` Andrew Lunn 2021-12-26 22:28 ` Petko Manolov 0 siblings, 2 replies; 8+ messages in thread From: Matthias-Christian Ott @ 2021-12-26 16:01 UTC (permalink / raw) To: Andrew Lunn; +Cc: Petko Manolov, linux-usb, netdev On 26/12/2021 16:39, Andrew Lunn wrote: > On Sun, Dec 26, 2021 at 02:29:30PM +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). >> >> Signed-off-by: Matthias-Christian Ott <ott@mirix.org> >> --- >> 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. Kind regards, Matthias-Christian Ott ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] net: usb: pegasus: Do not drop long Ethernet frames 2021-12-26 16:01 ` Matthias-Christian Ott @ 2021-12-26 16:31 ` Andrew Lunn 2021-12-26 22:21 ` Matthias-Christian Ott 2021-12-26 22:28 ` Petko Manolov 1 sibling, 1 reply; 8+ messages in thread From: Andrew Lunn @ 2021-12-26 16:31 UTC (permalink / raw) To: Matthias-Christian Ott; +Cc: Petko Manolov, linux-usb, netdev > > 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. Agreed, keep fixes simple. A few other hints. If you consider this a fix which should be back ported, please add a Fixes: tag, where the issue started. This can be back as far as the first commit for the driver. Fixes should also be sent to the net tree, not net-next. See the netdev FAQ about the two different trees. Andrew ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] net: usb: pegasus: Do not drop long Ethernet frames 2021-12-26 16:31 ` Andrew Lunn @ 2021-12-26 22:21 ` Matthias-Christian Ott 0 siblings, 0 replies; 8+ messages in thread From: Matthias-Christian Ott @ 2021-12-26 22:21 UTC (permalink / raw) To: Andrew Lunn; +Cc: Petko Manolov, linux-usb, netdev On 26/12/2021 17:31, Andrew Lunn wrote: >>> 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. > > Agreed, keep fixes simple. > > A few other hints. If you consider this a fix which should be back > ported, please add a Fixes: tag, where the issue started. This can be > back as far as the first commit for the driver. Fixes should also be > sent to the net tree, not net-next. See the netdev FAQ about the two > different trees. I made a v2 of the patch and also added the prefix flag to indicate that the patch is destined for the "next" tree. There is still something that can be improved a about it, please let me know. Otherwise, I will also resend the other patch that I sent for the driver to indicate that it is destined for the "next" tree. Kind regards, Matthias-Christian Ott ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] net: usb: pegasus: Do not drop long Ethernet frames 2021-12-26 16:01 ` Matthias-Christian Ott 2021-12-26 16:31 ` Andrew Lunn @ 2021-12-26 22:28 ` Petko Manolov 1 sibling, 0 replies; 8+ messages in thread From: Petko Manolov @ 2021-12-26 22:28 UTC (permalink / raw) To: Matthias-Christian Ott; +Cc: Andrew Lunn, linux-usb, netdev On 21-12-26 17:01:24, Matthias-Christian Ott wrote: > On 26/12/2021 16:39, Andrew Lunn wrote: > > On Sun, Dec 26, 2021 at 02:29:30PM +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). > >> > >> Signed-off-by: Matthias-Christian Ott <ott@mirix.org> > >> --- > >> 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] net: usb: pegasus: Do not drop long Ethernet frames 2021-12-26 13:29 [PATCH] net: usb: pegasus: Do not drop long Ethernet frames Matthias-Christian Ott 2021-12-26 15:39 ` Andrew Lunn @ 2021-12-27 9:56 ` Sergei Shtylyov 2022-01-02 14:22 ` Matthias-Christian Ott 1 sibling, 1 reply; 8+ messages in thread From: Sergei Shtylyov @ 2021-12-27 9:56 UTC (permalink / raw) To: Matthias-Christian Ott, Petko Manolov; +Cc: linux-usb, netdev Hello! On 26.12.2021 16:29, 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 ^^^^^ ^^^^^ Too many limits. :-) > 1536 octets (PEGASUS_MTU). > > Signed-off-by: Matthias-Christian Ott <ott@mirix.org> [...] MBR, Sergey ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] net: usb: pegasus: Do not drop long Ethernet frames 2021-12-27 9:56 ` Sergei Shtylyov @ 2022-01-02 14:22 ` Matthias-Christian Ott 0 siblings, 0 replies; 8+ messages in thread From: Matthias-Christian Ott @ 2022-01-02 14:22 UTC (permalink / raw) To: Sergei Shtylyov, Petko Manolov; +Cc: linux-usb, netdev On 27/12/2021 10:56, Sergei Shtylyov wrote: >> 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 > ^^^^^ ^^^^^ > Too many limits. :-) Thank you for pointing this out. Indeed, this sentence does make sense. :) Unfortunately, the patch was already merged and it seems that I can't change its commit message anymore. Nonetheless, thank you for reviewing it. Kind regards, Matthias-Christian Ott ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-01-02 14:22 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-12-26 13:29 [PATCH] net: usb: pegasus: Do not drop long Ethernet frames Matthias-Christian Ott 2021-12-26 15:39 ` Andrew Lunn 2021-12-26 16:01 ` Matthias-Christian Ott 2021-12-26 16:31 ` Andrew Lunn 2021-12-26 22:21 ` Matthias-Christian Ott 2021-12-26 22:28 ` Petko Manolov 2021-12-27 9:56 ` Sergei Shtylyov 2022-01-02 14:22 ` Matthias-Christian Ott
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).