Re: [PATCH v2 00/17] net: introduce Qualcomm IPA driver

[Dan Williams]

On Wed, 2019-06-12 at 10:31 +0200, Arnd Bergmann wrote:
> On Tue, Jun 11, 2019 at 7:23 PM Dan Williams <dcbw@xxxxxxxxxx> wrote:
> > On Tue, 2019-06-11 at 10:52 -0600, Subash Abhinov Kasiviswanathan
> > wrote:
> > 
> > rmnet should handle muxing the QMAP, QoS, and aggregation and pass
> > the
> > resulting packet to the lower layer. That lower layer could be IPA
> > or
> > qmi_wwan, which in turn passes that QMAP packet to USB or GSI or
> > whatever. This is typically how Linux handles clean abstractions
> > between different protocol layers in drivers.
> > 
> > Similar to some WiFi drivers (drivers/net/wireless/marvell/libertas
> > for
> > example) where the same firmware interface can be accessed via PCI,
> > SDIO, USB, SPI, etc. The bus-specific code is self-contained and
> > does
> > not creep into the upper more generic parts.
> 
> Yes, I think that is a good model. In case of libertas, we have
> multiple
> layers inheritence from the basic device (slightly different in the
> implementation,
> but that is how it should be):

To be clear (and I probably wasn't earlier) I wasn't talking as deep
about the actual code structures as you are here but this a great
discussion.

I was trying to make the point that rmnet doesn't need to care about
how the QMAP packets get to the device itself; it can be pretty generic
so that it can be used by IPA/qmi_wwan/rmnet_smd/etc.

Your points below are a great discussion though...

> struct if_cs_card { /* pcmcia specific */
>      struct lbs_private {  /* libertas specific */
>            struct wireless_dev { /* 802.11 specific */
>                   struct net_device {
>                         struct device {
>                               ...
>                         };
>                         ...
>                   };
>                   ...
>            };
>            ...
>       };
>       ...
> };

> The outer structure gets allocated when probing the hardware specific
> driver, and everything below it is implemented as direct function
> calls
> into the more generic code, or as function pointers into the more
> specific
> code.
> 
> The current rmnet model is different in that by design the upper
> layer
> (rmnet) and the lower layer (qmi_wwan, ipa, ...) are kept independent
> in
> both directions, i.e. ipa has (almost) no knowledge of rmnet, and
> just
> has pointers to the other net_device:
> 
>        ipa_device
>            net_device
> 
>        rmnet_port
>            net_device
> 
> I understand that the rmnet model was intended to provide a cleaner
> abstraction, but it's not how we normally structure subsystems in
> Linux, and moving to a model more like how wireless_dev works
> would improve both readability and performance, as you describe
> it, it would be more like (ignoring for now the need for multiple
> connections):
> 
>    ipa_dev
>         rmnet_dev
>                wwan_dev
>                       net_device

Perhaps I'm assuming too much from this diagram but this shows a 1:1
between wwan_dev and "lower" devices.

What Johannes is proposing (IIRC) is something a bit looser where a
wwan_dev does not necessarily provide netdev itself, but is instead the
central point that various channels (control, data, gps, sim card, etc)
register with. That way the wwan_dev can provide an overall view of the
WWAN device to userspace, and userspace can talk to the wwan_dev to ask
the lower drivers (ipa, rmnet, etc) to create new channels (netdev,
tty, otherwise) when the control channel has told the modem firmware to
expect one.

For example, say you have told the firmware to create a new data
channel with ID 5 via QMI (which the kernel is unaware of because it
does not process higher-level QMI requests).

Perhaps (and this is all just brainstorming) then userspace asks the
wwan_dev to create a new data channel with ID 5 and a certain QoS. IPA
(or rmnet because that's the data channel provider for IPA) has
registered callbacks to the wwan_dev, receives this request, and
creates a new rmnet_dev/net_device, and then the wwan_dev passes the
ifindex back to userspace so we don't have to play the dance of "match
up my request with a random netlink ADD event".

The point being that since data channels aren't actually useful until
the control channel agrees with the firmware that one should exist, if
we have a wwan_dev that represents the entire WWAN device then we don't
need the initial-but-useless net_device.

Just some thoughts; Johannes can feel free to correct me at any time :)

> Where each layer is a specialization of the next. Note: this is a
> common change when moving from proprietary code to upstream
> code. If a driver module is designed to live out of tree, there
> is a strong incentive to limit the number of interfaces it uses,
> but when it gets merged, it becomes much more flexible, as
> an internal interface between wwan_dev and the hardware driver(s)
> can be easily changed by modifying all drivers at once.

Yep, I've seen this time and time again.

Dan