On Tue, 7 Jun 2016 13:08:49 +0200 Martin Pieuchot <m...@openbsd.org> wrote:
> On 07/06/16(Tue) 11:53, Gerhard Roth wrote:
> > On Mon, 6 Jun 2016 10:30:11 +0200 Martin Pieuchot <m...@openbsd.org> wrote:
> > > On 01/06/16(Wed) 17:20, Gerhard Roth wrote:
> > > Any reason for using a different license in documentation an code?
> >
> > Different in what sense? Which paragraph do you mean?
>
> This is a BSD 2-clauses the code is ISC.
>
> > 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.
>
> Well if you don't mind, please use /usr/share/misc/license.template :)
Seriously, you don't mean that!
Did you take a look at that file?
>
> > > > +/*
> > > > + * 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.
>
> You're paraphrasing your code :) My unclear question was just why
> do you need such delay? ;)
Well, ask the firmware writer. I don't know.
>
> > > > + 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.
>
> I understand you point. My concern is just that SIOCSIFFLAGS indicates
> that the interface is down/up once the ioclt(2) is finished. It doesn't
> seem to be the case in your driver, I just hope this is not a problem.
How about WiFi? AFAIK it only starts scanning for an access point
but doesn't block until it finished association. So why should it
be a problem here?
>
> > > > + 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.
>
> I'm not saying it makes sense. I'm just explaining how it is.
>
> > > 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.
>
> What could happen? Good question. I don't know.
>
> > > > +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.
>
> So why restore? Don't you want to forget the current address? I don't
> get it.
I'm restoring the original pseudo-address that was put onto the
interface manually. In the examples I gave, it would restore
the "0.0.0.1 --> 0.0.0.2" addresses.
>
>
> > > 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.
>
> In your state machine you says that your link state is UP when you have
> an IP address assigned, right? I'm confused about that.
That's correct. Because before that, we won't be able to send/receive
any traffic.
>
> > > > +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.
>
> It doesn't matter, that's what should be used.
Come on. That will break the functionality of the interface once your
provider changes your IP address and all your argument is a phrase like
"that's what should be used"?
Please reconsider or we could just as well call it quits.
>
> > Suppose I had this configuration:
> >
> > inet 0.0.0.1 --> 0.0.0.2 netmask 0xff000000
>
> Having this doesn't make sense.
I just copied existing OpenBSD behaviour.
>
> > 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.
>
> Why?
Because either there is a time without any IP address or there
is one with two addresses. Neither seems right.
>
> > > > + 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.
>
> Too bad because we *do not* log(9) every driver failure.
>
> > > > + 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
>
> Well don't to that.
See above. That's the current OpenBSD model for PPP.
>
> > 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).
>
> Don't restore anything.
>
> > 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
>
> I know and I'm sorry for you but I'm fighting against this pattern. So
> I'm not in favor of yet-another-copy of it.
If any broken interface of OpenBSD has to be fixed before this driver
is accepted, I can come back in 20 years or so.
>
> > > 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.
>
> You're assuming that your driver will stay like it is. I though that
> you wanted to offload the configuration logic to userland. That's why
> I'm asking you to use the existing ioctl(2) interface to ease the future
> transition.
>
> All I'm saying is that you could call ifp->if_ioctl(SIOCSIFMTU).
That would be wrong. If would always set the MTU to the maximum
segment size supported by MBIM. But that is not what's intended.
If somebody set an MTU on the interface, it should be respected so
long as it is possible. However, if layer 2 only accepts smaller MTUs,
that we have to reduce it to the maximum allowed value.
> Like you could split the "driver" part from the p2p logic part in your
> code. Of course this will introduce a "space naming violation" but it
> encourage people to re-use your code.
Reuse it for what? A potential MBIM2 standard?
Well then, lets wait if that ever comes up first.
>
> > > > +/*
> > > > + * 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.
>
> You're clever. People reading your code are not and do copy/paste.
> That's the danger, a bad advice.
