Date:   Tue, 22 Jun 2021 19:11:43 +0200
From:   Loic Poulain <loic.poulain@...aro.org>
To:     Sergey Ryazanov <ryazanov.s.a@...il.com>
Cc:     Johannes Berg <johannes@...solutions.net>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Network Development <netdev@...r.kernel.org>,
        M Chetan Kumar <m.chetan.kumar@...el.com>,
        Intel Corporation <linuxwwan@...el.com>
Subject: Re: [PATCH net-next v2 08/10] wwan: core: support default netdev creation

On Tue, 22 Jun 2021 at 17:19, Sergey Ryazanov <ryazanov.s.a@...il.com> wrote:
>
> Hi Loic,
>
> On Tue, Jun 22, 2021 at 4:55 PM Loic Poulain <loic.poulain@...aro.org> wrote:
> > On Tue, 22 Jun 2021 at 00:51, Sergey Ryazanov <ryazanov.s.a@...il.com> wrote:
> >> Most, if not each WWAN device driver will create a netdev for the
> >> default data channel. Therefore, add an option for the WWAN netdev ops
> >> registration function to create a default netdev for the WWAN device.
> >>
> >> A WWAN device driver should pass a default data channel link id to the
> >> ops registering function to request the creation of a default netdev, or
> >> a special value WWAN_NO_DEFAULT_LINK to inform the WWAN core that the
> >> default netdev should not be created.
> >>
> >> For now, only wwan_hwsim utilize the default link creation option. Other
> >> drivers will be reworked next.
> >>
> >> Signed-off-by: Sergey Ryazanov <ryazanov.s.a@...il.com>
> >> CC: M Chetan Kumar <m.chetan.kumar@...el.com>
> >> CC: Intel Corporation <linuxwwan@...el.com>
> >> ---
> >>
> >> v1 -> v2:
> >>  * fix a comment style '/**' -> '/*' to avoid confusion with kernel-doc
> >>
> >>  drivers/net/mhi/net.c                 |  3 +-
> >>  drivers/net/wwan/iosm/iosm_ipc_wwan.c |  3 +-
> >>  drivers/net/wwan/wwan_core.c          | 75 ++++++++++++++++++++++++++-
> >>  drivers/net/wwan/wwan_hwsim.c         |  2 +-
> >>  include/linux/wwan.h                  |  8 ++-
> >>  5 files changed, 86 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/net/mhi/net.c b/drivers/net/mhi/net.c
> >> index ffd1c01b3f35..f36ca5c0dfe9 100644
> >> --- a/drivers/net/mhi/net.c
> >> +++ b/drivers/net/mhi/net.c
> >> @@ -397,7 +397,8 @@ static int mhi_net_probe(struct mhi_device *mhi_dev,
> >>         struct net_device *ndev;
> >>         int err;
> >>
> >> -       err = wwan_register_ops(&cntrl->mhi_dev->dev, &mhi_wwan_ops, mhi_dev);
> >> +       err = wwan_register_ops(&cntrl->mhi_dev->dev, &mhi_wwan_ops, mhi_dev,
> >> +                               WWAN_NO_DEFAULT_LINK);
> >>         if (err)
> >>                 return err;
> >>
> >> diff --git a/drivers/net/wwan/iosm/iosm_ipc_wwan.c b/drivers/net/wwan/iosm/iosm_ipc_wwan.c
> >> index bee9b278223d..adb2bd40a404 100644
> >> --- a/drivers/net/wwan/iosm/iosm_ipc_wwan.c
> >> +++ b/drivers/net/wwan/iosm/iosm_ipc_wwan.c
> >> @@ -317,7 +317,8 @@ struct iosm_wwan *ipc_wwan_init(struct iosm_imem *ipc_imem, struct device *dev)
> >>         ipc_wwan->dev = dev;
> >>         ipc_wwan->ipc_imem = ipc_imem;
> >>
> >> -       if (wwan_register_ops(ipc_wwan->dev, &iosm_wwan_ops, ipc_wwan)) {
> >> +       if (wwan_register_ops(ipc_wwan->dev, &iosm_wwan_ops, ipc_wwan,
> >> +                             WWAN_NO_DEFAULT_LINK)) {
> >>                 kfree(ipc_wwan);
> >>                 return NULL;
> >>         }
> >> diff --git a/drivers/net/wwan/wwan_core.c b/drivers/net/wwan/wwan_core.c
> >> index b634a0ba1196..ef6ec641d877 100644
> >> --- a/drivers/net/wwan/wwan_core.c
> >> +++ b/drivers/net/wwan/wwan_core.c
> >> @@ -903,17 +903,81 @@ static struct rtnl_link_ops wwan_rtnl_link_ops __read_mostly = {
> >>         .policy = wwan_rtnl_policy,
> >>  };
> >>
> >> +static void wwan_create_default_link(struct wwan_device *wwandev,
> >> +                                    u32 def_link_id)
> >> +{
> >> +       struct nlattr *tb[IFLA_MAX + 1], *linkinfo[IFLA_INFO_MAX + 1];
> >> +       struct nlattr *data[IFLA_WWAN_MAX + 1];
> >> +       struct net_device *dev;
> >> +       struct nlmsghdr *nlh;
> >> +       struct sk_buff *msg;
> >> +
> >> +       /* Forge attributes required to create a WWAN netdev. We first
> >> +        * build a netlink message and then parse it. This looks
> >> +        * odd, but such approach is less error prone.
> >> +        */
> >> +       msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
> >> +       if (WARN_ON(!msg))
> >> +               return;
> >> +       nlh = nlmsg_put(msg, 0, 0, RTM_NEWLINK, 0, 0);
> >> +       if (WARN_ON(!nlh))
> >> +               goto free_attrs;
> >> +
> >> +       if (nla_put_string(msg, IFLA_PARENT_DEV_NAME, dev_name(&wwandev->dev)))
> >> +               goto free_attrs;
> >> +       tb[IFLA_LINKINFO] = nla_nest_start(msg, IFLA_LINKINFO);
> >> +       if (!tb[IFLA_LINKINFO])
> >> +               goto free_attrs;
> >> +       linkinfo[IFLA_INFO_DATA] = nla_nest_start(msg, IFLA_INFO_DATA);
> >> +       if (!linkinfo[IFLA_INFO_DATA])
> >> +               goto free_attrs;
> >> +       if (nla_put_u32(msg, IFLA_WWAN_LINK_ID, def_link_id))
> >> +               goto free_attrs;
> >> +       nla_nest_end(msg, linkinfo[IFLA_INFO_DATA]);
> >> +       nla_nest_end(msg, tb[IFLA_LINKINFO]);
> >> +
> >> +       nlmsg_end(msg, nlh);
> >> +
> >> +       /* The next three parsing calls can not fail */
> >> +       nlmsg_parse_deprecated(nlh, 0, tb, IFLA_MAX, NULL, NULL);
> >> +       nla_parse_nested_deprecated(linkinfo, IFLA_INFO_MAX, tb[IFLA_LINKINFO],
> >> +                                   NULL, NULL);
> >> +       nla_parse_nested_deprecated(data, IFLA_WWAN_MAX,
> >> +                                   linkinfo[IFLA_INFO_DATA], NULL, NULL);
> >> +
> >> +       rtnl_lock();
> >> +
> >> +       dev = rtnl_create_link(&init_net, "wwan%d", NET_NAME_ENUM,
> >> +                              &wwan_rtnl_link_ops, tb, NULL);
> >
> > That can be a bit confusing since the same naming convention as for
> > the WWAN devices is used, so you can end with something like a wwan0
> > netdev being a link of wwan1 dev. Maybe something like ('%s.%d',
> > dev_name(&wwandev->dev), link_id) to have e.g. wwan1.0 for link 0 of
> > the wwan1 device. Or alternatively, the specific wwan driver to
> > specify the default name to use.
>
> Yeah. Having wwan1 netdev for wwan0 device with control port
> /dev/wwan0mbim0 could be йuite confusing for a _user_. While the
> ModemManager daemon will keep track of parent<->child relations
> without considering device names.
>
> It is a permanent pain to manually find the right modem control port.
> Even if you only have one modem, but have a UART-USB converter
> attached before the modem, then /dev/ttyUSB0, which previously
> belonged to a first modem AT-port, can become the UART-USB device, and
> the first modem AT-port can become /dev/ttyUSB1. We never have
> guarantees for consistent naming, and user space software should take
> care of the matching and predictable names :(
>
> wwanX.Y names look attractive. But how many users will actually use
> multiple data channels and create wwanX.2, wwanX.3, etc. netdevs?
> Moreover, some cellular modems do not support multiple data channels
> and will never do so.
>
> I assume that a typical usage scenario for an average user is a single
> modem connected to a laptop with a single data channel for Internet
> access. In this case, the user will not have a chance to see the wwan1
> netdev. And no confusion is possible in practice. Even more, seeing a
> wwan0.1 netdev when you have a single modem with a single data channel
> will be a more confusing case. Those who will use a more complex setup
> should be ready for some complexity. In fact, for a more complex setup
> (e.g. if you have a pair of modems) you will need some sort of
> management software (or udev scripts) since you can never be sure that
> a USB modem connected to a first USB port, will always be wwan0.
>
> Even if you have a couple of modems of different models (one works
> as-is and the other uses the WWAN core), it will be a big surprise for
> the user to find that the wwan0.1 netdev is not a second connection
> via the first modem, but it is a main connection of the second modem
> :)
>
> And finally, I assume that in the mid-term, we will switch all WWAN
> device drivers to the WWAN core usage, and the WWAN core will be the
> only producer of wwanX devices. As well as all Wi-Fi device drivers
> have been migrated to cfg80211/mac80211 usage. In that context,
> introducing the wwanX.Y naming scheme now and changing it back to the
> wwanX later will be some kind of API breakage.
>
> With all this in mind, I chose the wwanX as the naming template for
> the default data channel netdev.

Ok, you've convinced me... USB WWAN netdev drivers also use this
naming (wwan%d), so if we switch them to WWAN framework at some point,
it will not disturb users (though netdev name should not be considered
as something stable).

Regards,
Loic

Powered by blists - more mailing lists