em: minimum ethernet frame size

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

em: minimum ethernet frame size

Michele Curti
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_SIZE            4
#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 -0000 1.336
+++ sys/dev/pci/if_em.c 2 Feb 2018 11:13:43 -0000
@@ -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),

Reply | Threaded
Open this post in threaded view
|

Re: em: minimum ethernet frame size

Chris Cappuccio
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 [[hidden email]] 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_SIZE            4
> #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 -0000 1.336
> +++ sys/dev/pci/if_em.c 2 Feb 2018 11:13:43 -0000
> @@ -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),

Reply | Threaded
Open this post in threaded view
|

Re: em: minimum ethernet frame size

Martin Pieuchot
In reply to this post by Michele Curti
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_SIZE            4
> #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 -0000 1.336
> +++ sys/dev/pci/if_em.c 2 Feb 2018 11:13:43 -0000
> @@ -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),
>

Reply | Threaded
Open this post in threaded view
|

Re: em: minimum ethernet frame size

Michele Curti
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_SIZE            4
> > #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 -0000 1.336
> > +++ sys/dev/pci/if_em.c 2 Feb 2018 11:13:43 -0000
> > @@ -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),
> >