| Message ID | 20210621225100.21005-9-ryazanov.s.a@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | net: WWAN link creation improvements | expand |
Hi Sergey, 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. Regards, Loic
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
On Tue, 22 Jun 2021 at 17:19, Sergey Ryazanov <ryazanov.s.a@gmail.com> wrote: > > 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. 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
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); + if (WARN_ON(IS_ERR(dev))) + goto unlock; + + if (WARN_ON(wwan_rtnl_newlink(&init_net, dev, tb, data, NULL))) { + free_netdev(dev); + goto unlock; + } + +unlock: + rtnl_unlock(); + +free_attrs: + nlmsg_free(msg); +} + /** * wwan_register_ops - register WWAN device ops * @parent: Device to use as parent and shared by all WWAN ports and * created netdevs * @ops: operations to register * @ctxt: context to pass to operations + * @def_link_id: id of the default link that will be automatically created by + * the WWAN core for the WWAN device. The default link will not be created + * if the passed value is WWAN_NO_DEFAULT_LINK. * * Returns: 0 on success, a negative error code on failure */ int wwan_register_ops(struct device *parent, const struct wwan_ops *ops, - void *ctxt) + void *ctxt, u32 def_link_id) { struct wwan_device *wwandev; @@ -932,6 +996,15 @@ int wwan_register_ops(struct device *parent, const struct wwan_ops *ops, wwandev->ops = ops; wwandev->ops_ctxt = ctxt; + /* NB: we do not abort ops registration in case of default link + * creation failure. Link ops is the management interface, while the + * default link creation is a service option. And we should not prevent + * a user from manually creating a link latter if service option failed + * now. + */ + if (def_link_id != WWAN_NO_DEFAULT_LINK) + wwan_create_default_link(wwandev, def_link_id); + return 0; } EXPORT_SYMBOL_GPL(wwan_register_ops); diff --git a/drivers/net/wwan/wwan_hwsim.c b/drivers/net/wwan/wwan_hwsim.c index a8582a58a385..5b62cf3b3c42 100644 --- a/drivers/net/wwan/wwan_hwsim.c +++ b/drivers/net/wwan/wwan_hwsim.c @@ -288,7 +288,7 @@ static struct wwan_hwsim_dev *wwan_hwsim_dev_new(void) INIT_WORK(&dev->del_work, wwan_hwsim_dev_del_work); - err = wwan_register_ops(&dev->dev, &wwan_hwsim_wwan_rtnl_ops, dev); + err = wwan_register_ops(&dev->dev, &wwan_hwsim_wwan_rtnl_ops, dev, 1); if (err) goto err_unreg_dev; diff --git a/include/linux/wwan.h b/include/linux/wwan.h index e1981ea3a2fd..91590db70a12 100644 --- a/include/linux/wwan.h +++ b/include/linux/wwan.h @@ -126,6 +126,12 @@ void wwan_port_txon(struct wwan_port *port); */ void *wwan_port_get_drvdata(struct wwan_port *port); +/* + * Used to indicate that the WWAN core should not create a default network + * link. + */ +#define WWAN_NO_DEFAULT_LINK U32_MAX + /** * struct wwan_ops - WWAN device ops * @priv_size: size of private netdev data area @@ -143,7 +149,7 @@ struct wwan_ops { }; int wwan_register_ops(struct device *parent, const struct wwan_ops *ops, - void *ctxt); + void *ctxt, u32 def_link_id); void wwan_unregister_ops(struct device *parent);
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(-)