Date:   Tue, 25 May 2021 16:59:56 +0200
From:   Loic Poulain <loic.poulain@...aro.org>
To:     Johannes Berg <johannes@...solutions.net>
Cc:     "Kumar, M Chetan" <m.chetan.kumar@...el.com>,
        Network Development <netdev@...r.kernel.org>,
        "linux-wireless@...r.kernel.org" <linux-wireless@...r.kernel.org>,
        "Sudi, Krishna C" <krishna.c.sudi@...el.com>,
        linuxwwan <linuxwwan@...el.com>, Dan Williams <dcbw@...hat.com>,
        Bjørn Mork <bjorn@...k.no>,
        Jakub Kicinski <kuba@...nel.org>
Subject: Re: [PATCH V3 15/16] net: iosm: net driver

Hi Johannes,

On Tue, 25 May 2021 at 14:55, Johannes Berg <johannes@...solutions.net> wrote:
>
> On Tue, 2021-05-25 at 10:24 +0200, Loic Poulain wrote:
> > >
>
> > > Can you please share us more details on wwan_core changes(if any)/how we should
> > > use /sys/class/wwan0 for link creation ?
> >
> > Well, move rtnetlink ops to wwan_core (or wwan_rtnetlink), and parse
> > netlink parameters into the wwan core. Add support for registering
> > `wwan_ops`, something like:
> > wwan_register_ops(wwan_ops *ops, struct device *wwan_root_device)
> >
> > The ops could be basically:
> > struct wwan_ops {
> >     int (*add_intf)(struct device *wwan_root_device, const char *name,
> > struct wwan_intf_params *params);
> >     int (*del_intf) ...
> > }
> >
> > Then you could implement your own ops in iosm, with ios_add_intf()
> > allocating and registering the netdev as you already do.
> > struct wwan_intf_params would contain parameters of the interface,
> > like the session_id (and possibly extended later with others, like
> > checksum offload, etc...).
> >
> > What do you think?
>
> Note that I kind of tried this in my version back when:
>
> https://lore.kernel.org/netdev/20200225105149.59963c95aa29.Id0e40565452d0d5bb9ce5cc00b8755ec96db8559@changeid/#Z30include:net:wwan.h
>
> See struct wwan_component_device_ops.
>
> I had a different *generic* netlink family rather than rtnetlink ops,
> but that's mostly an implementation detail. I tend to like genetlink
> better, but having rtnetlink makes it easier in iproute2 (though it has
> some generic netlink code too, of course.)
>
> Nobody really seemed all that interested back then.
>
> And frankly, I'm really annoyed that while all of this played out we let
> QMI enter the tree with their home-grown stuff (and dummy netdevs,
> FWIW), *then* said the IOSM driver has to go to rtnetlink ops like them,
> instead of what older drivers are doing, and *now* are shifting
> goalposts again towards something like the framework I started writing
> early on for the IOSM driver, while the QMI driver was happening, and
> nobody cared ...

Yes, I guess it's all about timings... At least, I care now...
I've recently worked on the mhi_net driver, which is basically the
netdev driver for Qualcomm PCIe modems. MHI being similar to IOSM
(exposing logical channels over PCI). Like QCOM USB variants, data can
be transferred in QMAP format (I guess what you call QMI), via the
`rmnet` link type (setup via iproute2).

>
> Yeah, life's not fair and all that, but it does kind of feel like
> arbitrary shifting of the goal posts, while QMI is already in tree. Of
> course it's not like we have a competition with them here, but having
> some help from there would've been nice. Oh well.
>
> Not that I disagree with any of this, it does technically make sense.
>
> However, I think at this point it'd be good to see some comments here
> from DaveM/Jakub that they're going to force Qualcomm to also go down
> this route, because they're now *heavily* invested in their own APIs,
> and inventing a framework just for the IOSM driver is fairly pointless.

This a legitimate point, but it's not too late to do the 'right'
thing, + It should not be too much change in the IOSM driver.

Regarding Qualcomm, I think it should be possible to fit QCOM Modem
drivers into that solution. It would consist of creating a simple
wrapper in QMAP/rmnet so that the rmnet link can (also) be created
from the kernel side (e.g. from mhi_net driver):
wwan_new_link() => wwan->add_intf_cb() => mhi_net_add_intf() => rmnet_newlink()

That way mhi_net driver would comply with the new hw agnostic wwan link
API, without breaking backward compatibility if someone wants to
explicitly create a rmnet link. Moreover, it could also be applicable
to USB modems based on MBIM and their VLAN link types.

That's my guess, but maybe I do not have the whole picture and miss
something... Anyway, I'll not blame the IOSM driver for not doing
this. But If you decide to go with the wwan link type, I'll do my best
to adapt that to QCOM mhi_net as well.

> > I also plan to submit a change to add a wwan_register_netdevice()
> > function (similarly to WiFi cfg80211_register_netdevice), that could
> > be used instead of register_netdevice(), basically factorizing wwan
> > netdev registration (add "wwan" dev_type, add sysfs link to the 'wwan'
> > device...).
>
> Be careful with that, I tend to think we made some mistakes in this
> area, look at the recent locking things there. We used to do stuff from
> a netdev notifier, and that has caused all kinds of pain recently when I
> reworked the locking to not be so overly dependent on the RTNL all the
> time (which really has become the new BKL, at least for desktop work,
> sometimes I can't even type in the UI when the RTNL is blocked). So
> wwan_register_netdevice() is probably fine, doing netdev notifier I'd
> now recommend against.
> But in any case, that's just a side thread.

Sure, thanks for pointing this.

Regards,
Loic

Powered by blists - more mailing lists