Re: [PATCH net-next 09/10] net: mhi_net: create default link via WWAN core

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

Hi Sergey,

On Sun, 20 Jun 2021 at 15:51, Sergey Ryazanov <ryazanov.s.a@gmail.com> wrote:
>
> Hi Loic,
>
> CC Aleksander, as the talk drifts towards ModemManager.
>
> On Tue, Jun 15, 2021 at 10:08 AM Loic Poulain <loic.poulain@linaro.org> wrote:
> > On Tue, 15 Jun 2021 at 02:30, Sergey Ryazanov <ryazanov.s.a@gmail.com> wrote:
> >> Utilize the just introduced WWAN core feature to create a default netdev
> >> for the default data channel. Since the netdev is now created via the
> >> WWAN core, rely on it ability to destroy all child netdevs on ops
> >> unregistering.
> >>
> >> While at it, remove the RTNL lock acquiring hacks that were earlier used
> >> to call addlink/dellink without holding the RTNL lock. Also make the
> >> WWAN netdev ops structure static to make sparse happy.
> >>
> >> Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
> >> ---
> >>  drivers/net/mhi/net.c | 54 +++++--------------------------------------
> >>  1 file changed, 6 insertions(+), 48 deletions(-)
> >>
> >> diff --git a/drivers/net/mhi/net.c b/drivers/net/mhi/net.c
> >> index b003003cbd42..06253acecaa2 100644
> >> --- a/drivers/net/mhi/net.c
> >> +++ b/drivers/net/mhi/net.c
> >> @@ -342,10 +342,7 @@ static int mhi_net_newlink(void *ctxt, struct net_device *ndev, u32 if_id,
> >>         /* Number of transfer descriptors determines size of the queue */
> >>         mhi_netdev->rx_queue_sz = mhi_get_free_desc_count(mhi_dev, DMA_FROM_DEVICE);
> >>
> >> -       if (extack)
> >> -               err = register_netdevice(ndev);
> >> -       else
> >> -               err = register_netdev(ndev);
> >> +       err = register_netdevice(ndev);
> >>         if (err)
> >>                 goto out_err;
> >>
> >> @@ -370,10 +367,7 @@ static void mhi_net_dellink(void *ctxt, struct net_device *ndev,
> >>         struct mhi_net_dev *mhi_netdev = netdev_priv(ndev);
> >>         struct mhi_device *mhi_dev = ctxt;
> >>
> >> -       if (head)
> >> -               unregister_netdevice_queue(ndev, head);
> >> -       else
> >> -               unregister_netdev(ndev);
> >> +       unregister_netdevice_queue(ndev, head);
> >>
> >>         mhi_unprepare_from_transfer(mhi_dev);
> >>
> >> @@ -382,7 +376,7 @@ static void mhi_net_dellink(void *ctxt, struct net_device *ndev,
> >>         dev_set_drvdata(&mhi_dev->dev, NULL);
> >>  }
> >>
> >> -const struct wwan_ops mhi_wwan_ops = {
> >> +static const struct wwan_ops mhi_wwan_ops = {
> >>         .priv_size = sizeof(struct mhi_net_dev),
> >>         .setup = mhi_net_setup,
> >>         .newlink = mhi_net_newlink,
> >> @@ -392,55 +386,19 @@ const struct wwan_ops mhi_wwan_ops = {
> >>  static int mhi_net_probe(struct mhi_device *mhi_dev,
> >>                          const struct mhi_device_id *id)
> >>  {
> >> -       const struct mhi_device_info *info = (struct mhi_device_info *)id->driver_data;
> >>         struct mhi_controller *cntrl = mhi_dev->mhi_cntrl;
> >> -       struct net_device *ndev;
> >> -       int err;
> >> -
> >> -       err = wwan_register_ops(&cntrl->mhi_dev->dev, &mhi_wwan_ops, mhi_dev,
> >> -                               WWAN_NO_DEFAULT_LINK);
> >> -       if (err)
> >> -               return err;
> >> -
> >> -       if (!create_default_iface)
> >> -               return 0;
> >> -
> >> -       /* Create a default interface which is used as either RMNET real-dev,
> >> -        * MBIM link 0 or ip link 0)
> >> -        */
> >> -       ndev = alloc_netdev(sizeof(struct mhi_net_dev), info->netname,
> >> -                           NET_NAME_PREDICTABLE, mhi_net_setup);
> >
> > I like the idea of the default link, but here we need to create the
> > netdev manually for several reasons:
> > - In case of QMAP/rmnet, this link is the lower netdev (transport
> > layer) and is not associated with any link id.
> > - In case of MBIM, it changes the netdev parent device from the MHI
> > dev to the WWAN dev, which (currently) breaks how ModemManager groups
> > ports/netdevs (based on bus).
> >
> > For the last one, I don't think device hierarchy is considered as
> > UAPI, so we probably just need to add this new wwan link support to
> > user tools like MM. For the first one, I plan to split the mhi_net
> > driver into two different ones (mhi_net_qmap, mhi_net_mbim), and in
> > the case of qmap(rmnet) forward newlink/dellink call to rmnet
> > rtnetlink ops.
>
> Looks like I missed the complexity of WWAN devices handling. Thank you
> for pointing that out. Now I will drop this patch from the series.
>
> Just curious, am I right to say that any network interface created
> with wwan-core is not usable with ModemManager at the moment? AFAIU,
> ModemManager is unable to bundle a control port and a netdev into a
> common "modem" object, even if they both have the same parent Linux
> device, just because that device is not a physical USB device.

Right, there is an ongoing discussion about supporting iosm:
https://gitlab.freedesktop.org/mobile-broadband/ModemManager/-/issues/385#note_958408

So once we have it working for iosm, it should work for any WWAN
device using WWAN framework.

Regards,
Loic