Instantiating rmnet devices for data ports on QRTR-based modems

Thu Oct 15 20:57:17 UTC 2020

Hey,

> > > It does mean that MM will have to instantiate net ports and
> > > then assign the mux ID to the QmiDevice, and then set it again in the
> > > Bind Mux Data Port message, which leaves room for those potentially
> > > getting out of sync.
> >
> > Why would this not be a problem in the case of having everything in
> > libqmi? because you already know the mux ID in the QmiDevice and it
> > would be "automatically" set in the Bind Mux Data Port TLV? Not sure I
> > like that, and not sure if I'm understanding that flow correctly. The
> > potential out of sync operation would also happen in the case of
> > having everything in libqmi. But I don't think that's a great issue
> > really, we can probably have proper logic to handle all that.
>
> libqmi wouldn't do any magic here, but all MM would have to do in the
> other case is ask the QmiDevice for its mux ID, which the device
> instantiated automatically for itself (or got from the command line,
> potentially, if it's qmicli) and then plug that number into the WDS
> message. This isn't a huge concern at all; it's just something that I
> figure could get lost in the noise and in general I'd like to have as
> few points of "redundant" configuration like this as possible.
>

I see.

> > > It also means that under more generalized
> > > operation (where we don't necessarily follow the rmnet_dataX = mux ID
> > > X+1 convention[3]) we'd still need to use netlink from libqmi to fetch
> > > all of the rmnet interfaces and inspect their mux ID properties to
> > > find a match when performing reload_wwan_iface.
> >
> > The need to have the proper network interface loaded by
> > reload_wwan_iface(), if not using sysfs, is low. Yes, we do have that
> > API in the library, and yes that API currently works perfectly with
> > qmi_wwan and sysfs, but if you ask me, there is not much purpose for
> > that netlink based generic logic otherwise.
>
> Mostly this is for robustness -- if we assume the convention *always*
> holds, then no such netlink work is necessary; we just do
> sprintf("rmnet_data%u", mux_id - 1) and call it a day.

Ok, so in the case of implementing all this as part of the QmiDevice
logic, libqmi would take care of the mux_id vs net interface mapping,
it would expose which mux id to use during the connection attempt, and
it would also take care of bringing up the network interface
associated to that mux id. Right?

> But something
> outside of MM might mess with this, and the bugs that would cause
> might be incredibly hard to find in MM logs alone, I think.
>

Oh, this is something that, while it may happen, shouldn't be our main
focus. We shouldn't e.g. over-complicate the solution just because
there may be something out there messing around. At the end, even
malloc() may fail. If we provide a solution that makes all this
"automagically work", it should be fine to assume that no other
software will try to mess around. Doesn't your MR already assume that
rmnet_dataX maps to X-1 mux id?

> One way or the other we are going to need to use netlink to find an
> unused net interface name, so I don't think implementing the generic
> logic is much harder.
>

That is true.

> >
> > > (On an abstract level,
> > > this seems as valid a use for netlink for libqmi as it is valid for
> > > libqmi to crawl sysfs for net interface details. But it does increase
> > > the total complexity of the code, since MM and libqmi are essentially
> > > exchanging information through the kernel.)
> > >
> >
> > Your point comparing the sysfs crawling vs netlink makes sense. It's
> > one of the only places where libqmi does something other than plain
> > QMI protocol operations, and we did that to allow nicer qmicli
> > actions. In the case of MM, we're using that sysfs crawling inside
> > libqmi only when MM calls qmi_device_set_expected_data_format()
> > though, that is the only place where reload_wwan_iface_name() is run.
> > From MM's point of view there is no need to find what net interface
> > applies to a given QMI control port, but we already do the opposite,
> > looking for what control port applies to a given net port. If MM is
> > going to instantiate the network interfaces itself, there's also not
> > much point on having libqmi look for the interface associated to a
> > QmiDevice; as we already know it in advance.
>
> Actually, fussing around with the expected data format is not even
> necessary for QRTR-based modems as far as I can tell. I think they all
> use raw IP and there's nowhere equivalent to the sysfs nodes to even
> work with expected data format.
>

Yes yes, that was just an example. I do assume we're going to work
with raw-ip only from now on.

> > > One thing that we'll have to consider in the future, though not right
> > > now, is how each design works with multiple PDN operation -- I think
> > > the design implied by the libqmi MR[1] makes this difficult, but
> > > libqmi also seems to make the assumption that we only associate each
> > > device with one net port anyway, so we may have to unwind that
> > > assumption no matter what. The design where MM instantiates net ports
> > > might not run into this issue in so many places.
> > >
> >
> > The connection logic in MM is always based on looking for an "unused
> > net port" and then looking for the matching control port that can get
> > that net port connected. In the case of QRTR/IPA, there is a "master"
> > network interface already and we instantiate new network interfaces on
> > demand for each new connection that we want to make. I think this
> > instantiation would make sense inside MM; but see my last comment.
> >
> > > [1] https://gitlab.freedesktop.org/mobile-broadband/libqmi/-/merge_requests/135
> > > [2] https://chromium.googlesource.com/chromiumos/third_party/modemmanager-next/+/dbee3f23a9c27d8bf3f5e634dabb41ef856867f8
> > > [3] This convention was alluded to by QC, but there's no magic in it
> > > and a user could easily break this with two invocations of the "ip"
> > > command, which might cause some tricky bugs. So I don't know that we
> > > should try to keep the convention, and instead just find any open
> > > rmnet_dataX and mux ID and link those together.
> > >
> >
> > If it doesn't cost us much, I don't see why we wouldn't follow the
> > same convention, although not a strong opinion.
> >
> > So overall, my opinion is that instantiating the network interfaces on
> > demand by ModemManager would make most sense, especially since it
> > involves adding a new dependency on librmnetctl. I don't know if this
> > librmnetctl has a "cli" though, something qmicli users could use to
> > perform the same instantiation required to bring up the connection
> > when MM is not used; is there something like that already? If there is
> > some cli already, I would definitely suggest to take the MM
> > integration.
>
> The librmnetctl dependency is actually not necessary as it is a pretty
> small wrapper around netlink. I wrote raw netlink in my MR to avoid
> this dependency since I didn't know how easy it would be for users to
> get a hold of the library, or if we might have licensing issues.
>

That is good, if we can avoid that dependency and just do raw netlink
commands I think it would be a good idea.

> There is an associated cli[1] which might be useful for debugging, but
> I've never used it. Utilities for working with rmnet devices are
> actually also available as part of iproute2 as I mentioned before.
> Bjorn has used these heavily in the past and can speak to how it works
> and how users can set that up. If we choose not to use librmnetcl at
> all, the relevant bits can be moved out of both MM and libqmi into a
> separate, small library that can be used by both MM and qmicli if you
> want to make those operations available from either flow, but the fact
> that rmnet ports can be set up via iproute2 means this isn't strictly
> necessary in my opinion.
>

Having a separate library that both MM and qmicli can use is something
we already have ;) don't think we need yet another one.
But if the same tasks can be done with iproute2 commands, there's not
much point in having qmicli require a custom logic implemented for
this, right?

