On Sat, Aug 18, 2018 at 10:38:36AM +0200, Stefan Sperling wrote: > Thank you for taking care of axen(4). > The problems with this driver have been known for some time. > > On Tue, Aug 07, 2018 at 02:04:24PM +0000, sc.dy...@gmail.com wrote: > > - header: fix comments > > - header: fix unused L3 type mask definition > > - rxeof: Avoid allocating mbuf if checksum errors are detected. > > - rxeof: Avoid loop to extract packets if pkt_count is 0. > > - rxeof: Add more sanity checks. > > - rxeof: Increament if_ierror in some error paths. > > - qctrl: Apply queuing control parameters from FreeBSD axge(4). > > - qctrl: Set qctrl in miireg_statchg dynamically, not statically. > > - Use DMA buffer aligned at 64KB boundary to avoid xhci bug. > > You're actually switching to a 64KB buffer size as well, not only to > 64KB alignment. That is a relatively large size. The previous code > was using 8KB, 16KB, and 24KB buffers. Do I understand correctly > that the qctrl configuration allows hardware to provide only up > to 0x18*1024 = 24KB bytes of packet data per Rx interrupt? > > Regardless, in my opinion your diff improves this driver which > has not seen any regular maintainance in a long time. > If another developer (remi? mlarkin?) provides an OK I will commit it. >
I wanted to test this. Unfortunately the axen4.diff does not apply to my -current tree. Is it because of the white space issue mentioned in the thread? Or do I need to first apply one of the other patches? It would help if you could send a clean version that applies to -current. Thank you for looking at improving axen(4)! Remi > > --- sys/dev/usb/if_axenreg.h Fri Sep 16 22:17:07 2016 > > +++ sys/dev/usb/if_axenreg.h Mon Jun 19 10:54:28 2017 > > @@ -26,8 +26,8 @@ > > * | | ++-----L3_type (1:ipv4, 0/2:ipv6) > > * pkt_len(13) | | ||+ ++-L4_type(0: icmp, 1: UDP, 4: TCP) > > * |765|43210 76543210|7654 3210 7654 3210| > > - * ||+-crc_err |+-L4_err |+-L4_CSUM_ERR > > - * |+-mii_err +--L3_err +--L3_CSUM_ERR > > + * ||+-crc_err |+-L4_err |+-L4_CSUM_ERR > > + * |+-mii_err +--L3_err +--L3_CSUM_ERR > > * +-drop_err > > * > > * ex) pkt_hdr 0x00680820 > > @@ -70,7 +70,7 @@ > > #define AXEN_RXHDR_L4_TYPE_TCP 0x4 > > > > /* L3 packet type (2bit) */ > > -#define AXEN_RXHDR_L3_TYPE_MASK 0x00000600 > > +#define AXEN_RXHDR_L3_TYPE_MASK 0x00000060 > > #define AXEN_RXHDR_L3_TYPE_OFFSET 5 > > #define AXEN_RXHDR_L3_TYPE_UNDEF 0x0 > > #define AXEN_RXHDR_L3_TYPE_IPV4 0x1 > > --- sys/dev/usb/if_axen.c.orig Tue Jun 12 15:36:59 2018 > > +++ sys/dev/usb/if_axen.c Sun Jul 29 01:53:43 2018 > > @@ -53,6 +53,7 @@ > > #include <dev/usb/usbdi_util.h> > > #include <dev/usb/usbdivar.h> > > #include <dev/usb/usbdevs.h> > > +#include <dev/usb/usb_mem.h> > > > > #include <dev/usb/if_axenreg.h> > > > > @@ -121,6 +122,13 @@ void axen_unlock_mii(struct axen_softc *sc); > > > > void axen_ax88179_init(struct axen_softc *); > > > > +struct axen_qctrl axen_bulk_size[] = { > > + { 7, 0x4f, 0x00, 0x12, 0xff }, > > + { 7, 0x20, 0x03, 0x16, 0xff }, > > + { 7, 0xae, 0x07, 0x18, 0xff }, > > + { 7, 0xcc, 0x4c, 0x18, 0x08 } > > +}; > > + > > /* Get exclusive access to the MII registers */ > > void > > axen_lock_mii(struct axen_softc *sc) > > @@ -238,6 +246,8 @@ axen_miibus_statchg(struct device *dev) > > int err; > > uint16_t val; > > uWord wval; > > + uint8_t linkstat = 0; > > + int qctrl; > > > > ifp = GET_IFP(sc); > > if (mii == NULL || ifp == NULL || > > @@ -265,27 +275,49 @@ axen_miibus_statchg(struct device *dev) > > return; > > > > val = 0; > > - if ((IFM_OPTIONS(mii->mii_media_active) & IFM_FDX) != 0) > > + if ((IFM_OPTIONS(mii->mii_media_active) & IFM_FDX) != 0) { > > val |= AXEN_MEDIUM_FDX; > > + if ((IFM_OPTIONS(mii->mii_media_active) & IFM_ETH_TXPAUSE) != 0) > > + val |= AXEN_MEDIUM_TXFLOW_CTRL_EN; > > + if ((IFM_OPTIONS(mii->mii_media_active) & IFM_ETH_RXPAUSE) != 0) > > + val |= AXEN_MEDIUM_RXFLOW_CTRL_EN; > > + } > > > > - val |= (AXEN_MEDIUM_RECV_EN | AXEN_MEDIUM_ALWAYS_ONE); > > - val |= (AXEN_MEDIUM_RXFLOW_CTRL_EN | AXEN_MEDIUM_TXFLOW_CTRL_EN); > > + val |= AXEN_MEDIUM_RECV_EN; > > > > + /* bulkin queue setting */ > > + axen_lock_mii(sc); > > + axen_cmd(sc, AXEN_CMD_MAC_READ, 1, AXEN_USB_UPLINK, &linkstat); > > + axen_unlock_mii(sc); > > + > > switch (IFM_SUBTYPE(mii->mii_media_active)) { > > case IFM_1000_T: > > val |= AXEN_MEDIUM_GIGA | AXEN_MEDIUM_EN_125MHZ; > > + if (linkstat & AXEN_USB_SS) > > + qctrl = 0; > > + else if (linkstat & AXEN_USB_HS) > > + qctrl = 1; > > + else > > + qctrl = 3; > > break; > > case IFM_100_TX: > > val |= AXEN_MEDIUM_PS; > > + if (linkstat & (AXEN_USB_SS | AXEN_USB_HS)) > > + qctrl = 2; > > + else > > + qctrl = 3; > > break; > > case IFM_10_T: > > - /* doesn't need to be handled */ > > + default: > > + qctrl = 3; > > break; > > } > > > > DPRINTF(("axen_miibus_statchg: val=0x%x\n", val)); > > USETW(wval, val); > > axen_lock_mii(sc); > > + axen_cmd(sc, AXEN_CMD_MAC_SET_RXSR, 5, AXEN_RX_BULKIN_QCTRL, > > + &axen_bulk_size[qctrl]); > > err = axen_cmd(sc, AXEN_CMD_MAC_WRITE2, 2, AXEN_MEDIUM_STATUS, &wval); > > axen_unlock_mii(sc); > > if (err) { > > @@ -408,7 +440,6 @@ axen_ax88179_init(struct axen_softc *sc) > > uWord wval; > > uByte val; > > u_int16_t ctl, temp; > > - struct axen_qctrl qctrl; > > > > axen_lock_mii(sc); > > > > @@ -471,27 +502,12 @@ axen_ax88179_init(struct axen_softc *sc) > > switch (val) { > > case AXEN_USB_FS: > > DPRINTF(("uplink: USB1.1\n")); > > - qctrl.ctrl = 0x07; > > - qctrl.timer_low = 0xcc; > > - qctrl.timer_high= 0x4c; > > - qctrl.bufsize = AXEN_BUFSZ_LS - 1; > > - qctrl.ifg = 0x08; > > break; > > case AXEN_USB_HS: > > DPRINTF(("uplink: USB2.0\n")); > > - qctrl.ctrl = 0x07; > > - qctrl.timer_low = 0x02; > > - qctrl.timer_high= 0xa0; > > - qctrl.bufsize = AXEN_BUFSZ_HS - 1; > > - qctrl.ifg = 0xff; > > break; > > case AXEN_USB_SS: > > DPRINTF(("uplink: USB3.0\n")); > > - qctrl.ctrl = 0x07; > > - qctrl.timer_low = 0x4f; > > - qctrl.timer_high= 0x00; > > - qctrl.bufsize = AXEN_BUFSZ_SS - 1; > > - qctrl.ifg = 0xff; > > break; > > default: > > printf("%s: unknown uplink bus:0x%02x\n", > > @@ -499,7 +515,6 @@ axen_ax88179_init(struct axen_softc *sc) > > axen_unlock_mii(sc); > > return; > > } > > - axen_cmd(sc, AXEN_CMD_MAC_SET_RXSR, 5, AXEN_RX_BULKIN_QCTRL, &qctrl); > > > > /* Set MAC address. */ > > axen_cmd(sc, AXEN_CMD_MAC_WRITE_ETHER, ETHER_ADDR_LEN, > > @@ -622,22 +637,8 @@ axen_attach(struct device *parent, struct device *self > > > > id = usbd_get_interface_descriptor(sc->axen_iface); > > > > - /* decide on what our bufsize will be */ > > - switch (sc->axen_udev->speed) { > > - case USB_SPEED_FULL: > > - sc->axen_bufsz = AXEN_BUFSZ_LS * 1024; > > - break; > > - case USB_SPEED_HIGH: > > - sc->axen_bufsz = AXEN_BUFSZ_HS * 1024; > > - break; > > - case USB_SPEED_SUPER: > > - sc->axen_bufsz = AXEN_BUFSZ_SS * 1024; > > - break; > > - default: > > - printf("%s: not supported usb bus type", sc->axen_dev.dv_xname); > > - return; > > - } > > - > > + sc->axen_bufsz = 64 * 1024; > > + > > /* Find endpoints. */ > > for (i = 0; i < id->bNumEndpoints; i++) { > > ed = usbd_interface2endpoint_descriptor(sc->axen_iface, i); > > @@ -710,7 +711,8 @@ axen_attach(struct device *parent, struct device *self > > mii->mii_flags = MIIF_AUTOTSLEEP; > > > > ifmedia_init(&mii->mii_media, 0, axen_ifmedia_upd, axen_ifmedia_sts); > > - mii_attach(self, mii, 0xffffffff, MII_PHY_ANY, MII_OFFSET_ANY, 0); > > + mii_attach(self, mii, 0xffffffff, MII_PHY_ANY, MII_OFFSET_ANY, > > + MIIF_DOPAUSE); > > > > if (LIST_FIRST(&mii->mii_phys) == NULL) { > > ifmedia_add(&mii->mii_media, IFM_ETHER | IFM_NONE, 0, NULL); > > @@ -808,6 +810,23 @@ axen_newbuf(void) > > return m; > > } > > > > +static void * > > +axen_alloc_buffer(struct usbd_xfer *xfer, u_int32_t size) > > +{ > > + struct usbd_bus *bus = xfer->device->bus; > > + usbd_status err; > > + > > +#ifdef DIAGNOSTIC > > + if (xfer->rqflags & (URQ_DEV_DMABUF | URQ_AUTO_DMABUF)) > > + printf("axen_alloc_buffer: xfer already has a buffer\n"); > > +#endif > > + err = usb_allocmem(bus, size, 65536, &xfer->dmabuf); > > + if (err) > > + return (NULL); > > + xfer->rqflags |= URQ_DEV_DMABUF; > > + return (KERNADDR(&xfer->dmabuf, 0)); > > +} > > + > > int > > axen_rx_list_init(struct axen_softc *sc) > > { > > @@ -827,7 +846,7 @@ axen_rx_list_init(struct axen_softc *sc) > > c->axen_xfer = usbd_alloc_xfer(sc->axen_udev); > > if (c->axen_xfer == NULL) > > return ENOBUFS; > > - c->axen_buf = usbd_alloc_buffer(c->axen_xfer, > > + c->axen_buf = axen_alloc_buffer(c->axen_xfer, > > sc->axen_bufsz); > > if (c->axen_buf == NULL) { > > usbd_free_xfer(c->axen_xfer); > > @@ -858,7 +877,7 @@ axen_tx_list_init(struct axen_softc *sc) > > c->axen_xfer = usbd_alloc_xfer(sc->axen_udev); > > if (c->axen_xfer == NULL) > > return ENOBUFS; > > - c->axen_buf = usbd_alloc_buffer(c->axen_xfer, > > + c->axen_buf = axen_alloc_buffer(c->axen_xfer, > > sc->axen_bufsz); > > if (c->axen_buf == NULL) { > > usbd_free_xfer(c->axen_xfer); > > @@ -888,7 +907,6 @@ axen_rxeof(struct usbd_xfer *xfer, void *priv, usbd_st > > u_int32_t *hdr_p; > > u_int16_t hdr_offset, pkt_count; > > size_t pkt_len; > > - size_t temp; > > int s; > > > > DPRINTFN(10,("%s: %s: enter\n", sc->axen_dev.dv_xname,__func__)); > > @@ -914,10 +932,19 @@ axen_rxeof(struct usbd_xfer *xfer, void *priv, usbd_st > > usbd_get_xfer_status(xfer, NULL, NULL, &total_len, NULL); > > > > if (total_len < sizeof(pkt_hdr)) { > > + printf("%s: rxeof: too short transfer\n", > > + sc->axen_dev.dv_xname); > > ifp->if_ierrors++; > > goto done; > > } > > > > + if (total_len > sc->axen_bufsz) { > > + printf("%s: rxeof: too large transfer\n", > > + sc->axen_dev.dv_xname); > > + ifp->if_ierrors++; > > + goto done; > > + } > > + > > /* > > * buffer map > > * [packet #0]...[packet #n][pkt hdr#0]..[pkt hdr#n][recv_hdr] > > @@ -928,16 +955,10 @@ axen_rxeof(struct usbd_xfer *xfer, void *priv, usbd_st > > hdr_offset = (u_int16_t)(rx_hdr >> 16); > > pkt_count = (u_int16_t)(rx_hdr & 0xffff); > > > > - if (total_len > sc->axen_bufsz) { > > - printf("%s: rxeof: too large transfer\n", > > - sc->axen_dev.dv_xname); > > - goto done; > > - } > > - > > /* sanity check */ > > if (hdr_offset > total_len) { > > - ifp->if_ierrors++; > > usbd_delay_ms(sc->axen_udev, 100); > > + ifp->if_ierrors++; > > goto done; > > } > > > > @@ -954,11 +975,14 @@ axen_rxeof(struct usbd_xfer *xfer, void *priv, usbd_st > > if (pkt_count > AXEN_MAX_PACKED_PACKET) { > > DPRINTF(("Too many packets (%d) in a transaction, discard.\n", > > pkt_count)); > > + ifp->if_ierrors++; > > goto done; > > } > > #endif > > > > - do { > > + for (; pkt_count > 0; pkt_count--) { > > + uint16_t csum_flags = 0; > > + > > if ((buf[0] != 0xee) || (buf[1] != 0xee)){ > > printf("%s: invalid buffer(pkt#%d), continue\n", > > sc->axen_dev.dv_xname, pkt_count); > > @@ -972,34 +996,31 @@ axen_rxeof(struct usbd_xfer *xfer, void *priv, usbd_st > > DPRINTFN(10,("rxeof: packet#%d, pkt_hdr 0x%08x, pkt_len %zu\n", > > pkt_count, pkt_hdr, pkt_len)); > > > > + if (pkt_len > MCLBYTES || pkt_len < ETHER_MIN_LEN) { > > + printf("%s: invalid pkt_len %zu\n", > > + sc->axen_dev.dv_xname,pkt_len); > > + ifp->if_ierrors++; > > + goto nextpkt; > > + } > > + > > if ((pkt_hdr & AXEN_RXHDR_CRC_ERR) || > > (pkt_hdr & AXEN_RXHDR_DROP_ERR)) { > > - ifp->if_ierrors++; > > /* move to next pkt header */ > > DPRINTF(("crc err(pkt#%d)\n", pkt_count)); > > + ifp->if_ierrors++; > > goto nextpkt; > > } > > > > - /* process each packet */ > > - /* allocate mbuf */ > > - m = axen_newbuf(); > > - if (m == NULL) { > > - ifp->if_ierrors++; > > - goto nextpkt; > > - } > > - > > - /* skip pseudo header (2byte) and trailer padding (4Byte) */ > > - m->m_pkthdr.len = m->m_len = pkt_len - 6; > > - > > #ifdef AXEN_TOE > > /* cheksum err */ > > if ((pkt_hdr & AXEN_RXHDR_L3CSUM_ERR) || > > (pkt_hdr & AXEN_RXHDR_L4CSUM_ERR)) { > > printf("%s: checksum err (pkt#%d)\n", > > sc->axen_dev.dv_xname, pkt_count); > > + ifp->if_ierrors++; > > goto nextpkt; > > } else { > > - m->m_pkthdr.csum_flags |= M_IPV4_CSUM_IN_OK; > > + csum_flags |= M_IPV4_CSUM_IN_OK; > > } > > > > int l4_type; > > @@ -1008,12 +1029,23 @@ axen_rxeof(struct usbd_xfer *xfer, void *priv, > > usbd_st > > > > if ((l4_type == AXEN_RXHDR_L4_TYPE_TCP) || > > (l4_type == AXEN_RXHDR_L4_TYPE_UDP)) > > - m->m_pkthdr.csum_flags |= M_TCP_CSUM_IN_OK | > > - M_UDP_CSUM_IN_OK; > > + csum_flags |= M_TCP_CSUM_IN_OK | M_UDP_CSUM_IN_OK; > > #endif > > > > - memcpy(mtod(m, char *), buf + 2, pkt_len - 6); > > + /* process each packet */ > > + /* allocate mbuf */ > > + m = axen_newbuf(); > > + if (m == NULL) { > > + ifp->if_ierrors++; > > + goto nextpkt; > > + } > > > > + /* skip pseudo header (2byte) and trailer padding (4Byte) */ > > + m->m_pkthdr.len = m->m_len = pkt_len - 6; > > + m->m_pkthdr.csum_flags |= csum_flags; > > + > > + memcpy(mtod(m, char *), buf + 2, m->m_len); > > + > > ml_enqueue(&ml, m); > > > > nextpkt: > > @@ -1022,11 +1054,9 @@ nextpkt: > > * as each packet will be aligned 8byte boundary, > > * need to fix up the start point of the buffer. > > */ > > - temp = ((pkt_len + 7) & 0xfff8); > > - buf = buf + temp; > > + buf += roundup(pkt_len, 8); > > hdr_p++; > > - pkt_count--; > > - } while( pkt_count > 0); > > + } > > > > done: > > /* push the packet up */ > > @@ -1260,6 +1290,14 @@ axen_init(void *xsc) > > * Cancel pending I/O and free all RX/TX buffers. > > */ > > axen_reset(sc); > > + > > +#define AXEN_CONFIG_NO 1 > > +#define AXEN_IFACE_IDX 0 > > + if (usbd_set_config_no(sc->axen_udev, AXEN_CONFIG_NO, 1) || > > + usbd_device2interface_handle(sc->axen_udev, AXEN_IFACE_IDX, > > + &sc->axen_iface)) > > + printf("%s: set_config failed\n", sc->axen_dev.dv_xname); > > + usbd_delay_ms(sc->axen_udev, 10); > > > > /* XXX: ? */ > > bval = 0x01; > >