netdev - Re: [PATCH V3 15/16] net: iosm: net driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 27 May 2021 11:40:42 +0200
From:   Johannes Berg <johannes@...solutions.net>
To:     Loic Poulain <loic.poulain@...aro.org>
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 Loic,

> 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).

Right.

(I know nothing about the formats, so if I said anything about 'QMI'
just ignore and think 'qualcomm stuff')

> 
> 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.

Agree. Though I looked at it now in the last couple of hours, and it's
actually not easy to do.

I came up with these patches for now:
https://p.sipsolutions.net/d8d8897c3f43cb85.txt

(on top of 5.13-rc3 + the patchset we're discussing here)

The key problem is that rtnetlink ops are meant to be for a single
device family, and don't really generalize well. For example:

+static void wwan_rtnl_setup(struct net_device *dev)
+{
+       /* FIXME - how do we implement this? we dont have any data
+        * at this point ..., i.e. we can't look up the context yet?
+        * We'd need data[IFLA_WWAN_DEV_NAME], see wwan_rtnl_newlink().
+        */
+}

or 

+static struct rtnl_link_ops wwan_rtnl_link_ops __read_mostly = {
[...]
+       .priv_size = WWAN_MAX_NETDEV_PRIV,

are both rather annoying.

Making this more generic should of course be possible, but would require
fairly large changes all over the kernel - passing the tb/data to all
the handlers involved here, etc. That seems awkward?

What do you think?

The alternative I could see is doing what wifi did and create a
completely new (generic) netlink family, but that's also awkward to some
extend and requires writing more code to handle stuff that rtnetlink
already does ...

Please take a look. I suppose we could change rtnetlink to make it
possible to have this behind it ... but that might even be tricky,
because setup() is called in the context of alloc_netdev_mqs(), and that
also has no private data to pass through. So would we have to extend
rtnetlink ops with a "get_setup()" method that actually *returns* a
pointer to the setup method, so that it can be per-user (such as IOSM)?
Tricky stuff.

> 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.

Maybe. It'd just need some incentive, and there's none now that the
Qualcomm stuff is already there :)

johannes