| Message ID | 20210615003016.477-10-ryazanov.s.a@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | net: WWAN link creation improvements | expand |
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. -- Sergey
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
Hi Loic, On Mon, Jun 21, 2021 at 9:44 AM Loic Poulain <loic.poulain@linaro.org> wrote: > On Sun, 20 Jun 2021 at 15:51, Sergey Ryazanov <ryazanov.s.a@gmail.com> wrote: >> 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. Thank you for the clarification, I will keep in mind. -- Sergey
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); - if (!ndev) { - err = -ENOMEM; - goto err_unregister; - } - - SET_NETDEV_DEV(ndev, &mhi_dev->dev); - err = mhi_net_newlink(mhi_dev, ndev, 0, NULL); - if (err) - goto err_release; - - return 0; - -err_release: - free_netdev(ndev); -err_unregister: - wwan_unregister_ops(&cntrl->mhi_dev->dev); - - return err; + return wwan_register_ops(&cntrl->mhi_dev->dev, &mhi_wwan_ops, mhi_dev, + create_default_iface ? 0 : + WWAN_NO_DEFAULT_LINK); } static void mhi_net_remove(struct mhi_device *mhi_dev) { - struct mhi_net_dev *mhi_netdev = dev_get_drvdata(&mhi_dev->dev); struct mhi_controller *cntrl = mhi_dev->mhi_cntrl; /* rtnetlink takes care of removing remaining links */ wwan_unregister_ops(&cntrl->mhi_dev->dev); - - if (create_default_iface) - mhi_net_dellink(mhi_dev, mhi_netdev->ndev, NULL); } static const struct mhi_device_info mhi_hwip0 = {
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(-)