Re: em: minimum ethernet frame size
On Fri, Feb 09, 2018 at 03:40:13PM +0100, Martin Pieuchot wrote: > On 02/02/18(Fri) 12:18, Michele Curti wrote: > > Hi, > > in sys/dev/pci/if_em.c at line 469 there is: > > sc->hw.min_frame_size = > > ETHER_MIN_LEN + ETHER_CRC_LEN; > > > > But ETHER_MIN_LEN already includes the CRC size: > > #define ETHER_MIN_LEN 64 /* Minimum frame length, CRC included */ > > > > In revision 1.41 the code changed in this way: > > sc->hw.min_frame_size = > > - MINIMUM_ETHERNET_PACKET_SIZE + ETHER_CRC_LEN; > > + ETHER_MIN_LEN + ETHER_CRC_LEN; > > > > But MINIMUM_ETHERNET_PACKET_SIZE was the size without CRC. > > > > #define MINIMUM_ETHERNET_FRAME_SIZE 64 /* With FCS */ > > #define ETHERNET_FCS_SIZE4 > > #define MINIMUM_ETHERNET_PACKET_SIZE \ > > (MINIMUM_ETHERNET_FRAME_SIZE - ETHERNET_FCS_SIZE) > > > > So I attached the following diff to restore the original minimum > > size (by the way, what's the effect of having min_frame_size set > > to 68? Discarding valid packets sized from 64 to 67?) > > I agree with your analyse. How did you end up looking at this? Does > this fix a problem for you? Hi Martin, no, I had no problems. I was reading "OpenBSD network stack internals" by Claudio Jeker, and I was just curious about it. So I started reading the code from the interrupt routine and had trouble with the byte count. The resolution led to this patch, but I haven't been able to understand if the patch may lead to regressions, despite the hints from other developers eheh... Regards, Michele > > > Index: sys/dev/pci/if_em.c > > === > > RCS file: /cvs/src/sys/dev/pci/if_em.c,v > > retrieving revision 1.336 > > diff -u -p -r1.336 if_em.c > > --- sys/dev/pci/if_em.c 25 Jul 2017 20:45:18 - 1.336 > > +++ sys/dev/pci/if_em.c 2 Feb 2018 11:13:43 - > > @@ -466,8 +466,7 @@ em_attach(struct device *parent, struct > > MAX_JUMBO_FRAME_SIZE; > > } > > > > - sc->hw.min_frame_size = > > - ETHER_MIN_LEN + ETHER_CRC_LEN; > > + sc->hw.min_frame_size = ETHER_MIN_LEN; > > > > /* Allocate Transmit Descriptor ring */ > > if (em_dma_malloc(sc, sc->sc_tx_slots * sizeof(struct em_tx_desc), > >
Re: em: minimum ethernet frame size
On 02/02/18(Fri) 12:18, Michele Curti wrote: > Hi, > in sys/dev/pci/if_em.c at line 469 there is: > sc->hw.min_frame_size = > ETHER_MIN_LEN + ETHER_CRC_LEN; > > But ETHER_MIN_LEN already includes the CRC size: > #define ETHER_MIN_LEN 64 /* Minimum frame length, CRC included */ > > In revision 1.41 the code changed in this way: > sc->hw.min_frame_size = > - MINIMUM_ETHERNET_PACKET_SIZE + ETHER_CRC_LEN; > + ETHER_MIN_LEN + ETHER_CRC_LEN; > > But MINIMUM_ETHERNET_PACKET_SIZE was the size without CRC. > > #define MINIMUM_ETHERNET_FRAME_SIZE 64 /* With FCS */ > #define ETHERNET_FCS_SIZE4 > #define MINIMUM_ETHERNET_PACKET_SIZE \ > (MINIMUM_ETHERNET_FRAME_SIZE - ETHERNET_FCS_SIZE) > > So I attached the following diff to restore the original minimum > size (by the way, what's the effect of having min_frame_size set > to 68? Discarding valid packets sized from 64 to 67?) I agree with your analyse. How did you end up looking at this? Does this fix a problem for you? > Index: sys/dev/pci/if_em.c > === > RCS file: /cvs/src/sys/dev/pci/if_em.c,v > retrieving revision 1.336 > diff -u -p -r1.336 if_em.c > --- sys/dev/pci/if_em.c 25 Jul 2017 20:45:18 - 1.336 > +++ sys/dev/pci/if_em.c 2 Feb 2018 11:13:43 - > @@ -466,8 +466,7 @@ em_attach(struct device *parent, struct > MAX_JUMBO_FRAME_SIZE; > } > > - sc->hw.min_frame_size = > - ETHER_MIN_LEN + ETHER_CRC_LEN; > + sc->hw.min_frame_size = ETHER_MIN_LEN; > > /* Allocate Transmit Descriptor ring */ > if (em_dma_malloc(sc, sc->sc_tx_slots * sizeof(struct em_tx_desc), >
Re: em: minimum ethernet frame size
sc->hw.min_frame_size is only used for TBI mode, which is only available on the 82543 according to em_set_media_type() You'd need to carefully analyze how the TBI_ACCEPT macro is used to see what's right here. The macro even seems to assume that the vlan tag size might be part of min_frame_size. Michele Curti [michele.cu...@gmail.com] wrote: > Hi, > in sys/dev/pci/if_em.c at line 469 there is: > sc->hw.min_frame_size = > ETHER_MIN_LEN + ETHER_CRC_LEN; > > But ETHER_MIN_LEN already includes the CRC size: > #define ETHER_MIN_LEN 64 /* Minimum frame length, CRC included */ > > In revision 1.41 the code changed in this way: > sc->hw.min_frame_size = > - MINIMUM_ETHERNET_PACKET_SIZE + ETHER_CRC_LEN; > + ETHER_MIN_LEN + ETHER_CRC_LEN; > > But MINIMUM_ETHERNET_PACKET_SIZE was the size without CRC. > > #define MINIMUM_ETHERNET_FRAME_SIZE 64 /* With FCS */ > #define ETHERNET_FCS_SIZE4 > #define MINIMUM_ETHERNET_PACKET_SIZE \ > (MINIMUM_ETHERNET_FRAME_SIZE - ETHERNET_FCS_SIZE) > > So I attached the following diff to restore the original minimum > size (by the way, what's the effect of having min_frame_size set > to 68? Discarding valid packets sized from 64 to 67?) > > Regards, > Michele > > > Index: sys/dev/pci/if_em.c > === > RCS file: /cvs/src/sys/dev/pci/if_em.c,v > retrieving revision 1.336 > diff -u -p -r1.336 if_em.c > --- sys/dev/pci/if_em.c 25 Jul 2017 20:45:18 - 1.336 > +++ sys/dev/pci/if_em.c 2 Feb 2018 11:13:43 - > @@ -466,8 +466,7 @@ em_attach(struct device *parent, struct > MAX_JUMBO_FRAME_SIZE; > } > > - sc->hw.min_frame_size = > - ETHER_MIN_LEN + ETHER_CRC_LEN; > + sc->hw.min_frame_size = ETHER_MIN_LEN; > > /* Allocate Transmit Descriptor ring */ > if (em_dma_malloc(sc, sc->sc_tx_slots * sizeof(struct em_tx_desc),
em: minimum ethernet frame size
Hi, in sys/dev/pci/if_em.c at line 469 there is: sc->hw.min_frame_size = ETHER_MIN_LEN + ETHER_CRC_LEN; But ETHER_MIN_LEN already includes the CRC size: #define ETHER_MIN_LEN 64 /* Minimum frame length, CRC included */ In revision 1.41 the code changed in this way: sc->hw.min_frame_size = - MINIMUM_ETHERNET_PACKET_SIZE + ETHER_CRC_LEN; + ETHER_MIN_LEN + ETHER_CRC_LEN; But MINIMUM_ETHERNET_PACKET_SIZE was the size without CRC. #define MINIMUM_ETHERNET_FRAME_SIZE 64 /* With FCS */ #define ETHERNET_FCS_SIZE4 #define MINIMUM_ETHERNET_PACKET_SIZE \ (MINIMUM_ETHERNET_FRAME_SIZE - ETHERNET_FCS_SIZE) So I attached the following diff to restore the original minimum size (by the way, what's the effect of having min_frame_size set to 68? Discarding valid packets sized from 64 to 67?) Regards, Michele Index: sys/dev/pci/if_em.c === RCS file: /cvs/src/sys/dev/pci/if_em.c,v retrieving revision 1.336 diff -u -p -r1.336 if_em.c --- sys/dev/pci/if_em.c 25 Jul 2017 20:45:18 - 1.336 +++ sys/dev/pci/if_em.c 2 Feb 2018 11:13:43 - @@ -466,8 +466,7 @@ em_attach(struct device *parent, struct MAX_JUMBO_FRAME_SIZE; } - sc->hw.min_frame_size = - ETHER_MIN_LEN + ETHER_CRC_LEN; + sc->hw.min_frame_size = ETHER_MIN_LEN; /* Allocate Transmit Descriptor ring */ if (em_dma_malloc(sc, sc->sc_tx_slots * sizeof(struct em_tx_desc),
