Re: MBIM Patch - Part 2 of 4

[Martin Pieuchot]

On 31/05/16(Tue) 13:08, Gerhard Roth wrote:
> [...] 
> I'm quite sure this is a firmware bug and the only workaround I have
> is the match by VID/PID. That still allows to match other MBIM devices
> not listed in the table to be matched by Class/SubClass.

I'd prefer if we could start like that, that would allow to match
non-buggy devices without modifying the driver.

Re: MBIM Patch - Part 2 of 4

[Martin Pieuchot]

On 31/05/16(Tue) 15:52, Gerhard Roth wrote:
> On Mon, 23 May 2016 17:47:28 +0200 Martin Pieuchot  wrote:
> > On 23/05/16(Mon) 16:51, Gerhard Roth wrote:
> > > On Mon, 23 May 2016 16:18:29 +0200 Martin Pieuchot  
> > > wrote:
> > > > On 23/05/16(Mon) 15:38, Gerhard Roth wrote:
> > > > 
> > > > 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.
> > 
> > SIOCAIFADDR/SIOCDIFADDR It is the way to go.  The driver should not
> > manipulate addresses or route entry.
> 
> Not manipulating the route entries is simple to fix. Will do that.
> 
> But using SIOCAIFADDR/SIOCDIFADDR seems rather awkward since in_control()
> requires a 'struct socket *so' argument (even though it does nothing with
> it, except checking 'so->so_state & SS_PRIV'). Creating a socket inside
> the driver for this sole purpose seems just as weird as setting up a
> fake struct socket.
> 
> Are you really sure, this is the better way to go?

I'm not sure it it is the better way to go.  But I'm sure a driver
should not manipulate `struct ifa' or `struct rtentry'.

Now it should be fairly easy to split in_control() in two and pass a
``privileged'' variable based on the SS_PRIV flag.  in6_control()
already does half of this ;)

I'd be interested to know if the diff below help, and if it does, does
it simplifies your actual code?

Index: netinet/in.c
===
RCS file: /cvs/src/sys/netinet/in.c,v
retrieving revision 1.127
diff -u -p -r1.127 in.c
--- netinet/in.c18 Apr 2016 06:43:51 -  1.127
+++ netinet/in.c1 Jun 2016 07:50:29 -
@@ -84,8 +84,8 @@
 
 void in_socktrim(struct sockaddr_in *);
 void in_len2mask(struct in_addr *, int);
-int in_lifaddr_ioctl(struct socket *, u_long, caddr_t,
-   struct ifnet *);
+int in_ioctl(u_long, caddr_t, struct ifnet *, int);
+int in_lifaddr_ioctl(u_long, caddr_t, struct ifnet *, int);
 
 void in_purgeaddr(struct ifaddr *);
 int in_addhost(struct in_ifaddr *, struct sockaddr_in *);
