On Mon, 6 Jun 2016 10:30:11 +0200 Martin Pieuchot <[email protected]> wrote: > On 01/06/16(Wed) 17:20, Gerhard Roth wrote: > > [...] > > Thanks for all the feedback. > > More comments inline.
Replies too. > > > Index: sbin/ifconfig/ifconfig.c > > =================================================================== > > RCS file: /cvs/src/sbin/ifconfig/ifconfig.c,v > > retrieving revision 1.322 > > diff -u -p -u -p -r1.322 ifconfig.c > > --- sbin/ifconfig/ifconfig.c 3 May 2016 17:52:33 -0000 1.322 > > +++ sbin/ifconfig/ifconfig.c 1 Jun 2016 14:32:18 -0000 > > @@ -107,6 +107,8 @@ > > #include <ifaddrs.h> > > > > #include "brconfig.h" > > +#include <dev/usb/umbim.h> > > +#include <dev/usb/if_umbim.h> > > Does USB mobile broadband interfaces share a spec with non-USB devices? > In other words would it make sense to move (some of) the defines in net/ > rather than usb/? No, the MBIM spec is pure USB stuff that was schemed by usb.org. > In any case these two include lines should be before the standard ones > and under #ifndef SMALL. That's right. > [...] > > Index: share/man/man4/umbim.4 > > =================================================================== > > RCS file: share/man/man4/umbim.4 > > diff -N share/man/man4/umbim.4 > > --- /dev/null 1 Jan 1970 00:00:00 -0000 > > +++ share/man/man4/umbim.4 1 Jun 2016 14:32:18 -0000 > > @@ -0,0 +1,95 @@ > > +.\" Copyright (c) 2016 genua mbH > > +.\" All rights reserved. > > +.\" > > +.\" Redistribution and use in source and binary forms, with or without > > +.\" modification, are permitted provided that the following conditions > > +.\" are met: > > +.\" > > +.\" - Redistributions of source code must retain the above copyright > > +.\" notice, this list of conditions and the following disclaimer. > > +.\" - Redistributions in binary form must reproduce the above > > +.\" copyright notice, this list of conditions and the following > > +.\" disclaimer in the documentation and/or other materials provided > > +.\" with the distribution. > > +.\" > > +.\" THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS > > +.\" "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT > > +.\" LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS > > +.\" FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE > > +.\" COPYRIGHT HOLDERS OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, > > +.\" INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, > > +.\" BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; > > +.\" LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER > > +.\" CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT > > +.\" LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN > > +.\" ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE > > +.\" POSSIBILITY OF SUCH DAMAGE. > > Any reason for using a different license in documentation an code? Different in what sense? Which paragraph do you mean? I just copied it from some other man page in share/man/man4. Can't remember which one it was. But they all say pretty much the same thing. > > > +.Sh CAVEATS > > +The > > +.Nm > > +driver currently does not support IPv6 addresses. > > +.Pp > > +Roaming can be prevented by the driver. This feature hasn't been tested. > > +Please don't kill me in case your phone bills rack up sky high. > > I wouldn't put the last sentence in this manual. The license already > say that. Ok, it's gone. > > > Index: sys/dev/usb/if_umbim.c > > =================================================================== > > RCS file: sys/dev/usb/if_umbim.c > > diff -N sys/dev/usb/if_umbim.c > > --- /dev/null 1 Jan 1970 00:00:00 -0000 > > +++ sys/dev/usb/if_umbim.c 1 Jun 2016 14:32:19 -0000 > > @@ -0,0 +1,2441 @@ > > +/* $OpenBSD$ */ > > + > > +/* > > + * Copyright (c) 2016 genua mbH > > + * All rights reserved. > > + * > > + * Permission to use, copy, modify, and distribute this software for any > > + * purpose with or without fee is hereby granted, provided that the above > > + * copyright notice and this permission notice appear in all copies. > > + * > > + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES > > + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF > > + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR > > + * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES > > + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN > > + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF > > + * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. > > + */ > > + > > +/* > > + * Mobile Broadband Interface Model > > + * http://www.usb.org/developers/docs/devclass_docs/MBIM-Compliance-1.0.pdf > > + */ > > +#include "bpfilter.h" > > + > > +#include <sys/param.h> > > +#include <sys/device.h> > > +#include <sys/socket.h> > > +#include <sys/systm.h> > > +#include <sys/malloc.h> > > +#include <sys/mbuf.h> > > +#include <sys/time.h> > > +#include <sys/timeout.h> > > +#include <sys/kernel.h> > > +#include <sys/syslog.h> > > + > > +#if NBPFILTER > 0 > > +#include <net/bpf.h> > > +#endif > > +#include <net/if.h> > > +#include <net/netisr.h> > > +#include <net/if_arp.h> > > +#include <net/if_var.h> > > +#include <net/if_types.h> > > +#include <net/route.h> > > + > > +#include <netinet/in.h> > > +#include <netinet/in_var.h> > > +#include <netinet/ip.h> > > > + > > +#include <machine/bus.h> > > + > > +#include <dev/usb/usb.h> > > +#include <dev/usb/usbdi.h> > > +#include <dev/usb/usbdivar.h> > > +#include <dev/usb/usbdi_util.h> > > +#include <dev/usb/usbdevs.h> > > +#include <dev/usb/usbcdc.h> > > + > > +#include <dev/usb/umbim.h> > > +#include <dev/usb/if_umbim.h> > > Please remove unused headers. You have many of them. Done. > > > > +/* > > + * State change timeout > > + */ > > +#define UMBIM_STATE_CHANGE_TIMEOUT 30 > > + > > +/* > > + * State change flags > > + */ > > +#define UMBIM_NS_DONT_DROP 0x0001 /* do not drop below current state */ > > +#define UMBIM_NS_DONT_RAISE 0x0002 /* do not raise below current > > state */ > > + > > +/* > > + * Diagnostic macros > > + */ > > +const struct umbim_valdescr umbim_regstates[] = > > UMBIM_REGSTATE_DESCRIPTIONS; > > +const struct umbim_valdescr umbim_dataclasses[] = > > UMBIM_DATACLASS_DESCRIPTIONS; > > +const struct umbim_valdescr umbim_simstate[] = UMBIM_SIMSTATE_DESCRIPTIONS; > > +const struct umbim_valdescr umbim_messages[] = UMBIM_MESSAGES_DESCRIPTIONS; > > +const struct umbim_valdescr umbim_status[] = UMBIM_STATUS_DESCRIPTIONS; > > +const struct umbim_valdescr umbim_cids[] = UMBIM_CID_DESCRIPTIONS; > > +const struct umbim_valdescr umbim_pktstate[] = > > UMBIM_PKTSRV_STATE_DESCRIPTIONS; > > +const struct umbim_valdescr umbim_actstate[] = > > UMBIM_ACTIVATION_STATE_DESCRIPTIONS; > > +const struct umbim_valdescr umbim_error[] = UMBIM_ERROR_DESCRIPTIONS; > > +const struct umbim_valdescr umbim_pintype[] = UMBIM_PINTYPE_DESCRIPTIONS; > > +const struct umbim_valdescr umbim_istate[] = > > UMBIM_INTERNAL_STATE_DESCRIPTIONS; > > + > > +#define umbim_regstate(c) umbim_val2descr(umbim_regstates, (c)) > > +#define umbim_dataclass(c) umbim_val2descr(umbim_dataclasses, (c)) > > +#define umbim_simstate(s) umbim_val2descr(umbim_simstate, (s)) > > +#define umbim_request2str(m) umbim_val2descr(umbim_messages, (m)) > > +#define umbim_status2str(s) umbim_val2descr(umbim_status, (s)) > > +#define umbim_cid2str(c) umbim_val2descr(umbim_cids, (c)) > > +#define umbim_packet_state(s) umbim_val2descr(umbim_pktstate, (s)) > > +#define umbim_activation(s) umbim_val2descr(umbim_actstate, (s)) > > +#define umbim_error2str(e) umbim_val2descr(umbim_error, (e)) > > +#define umbim_pin_type(t) umbim_val2descr(umbim_pintype, (t)) > > +#define umbim_istate(s) umbim_val2descr(umbim_istate, (s)) > > Please do not mix "#define<tab>" and "#define<space>" in the same file. Fixed. > > > +/* > > + * Saved original interface address > > + * See "struct in_ifaddr". > > + */ > > +struct umbim_ifaddr { > > + struct sockaddr_in addr; > > + struct sockaddr_in dst; > > + struct sockaddr_in netmask; > > +}; > > Sorry but this still need some improvement, see below. > > > +/* > > + * Some devices are picky about too frequent control messages. > > + * Query device state not more often than every 0.5 secs. > > + */ > > +struct timeval umbim_update_rate = { 0, 500000 }; > ^^^^^^^^^^^^^^^^^ > This variable seems unused. What it is for? Some remnant from an earlier version. > > > +int umbim_delay = 4000; > > What is this supposed to be? A value? A litte delay in between the requests sent to the device. Using a global variable helps to tune the value via DDB in the development stage. > > > +/* > > + * 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. > > + */ > > +struct umbim_products { > > + struct usb_devno dev; > > + int confno; > > +}; > > +const struct umbim_products umbim_devs[] = { > > + /* > > + * Add devices here to force them to attach as umbim. > > + * Format: { { VID, PID }, CONFIGNO } > > + */ > > +}; > > + > > +#define umbim_lookup(vid, pid) \ > > + ((const struct umbim_products *)usb_lookup(umbim_devs, vid, pid)) > > + > > +int > > +umbim_match(struct device *parent, void *match, void *aux) > > +{ > > + struct usb_attach_arg *uaa = aux; > > + usb_interface_descriptor_t *id; > > + > > + if (umbim_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; > > No need to check twice for "uaa->iface". Thanks for spotting this one. > > > + if (id->bInterfaceClass != UICLASS_CDC || > > + id->bInterfaceSubClass != > > + UISUBCLASS_MOBILE_BROADBAND_INTERFACE_MODEL || > > + id->bNumEndpoints != 1) > > + return UMATCH_NONE; > > + > > + return UMATCH_DEVCLASS_DEVSUBCLASS; > > +} > > + > > +void > > +umbim_attach(struct device *parent, struct device *self, void *aux) > > +{ > > + struct umbim_softc *sc = (struct umbim_softc *)self; > > + struct usb_attach_arg *uaa = aux; > > + usbd_status status; > > + struct usbd_desc_iter iter; > > + const usb_descriptor_t *desc; > > + int v; > > + struct umbim_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 altnum; > > + int s; > > + struct ifnet *ifp; > > + int hard_mtu; > > + > > + sc->sc_udev = uaa->device; > > + > > + if (uaa->configno < 0) { > > Could you please add a comment explaining the weird behavior of your > device here? The fact that the stack MUST not set a configuration other > than the one we want to use otherwise it detaches itself. The logic of > the driver is hard to understand otherwise. Done. > > > + uaa->configno = umbim_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 = UMBIM_MAXSEGSZ_MINVAL; > > + while ((desc = usbd_desc_iter_next(&iter))) { > > + if (desc->bDescriptorType != UDESC_CS_INTERFACE) > > + continue; > > + switch (desc->bDescriptorSubtype) { > > + case UDESCSUB_UMBIM: > > + md = (struct umbim_descriptor *)desc; > > + v = UGETW(md->bcdMBIMVersion); > > + sc->sc_ver_maj = UMBIM_VER_MAJOR(v); > > + sc->sc_ver_min = UMBIM_VER_MINOR(v); > > + sc->sc_ctrl_len = UGETW(md->wMaxControlMessage); > > + /* Never trust a USB device! Could try to exploit us */ > > That's true for the whole stack ;) Sadly we're missing *a lot* of > checks. > > > + if (sc->sc_ctrl_len < UMBIM_CTRLMSG_MINLEN || > > + sc->sc_ctrl_len > UMBIM_CTRLMSG_MAXLEN) { > > + printf("%s: control message len %d out of " > > + "bounds [%d .. %d]\n", DEVNAM(sc), > > + sc->sc_ctrl_len, UMBIM_CTRLMSG_MINLEN, > > + UMBIM_CTRLMSG_MAXLEN); > > + /* cont. anyway */ > > + } > > + sc->sc_maxpktlen = UGETW(md->wMaxSegmentSize); > > + if (sc->sc_maxpktlen < UMBIM_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); > > This is confusing, could you move that below? Just before you use "cd". > This function does not generate any I/O anyway. ok. > > > + 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)); > > + goto fail; > > + } > > + 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); > > + altnum = usbd_get_no_alts(cd, id->bInterfaceNumber); > > + if (UMBIM_INTERFACE_ALTSETTING >= altnum) { > > + printf("%s: mussing alt setting %d for interface #%d\n", > > + DEVNAM(sc), UMBIM_INTERFACE_ALTSETTING, data_ifaceno); > > + goto fail; > > + } > > + sc->sc_rx_ep = sc->sc_tx_ep = -1; > > + if ((status = usbd_set_interface(sc->sc_data_iface, > > + UMBIM_INTERFACE_ALTSETTING))) { > > + printf("%s: select alt setting %d for interface #%d " > > + "failed: %s\n", DEVNAM(sc), UMBIM_INTERFACE_ALTSETTING, > > + data_ifaceno, 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_umbim_task, umbim_state_task, sc, > > + USB_TASK_TYPE_GENERIC); > > + usb_init_task(&sc->sc_get_response_task, umbim_get_response_task, sc, > > + USB_TASK_TYPE_GENERIC); > > + timeout_set(&sc->sc_statechg_timer, umbim_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), > > + umbim_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 = UMBIM_REGSTATE_UNKNOWN; > > + sc->sc_info.pin_attempts_left = UMBIM_VALUE_UNKNOWN; > > + sc->sc_info.rssi = UMBIM_VALUE_UNKNOWN; > > + sc->sc_info.ber = UMBIM_VALUE_UNKNOWN; > > + > > + s = splnet(); > > + ifp = GET_IFP(sc); > > + ifp->if_flags = IFF_SIMPLEX | IFF_MULTICAST | IFF_POINTOPOINT; > > + ifp->if_ioctl = umbim_ioctl; > > + ifp->if_start = umbim_start; > > + > > + ifp->if_watchdog = umbim_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 = umbim_output; > > + if_attach(ifp); > > + if_ih_insert(ifp, umbim_input, NULL); > > If it non-USB MBI exist, I'd suggest to rename umbim_{input,output} and > all the generic functions to mbim_* :) As said, I don't think there is some common 'MBIM' functionality. An although I would appreciate the name change, this would be namespace violation if if_umb.c would use 'mbim' as a prefix to global symbols. > > > + 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? > > + */ > > + umbim_open(sc); > > Is this really needed? open/close are confusing names because their > generally used for openting/closing /dev/nodes. It is needed. Without it, you cannot talk to the device on MBIM protocol level. It might be confusing to a driver developer. But OTOH, MBIM calls this operation MBIM_OPEN. So changing the name would it make harder to match the function with the matching part of the MBIM spec. > [...] > > +int > > +umbim_alloc_bulkpipes(struct umbim_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; > > IFF_OACTIVE should no longer bet set/remove. Use ifq_clr_oactive() if > that's really what you want. Thanks > > > + umbim_rx(sc); > > + } > > + return 1; > > +} > > + > > +void > > +umbim_close_bulkpipes(struct umbim_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 > > +umbim_ioctl(struct ifnet *ifp, u_long cmd, caddr_t data) > > +{ > > + struct proc *p = curproc; > > + struct umbim_softc *sc = ifp->if_softc; > > + struct ifreq *ifr = (struct ifreq *)data; > > + int s, error = 0; > > + struct umbim_parameter mp; > > + > > + if (usbd_is_dying(sc->sc_udev)) > > + return EIO; > > + > > + s = splnet(); > > + switch (cmd) { > > + case SIOCSIFADDR: > > + sc->sc_if.if_rtrequest = p2p_rtrequest; > > Could we share more code from this driver with other p2p interfaces? I woudn't know which code. BTW: I took a lot of code from the ppp drivers and was told that this is not the way to go. > > > + break; > > + case SIOCSIFFLAGS: > > + usb_add_task(sc->sc_udev, &sc->sc_umbim_task); > > I guess you want to wait for the task to complete here. You're not > respecting the API by returning directly. But why using a task? To > serialize up/down operations? No, waiting here is not an option. In order to get a complete configuration of the device you need a mobile connection with the next base station. Would you really want "ifconfig up" to block indefinitely when you're in the basement without network access? The task is there, because going "up" needs a lot of steps with requests being sent to device and then waiting for the device to respond to them. > > > + break; > > + case SIOCGUMBIMINFO: > > + error = copyout(&sc->sc_info, ifr->ifr_data, > > + sizeof (sc->sc_info)); > > + break; > > + case SIOCSUMBIMPARAM: > > + if ((error = suser(p, 0)) != 0) > > + break; > > + if ((error = copyin(ifr->ifr_data, &mp, sizeof (mp))) != 0) > > + break; > > + > > + if ((error = umbim_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; > > + umbim_setdataclass(sc); > > + break; > > + case SIOCGUMBIMPARAM: > > + 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? */ > > The reader will ask the same question. Well the MBIM spec isn't clear about it. It just says that, "The Maximum segment size in bytes that the function is capable of supporting. This number has to be larger than the MTU set for IP traffic by the network." That "has to be larger" could be an indication, that it includes the size of the NCM headers. But I'm not sure. And the implementers of MBIM devices aren't probably sure, either. > > > + 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: > > It is accepted that this ioctl(2) implicitly bring the interface up. But this is a point-to-point interface and behaves a little bit different from regular Ethernet interfaces. You're not supposed to add IP destination addresses manually. Your provider will tell them to you. And in case you do try to add them manually, that will not get a working network interface. > > > + case SIOCSIFDSTADDR: > > + case SIOCADDMULTI: > > + case SIOCDELMULTI: > > + break; > > + default: > > + error = ENOTTY; > > + break; > > + } > > + splx(s); > > + return error; > > +} > > + > > +int > > +umbim_output(struct ifnet *ifp, struct mbuf *m, struct sockaddr *dst, > > + struct rtentry *rtp) > > +{ > > + struct umbim_softc *sc = ifp->if_softc; > > + > > + if (usbd_is_dying(sc->sc_udev) || !(ifp->if_flags & IFF_RUNNING)) { > > + m_freem(m); > > + return ENETDOWN; > > + } > > Please do not check for USB stuff here, that's a another layer. The > IFF_RUNNING check should be (IFF_UP|IFF_RUNNING) != (IFF_UP|IFF_RUNNING). Fine. > What about routing domains? Did you try your device in a rdomain != 0? No I didn't try them. But what could happen? This is a point-to-point interface. No more routing decisions should be necessary on that level. > > > + return if_enqueue(ifp, m); > > +} > > + > > +int > > +umbim_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; > > + } > > Please remove the M_FILDROP, it doesn't exist anymore. done. > > > + 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 > > +umbim_start(struct ifnet *ifp) > > +{ > > + struct umbim_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; > > IFF_OACTIVE here again :) fixed, too. > > > + > > + m_head = ifq_deq_begin(&ifp->if_snd); > > + if (m_head == NULL) > > + return; > > + > > + if (!umbim_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 * umbim_xfer_tout) / 1000; > > +} > > + > > +void > > +umbim_watchdog(struct ifnet *ifp) > > +{ > > + struct umbim_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)); > > I'm not sure drivers should start using log(9). They all use printf(9) > for things that should not happen. Ok. > > > + /* XXX FIXME: re-initialize device */ > > + return; > > +} > > + > > +void > > +umbim_statechg_timeout(void *arg) > > +{ > > + struct umbim_softc *sc = arg; > > + > > + log(LOG_INFO, "%s: state change time out\n",DEVNAM(sc)); > > Same here. Ok. > > > + usb_add_task(sc->sc_udev, &sc->sc_umbim_task); > > +} > > + > > +void > > +umbim_newstate(struct umbim_softc *sc, enum umbim_state newstate, int > > flags) > > +{ > > + if (newstate == sc->sc_state) > > + return; > > + if (((flags & UMBIM_NS_DONT_DROP) && newstate < sc->sc_state) || > > + ((flags & UMBIM_NS_DONT_RAISE) && newstate > sc->sc_state)) > > + return; > > + log(LOG_DEBUG, "%s: state going %s from '%s' to '%s'\n", DEVNAM(sc), > > + newstate > sc->sc_state ? "up" : "down", > > + umbim_istate(sc->sc_state), umbim_istate(newstate)); > > > + usb_add_task(sc->sc_udev, &sc->sc_umbim_task); > > +} > > + > > +void > > +umbim_state_task(void *arg) > > +{ > > + struct umbim_softc *sc = arg; > > + struct ifnet *ifp = GET_IFP(sc); > > + > > + int s; > > + > > + s = splnet(); > > + if (ifp->if_flags & IFF_UP) > > + umbim_up(sc); > > + else > > + umbim_down(sc, 0); > > + umbim_linkstate(sc, > > + sc->sc_state == UMBIM_S_UP ? LINK_STATE_UP : LINK_STATE_DOWN); > > This function is used only here, right? So no need for a function. Merged. > [...] > > +void > > +umbim_linkstate(struct umbim_softc *sc, int state) > > +{ > > + struct ifnet *ifp = GET_IFP(sc); > > + int s; > > + > > + s = splnet(); > > + if (ifp->if_link_state != state) { > > + log(LOG_INFO, "%s: link state changed from %s to %s\n", > > + DEVNAM(sc), > > + LINK_STATE_IS_UP(ifp->if_link_state) ? "up" : "down", > > + LINK_STATE_IS_UP(state) ? "up" : "down"); > > + ifp->if_link_state = state; > > + if_link_state_change(ifp); > > + if (!LINK_STATE_IS_UP(state)) { > > + memset(sc->sc_info.ipv4dns, 0, > > + sizeof (sc->sc_info.ipv4dns)); > > + umbim_restore_ipv4addr(sc); > > Why do you need to change the address based on the link state? Well, once you're connected, your provider will give you an IP address and the driver will put that address onto the network interface. But when you lose the connection with the network or you manually set the interface down, this IP address once given to you by the provider is no longer yours and must not be used anymore. > > Anyway you current code is racy because if_link_state_change() is > asynchronous. Right. It's better to update the IP address first and then call if_link_state_change() afterwards. > > > + } > > + } > > + splx(s); > > +} > > + > > +int > > +umbim_get_ipv4addr(struct ifnet *ifp, struct umbim_ifaddr *uif) > > +{ > > + int error; > > + struct ifreq ifr; > > + > > + memset(uif, 0, sizeof (*uif)); > > + if ((error = in_ioctl(SIOCGIFADDR, (caddr_t)&ifr, ifp, 1)) != 0) > > + return error; > > + memcpy(&uif->addr, &ifr.ifr_addr, sizeof (uif->addr)); > > + > > + if ((error = in_ioctl(SIOCGIFDSTADDR, (caddr_t)&ifr, ifp, 1)) != 0) > > + return error; > > + memcpy(&uif->dst, &ifr.ifr_dstaddr, sizeof (uif->dst)); > > + > > + if ((error = in_ioctl(SIOCGIFNETMASK, (caddr_t)&ifr, ifp, 1)) != 0) > > + return error; > > + memcpy(&uif->netmask, &ifr.ifr_addr, sizeof (uif->netmask)); > > + return 0; > > +} > > + > > +void > > +umbim_set_ipv4addr(struct ifnet *ifp, struct umbim_ifaddr *uif, char *op) > > +{ > > + struct ifreq ifr; > > + int error; > > + > > + splassert(IPL_NET); > > + > > + if (uif == NULL) > > + return; > > + log(LOG_INFO, "%s: %s IPv4 addr %s, mask %s, gateway %s\n", > > + DEVNAM(ifp->if_softc), op, > > + umbim_ntop((struct sockaddr *)&uif->addr), > > + umbim_ntop((struct sockaddr *)&uif->netmask), > > + umbim_ntop((struct sockaddr *)&uif->dst)); > > + > > + memset(&ifr, 0, sizeof (ifr)); > > + memcpy(&ifr.ifr_addr, &uif->addr, sizeof (ifr.ifr_addr)); > > + if ((error = in_ioctl(SIOCSIFADDR, (caddr_t)&ifr, ifp, 1)) != 0) > > + log(LOG_ERR, "%s: unable to set IPv4 address, error %d\n", > > + DEVNAM(ifp->if_softc), error); > > These ioctl(2) are deprecated and should not be used. That's > why I motioned SIOCAIFADDR in my previous email. But SIOCAIFADDR is not the same. If will *add* another address instead of changing the current on. Suppose I had this configuration: inet 0.0.0.1 --> 0.0.0.2 netmask 0xff000000 Now I get an IP address from my provider, I want something like this: inet 10.75.178.41 --> 10.75.178.42 netmask 0xfffffffc But if I used SIOCAIFADDR I would get this instead: inet 0.0.0.1 --> 0.0.0.2 netmask 0xff000000 inet 10.75.178.41 --> 10.75.178.42 netmask 0xfffffffc And deleting the old one first seems more racy. > > > + > > + memset(&ifr, 0, sizeof (ifr)); > > + memcpy(&ifr.ifr_dstaddr, &uif->dst, sizeof (ifr.ifr_dstaddr)); > > + if ((error = in_ioctl(SIOCSIFDSTADDR, (caddr_t)&ifr, ifp, 1)) != 0) > > + log(LOG_ERR, "%s: unable to set IPv4 gateway, error %d\n", > > + DEVNAM(ifp->if_softc), error); > > + > > + memset(&ifr, 0, sizeof (ifr)); > > + memcpy(&ifr.ifr_addr, &uif->netmask, sizeof (ifr.ifr_addr)); > > + if ((error = in_ioctl(SIOCSIFNETMASK, (caddr_t)&ifr, ifp, 1)) != 0) > > + log(LOG_ERR, "%s: unable to set IPv4 netmask, error %d\n", > > + DEVNAM(ifp->if_softc), error); > > Please return an error instead of using log(9) for error reporting. We're somewhere in a callback handler for parsing an processing unsolicited notifications from the device. There is no one there, whom I could return the error to. > [...] > > +int > > +umbim_decode_ip_configuration(struct umbim_softc *sc, void *data, int len) > > +{ > > + struct umbim_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 umbim_cid_ipv4_element ipv4elem; > > + struct umbim_ifaddr uif; > > + int newifaddr = 0; > > + int ifaddr_changed = 0; > > + int state = -1; > > + > > + if (len < sizeof (*ic)) > > + return 0; > > + if (letoh32(ic->sessionid) != umbim_session_id) { > > + DPRINTF("%s: ignore IP configration for session id %d\n", > > + DEVNAM(sc), letoh32(ic->sessionid)); > > + return 0; > > + } > > + s = splnet(); > > + > > + /* > > + * IPv4 configuation > > + */ > > + avail = letoh32(ic->ipv4_available); > > + if (avail & UMBIM_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); > > + > > + umbim_get_ipv4addr(ifp, &uif); > > + if (sc->sc_saved_ifaddr == NULL) { > > + if (uif.addr.sin_addr.s_addr == INADDR_ANY && > > + uif.dst.sin_addr.s_addr == INADDR_ANY) { > > + newifaddr = 1; > > + /* No original addr */ > > + sc->sc_saved_ifaddr = UMBIM_IFADDR_NONE; > > + } else { > > + sc->sc_saved_ifaddr = malloc(sizeof (uif), > > + M_USBDEV, M_WAITOK | M_ZERO); > > + memcpy(sc->sc_saved_ifaddr, &uif, sizeof (uif)); > > + } > > + newifaddr = 1; > > + } else if (ipv4elem.addr != uif.addr.sin_addr.s_addr) > > + ifaddr_changed = 1; > > + state = UMBIM_S_UP; > > + } > > + if (newifaddr || ifaddr_changed) { > > + memset(&uif, 0, sizeof (uif)); > > + uif.addr.sin_family = AF_INET; > > + uif.addr.sin_len = sizeof(uif.addr); > > + uif.addr.sin_addr.s_addr = ipv4elem.addr; > > + uif.dst.sin_family = AF_INET; > > + uif.dst.sin_len = sizeof(uif.dst); > > + if (avail & UMBIM_IPCONF_HAS_GWINFO) { > > + off = letoh32(ic->ipv4_gwoffs); > > + uif.dst.sin_addr.s_addr = > > + letoh32(*((uint32_t *)(data + off))); > > + } else if (newifaddr) > > + uif.dst.sin_addr.s_addr = INADDR_BROADCAST; > > + uif.netmask.sin_family = AF_INET; > > + uif.netmask.sin_len = sizeof(uif.netmask); > > + in_len2mask(&uif.netmask.sin_addr, ipv4elem.prefixlen); > > + umbim_set_ipv4addr(ifp, &uif, newifaddr ? "new" : "change"); > > I still fail to understand why you keep track of the previous address. Just a said above, we start with some fake address: inet 0.0.0.1 --> 0.0.0.2 netmask 0xff000000 Once registered with the network provider, we get an IP address: inet 10.75.178.41 --> 10.75.178.42 netmask 0xfffffffc And now we de-register from the network. The IP address given to us by the provider cannot be used anymore. So we want to restore the original one (0.0.0.1 --> 0.0.0.2). Maybe we don't need the initial fake IP address. But this wasn't my invention; it is just the way other point-to-point interfaces do it > > > + } > > + > > + memset(sc->sc_info.ipv4dns, 0, sizeof (sc->sc_info.ipv4dns)); > > + if (avail & UMBIM_IPCONF_HAS_DNSINFO) { > > + n = letoh32(ic->ipv4_ndnssrv); > > + off = letoh32(ic->ipv4_dnssrvoffs); > > + i = 0; > > + while (n-- > 0) { > > + if (off + sizeof (uint32_t) > len) > > + break; > > + val = letoh32(*((uint32_t *)(data + off))); > > + if (i < UMBIM_MAX_DNSSRV) > > + sc->sc_info.ipv4dns[i++] = val; > > + off += sizeof (uint32_t); > > + } > > + } > > + > > + if ((avail & UMBIM_IPCONF_HAS_MTUINFO)) { > > + val = letoh32(ic->ipv4_mtu); > > + if (ifp->if_hardmtu != val && val <= sc->sc_maxpktlen) { > > + ifp->if_hardmtu = val; > > + if (ifp->if_mtu > val) > > + ifp->if_mtu = val; > > + log(LOG_INFO, "%s: MTU is %d\n", DEVNAM(sc), val); > > + } > > Could you move that to SIOCSIFMTU, this would already be ready when a > userland tool will parse the messages. No that's not possible. That is not the way that MBIM is working. We get into this function once the device negotioated an IP configuration with the provider network and feels ready to inform us with an unsolicited MBIM_CID_IP_CONFIGURATION message. There is no way to force the device to generate this message on our request. So when we receive it, we might have to adjust the MTU of the interface or else we might not be able to send further data traffic. > [...] > > +usbd_status > > +umbim_send_encap_command(struct umbim_softc *sc, void *data, int len) > > +{ > > + struct usbd_xfer *xfer; > > + usb_device_request_t req; > > + char *buf; > > + > > + if (len > sc->sc_ctrl_len) > > + return USBD_INVAL; > > + > > + if ((xfer = usbd_alloc_xfer(sc->sc_udev)) == NULL) > > + return USBD_NOMEM; > > + if ((buf = usbd_alloc_buffer(xfer, len)) == NULL) { > > + usbd_free_xfer(xfer); > > + return USBD_NOMEM; > > + } > > + memcpy(buf, data, len); > > Why do you allocate an xfer for every command? When you initialize the > interrut pipe you already know that you're going to use a single xfer > because commands are synchronous. What makes you think that commands are synchronous? They're not. Ready a few lines further down and you can see that is calls usbd_request_async(). > > You could also avoid the memcpy by allocating DMA-reachable memory > directly instead of using malloc(9) for the command buffer. That would require a major rewrite of the driver. Currently, a function prepares the message contents on its stack and then calls umbim_ctrl_msg() -> umbim_send_encap_command() to send it. And after all, these messages are rather small. Hence the memcpy() isn't particularly expensive. > > > + > > + /* XXX FIXME: if (total len > sc->sc_ctrl_len) => must fragment */ > > + req.bmRequestType = UT_WRITE_CLASS_INTERFACE; > > + req.bRequest = UCDC_SEND_ENCAPSULATED_COMMAND; > > + USETW(req.wValue, 0); > > + USETW(req.wIndex, sc->sc_ctrl_ifaceno); > > + USETW(req.wLength, len); > > + DELAY(umbim_delay); > > + return usbd_request_async(xfer, &req, NULL, NULL); > > +} > > + > [...] > > +/* > > + * Diagnostic routines > > + */ > > +char * > > +umbim_ntop(struct sockaddr *sa) > > +{ > > +#define NUMBUFS 4 > > + static char astr[NUMBUFS][INET_ADDRSTRLEN]; > > + static unsigned nbuf = 0; > > + char *s; > > + > > + s = astr[nbuf++]; > > + if (nbuf >= NUMBUFS) > > + nbuf = 0; > > + > > + switch (sa->sa_family) { > > + case AF_INET: > > + default: > > + inet_ntop(AF_INET, &satosin(sa)->sin_addr, s, sizeof (astr[0])); > > + break; > > + case AF_INET6: > > + inet_ntop(AF_INET6, &satosin6(sa)->sin6_addr, s, > > + sizeof (astr[0])); > > + break; > > + } > > + return s; > > +} > > Please do not introduce this function. The generic one has been removed > in order to stop using static buffers, introducing a new one will on > lead to confusion. I can't see any danger here as I know exactly how many static buffers are required. And honestly, something like: printf("addr %s mask %s\n", umbim_ntop(&addr), umbim_ntop(&mask)); is much simplier that anything else I could imagine. After all, there's no asprintf(3) inside the kernel. Gerhard
