On 23/05/16(Mon) 16:51, Gerhard Roth wrote:
> 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
> > > +/*
> > > + * 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.
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->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?
Yes please. Driver should not be verbose. In this case there's a
counter for Tx error: ``ifp->if_oerrors'' which can be seen by netstat(8).
I did not check all of them, but if they are superfluous printf/log
should be removed :)