I'm not going to lie, I do like the simple API you suggest in the
libqmi MR, I'm just trying to understand to which extent we want to
start doing netlink inside libqmi and not in MM.

> > If there is no cli that qmicli users can use to instantiate the
> > network interfaces, maybe the integration within libqmi would make
> > sense, to be able to provide qmicli users with a complete solution.
> > Although extending librmnetctl to implement a proper CLI there could
> > also make more sense. My feel is that libqmi should be the last
> > option, if there are other viable ones.
> >
> > One last thing regarding this integration, is that it looks like there
> > is no "official" single librmnetctl release. That is something to
> > solve out of MM/libqmi, unless we want to "incorporate" such a library
> > within our sources and just use some given snapshot of it. Or just run
> > the netlink operations ourselves.
>
> This does seem like an issue with depending on librmnetctl. Again,
> it's very small, so replicating its functionality shouldn't be too
> hard (and I think most of it is already written out in the MR anyway).
>

Ok, I think that whatever we do, we should not depend on librmnetctl,
we can probably remove that from the question.

In your implementation, is it that when the QmiDevice for a QRTR node
(created with QMI_DEVICE_MUX_ID_AUTOMATIC) is opened, you're
automatically instantiating the associated network interface via
netlink? I can see qmi_net_port_manager_add_link() being called in the
new DEVICE_OPEN_CONTEXT_STEP_ASSIGN_MUX_ID step. Is this something we
would really want to do?

What happens if MM crashes and is restarted by init, would the network
interface get automatically de-instantiated? Or would it be left
around "unowned"? And how would your setup work in that case? would
qmi_device_open() fail? Or would it just "find" the next free mux_id
and instantiate yet another interface? Should we maybe have MM perform
an initial cleanup of unowned interfaces?

If using qmicli only and this new logic, what if two different qmicli
calls for the same mux id happen at the same time? Would they both
attempt to instantiate the same exact network interface? Also, would
it make any sense to have this new logic you suggest used by qmicli,
given that qmicli opens/closes the QmiDevice on every command?

Maybe separate qmi_device_qrtr_add_link()/remove_link() methods to
explicitly run the action only when the user (MM, qmicli) requests it
would make more sense? That would allow us to have separate actions
also for qmicli.

-- 
Aleksander
https://aleksander.es