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

[Loic Poulain <loic.poulain@linaro.org>]

Hi Chetan,

On Mon, 24 May 2021 at 12:36, Kumar, M Chetan <m.chetan.kumar@intel.com> wrote:
>
> Hi Loic,
>
> > > +static void ipc_netdev_setup(struct net_device *dev) {}
> > > +
> > > +struct iosm_wwan *ipc_wwan_init(struct iosm_imem *ipc_imem, struct
> > > +device *dev) {
> > > +       static const struct net_device_ops iosm_wwandev_ops = {};
> > > +       struct iosm_wwan *ipc_wwan;
> > > +       struct net_device *netdev;
> > > +
> > > +       netdev = alloc_netdev(sizeof(*ipc_wwan), "wwan%d",
> > NET_NAME_ENUM,
> > > +                             ipc_netdev_setup);
> > > +
> > > +       if (!netdev)
> > > +               return NULL;
> > > +
> > > +       ipc_wwan = netdev_priv(netdev);
> > > +
> > > +       ipc_wwan->dev = dev;
> > > +       ipc_wwan->netdev = netdev;
> > > +       ipc_wwan->is_registered = false;
> > > +
> > > +       ipc_wwan->ipc_imem = ipc_imem;
> > > +
> > > +       mutex_init(&ipc_wwan->if_mutex);
> > > +
> > > +       /* allocate random ethernet address */
> > > +       eth_random_addr(netdev->dev_addr);
> > > +       netdev->addr_assign_type = NET_ADDR_RANDOM;
> > > +
> > > +       netdev->netdev_ops = &iosm_wwandev_ops;
> > > +       netdev->flags |= IFF_NOARP;
> > > +
> > > +       SET_NETDEV_DEVTYPE(netdev, &wwan_type);
> > > +
> > > +       if (register_netdev(netdev)) {
> > > +               dev_err(ipc_wwan->dev, "register_netdev failed");
> > > +               goto reg_fail;
> > > +       }
> >
> > So you register a no-op netdev which is only used to represent the modem
> > instance, and to be referenced for link creation over IOSM rtnetlinks?
>
> That’s correct driver creates wwan0 (no-op netdev) to represent the
> modem instance and is referenced for link creation over IOSM rtnetlinks.
>
> > The new WWAN framework creates a logical WWAN device instance (e.g;
> > /sys/class/wwan0), I think it would make sense to use its index as parameter
> > when creating the new links, instead of relying on this dummy netdev. Note
> > that for now the wwan_device is private to wwan_core and created implicitly
> > on the WWAN control port registration.
>
> In order to use WWAN device instance any additional changes required inside
> wwan_core ?  Or simply passing /sys/class/wwan0 device to ip link add is enough.

So basically the rtnetlink ops would be implemented and define in
wwan_core, as "wwan" link  type.
Allowing users to create a new WWAN link/context whatever the
underlying hardware is. We could therefore pass the WWAN device name
or index to e.g:
ip link add wwan0.1 type wwan hw wwan0 session 1

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

>
> > Moreover I wonder if it could also be possible to create a generic WWAN link
> > type instead of creating yet-another hw specific one, that could benefit
> > future WWAN drivers, and simplify user side integration, with a common
> > interface to create links and multiplex PDN (a bit like wlan vif).
>
> Common interface could benefit both wwan drivers and user side integration.
> WWAN framework generalizes WWAN device control port, would it also consider
> WWAN netdev part ? Is there a plan to support such implementation inside
> wwan_core.

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

Regards,
Loic