@@ -172,14 +172,11 @@ in_len2mask(struct in_addr *mask, int le
 int
 in_control(struct socket *so, u_long cmd, caddr_t data, struct ifnet *ifp)
 {
-   struct ifreq *ifr = (struct ifreq *)data;
-   struct ifaddr *ifa;
-   struct in_ifaddr *ia = NULL;
-   struct in_aliasreq *ifra = (struct in_aliasreq *)data;
-   struct sockaddr_in oldaddr;
-   int error;
-   int newifaddr;
-   int s;
+   int privileged;
+
+   privileged = 0;
+   if ((so->so_state & SS_PRIV) != 0)
+   privileged++;
 
switch (cmd) {
 #ifdef MROUTING
@@ -189,18 +186,33 @@ in_control(struct socket *so, u_long cmd
 #endif /* MROUTING */
case SIOCALIFADDR:
case SIOCDLIFADDR:
-   if ((so->so_state & SS_PRIV) == 0)
+   if (!privileged)
return (EPERM);
/* FALLTHROUGH */
case SIOCGLIFADDR:
if (ifp == NULL)
return (EINVAL);
-   return in_lifaddr_ioctl(so, cmd, data, ifp);
+   return in_lifaddr_ioctl(cmd, data, ifp, privileged);
default:
if (ifp == NULL)
return (EOPNOTSUPP);
}
 
+   return (in_ioctl(cmd, data, ifp, privileged));
+}
+
+int
+in_ioctl(u_long cmd, caddr_t data, struct ifnet *ifp, int privileged)
+{
+   struct ifreq *ifr = (struct ifreq *)data;
+   struct ifaddr *ifa;
+   struct in_ifaddr *ia = NULL;
+   struct in_aliasreq *ifra = (struct in_aliasreq *)data;
+   struct sockaddr_in oldaddr;
+   int error;
+   int newifaddr;
+   int s;
+
TAILQ_FOREACH(ifa, >if_addrlist, ifa_list) {
if (ifa->ifa_addr->sa_family == AF_INET) {
ia = ifatoia(ifa);
@@ -225,7 +237,7 @@ in_control(struct socket *so, u_long cmd
return (EADDRNOTAVAIL);
/* FALLTHROUGH */
case SIOCSIFADDR:
-   if ((so->so_state & SS_PRIV) == 0)
+   if (!privileged)
return (EPERM);
 
if (ia == NULL) {
@@ -250,7 +262,7 @@ in_control(struct socket *so, u_long cmd
case SIOCSIFNETMASK:
case SIOCSIFDSTADDR:
case SIOCSIFBRDADDR:
-   if ((so->so_state & SS_PRIV) == 0)
+   if (!privileged)
return (EPERM);
/* FALLTHROUGH */
 
@@ -410,8 +422,7 @@ in_control(struct socket *so, u_long cmd
  * other values may be returned from in_ioctl()
  */
 int
-in_lifaddr_ioctl(struct socket *so, u_long cmd, caddr_t data,
-struct ifnet *ifp)
+in_lifaddr_ioctl(u_long cmd, caddr_t data, struct ifnet *ifp, int 

Re: MBIM Patch - Part 2 of 4

[Theo de Raadt]

> But using SIOCAIFADDR/SIOCDIFADDR seems rather awkward since in_control()
> requires a 'struct socket *so' argument (even though it does nothing with
> it, except checking 'so->so_state & SS_PRIV'). Creating a socket inside
> the driver for this sole purpose seems just as weird as setting up a
> fake struct socket.
> 
> Are you really sure, this is the better way to go?

No.  This particular layer violation does not belong inside this
driver.  Instead, the driver should probably generate a route socket
message, and then something on the outside would cause that effect of
configuring addresses, routes, etc etc.

But anyways, this conversation is not getting any code commited.

The driver -- minus the contentious bits -- should be be shown, in
just good enough form to compile.  Then the contentious bits can be
argued about and worked on together as a team...

Re: MBIM Patch - Part 2 of 4

[Gerhard Roth]

On Mon, 23 May 2016 17:47:28 +0200 Martin Pieuchot  wrote:
> On 23/05/16(Mon) 16:51, Gerhard Roth wrote:
> > On Mon, 23 May 2016 16:18:29 +0200 Martin Pieuchot  wrote:
> > > On 23/05/16(Mon) 15:38, Gerhard Roth wrote:
> > > 
> > > 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.
> 
> SIOCAIFADDR/SIOCDIFADDR It is the way to go.  The driver should not
> manipulate addresses or route entry.

Not manipulating the route entries is simple to fix. Will do that.

But using SIOCAIFADDR/SIOCDIFADDR seems rather awkward since in_control()
requires a 'struct socket *so' argument (even though it does nothing with
it, except checking 'so->so_state & SS_PRIV'). Creating a socket inside
the driver for this sole purpose seems just as weird as setting up a
fake struct socket.

Are you really sure, this is the better way to go?

Re: MBIM Patch - Part 2 of 4

[Gerhard Roth]

On Mon, 23 May 2016 17:47:28 +0200 Martin Pieuchot  wrote:
> On 23/05/16(Mon) 16:51, Gerhard Roth wrote:
> > On Mon, 23 May 2016 16:18:29 +0200 Martin Pieuchot  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
> > > > +/*
> > > > + * 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, 50 };
> > > > +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.
> 
> Well the rule is first match win.  Since umsm(4) uses a vendor/product
> table as long as you don't add your device to this table it should be
> ok.
> 

Now I remember why I needed to match on VID/PID, too. I have one
MBIM module (Sierra Wireless MC8305) that shows some strange behaviour:

If you read the configuration descriptor #1 and then set
configuration #2, the device resets itself and shortly disapears
from the USB bus.

When matching VID/PID, I can prevent usbd_probe_and_attach() from reading
configuration #1.

Without matching VID/PID, usbd_probe_and_attach() will read configuration #1
to try matching all the interfaces provided by the device. But then I not
able to switch to configuration #2 anymore.

I'm quite sure this is a firmware bug and the only workaround I have
is the match by VID/PID. That still allows to match other MBIM devices
not listed in the table to be matched by Class/SubClass.

Any suggestions?

Gerhard

Re: MBIM Patch - Part 2 of 4

[Gerhard Roth]

On 23.05.2016 17:47, Martin Pieuchot wrote:

On 23/05/16(Mon) 16:51, Gerhard Roth wrote:

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.


Sure, my point is can you add the route *once* you get an uplink?



Ok, all of this was meant to be an intermediate solution ;)

The idea was that once the MBIM driver was accepted into the OpenBSD
kernel, the information provided by the MBIM_CID_IP_CONFIGURATION msg
would generate an extended version of the RTM_IFINFO routing message.
This message would pass the DNS server and the default gateway that
was reported by the provider up to any listeners as RTAX_DNS1,
RTAX_DNS2, and RTAX_GATEWAY.

Then I would write a daemon sitting there and listening for this kind
of routing messages and once it receives them, perform the required
actions, i.e. updating /etc/resolv.conf and/or setting the default
route.

But that was planned as a second step. The first step would be the
simple solution we have there right now. And because it would go
away later on, I didn't give it too much thought.


Gerhard

Re: MBIM Patch - Part 2 of 4

[Martin Pieuchot]

On 23/05/16(Mon) 16:40, Stuart Henderson wrote:
> What I don't understand is why routing to a point-to-point interface needs
> anything other than the interface name to be used for the destination.

It doesn't really need anything else.  At least not from the network
stack point of view.

Well I'm lying, it needs somebody who can do the plumbing :)

Re: MBIM Patch - Part 2 of 4

[Martin Pieuchot]

On 23/05/16(Mon) 16:51, Gerhard Roth wrote:
> On Mon, 23 May 2016 16:18:29 +0200 Martin Pieuchot  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
> > > +/*
> > > + * 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, 50 };
> > > +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.

Well the rule is first match win.  Since umsm(4) uses a vendor/product
table as long as you don't add your device to this table it should be
ok.

> > > +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?

I mean that most of the watchdog for USB drivers do nothing
because historically they could not call _stop() and _start().

PCI devices generally print a message then call _init() which does
_stop() and _start().  It should be possible to do the same for USB
devices now.

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

SIOCAIFADDR/SIOCDIFADDR It is the way to go.  The driver should not
manipulate addresses or route entry.


> > > +/* 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.

Sure, my point is can you add the route *once* you get an uplink?

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

I'm not talking about "sensible routing",  I'm saying that copy/paste
programing is bad, especially if you copy the wrong pattern that we have
in the kernel.

> > > +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_rx_ratechk))
> > > + DPRINTF("%s: rx error: %s\n", DEVNAM(sc),
> > > + 

Re: MBIM Patch - Part 2 of 4

[Theo de Raadt]

> > 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.
> 
> Routing just certain destinations over a particular PPP is definitely
> valid, and I think the same applies here. (Maybe you have a fast ISP and a
> cheap ISP and want to use the fast one for some VPN or important traffic).
> 
> What I don't understand is why routing to a point-to-point interface needs
> anything other than the interface name to be used for the destination.

Such a choice being made at the driver level is quite a layer violation.
Pants on fire...

Re: MBIM Patch - Part 2 of 4

[Stuart Henderson]

On 2016/05/23 16:51, Gerhard Roth wrote:
> > 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.

Routing just certain destinations over a particular PPP is definitely
valid, and I think the same applies here. (Maybe you have a fast ISP and a
cheap ISP and want to use the fast one for some VPN or important traffic).

What I don't understand is why routing to a point-to-point interface needs
anything other than the interface name to be used for the destination.

Re: MBIM Patch - Part 2 of 4

[Theo de Raadt]

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

Please start simple, and solve that problem later.  Too much arriving in
one set of diffs.

Re: MBIM Patch - Part 2 of 4

[Gerhard Roth]

On Mon, 23 May 2016 16:18:29 +0200 Martin Pieuchot  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 -
> > +++ sys/dev/usb/if_mbim.c   23 May 2016 09:50:08 -
> > +
> > +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, 50 };
> > +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, );
> > +   hard_mtu = MBIM_MAXSEGSZ_MINVAL;
> > +   while ((desc = usbd_desc_iter_next())) {
> > +   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", 

Re: MBIM Patch - Part 2 of 4

[Martin Pieuchot]

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.

> 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 -
> +++ sys/dev/usb/if_mbim.c 23 May 2016 09:50:08 -
> +
> +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.

> +/*
> + * 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, 50 };
> +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.

> +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, );
> + hard_mtu = MBIM_MAXSEGSZ_MINVAL;
> + while ((desc = usbd_desc_iter_next())) {
> + 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",
> +