by Loic Poulain

[permalink] [raw]

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

Hi Johannes,

On Thu, 27 May 2021 at 11:40, Johannes Berg <[email protected]> wrote:
>
> 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)

Great, that looks exactly what we need.

>
> 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().
> + */
> +}

Argh, yes I've overlooked that issue. But, do we even need to do
something here? What if we do nothing here and call wwan->ops->setup()
early in wwan_rtnl_newlink(). AFAIU, we don't really use data setup
info until we actually register the netdev, except maybe for the
netdev->txqueue_len. Though it's probably not a robust solution...

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

Yes, or alternatively add an optional alloc_netdev() rtnl ops, e.g. in
rtnl_create_link:

- dev = alloc_netdev_mqs(ops->priv_size, ifname, name_assign_type,
- ops->setup, num_tx_queues, num_rx_queues);
+ if (ops->alloc_netdev) {
+ dev = ops->alloc_netdev(ifname, name_assign_type, num_tx_queues,
+ num_rx_queues, tb);
+ } else {
+ dev = alloc_netdev_mqs(ops->priv_size, ifname, name_assign_type,
+ ops->setup, num_tx_queues,
num_rx_queues);
+ }

That would solve both the issues (setup, priv_size), without entire
kernel refactoring.

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

That would work indeed, but I would prefer avoiding such 'complexity',
mainly because link management is all we need. Indeed, except if we
want to abstract and handle control protocols (MBIM, QMI, AT) in the
kernel, we should not have to expose additional high-level operations
as in nl80211/cfg80211.

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

What do you think about this alloc_netdev() ops? we should be able to
retrieve all we need from that.

Regards,
Loic

2021-06-01 07:32:42

by Johannes Berg

[permalink] [raw]

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

Hi,

> Yes, or alternatively add an optional alloc_netdev() rtnl ops, e.g. in
> rtnl_create_link:

Yes, that works. It needs some more fiddling (we need 'data', not just
'tb') and it cannot be called 'alloc_netdev' (that's a macro!), but
yeah, otherwise works. I'll just call it alloc().

I'll send a few updated patches in a few minutes.

johannes