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.
> --- 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;
>
