Re: kern/34799: IP Filter does not work correctly with gem(4) when hardware chec

To: kern-bug-people%netbsd.org@localhost,gnats-admin%netbsd.org@localhost,netbsd-bugs%netbsd.org@localhost,dhgutteridge%sympatico.ca@localhost
Subject: Re: kern/34799: IP Filter does not work correctly with gem(4) when hardware chec
From: Julian Coleman <jdc%coris.org.uk@localhost>
Date: Tue, 29 Jan 2008 19:50:02 +0000 (UTC)

The following reply was made to PR kern/34799; it has been noted by GNATS.

From: Julian Coleman <jdc%coris.org.uk@localhost>
To: "David H. Gutteridge" <dhgutteridge%sympatico.ca@localhost>
Cc: gnats-bugs%netbsd.org@localhost
Subject: Re: kern/34799: IP Filter does not work correctly with gem(4) when 
hardware chec
Date: Tue, 29 Jan 2008 19:48:31 +0000

 --rwEMma7ioTxnRzrJ
 Content-Type: text/plain; charset=us-ascii
 Content-Disposition: inline
 
 Hi,
 
 > The CSUM_TCP_SUM16 flag is used to indicate that the GMAC's checksum is
 > being used. The GMAC hardware checksum has some limitations. There are a lot
 > of checks for the various limitations to avoid cases where the hardware does
 > the wrong thing. I believe IP fragments may be one of the cases where the
 > chipset does the wrong thing."
 
 It looks like other people have problems here - which is good news in that
 it is the hardware that's broken, not our driver (well, not here anyway ...).
 
 > Could it be that this problem only appears on macppc because specific
 > GMAC chips are defective?  I don't know anything about GMAC, but I
 > wonder if there were bugs present in the chips shipped in (some) Apple
 > machines, but perhaps not in the ones used by Sun?  (Though there was
 > that old IPFilter FAQ entry about the eri driver under Solaris not
 > working with hardware checksumming too...  Maybe it's the same problem,
 > and it affects very specific Sun hardware as well.)
 > 
 > My gem card shows it's revision 0:
 > gem0 at pci2 dev 15 function 0: Apple Computer GMAC Ethernet (rev. 0x00)
 > 
 > My guess is that revision 1 GMAC chips behave differently in this
 > regard.
 
 It could be.  The cards that I have are both rev. 0x01 and they exhibit
 the problem, but they are Sun cards (one is ERI and one is GEM).  So, I
 think that disabling HW receive checksums for Apple rev. 0 and for Sun
 rev. 1 chips is the way to go.
 
 > PS Having tested the GENERIC kernel from HEAD, it seems that gem no
 > longer supports UDP checksumming?  It did with 4.0.
 
 Yes.  I took that out after reading comments in the FreeBSD version of the
 driver - it notes that the hardware checksum doesn't compensate for the UDP
 case where the checksum ends up being all 0.  They did leave the ability to
 enable UDP checksums with `ifconfig gem0 link0`, but I don't see the
 usefulness of that, so I didn't put it in our driver.  So, we are left with
 just TCP hardware checksums, and only outgoing on the buggy revisions.
 
 If you could try the attached patch, that would be great.  If it's OK, I'll
 check it in and close the PR (if that's OK with you?).
 
 Thanks,
 
 J
 
 PS.  Would you have any interest in the newer gem code being in the 4.0
 branch?  My test machine is running that, so I have the changes that I've
 made to the driver working on the branch.  It'll just be a case of sorting
 out the appropriate revisions for the pull up request.
 
 -- 
   My other computer also runs NetBSD    /        Sailing at Newbiggin
         http://www.netbsd.org/        /   http://www.newbigginsailingclub.org/
 
 --rwEMma7ioTxnRzrJ
 Content-Type: text/plain; charset=us-ascii
 Content-Disposition: attachment; filename="gem.diffs"
 
 Index: sys/dev/ic/gem.c
 ===================================================================
 RCS file: /cvsroot/src/sys/dev/ic/gem.c,v
 retrieving revision 1.72
 diff -u -r1.72 gem.c
 --- sys/dev/ic/gem.c   22 Jan 2008 23:19:14 -0000      1.72
 +++ sys/dev/ic/gem.c   29 Jan 2008 19:34:09 -0000
 @@ -416,9 +416,15 @@
        ifp->if_flags =
            IFF_BROADCAST | IFF_SIMPLEX | IFF_NOTRAILERS | IFF_MULTICAST;
        sc->sc_if_flags = ifp->if_flags;
 -      /* The GEM hardware supports basic TCP checksum offloading only. */
 -      ifp->if_capabilities |=
 -          IFCAP_CSUM_TCPv4_Tx | IFCAP_CSUM_TCPv4_Rx;
 +      /*
 +       * The GEM hardware supports basic TCP checksum offloading only.
 +       * Sun revision 1 and Apple revision 0 (at least) have bugs in the
 +       * receive checksum, so don't enable it for them.
 +       */
 +      ifp->if_capabilities |= IFCAP_CSUM_TCPv4_Tx;
 +      if ((GEM_IS_SUN(sc) && sc->sc_chiprev != 1) ||
 +          (GEM_IS_APPLE(sc) && sc->sc_chiprev != 0))
 +              ifp->if_capabilities |= IFCAP_CSUM_TCPv4_Rx;
        ifp->if_start = gem_start;
        ifp->if_ioctl = gem_ioctl;
        ifp->if_watchdog = gem_watchdog;
 @@ -2144,9 +2150,6 @@
        }
        if (status & GEM_INTR_RX_MAC) {
                int rxstat = bus_space_read_4(t, h, GEM_MAC_RX_STATUS);
 -              if (rxstat & ~GEM_MAC_RX_DONE)
 -                      printf("%s: MAC rx fault, status %x\n",
 -                          sc->sc_dev.dv_xname, rxstat);
                /*
                 * At least with GEM_SUN_GEM and some GEM_SUN_ERI
                 * revisions GEM_MAC_RX_OVERFLOW happen often due to a
 @@ -2157,7 +2160,7 @@
                        ifp->if_ierrors++;
                        gem_reset_rxdma(sc);
                } else if (rxstat & ~(GEM_MAC_RX_DONE | GEM_MAC_RX_FRAME_CNT))
 -                      printf("%s: MAC rx fault, status %x\n",
 +                      printf("%s: MAC rx fault, status 0x%02x\n",
                            sc->sc_dev.dv_xname, rxstat);
        }
        if (status & GEM_INTR_PCS) {
 Index: sys/dev/ic/gemvar.h
 ===================================================================
 RCS file: /cvsroot/src/sys/dev/ic/gemvar.h,v
 retrieving revision 1.17
 diff -u -r1.17 gemvar.h
 --- sys/dev/ic/gemvar.h        19 Jan 2008 22:20:42 -0000      1.17
 +++ sys/dev/ic/gemvar.h        29 Jan 2008 19:34:14 -0000
 @@ -139,10 +139,15 @@
  #define       GEM_APPLE_GMAC          3       /* Apple GMAC variant */
  #define GEM_APPLE_K2_GMAC     4       /* Apple K2 GMAC */
  
 +#define GEM_IS_SUN(sc) \
 +      ((sc)->sc_variant == GEM_SUN_GEM || \
 +      (sc)->sc_variant == GEM_SUN_ERI)
  #define       GEM_IS_APPLE(sc) \
        ((sc)->sc_variant == GEM_APPLE_GMAC || \
        (sc)->sc_variant == GEM_APPLE_K2_GMAC)
  
 +      int             sc_chiprev;     /* hardware revision */
 +
        u_int           sc_flags;       /* */
        short           sc_if_flags;    /* copy of ifp->if_flags */
  #define       GEM_GIGABIT             0x0001  /* has a gigabit PHY */
 Index: sys/dev/pci/if_gem_pci.c
 ===================================================================
 RCS file: /cvsroot/src/sys/dev/pci/if_gem_pci.c,v
 retrieving revision 1.25
 diff -u -r1.25 if_gem_pci.c
 --- sys/dev/pci/if_gem_pci.c   5 Jan 2008 20:30:22 -0000       1.25
 +++ sys/dev/pci/if_gem_pci.c   29 Jan 2008 19:34:19 -0000
 @@ -194,8 +194,8 @@
        aprint_naive(": Ethernet controller\n");
  
        pci_devinfo(pa->pa_id, pa->pa_class, 0, devinfo, sizeof(devinfo));
 -      aprint_normal(": %s (rev. 0x%02x)\n", devinfo,
 -          PCI_REVISION(pa->pa_class));
 +      sc->sc_chiprev = PCI_REVISION(pa->pa_class);
 +      aprint_normal(": %s (rev. 0x%02x)\n", devinfo, sc->sc_chiprev);
  
        /*
         * Some Sun GEMs/ERIs do have their intpin register bogusly set to 0,
 
 --rwEMma7ioTxnRzrJ--

Prev by Date: port-hpcmips/37911: Missing entries in TX3922-config template
Next by Date: NetBSD Nightly Trouble Ticket Report
Previous by Thread: Re: kern/34799: IP Filter does not work correctly with gem(4) when hardware chec
Next by Thread: Re: kern/34799: IP Filter does not work correctly with gem(4) when hardware chec
Indexes:

Home | Main Index | Thread Index | Old Index