| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAHNKnsRsfkjiN0VDPRmipXFe_OaSnM=xdF17BPEsuf+J+LAO2w@mail.gmail.com>
Date: Tue, 22 Jun 2021 18:18:53 +0300
From: Sergey Ryazanov <ryazanov.s.a@...il.com>
To: Loic Poulain <loic.poulain@...aro.org>
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
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.
--
Sergey
Powered by blists - more mailing lists