All of lore.kernel.org
 help / color / mirror / Atom feed
From: Sergey Ryazanov <ryazanov.s.a@gmail.com>
To: Loic Poulain <loic.poulain@linaro.org>
Cc: Johannes Berg <johannes@sipsolutions.net>,
	"David S. Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>,
	Network Development <netdev@vger.kernel.org>,
	M Chetan Kumar <m.chetan.kumar@intel.com>,
	Intel Corporation <linuxwwan@intel.com>
Subject: Re: [PATCH net-next v2 08/10] wwan: core: support default netdev creation
Date: Tue, 22 Jun 2021 18:18:53 +0300	[thread overview]
Message-ID: <CAHNKnsRsfkjiN0VDPRmipXFe_OaSnM=xdF17BPEsuf+J+LAO2w@mail.gmail.com> (raw)
In-Reply-To: <CAMZdPi-6Fpft80Vis-NR4grfcyfdH9DTs9BHfE-J+-6_c+2dJw@mail.gmail.com>

Hi Loic,

On Tue, Jun 22, 2021 at 4:55 PM Loic Poulain <loic.poulain@linaro.org> wrote:
> On Tue, 22 Jun 2021 at 00:51, Sergey Ryazanov <ryazanov.s.a@gmail.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@gmail.com>
>> CC: M Chetan Kumar <m.chetan.kumar@intel.com>
>> CC: Intel Corporation <linuxwwan@intel.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.

-- 
Sergey

  reply	other threads:[~2021-06-22 15:19 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-21 22:50 [PATCH net-next v2 00/10] net: WWAN link creation improvements Sergey Ryazanov
2021-06-21 22:50 ` [PATCH net-next v2 01/10] wwan_hwsim: support network interface creation Sergey Ryazanov
2021-06-21 22:50 ` [PATCH net-next v2 02/10] wwan: core: relocate ops registering code Sergey Ryazanov
2021-06-21 22:50 ` [PATCH net-next v2 03/10] wwan: core: require WWAN netdev setup callback existence Sergey Ryazanov
2021-06-22 13:47   ` Loic Poulain
2021-06-21 22:50 ` [PATCH net-next v2 04/10] wwan: core: multiple netdevs deletion support Sergey Ryazanov
2021-06-22 13:48   ` Loic Poulain
2021-06-21 22:50 ` [PATCH net-next v2 05/10] wwan: core: remove all netdevs on ops unregistering Sergey Ryazanov
2021-06-22 13:50   ` Loic Poulain
2021-06-21 22:50 ` [PATCH net-next v2 06/10] net: iosm: drop custom netdev(s) removing Sergey Ryazanov
2021-06-21 22:50 ` [PATCH net-next v2 07/10] wwan: core: no more hold netdev ops owning module Sergey Ryazanov
2021-06-22 13:51   ` Loic Poulain
2021-06-21 22:50 ` [PATCH net-next v2 08/10] wwan: core: support default netdev creation Sergey Ryazanov
2021-06-22 14:04   ` Loic Poulain
2021-06-22 15:18     ` Sergey Ryazanov [this message]
2021-06-22 17:11       ` Loic Poulain
2021-06-21 22:50 ` [PATCH net-next v2 09/10] net: iosm: create default link via WWAN core Sergey Ryazanov
2021-06-21 22:51 ` [PATCH net-next v2 10/10] wwan: core: add WWAN common private data for netdev Sergey Ryazanov
2021-06-22 17:20 ` [PATCH net-next v2 00/10] net: WWAN link creation improvements patchwork-bot+netdevbpf

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAHNKnsRsfkjiN0VDPRmipXFe_OaSnM=xdF17BPEsuf+J+LAO2w@mail.gmail.com' \
    --to=ryazanov.s.a@gmail.com \
    --cc=davem@davemloft.net \
    --cc=johannes@sipsolutions.net \
    --cc=kuba@kernel.org \
    --cc=linuxwwan@intel.com \
    --cc=loic.poulain@linaro.org \
    --cc=m.chetan.kumar@intel.com \
    --cc=netdev@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.