On Mon, 23 May 2016 16:18:29 +0200 Martin Pieuchot <m...@openbsd.org> wrote:
> On 23/05/16(Mon) 15:38, Gerhard Roth wrote:
> > This is part 2 of the MBIM patch. It adds the mbim driver to i386
>
> Comments inline.
Replies too.
>
> > Index: sys/dev/usb/if_mbim.c
> > ===================================================================
> > RCS file: sys/dev/usb/if_mbim.c
> > diff -N sys/dev/usb/if_mbim.c
> > --- /dev/null 1 Jan 1970 00:00:00 -0000
> > +++ sys/dev/usb/if_mbim.c 23 May 2016 09:50:08 -0000
> > +
> > +struct cfdriver mbim_cd = {
> > + NULL, "mbim", DV_DULL
> > +};
> > +
> > +struct cfattach mbim_ca = {
> > + sizeof (struct mbim_softc),
> > + mbim_match,
> > + mbim_attach,
> > + mbim_detach,
> > + mbim_activate,
> > +};
>
> This struct can be const.
Will do
>
> > +/*
> > + * Some devices are picky about too frequent control messages.
> > + * Query device state not more often than every 0.5 secs.
> > + */
> > +struct timeval mbim_update_rate = { 0, 500000 };
> > +int mbim_delay = 4000;
> > +
> > +/*
> > + * Normally, MBIM devices are detected by their interface class and
> > subclass.
> > + * But for some models that have multiple configurations, it is better to
> > + * match by vendor and product id so that we can select the desired
> > + * configuration ourselves.
> > + *
> > + * OTOH, some devices identifiy themself als an MBIM device but fail to
> > speak
> > + * the MBIM protocol.
> > + */
>
> Why is it better? This is just working around usb_probe_and_attach()
> and require developer to add an entry for every device we need to
> support.
I just thought that some modules that are already in use say with a
umsm config would otherwise change to mbim and break the setup. The idea
was to keep existing modules first as they are now and start adding
new one to the list.
>
> > +int
> > +mbim_match(struct device *parent, void *match, void *aux)
> > +{
> > + struct usb_attach_arg *uaa = aux;
> > + usb_interface_descriptor_t *id;
> > +
> > + if (usb_lookup(mbim_blacklist, uaa->vendor, uaa->product) != NULL)
> > + return UMATCH_NONE;
> > + if (mbim_lookup(uaa->vendor, uaa->product) != NULL)
> > + return UMATCH_VENDOR_PRODUCT;
> > + if (!uaa->iface)
> > + return UMATCH_NONE;
> > + if (!uaa->iface ||
> > + (id = usbd_get_interface_descriptor(uaa->iface)) == NULL)
> > + return UMATCH_NONE;
> > + if (id->bInterfaceClass != UICLASS_CDC ||
> > + id->bInterfaceSubClass !=
> > + UISUBCLASS_MOBILE_BROADBAND_INTERFACE_MODEL ||
> > + id->bNumEndpoints != 1)
> > + return UMATCH_NONE;
> > +
> > + return UMATCH_DEVCLASS_DEVSUBCLASS;
> > +}
> > +
> > +void
> > +mbim_attach(struct device *parent, struct device *self, void *aux)
> > +{
> > + struct mbim_softc *sc = (struct mbim_softc *)self;
> > + struct usb_attach_arg *uaa = aux;
> > + usbd_status status;
> > + struct usbd_desc_iter iter;
> > + const usb_descriptor_t *desc;
> > + int v;
> > + struct mbim_descriptor *md;
> > + int i;
> > + struct usbd_interface *ctrl_iface = NULL;
> > + int ctrl_ep;
> > + uint8_t data_ifaceno;
> > + usb_interface_descriptor_t *id;
> > + usb_config_descriptor_t *cd;
> > + usb_endpoint_descriptor_t *ed;
> > + int s;
> > + struct ifnet *ifp;
> > + int hard_mtu;
> > +
> > + sc->sc_udev = uaa->device;
> > +
> > + if (uaa->configno < 0) {
> > + uaa->configno = mbim_lookup(uaa->vendor, uaa->product)->confno;
> > + DPRINTF("%s: switching to config #%d\n", DEVNAM(sc),
> > + uaa->configno);
> > + status = usbd_set_config_no(sc->sc_udev, uaa->configno, 1);
> > + if (status) {
> > + printf("%s: failed to switch to config #%d: %s\n",
> > + DEVNAM(sc), uaa->configno, usbd_errstr(status));
> > + goto fail;
> > + }
> > + }
> > +
> > + sc->sc_ver_maj = sc->sc_ver_min = -1;
> > + usbd_desc_iter_init(sc->sc_udev, &iter);
> > + hard_mtu = MBIM_MAXSEGSZ_MINVAL;
> > + while ((desc = usbd_desc_iter_next(&iter))) {
> > + if (desc->bDescriptorType != UDESC_CS_INTERFACE)
> > + continue;
> > + switch (desc->bDescriptorSubtype) {
> > + case UDESCSUB_MBIM:
> > + md = (struct mbim_descriptor *)desc;
> > + v = UGETW(md->bcdMBIMVersion);
> > + sc->sc_ver_maj = MBIM_VER_MAJOR(v);
> > + sc->sc_ver_min = MBIM_VER_MINOR(v);
> > + sc->sc_ctrl_len = UGETW(md->wMaxControlMessage);
> > + /* Never trust a USB device! Could try to exploit us */
> > + if (sc->sc_ctrl_len < MBIM_CTRLMSG_MINLEN ||
> > + sc->sc_ctrl_len > MBIM_CTRLMSG_MAXLEN) {
> > + printf("%s: control message len %d out of "
> > + "bounds [%d .. %d]\n", DEVNAM(sc),
> > + sc->sc_ctrl_len, MBIM_CTRLMSG_MINLEN,
> > + MBIM_CTRLMSG_MAXLEN);
> > + /* cont. anyway */
> > + }
> > + sc->sc_maxpktlen = UGETW(md->wMaxSegmentSize);
> > + if (sc->sc_maxpktlen < MBIM_MAXSEGSZ_MINVAL) {
> > + printf("%s: ignoring invalid segment size %d\n",
> > + DEVNAM(sc), sc->sc_maxpktlen);
> > + /* cont. anyway */
> > + sc->sc_maxpktlen = 8 * 1024;
> > + }
> > + hard_mtu = sc->sc_maxpktlen;
> > + DPRINTFN(2, "%s: ctrl_len=%d, maxpktlen=%d, cap=0x%x\n",
> > + DEVNAM(sc), sc->sc_ctrl_len, sc->sc_maxpktlen,
> > + md->bmNetworkCapabilities);
> > + break;
> > + default:
> > + break;
> > + }
> > + }
> > + cd = usbd_get_config_descriptor(sc->sc_udev);
> > + if (sc->sc_ver_maj < 0) {
> > + printf("%s: missing MBIM descriptor\n", DEVNAM(sc));
> > + goto fail;
> > + }
> > +
> > + for (i = 0; i < sc->sc_udev->cdesc->bNumInterface; i++) {
> > + if (usbd_iface_claimed(sc->sc_udev, i))
> > + continue;
> > + id = usbd_get_interface_descriptor(&sc->sc_udev->ifaces[i]);
> > + if (id == NULL)
> > + continue;
> > + if (id->bInterfaceClass == UICLASS_CDC &&
> > + id->bInterfaceSubClass ==
> > + UISUBCLASS_MOBILE_BROADBAND_INTERFACE_MODEL) {
> > + ctrl_iface = &sc->sc_udev->ifaces[i];
> > + sc->sc_ctrl_ifaceno = id->bInterfaceNumber;
> > + usbd_claim_iface(sc->sc_udev, i);
> > + } else if (id->bInterfaceClass == UICLASS_CDC_DATA &&
> > + id->bInterfaceSubClass == UISUBCLASS_DATA &&
> > + id->bInterfaceProtocol == UIPROTO_DATA_MBIM) {
> > + sc->sc_data_iface = &sc->sc_udev->ifaces[i];
> > + data_ifaceno = id->bInterfaceNumber;
> > + usbd_claim_iface(sc->sc_udev, i);
> > + }
> > + }
> > + if (ctrl_iface == NULL) {
> > + printf("%s: no control interface found\n", DEVNAM(sc));
> > + return;
> > + }
> > + if (sc->sc_data_iface == NULL) {
> > + printf("%s: no data interface found\n", DEVNAM(sc));
> > + goto fail;
> > + }
> > +
> > + id = usbd_get_interface_descriptor(ctrl_iface);
> > + ctrl_ep = -1;
> > + for (i = 0; i < id->bNumEndpoints && ctrl_ep == -1; i++) {
> > + ed = usbd_interface2endpoint_descriptor(ctrl_iface, i);
> > + if (ed == NULL)
> > + break;
> > + if (UE_GET_XFERTYPE(ed->bmAttributes) == UE_INTERRUPT &&
> > + UE_GET_DIR(ed->bEndpointAddress) == UE_DIR_IN)
> > + ctrl_ep = ed->bEndpointAddress;
> > + }
> > + if (ctrl_ep == -1) {
> > + printf("%s: missing interrupt endpoint\n", DEVNAM(sc));
> > + goto fail;
> > + }
> > +
> > + id = usbd_get_interface_descriptor(sc->sc_data_iface);
> > + sc->sc_rx_ep = sc->sc_tx_ep = -1;
> > + if ((status = usbd_set_interface(sc->sc_data_iface,
> > + MBIM_INTERFACE_ALTSETTING))) {
> > + printf("%s: select alt interface %d failed: %s\n",
> > + DEVNAM(sc), MBIM_INTERFACE_ALTSETTING, usbd_errstr(status));
> > + goto fail;
> > + }
> > + id = usbd_get_interface_descriptor(sc->sc_data_iface);
> > + for (i = 0; i < id->bNumEndpoints; i++) {
> > + if ((ed = usbd_interface2endpoint_descriptor(sc->sc_data_iface,
> > + i)) == NULL)
> > + break;
> > + if (UE_GET_XFERTYPE(ed->bmAttributes) == UE_BULK &&
> > + UE_GET_DIR(ed->bEndpointAddress) == UE_DIR_IN)
> > + sc->sc_rx_ep = ed->bEndpointAddress;
> > + else if (UE_GET_XFERTYPE(ed->bmAttributes) == UE_BULK &&
> > + UE_GET_DIR(ed->bEndpointAddress) == UE_DIR_OUT)
> > + sc->sc_tx_ep = ed->bEndpointAddress;
> > + }
> > + if (sc->sc_rx_ep == -1 || sc->sc_tx_ep == -1) {
> > + printf("%s: missing bulk endpoints\n", DEVNAM(sc));
> > + goto fail;
> > + }
> > +
> > + DPRINTFN(2, "%s: ctrl-ifno#%d: ep-ctrl=%d, data-ifno#%d: ep-rx=%d, "
> > + "ep-tx=%d\n", DEVNAM(sc), sc->sc_ctrl_ifaceno,
> > + UE_GET_ADDR(ctrl_ep), data_ifaceno,
> > + UE_GET_ADDR(sc->sc_rx_ep), UE_GET_ADDR(sc->sc_tx_ep));
> > +
> > + usb_init_task(&sc->sc_mbim_task, mbim_state_task, sc,
> > + USB_TASK_TYPE_GENERIC);
> > + usb_init_task(&sc->sc_get_response_task, mbim_get_response_task, sc,
> > + USB_TASK_TYPE_GENERIC);
> > + timeout_set(&sc->sc_statechg_timer, mbim_statechg_timeout, sc);
> > +
> > + if (usbd_open_pipe_intr(ctrl_iface, ctrl_ep, USBD_SHORT_XFER_OK,
> > + &sc->sc_ctrl_pipe, sc, &sc->sc_intr_msg, sizeof (sc->sc_intr_msg),
> > + mbim_intr, USBD_DEFAULT_INTERVAL)) {
> > + printf("%s: failed to open control pipe\n", DEVNAM(sc));
> > + goto fail;
> > + }
> > + sc->sc_resp_buf = malloc(sc->sc_ctrl_len, M_USBDEV, M_NOWAIT);
> > + if (sc->sc_resp_buf == NULL) {
> > + printf("%s: allocation of resp buffer failed\n", DEVNAM(sc));
> > + goto fail;
> > + }
> > + sc->sc_ctrl_msg = malloc(sc->sc_ctrl_len, M_USBDEV, M_NOWAIT);
> > + if (sc->sc_ctrl_msg == NULL) {
> > + printf("%s: allocation of ctrl msg buffer failed\n",
> > + DEVNAM(sc));
> > + goto fail;
> > + }
> > +
> > + sc->sc_info.regstate = MBIM_REGSTATE_UNKNOWN;
> > + sc->sc_info.pin_attempts_left = MBIM_VALUE_UNKNOWN;
> > + sc->sc_info.rssi = MBIM_VALUE_UNKNOWN;
> > + sc->sc_info.ber = MBIM_VALUE_UNKNOWN;
> > +
> > + s = splnet();
> > + ifp = GET_IFP(sc);
> > + ifp->if_flags = IFF_SIMPLEX | IFF_MULTICAST | IFF_POINTOPOINT;
> > + ifp->if_ioctl = mbim_ioctl;
> > + ifp->if_start = mbim_start;
> > +
> > + ifp->if_watchdog = mbim_watchdog;
> > + strlcpy(ifp->if_xname, DEVNAM(sc), IFNAMSIZ);
> > + ifp->if_link_state = LINK_STATE_DOWN;
> > +
> > + ifp->if_type = IFT_MBIM;
> > + ifp->if_addrlen = 0;
> > + ifp->if_hdrlen = sizeof (struct ncm_header16) +
> > + sizeof (struct ncm_pointer16);
> > + ifp->if_mtu = 1500; /* use a common default */
> > + ifp->if_hardmtu = hard_mtu;
> > + ifp->if_output = mbim_output;
> > + if_attach(ifp);
> > + if_ih_insert(ifp, mbim_input, NULL);
>
>
>
>
> > + if_alloc_sadl(ifp);
> > + ifp->if_softc = sc;
> > +#if NBPFILTER > 0
> > + bpfattach(&ifp->if_bpf, ifp, DLT_RAW, 0);
> > +#endif
> > + /*
> > + * Open the device now so that we are able to query device information.
> > + * XXX maybe close when done?
> > + */
> > + mbim_open(sc);
> > + splx(s);
> > +
> > + printf("%s: vers %d.%d\n", DEVNAM(sc), sc->sc_ver_maj, sc->sc_ver_min);
> > + return;
> > +
> > +fail:
> > + usbd_deactivate(sc->sc_udev);
> > + return;
> > +}
> > +
> > +int
> > +mbim_detach(struct device *self, int flags)
> > +{
> > + struct mbim_softc *sc = (struct mbim_softc *)self;
> > + struct ifnet *ifp = GET_IFP(sc);
> > + int s;
> > +
> > + s = splnet();
> > + if (ifp->if_flags & IFF_RUNNING)
> > + mbim_down(sc, 1);
> > + mbim_close(sc);
> > +
> > + usb_rem_wait_task(sc->sc_udev, &sc->sc_get_response_task);
> > + if (timeout_initialized(&sc->sc_statechg_timer))
> > + timeout_del(&sc->sc_statechg_timer);
> > + sc->sc_nresp = 0;
> > + usb_rem_wait_task(sc->sc_udev, &sc->sc_mbim_task);
> > + if (sc->sc_ctrl_pipe) {
> > + usbd_close_pipe(sc->sc_ctrl_pipe);
> > + sc->sc_ctrl_pipe = NULL;
> > + }
> > + if (sc->sc_ctrl_msg) {
> > + free(sc->sc_ctrl_msg, M_USBDEV, sc->sc_ctrl_len);
> > + sc->sc_ctrl_msg = NULL;
> > + }
> > + if (sc->sc_resp_buf) {
> > + free(sc->sc_resp_buf, M_USBDEV, sc->sc_ctrl_len);
> > + sc->sc_resp_buf = NULL;
> > + }
> > + if (sc->sc_saved_ifaddr != NULL &&
> > + sc->sc_saved_ifaddr != MBIM_IFADDR_NONE)
> > + free(sc->sc_saved_ifaddr, M_USBDEV,
> > + sizeof (struct saved_ifaddr));
> > + sc->sc_saved_ifaddr = NULL;
> > +
> > + if (ifp->if_softc != NULL) {
> > + if_ih_remove(ifp, mbim_input, NULL);
> > + if_detach(ifp);
> > + }
> > +
> > + splx(s);
> > + return 0;
> > +}
> > +
> > +int
> > +mbim_activate(struct device *self, int act)
> > +{
> > + struct mbim_softc *sc = (struct mbim_softc *)self;
> > +
> > + switch (act) {
> > + case DVACT_DEACTIVATE:
> > + usbd_deactivate(sc->sc_udev);
> > + break;
> > + }
> > + return 0;
> > +}
>
> This is no longer needed.
Ok
>
>
> > +
> > +int
> > +mbim_alloc_xfers(struct mbim_softc *sc)
> > +{
> > + if (!sc->sc_rx_xfer) {
> > + if ((sc->sc_rx_xfer = usbd_alloc_xfer(sc->sc_udev)) != NULL)
> > + sc->sc_rx_buf = usbd_alloc_buffer(sc->sc_rx_xfer,
> > + sc->sc_maxpktlen + MBIM_HDR32_LEN);
> > + }
> > + if (!sc->sc_tx_xfer) {
> > + if ((sc->sc_tx_xfer = usbd_alloc_xfer(sc->sc_udev)) != NULL)
> > + sc->sc_tx_buf = usbd_alloc_buffer(sc->sc_tx_xfer,
> > + sc->sc_maxpktlen + MBIM_HDR16_LEN);
> > + }
> > + return (sc->sc_rx_buf && sc->sc_tx_buf) ? 1 : 0;
> > +}
> > +
> > +void
> > +mbim_free_xfers(struct mbim_softc *sc)
> > +{
> > + if (sc->sc_rx_xfer) {
> > + /* implicit usbd_free_buffer() */
> > + usbd_free_xfer(sc->sc_rx_xfer);
> > + sc->sc_rx_xfer = NULL;
> > + sc->sc_rx_buf = NULL;
> > + }
> > + if (sc->sc_tx_xfer) {
> > + usbd_free_xfer(sc->sc_tx_xfer);
> > + sc->sc_tx_xfer = NULL;
> > + sc->sc_tx_buf = NULL;
> > + }
> > + if (sc->sc_tx_m) {
> > + m_freem(sc->sc_tx_m);
> > + sc->sc_tx_m = NULL;
> > + }
> > +}
> > +
> > +int
> > +mbim_alloc_bulkpipes(struct mbim_softc *sc)
> > +{
> > + struct ifnet *ifp = GET_IFP(sc);
> > +
> > + if (!(ifp->if_flags & IFF_RUNNING)) {
> > + if (usbd_open_pipe(sc->sc_data_iface, sc->sc_rx_ep,
> > + USBD_EXCLUSIVE_USE, &sc->sc_rx_pipe))
> > + return 0;
> > + if (usbd_open_pipe(sc->sc_data_iface, sc->sc_tx_ep,
> > + USBD_EXCLUSIVE_USE, &sc->sc_tx_pipe))
> > + return 0;
> > +
> > + ifp->if_flags |= IFF_RUNNING;
> > + ifp->if_flags &= ~IFF_OACTIVE;
> > + mbim_rx(sc);
> > + }
> > + return 1;
> > +}
> > +
> > +void
> > +mbim_close_bulkpipes(struct mbim_softc *sc)
> > +{
> > + struct ifnet *ifp = GET_IFP(sc);
> > +
> > + ifp->if_flags &= ~(IFF_RUNNING | IFF_OACTIVE);
> > + ifp->if_timer = 0;
> > + if (sc->sc_rx_pipe) {
> > + usbd_close_pipe(sc->sc_rx_pipe);
> > + sc->sc_rx_pipe = NULL;
> > + }
> > + if (sc->sc_tx_pipe) {
> > + usbd_close_pipe(sc->sc_tx_pipe);
> > + sc->sc_tx_pipe = NULL;
> > + }
> > +}
> > +
> > +int
> > +mbim_ioctl(struct ifnet *ifp, u_long cmd, caddr_t data)
> > +{
> > + struct proc *p = curproc;
> > + struct mbim_softc *sc = ifp->if_softc;
> > + struct ifreq *ifr = (struct ifreq *)data;
> > + int s, error = 0;
> > + struct mbim_parameter mp;
> > +
> > + if (usbd_is_dying(sc->sc_udev))
> > + return EIO;
> > +
> > + s = splnet();
> > + switch (cmd) {
> > + case SIOCSIFADDR:
> > + sc->sc_if.if_rtrequest = p2p_rtrequest;
> > + break;
> > + case SIOCSIFFLAGS:
> > + usb_add_task(sc->sc_udev, &sc->sc_mbim_task);
> > + break;
> > + case SIOCGMBIMINFO:
> > + error = copyout(&sc->sc_info, ifr->ifr_data,
> > + sizeof (sc->sc_info));
> > + break;
> > + case SIOCSMBIMPARAM:
> > + if ((error = suser(p, 0)) != 0)
> > + break;
> > + if ((error = copyin(ifr->ifr_data, &mp, sizeof (mp))) != 0)
> > + break;
> > +
> > + if ((error = mbim_setpin(sc, mp.op, mp.is_puk,
> > + mp.pin, mp.pinlen, mp.newpin, mp.newpinlen)) != 0)
> > + break;
> > +
> > + if (mp.apnlen < 0 || mp.apnlen > sizeof (sc->sc_info.apn)) {
> > + error = EINVAL;
> > + break;
> > + }
> > + sc->sc_roaming = mp.roaming ? 1 : 0;
> > + memset(sc->sc_info.apn, 0, sizeof (sc->sc_info.apn));
> > + memcpy(sc->sc_info.apn, mp.apn, mp.apnlen);
> > + sc->sc_info.apnlen = mp.apnlen;
> > + sc->sc_info.preferredclasses = mp.preferredclasses;
> > + mbim_setdataclass(sc);
> > + break;
> > + case SIOCGMBIMPARAM:
> > + memset(&mp, 0, sizeof (mp));
> > + memcpy(mp.apn, sc->sc_info.apn, sc->sc_info.apnlen);
> > + mp.apnlen = sc->sc_info.apnlen;
> > + mp.roaming = sc->sc_roaming;
> > + mp.preferredclasses = sc->sc_info.preferredclasses;
> > + error = copyout(&mp, ifr->ifr_data, sizeof (mp));
> > + break;
> > + case SIOCSIFMTU:
> > + /* Does this include the NCM headers and tail? */
> > + if (ifr->ifr_mtu > ifp->if_hardmtu) {
> > + error = EINVAL;
> > + break;
> > + }
> > + ifp->if_mtu = ifr->ifr_mtu;
> > + break;
> > + case SIOCGIFMTU:
> > + ifr->ifr_mtu = ifp->if_mtu;
> > + break;
> > + case SIOCGIFHARDMTU:
> > + ifr->ifr_hardmtu = ifp->if_hardmtu;
> > + break;
> > + case SIOCAIFADDR:
> > + case SIOCSIFDSTADDR:
> > + case SIOCADDMULTI:
> > + case SIOCDELMULTI:
> > + break;
> > + default:
> > + error = ENOTTY;
> > + break;
> > + }
> > + splx(s);
> > + return error;
> > +}
> > +
> > +int
> > +mbim_output(struct ifnet *ifp, struct mbuf *m, struct sockaddr *dst,
> > + struct rtentry *rtp)
> > +{
> > + struct mbim_softc *sc = ifp->if_softc;
> > +
> > + if (usbd_is_dying(sc->sc_udev) || !(ifp->if_flags & IFF_RUNNING)) {
> > + m_freem(m);
> > + return ENETDOWN;
> > + }
> > + return if_enqueue(ifp, m);
> > +}
> > +
> > +int
> > +mbim_input(struct ifnet *ifp, struct mbuf *m, void *cookie)
> > +{
> > + struct niqueue *inq;
> > + uint8_t ipv;
> > +
> > + if (((ifp->if_flags & IFF_UP) == 0) || (m->m_flags & M_FILDROP) != 0) {
> > + m_freem(m);
> > + return 1;
> > + }
> > + if (m->m_pkthdr.len < sizeof (struct ip)) {
> > + ifp->if_ierrors++;
> > + DPRINTFN(4, "%s: dropping short packet (len %d)\n", __func__,
> > + m->m_pkthdr.len);
> > + m_freem(m);
> > + return 1;
> > + }
> > + m->m_pkthdr.ph_rtableid = ifp->if_rdomain;
> > + m_copydata(m, 0, sizeof (ipv), &ipv);
> > + ipv >>= 4;
> > +
> > + ifp->if_ibytes += m->m_pkthdr.len;
> > + switch (ipv) {
> > + case 4:
> > + inq = &ipintrq;
> > + break;
> > + case 6:
> > + inq = &ip6intrq;
> > + break;
> > + default:
> > + ifp->if_ierrors++;
> > + DPRINTFN(4, "%s: dropping packet with bad IP version (%d)\n",
> > + __func__, ipv);
> > + m_freem(m);
> > + return 1;
> > + }
> > + niq_enqueue(inq, m);
> > + return 1;
> > +}
> > +
> > +void
> > +mbim_start(struct ifnet *ifp)
> > +{
> > + struct mbim_softc *sc = ifp->if_softc;
> > + struct mbuf *m_head = NULL;
> > +
> > + if (usbd_is_dying(sc->sc_udev) ||
> > + !(ifp->if_flags & IFF_RUNNING) ||
> > + (ifp->if_flags & IFF_OACTIVE))
> > + return;
> > +
> > + m_head = ifq_deq_begin(&ifp->if_snd);
> > + if (m_head == NULL)
> > + return;
> > +
> > + if (!mbim_encap(sc, m_head)) {
> > + ifq_deq_rollback(&ifp->if_snd, m_head);
> > + ifp->if_flags |= IFF_OACTIVE;
> > + return;
> > + }
> > + ifq_deq_commit(&ifp->if_snd, m_head);
> > +
> > +#if NBPFILTER > 0
> > + if (ifp->if_bpf)
> > + bpf_mtap(ifp->if_bpf, m_head, BPF_DIRECTION_OUT);
> > +#endif
> > +
> > + ifp->if_flags |= IFF_OACTIVE;
> > + ifp->if_timer = (2 * mbim_xfer_tout) / 1000;
> > +}
> > +
> > +void
> > +mbim_watchdog(struct ifnet *ifp)
> > +{
> > + struct mbim_softc *sc = ifp->if_softc;
> > +
> > + if (usbd_is_dying(sc->sc_udev))
> > + return;
> > +
> > + ifp->if_oerrors++;
> > + log(LOG_WARNING, "%s: watchdog timeout\n", DEVNAM(sc));
>
> Watchdog are run in a task now, so it should be possible to stop/start
> devices even if you need to sleep.
What do you mean? That there's no need for the usbd_is_dying() call?
>
> > + return;
> > +}
> > +
> > [...]
> > +int
> > +mbim_decode_ip_configuration(struct mbim_softc *sc, void *data, int len)
> > +{
> > + struct mbim_cid_ip_configuration_info *ic = data;
> > + struct ifnet *ifp = GET_IFP(sc);
> > + int s;
> > + uint32_t avail;
> > + uint32_t val;
> > + int n, i;
> > + int off;
> > + struct mbim_cid_ipv4_element ipv4elem;
> > + struct in_ifaddr *ia = NULL;
> > + struct ifaddr *ifa;
> > + int newifaddr = 0;
> > + int ifaddr_changed = 0;
> > + int state = -1;
> > +
> > + if (len < sizeof (*ic))
> > + return 0;
> > + if (letoh32(ic->sessionid) != mbim_session_id) {
> > + DPRINTF("%s: ignore IP configration for session id %d\n",
> > + DEVNAM(sc), letoh32(ic->sessionid));
> > + return 0;
> > + }
> > + s = splnet();
> > +
> > + /*
> > + * IPv4 configuation
> > + */
> > + TAILQ_FOREACH(ifa, &ifp->if_addrlist, ifa_list) {
> > + if (ifa->ifa_addr->sa_family == AF_INET) {
> > + if ((ia = ifatoia(ifa)) != NULL)
> > + break;
> > + }
> > + }
> > +
> > + avail = letoh32(ic->ipv4_available);
> > + if (avail & MBIM_IPCONF_HAS_ADDRINFO) {
> > + n = letoh32(ic->ipv4_naddr);
> > + off = letoh32(ic->ipv4_addroffs);
> > +
> > + if (n == 0 || off + sizeof (ipv4elem) > len)
> > + goto done;
> > +
> > + /* Only pick the first one */
> > + memcpy(&ipv4elem, data + off, sizeof (ipv4elem));
> > + ipv4elem.addr = letoh32(ipv4elem.addr);
> > + ipv4elem.prefixlen = letoh32(ipv4elem.prefixlen);
> > + if (ia == NULL) {
> > + ia = malloc(sizeof (*ia), M_IFADDR, M_WAITOK | M_ZERO);
> > + ia->ia_ifa.ifa_ifp = ifp;
> > + newifaddr = 1;
> > + /* No previous addr */
> > + sc->sc_saved_ifaddr = MBIM_IFADDR_NONE;
> > + } else if (memcmp(&ipv4elem.addr, &ia->ia_addr.sin_addr.s_addr,
> > + sizeof (&ipv4elem.addr))) {
> > + ifaddr_changed = 1;
> > +
> > + /*
> > + * First time here, save original settings so we can
> > + * restore them later, when link goes down.
> > + */
> > + if (sc->sc_saved_ifaddr == NULL) {
> > + struct saved_ifaddr *sif;
> > +
> > + sif = malloc(sizeof (*sif), M_USBDEV,
> > + M_WAITOK | M_ZERO);
> > + memcpy(&sif->netmask, &ia->ia_netmask,
> > + sizeof (sif->netmask));
> > + memcpy(&sif->addr, &ia->ia_addr,
> > + sizeof (sif->addr));
> > + memcpy(&sif->dst, &ia->ia_dstaddr,
> > + sizeof (sif->dst));
> > + memcpy(&sif->sockmask, &ia->ia_sockmask,
> > + sizeof (sif->sockmask));
> > + sc->sc_saved_ifaddr = sif;
> > + }
> > + }
> > + state = MBIM_S_UP;
> > + } else if (ia != NULL) {
> > + /* IP config has no address, but our interface has one. */
> > + state = MBIM_S_CONNECTED;
> > + }
> > + if (newifaddr || ifaddr_changed) {
> > + ia->ia_addr.sin_family = AF_INET;
> > + ia->ia_addr.sin_len = sizeof(ia->ia_addr);
> > + ia->ia_addr.sin_addr.s_addr = ipv4elem.addr;
> > + ia->ia_dstaddr.sin_family = AF_INET;
> > + ia->ia_dstaddr.sin_len = sizeof(ia->ia_addr);
> > + if (avail & MBIM_IPCONF_HAS_GWINFO) {
> > + off = letoh32(ic->ipv4_gwoffs);
> > + ia->ia_dstaddr.sin_addr.s_addr =
> > + letoh32(*((uint32_t *)(data + off)));
> > + } else if (newifaddr)
> > + ia->ia_dstaddr.sin_addr.s_addr = INADDR_BROADCAST;
> > + ia->ia_sockmask.sin_family = AF_INET;
> > + ia->ia_sockmask.sin_len = sizeof(ia->ia_addr);
> > + in_len2mask(&ia->ia_sockmask.sin_addr, ipv4elem.prefixlen);
> > + ia->ia_netmask = ia->ia_sockmask.sin_addr.s_addr;
> > + ia->ia_ifa.ifa_addr = sintosa(&ia->ia_addr);
> > + ia->ia_ifa.ifa_dstaddr = sintosa(&ia->ia_dstaddr);
> > + ia->ia_ifa.ifa_netmask = sintosa(&ia->ia_sockmask);
> > + mbim_set_ipv4addr(ifp, ia, newifaddr);
>
> This is crazy :) No driver should ever modify `ia' directly. This
> code should call in_control() via the ioctl path.
As mentioned in a previous mail: this was mostly copied from
if_spppsubr.c:sppp_set_ip_addrs(). But doing an SIOCSIFADDR
ioctl() from inside the kernel seems weird, too.
>
> > +}
> > +
> > +/* Code copied from if_spppsubr.c */
> > +void
> > +mbim_update_gw(struct ifnet *ifp)
> > +{
> > + unsigned int tid;
> > +
> > + /* update routing table */
> > + for (tid = 0; tid <= RT_TABLEID_MAX; tid++) {
> > + while (rtable_walk(tid, AF_INET, mbim_update_gw_walker, ifp) ==
> > + EAGAIN)
> > + ; /* nothing */
> > + }
> > +}
> > +
> > +int
> > +mbim_update_gw_walker(struct rtentry *rt, void *arg, unsigned int id)
> > +{
> > + struct ifnet *ifp = arg;
> > +
> > + if (rt->rt_ifidx == ifp->if_index) {
> > + if (rt->rt_ifa->ifa_dstaddr->sa_family !=
> > + rt->rt_gateway->sa_family ||
> > + !ISSET(rt->rt_flags, RTF_GATEWAY))
> > + return 0; /* do not modify non-gateway routes */
> > + log(LOG_INFO, "%s: update gw %s -> %s\n", DEVNAM(ifp->if_softc),
> > + mbim_ntop(rt->rt_gateway),
> > + mbim_ntop(rt->rt_ifa->ifa_dstaddr));
> > + rt_setgate(rt, rt->rt_ifa->ifa_dstaddr);
> > + }
>
> This is the kind of horrors I have been removing during the past years.
>
> Why do you need to set a default route in the first place?
Just like PPP this was designed as a point-to-point interface. The idea
is that once you get an uplink, all traffic should be routed through it.
What other sensible routing could there be? Only routing some selected IP
addresses through your mobile uplink doesn't seem like the normal use case.
>
> > +void
> > +mbim_rxeof(struct usbd_xfer *xfer, void *priv, usbd_status status)
> > +{
> > + struct mbim_softc *sc = priv;
> > + struct ifnet *ifp = GET_IFP(sc);
> > +
> > + if (usbd_is_dying(sc->sc_udev) || !(ifp->if_flags & IFF_RUNNING))
> > + return;
> > +
> > + if (status != USBD_NORMAL_COMPLETION) {
> > + if (status == USBD_NOT_STARTED || status == USBD_CANCELLED)
> > + return;
> > + if (usbd_ratecheck(&sc->sc_rx_ratechk))
> > + DPRINTF("%s: rx error: %s\n", DEVNAM(sc),
> > + usbd_errstr(status));
>
> Why do you need a ratecheck, do you see that many errors?
Just wanted to make sure that a broken device won't flood the console.
Should I remove it?
>
> > + if (status == USBD_STALLED)
> > + usbd_clear_endpoint_stall_async(sc->sc_rx_pipe);
> > + if (++sc->sc_rx_nerr > 100) {
> > + log(LOG_ERR, "%s: too many rx errors, disabling\n",
> > + DEVNAM(sc));
> > + usbd_deactivate(sc->sc_udev);
> > + }
> > + } else {
> > + sc->sc_rx_nerr = 0;
> > + mbim_decap(sc, xfer);
> > + }
> > +
> > + mbim_rx(sc);
> > + return;
> > +}
