This series is intended to make the WWAN network links management easier
for WWAN device drivers.
The series begins with adding support for network links creation to the
WWAN HW simulator to facilitate code testing. Then there are a couple of
changes that prepe the WWAN core code for further modifications. The
following patches (4-6) simplify driver unregistering procedures by
performing the created links cleanup in the WWAN core. 7th patch is to
avoid the odd hold of a driver module. Next patches (8th and 9th) make
it easier for drivers to create a network interface for a default data
channel. Finally, 10th patch adds support for reporting of data link
(aka channel aka context) id to make user aware which network
interface is binded to which WWAN device data channel.
I have a quite busy last week, and I am sorry publishing these changes
so too late, after all frameworks and drivers have been merged to the
net-next tree. On the other hand, it may be good that we have all
drivers in the tree, so we have a more complete picture.
All core changes have been tested with the HW simulator. The MHI and
IOSM drivers were only compile tested as I have no access to this
hardware. So the coresponding patches require ACK from the driver
authors.
Sergey Ryazanov (10):
wwan_hwsim: support network interface creation
wwan: core: relocate ops registering code
wwan: core: require WWAN netdev setup callback existence
wwan: core: multiple netdevs deletion support
wwan: core: remove all netdevs on ops unregistering
net: iosm: drop custom netdev(s) removing
wwan: core: no more hold netdev ops owning module
wwan: core: support default netdev creation
net: mhi_net: create default link via WWAN core
wwan: core: add WWAN common private data for netdev
drivers/net/mhi/net.c | 66 ++-----
drivers/net/mhi/proto_mbim.c | 5 +-
drivers/net/wwan/iosm/iosm_ipc_wwan.c | 30 +--
drivers/net/wwan/wwan_core.c | 258 ++++++++++++++++++--------
drivers/net/wwan/wwan_hwsim.c | 47 +++++
include/linux/wwan.h | 28 ++-
6 files changed, 281 insertions(+), 153 deletions(-)
--
2.26.3
It is unlikely that RTNL callbacks will call WWAN ops (un-)register
functions, but it is highly likely that the ops (un-)register functions
will use RTNL link create/destroy handlers. So move the WWAN network
interface ops (un-)register functions below the RTNL callbacks to be
able to call them without forward declarations.
No functional changes, just code relocation.
Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
---
drivers/net/wwan/wwan_core.c | 142 +++++++++++++++++------------------
1 file changed, 71 insertions(+), 71 deletions(-)
Use unregister_netdevice_queue() instead of simple
unregister_netdevice() if the WWAN netdev ops does not provide a dellink
callback. This will help to accelerate deletion of multiple netdevs.
Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
---
drivers/net/wwan/wwan_core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
The WWAN netdev ops owner holding was used to protect from the
unexpected memory disappear. This approach causes a dependency cycle
(driver -> core -> driver) and effectively prevents a WWAN driver
unloading. E.g. WWAN hwsim could not be unloaded until all simulated
devices are removed:
~# modprobe wwan_hwsim devices=2
~# lsmod | grep wwan
wwan_hwsim 16384 2
wwan 20480 1 wwan_hwsim
~# rmmod wwan_hwsim
rmmod: ERROR: Module wwan_hwsim is in use
~# echo > /sys/kernel/debug/wwan_hwsim/hwsim0/destroy
~# echo > /sys/kernel/debug/wwan_hwsim/hwsim1/destroy
~# lsmod | grep wwan
wwan_hwsim 16384 0
wwan 20480 1 wwan_hwsim
~# rmmod wwan_hwsim
For a real device driver this will cause an inability to unload module
until a served device is physically detached.
Since the last commit we are removing all child netdev(s) when a driver
unregister the netdev ops. This allows us to permit the driver
unloading, since any sane driver will call ops unregistering on a device
deinitialization. So, remove the holding of an ops owner to make it
easier to unload a driver module. The owner field has also beed removed
from the ops structure as there are no more users of this field.
Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
---
drivers/net/mhi/net.c | 1 -
drivers/net/wwan/wwan_core.c | 10 ----------
drivers/net/wwan/wwan_hwsim.c | 1 -
include/linux/wwan.h | 2 --
4 files changed, 14 deletions(-)
Add support for networking interface creation via the WWAN core by
registering the WWAN netdev creation ops for each simulated WWAN device.
Implemented minimalistic netdev support where the xmit callback just
consumes all egress skbs.
This should help with WWAN network interfaces creation testing.
Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
---
drivers/net/wwan/wwan_hwsim.c | 48 +++++++++++++++++++++++++++++++++++
1 file changed, 48 insertions(+)
@@ -265,6 +304,12 @@ static struct wwan_hwsim_dev *wwan_hwsim_dev_new(void)returndev;+err_unreg_dev:+device_unregister(&dev->dev);+/* Memory will be freed in the device release callback */++returnERR_PTR(err);+err_free_dev:kfree(dev);
@@ -290,6 +335,9 @@ static void wwan_hwsim_dev_del(struct wwan_hwsim_dev *dev)debugfs_remove(dev->debugfs_topdir);+/* This will remove all child netdev(s) */+wwan_unregister_ops(&dev->dev);+/* Make sure that there is no pending deletion work */if(current_work()!=&dev->del_work)cancel_work_sync(&dev->del_work);
Since the last commit, the WWAN core will remove all our network
interfaces for us at the time of the WWAN netdev ops unregistering.
Therefore, we can safely drop the custom code that cleaning the list of
created netdevs. Anyway it no longer removes any netdev, since all
netdevs were removed earlier in the wwan_unregister_ops() call.
Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
CC: M Chetan Kumar <redacted>
CC: Intel Corporation <redacted>
---
drivers/net/wwan/iosm/iosm_ipc_wwan.c | 15 +--------------
1 file changed, 1 insertion(+), 14 deletions(-)
The setup callback will be unconditionally passed to the
alloc_netdev_mqs(), where the NULL pointer dereference will cause the
kernel panic. So refuse to register WWAN netdev ops with warning
generation if the setup callback is not provided.
Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
---
drivers/net/wwan/wwan_core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
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(-)
@@ -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)gotoout_err;
@@ -392,55 +386,19 @@ const struct wwan_ops mhi_wwan_ops = {staticintmhi_net_probe(structmhi_device*mhi_dev,conststructmhi_device_id*id){-conststructmhi_device_info*info=(structmhi_device_info*)id->driver_data;structmhi_controller*cntrl=mhi_dev->mhi_cntrl;-structnet_device*ndev;-interr;--err=wwan_register_ops(&cntrl->mhi_dev->dev,&mhi_wwan_ops,mhi_dev,-WWAN_NO_DEFAULT_LINK);-if(err)-returnerr;--if(!create_default_iface)-return0;--/* Create a default interface which is used as either RMNET real-dev,-*MBIMlink0oriplink0)-*/-ndev=alloc_netdev(sizeof(structmhi_net_dev),info->netname,-NET_NAME_PREDICTABLE,mhi_net_setup);-if(!ndev){-err=-ENOMEM;-gotoerr_unregister;-}--SET_NETDEV_DEV(ndev,&mhi_dev->dev);-err=mhi_net_newlink(mhi_dev,ndev,0,NULL);-if(err)-gotoerr_release;--return0;--err_release:-free_netdev(ndev);-err_unregister:-wwan_unregister_ops(&cntrl->mhi_dev->dev);--returnerr;+returnwwan_register_ops(&cntrl->mhi_dev->dev,&mhi_wwan_ops,mhi_dev,+create_default_iface?0:+WWAN_NO_DEFAULT_LINK);}staticvoidmhi_net_remove(structmhi_device*mhi_dev){-structmhi_net_dev*mhi_netdev=dev_get_drvdata(&mhi_dev->dev);structmhi_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);}staticconststructmhi_device_infomhi_hwip0={
We use the ops owner module hold to protect against ops memory
disappearing. But this approach does not protect us from a driver that
unregisters ops but forgets to remove netdev(s) that were created using
this ops. In such case, we are left with netdev(s), which can not be
removed since ops is gone. Moreover, batch netdevs removing on
deinitialization is a desireable option for WWAN drivers as it is a
quite common task.
Implement deletion of all created links on WWAN netdev ops unregistering
in the same way that RTNL removes all links on RTNL ops unregistering.
Simply remove all child netdevs of a device whose WWAN netdev ops is
unregistering. This way we protecting the kernel from buggy drivers and
make it easier to write a driver deinitialization code.
Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
---
drivers/net/wwan/wwan_core.c | 40 ++++++++++++++++++++++++++++--------
1 file changed, 31 insertions(+), 9 deletions(-)
The WWAN core not only multiplex the netdev configuration data, but
process it too, and needs some space to store its private data
associated with the netdev. Add a structure to keep common WWAN core
data. The structure will be stored inside the netdev private data before
WWAN driver private data and have a field to make it easier to access
the driver data. Also add a helper function that simplifies drivers
access to their data.
At the moment we use the common WWAN private data to store the WWAN data
link (channel) id at the time the link is created, and report it back to
user using the .fill_info() RTNL callback. This should help the user to
be aware which network interface is binded to which WWAN device data
channel.
Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
CC: M Chetan Kumar <redacted>
CC: Intel Corporation <redacted>
---
drivers/net/mhi/net.c | 12 +++++------
drivers/net/mhi/proto_mbim.c | 5 +++--
drivers/net/wwan/iosm/iosm_ipc_wwan.c | 12 +++++------
drivers/net/wwan/wwan_core.c | 29 ++++++++++++++++++++++++++-
include/linux/wwan.h | 18 +++++++++++++++++
5 files changed, 61 insertions(+), 15 deletions(-)
@@ -51,7 +51,7 @@ struct iosm_wwan {/* Bring-up the wwan net link */staticintipc_wwan_link_open(structnet_device*netdev){-structiosm_netdev_priv*priv=netdev_priv(netdev);+structiosm_netdev_priv*priv=wwan_netdev_drvpriv(netdev);structiosm_wwan*ipc_wwan=priv->ipc_wwan;intif_id=priv->if_id;intret;
@@ -88,7 +88,7 @@ static int ipc_wwan_link_open(struct net_device *netdev)/* Bring-down the wwan net link */staticintipc_wwan_link_stop(structnet_device*netdev){-structiosm_netdev_priv*priv=netdev_priv(netdev);+structiosm_netdev_priv*priv=wwan_netdev_drvpriv(netdev);netif_stop_queue(netdev);
@@ -105,7 +105,7 @@ static int ipc_wwan_link_stop(struct net_device *netdev)staticintipc_wwan_link_transmit(structsk_buff*skb,structnet_device*netdev){-structiosm_netdev_priv*priv=netdev_priv(netdev);+structiosm_netdev_priv*priv=wwan_netdev_drvpriv(netdev);structiosm_wwan*ipc_wwan=priv->ipc_wwan;intif_id=priv->if_id;intret;
@@ -117,6 +118,23 @@ void wwan_port_txon(struct wwan_port *port);*/void*wwan_port_get_drvdata(structwwan_port*port);+/**+*structwwan_netdev_priv-WWANcorenetworkdeviceprivatedata+*@link_id:WWANdevicedatalinkid+*@drv_priv:driverprivatedataarea,sizeisdeterminedin&wwan_ops+*/+structwwan_netdev_priv{+u32link_id;++/* must be last */+u8drv_priv[]__aligned(sizeof(void*));+};++staticinlinevoid*wwan_netdev_drvpriv(structnet_device*dev)+{+return((structwwan_netdev_priv*)netdev_priv(dev))->drv_priv;+}+/***UsedtoindicatethattheWWANcoreshouldnotcreateadefaultnetwork*link.
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 <redacted>
CC: Intel Corporation <redacted>
---
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(-)
@@ -895,17 +895,81 @@ static struct rtnl_link_ops wwan_rtnl_link_ops __read_mostly = {.policy=wwan_rtnl_policy,};+staticvoidwwan_create_default_link(structwwan_device*wwandev,+u32def_link_id)+{+structnlattr*tb[IFLA_MAX+1],*linkinfo[IFLA_INFO_MAX+1];+structnlattr*data[IFLA_WWAN_MAX+1];+structnet_device*dev;+structnlmsghdr*nlh;+structsk_buff*msg;++/* Forge attributes required to create a WWAN netdev. We first+*buildanetlinkmessageandthenparseit.Thislooks+*odd,butsuchapproachislesserrorprone.+*/+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))+gotofree_attrs;++if(nla_put_string(msg,IFLA_PARENT_DEV_NAME,dev_name(&wwandev->dev)))+gotofree_attrs;+tb[IFLA_LINKINFO]=nla_nest_start(msg,IFLA_LINKINFO);+if(!tb[IFLA_LINKINFO])+gotofree_attrs;+linkinfo[IFLA_INFO_DATA]=nla_nest_start(msg,IFLA_INFO_DATA);+if(!linkinfo[IFLA_INFO_DATA])+gotofree_attrs;+if(nla_put_u32(msg,IFLA_WWAN_LINK_ID,def_link_id))+gotofree_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)))+gotounlock;++if(WARN_ON(wwan_rtnl_newlink(&init_net,dev,tb,data,NULL))){+free_netdev(dev);+gotounlock;+}++unlock:+rtnl_unlock();++free_attrs:+nlmsg_free(msg);+}+/***wwan_register_ops-registerWWANdeviceops*@parent:DevicetouseasparentandsharedbyallWWANportsand*creatednetdevs*@ops:operationstoregister*@ctxt:contexttopasstooperations+*@def_link_id:idofthedefaultlinkthatwillbeautomaticallycreatedby+*theWWANcorefortheWWANdevice.Thedefaultlinkwillnotbecreated+*ifthepassedvalueisWWAN_NO_DEFAULT_LINK.**Returns:0onsuccess,anegativeerrorcodeonfailure*/intwwan_register_ops(structdevice*parent,conststructwwan_ops*ops,-void*ctxt)+void*ctxt,u32def_link_id){structwwan_device*wwandev;
@@ -924,6 +988,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+*creationfailure.Linkopsisthemanagementinterface,whilethe+*defaultlinkcreationisaserviceoption.Andweshouldnotprevent+*auserfrommanuallycreatingalinklatterifserviceoptionfailed+*now.+*/+if(def_link_id!=WWAN_NO_DEFAULT_LINK)+wwan_create_default_link(wwandev,def_link_id);+return0;}EXPORT_SYMBOL_GPL(wwan_register_ops);
Hi Sergey,
On Tue, 15 Jun 2021 at 02:30, Sergey Ryazanov [off-list ref] wrote:
quoted hunk
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(-)
@@ -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)gotoout_err;
@@ -392,55 +386,19 @@ const struct wwan_ops mhi_wwan_ops = {staticintmhi_net_probe(structmhi_device*mhi_dev,conststructmhi_device_id*id){-conststructmhi_device_info*info=(structmhi_device_info*)id->driver_data;structmhi_controller*cntrl=mhi_dev->mhi_cntrl;-structnet_device*ndev;-interr;--err=wwan_register_ops(&cntrl->mhi_dev->dev,&mhi_wwan_ops,mhi_dev,-WWAN_NO_DEFAULT_LINK);-if(err)-returnerr;--if(!create_default_iface)-return0;--/* Create a default interface which is used as either RMNET real-dev,-*MBIMlink0oriplink0)-*/-ndev=alloc_netdev(sizeof(structmhi_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.
Regards,
Loic
Hi Sergey,
On Tue, 15 Jun 2021 at 02:30, Sergey Ryazanov [off-list ref] wrote:
The WWAN core not only multiplex the netdev configuration data, but
process it too, and needs some space to store its private data
associated with the netdev. Add a structure to keep common WWAN core
data. The structure will be stored inside the netdev private data before
WWAN driver private data and have a field to make it easier to access
the driver data. Also add a helper function that simplifies drivers
access to their data.
Would it be possible to store wwan_netdev_priv at the end of priv data instead?
That would allow drivers to use the standard netdev_priv without any change.
And would also simplify forwarding to rmnet (in mhi_net) since rmnet
uses netdev_priv.
Regards,
Loic
At the moment we use the common WWAN private data to store the WWAN data
link (channel) id at the time the link is created, and report it back to
user using the .fill_info() RTNL callback. This should help the user to
be aware which network interface is binded to which WWAN device data
channel.
Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
CC: M Chetan Kumar <redacted>
CC: Intel Corporation <redacted>
---
drivers/net/mhi/net.c | 12 +++++------
drivers/net/mhi/proto_mbim.c | 5 +++--
drivers/net/wwan/iosm/iosm_ipc_wwan.c | 12 +++++------
drivers/net/wwan/wwan_core.c | 29 ++++++++++++++++++++++++++-
include/linux/wwan.h | 18 +++++++++++++++++
5 files changed, 61 insertions(+), 15 deletions(-)
From: Johannes Berg <johannes@sipsolutions.net> Date: 2021-06-15 07:31:36
On Tue, 2021-06-15 at 03:30 +0300, Sergey Ryazanov wrote:
The WWAN core not only multiplex the netdev configuration data, but
process it too, and needs some space to store its private data
associated with the netdev. Add a structure to keep common WWAN core
data. The structure will be stored inside the netdev private data before
WWAN driver private data and have a field to make it easier to access
the driver data. Also add a helper function that simplifies drivers
access to their data.
At the moment we use the common WWAN private data to store the WWAN data
link (channel) id at the time the link is created, and report it back to
user using the .fill_info() RTNL callback. This should help the user to
be aware which network interface is binded to which WWAN device data
Hi Loic,
CC Aleksander, as the talk drifts towards ModemManager.
On Tue, Jun 15, 2021 at 10:08 AM Loic Poulain [off-list ref] wrote:
On Tue, 15 Jun 2021 at 02:30, Sergey Ryazanov [off-list ref] wrote:
quoted
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(-)
@@ -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)gotoout_err;
@@ -392,55 +386,19 @@ const struct wwan_ops mhi_wwan_ops = {staticintmhi_net_probe(structmhi_device*mhi_dev,conststructmhi_device_id*id){-conststructmhi_device_info*info=(structmhi_device_info*)id->driver_data;structmhi_controller*cntrl=mhi_dev->mhi_cntrl;-structnet_device*ndev;-interr;--err=wwan_register_ops(&cntrl->mhi_dev->dev,&mhi_wwan_ops,mhi_dev,-WWAN_NO_DEFAULT_LINK);-if(err)-returnerr;--if(!create_default_iface)-return0;--/* Create a default interface which is used as either RMNET real-dev,-*MBIMlink0oriplink0)-*/-ndev=alloc_netdev(sizeof(structmhi_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
On Tue, Jun 15, 2021 at 10:24 AM Loic Poulain [off-list ref] wrote:
On Tue, 15 Jun 2021 at 02:30, Sergey Ryazanov [off-list ref] wrote:
quoted
The WWAN core not only multiplex the netdev configuration data, but
process it too, and needs some space to store its private data
associated with the netdev. Add a structure to keep common WWAN core
data. The structure will be stored inside the netdev private data before
WWAN driver private data and have a field to make it easier to access
the driver data. Also add a helper function that simplifies drivers
access to their data.
Would it be possible to store wwan_netdev_priv at the end of priv data instead?
That would allow drivers to use the standard netdev_priv without any change.
And would also simplify forwarding to rmnet (in mhi_net) since rmnet
uses netdev_priv.
I do not think that mimicking something by one subsystem for another
is generally a good idea. This could look good in a short term, but
finally it will become a headache due to involvement of too many
entities.
IMHO, a suitable approach to share the rmnet library and data
structures among drivers is to make the rmnet interface more generic.
E.g. consider such netdev/rtnl specific function:
static int rmnet_foo_action(struct net_device *dev, ...)
{
struct rmnet_priv *rmdev = netdev_priv(dev);
<do a foo action here>
}
It could be split into a wrapper and an actual handler:
int __rmnet_foo_action(struct rmnet_priv *rmdev, ...)
{
<do a foo action here>
}
EXPORT_GPL(__rmnet_foo_action)
static int rmnet_foo_action(struct net_device *dev, ...)
{
struct rmnet_priv *rmdev = netdev_priv(dev);
return __rmnet_foo_action(rmdev, ...)
}
So a call from mhi_net to rmnet could looks like this:
static int mhi_net_foo_action(struct net_device *dev, ...)
{
struct rmnet_priv *rmdev = wwan_netdev_drvpriv(dev);
return __rmnet_foo_action(rmdev, ...)
}
In such a way, only the rmnet users know something special, while
other wwan core users and the core itself behave without any
surprises. E.g. any regular wwan core minidriver can access the
link_id field of the wwan common data by calling netdev_priv() without
further calculating the common data offset.
quoted
At the moment we use the common WWAN private data to store the WWAN data
link (channel) id at the time the link is created, and report it back to
user using the .fill_info() RTNL callback. This should help the user to
be aware which network interface is binded to which WWAN device data
channel.
Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
CC: M Chetan Kumar <redacted>
CC: Intel Corporation <redacted>
---
drivers/net/mhi/net.c | 12 +++++------
drivers/net/mhi/proto_mbim.c | 5 +++--
drivers/net/wwan/iosm/iosm_ipc_wwan.c | 12 +++++------
drivers/net/wwan/wwan_core.c | 29 ++++++++++++++++++++++++++-
include/linux/wwan.h | 18 +++++++++++++++++
5 files changed, 61 insertions(+), 15 deletions(-)
Hello Johannes,
On Tue, Jun 15, 2021 at 10:31 AM Johannes Berg
[off-list ref] wrote:
On Tue, 2021-06-15 at 03:30 +0300, Sergey Ryazanov wrote:
quoted
The WWAN core not only multiplex the netdev configuration data, but
process it too, and needs some space to store its private data
associated with the netdev. Add a structure to keep common WWAN core
data. The structure will be stored inside the netdev private data before
WWAN driver private data and have a field to make it easier to access
the driver data. Also add a helper function that simplifies drivers
access to their data.
At the moment we use the common WWAN private data to store the WWAN data
link (channel) id at the time the link is created, and report it back to
user using the .fill_info() RTNL callback. This should help the user to
be aware which network interface is binded to which WWAN device data
Not sure I like that code style, but I guess I don't care much either :)
Yeah. I am not happy with that code style either. But this is the only
git-blame friendly style known to me that allows us to add new lines
without touching existing ones :(
--
Sergey
Hello Chetan,
On Tue, Jun 15, 2021 at 3:30 AM Sergey Ryazanov [off-list ref] wrote:
Since the last commit, the WWAN core will remove all our network
interfaces for us at the time of the WWAN netdev ops unregistering.
Therefore, we can safely drop the custom code that cleaning the list of
created netdevs. Anyway it no longer removes any netdev, since all
netdevs were removed earlier in the wwan_unregister_ops() call.
Are you Ok with this change? I plan to submit a next version of the
series. If you have any objections, I can address them in V2.
BTW, if IOSM modems have a default data channel, I can add a separate
patch to the series to create a default network interface for IOSM if
you tell me which link id is used for the default data channel.
From: Kumar, M Chetan <hidden> Date: 2021-06-20 15:42:53
Hi Sergey,
On Tue, Jun 15, 2021 at 3:30 AM Sergey Ryazanov
[off-list ref] wrote:
quoted
Since the last commit, the WWAN core will remove all our network
interfaces for us at the time of the WWAN netdev ops unregistering.
Therefore, we can safely drop the custom code that cleaning the list
of created netdevs. Anyway it no longer removes any netdev, since all
netdevs were removed earlier in the wwan_unregister_ops() call.
Are you Ok with this change? I plan to submit a next version of the series. If
you have any objections, I can address them in V2.
Changes looks fine.
BTW, if IOSM modems have a default data channel, I can add a separate
patch to the series to create a default network interface for IOSM if you tell
me which link id is used for the default data channel.
Link id 1 is always associated as default data channel.
Thanks,
Reviewed-by: M Chetan Kumar <redacted>
On Sun, Jun 20, 2021 at 6:42 PM Kumar, M Chetan
[off-list ref] wrote:
quoted
On Tue, Jun 15, 2021 at 3:30 AM Sergey Ryazanov
[off-list ref] wrote:
quoted
Since the last commit, the WWAN core will remove all our network
interfaces for us at the time of the WWAN netdev ops unregistering.
Therefore, we can safely drop the custom code that cleaning the list
of created netdevs. Anyway it no longer removes any netdev, since all
netdevs were removed earlier in the wwan_unregister_ops() call.
Are you Ok with this change? I plan to submit a next version of the series. If
you have any objections, I can address them in V2.
Changes looks fine.
quoted
BTW, if IOSM modems have a default data channel, I can add a separate
patch to the series to create a default network interface for IOSM if you tell
me which link id is used for the default data channel.
Link id 1 is always associated as default data channel.
Thank you, will add default interface creation with this Id in the V2 series.
Hi Sergey,
On Sun, 20 Jun 2021 at 15:51, Sergey Ryazanov [off-list ref] wrote:
Hi Loic,
CC Aleksander, as the talk drifts towards ModemManager.
On Tue, Jun 15, 2021 at 10:08 AM Loic Poulain [off-list ref] wrote:
quoted
On Tue, 15 Jun 2021 at 02:30, Sergey Ryazanov [off-list ref] wrote:
quoted
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(-)
@@ -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)gotoout_err;
@@ -392,55 +386,19 @@ const struct wwan_ops mhi_wwan_ops = {staticintmhi_net_probe(structmhi_device*mhi_dev,conststructmhi_device_id*id){-conststructmhi_device_info*info=(structmhi_device_info*)id->driver_data;structmhi_controller*cntrl=mhi_dev->mhi_cntrl;-structnet_device*ndev;-interr;--err=wwan_register_ops(&cntrl->mhi_dev->dev,&mhi_wwan_ops,mhi_dev,-WWAN_NO_DEFAULT_LINK);-if(err)-returnerr;--if(!create_default_iface)-return0;--/* Create a default interface which is used as either RMNET real-dev,-*MBIMlink0oriplink0)-*/-ndev=alloc_netdev(sizeof(structmhi_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.
Hi Sergey,
On Sun, 20 Jun 2021 at 16:39, Sergey Ryazanov [off-list ref] wrote:
On Tue, Jun 15, 2021 at 10:24 AM Loic Poulain [off-list ref] wrote:
quoted
On Tue, 15 Jun 2021 at 02:30, Sergey Ryazanov [off-list ref] wrote:
quoted
The WWAN core not only multiplex the netdev configuration data, but
process it too, and needs some space to store its private data
associated with the netdev. Add a structure to keep common WWAN core
data. The structure will be stored inside the netdev private data before
WWAN driver private data and have a field to make it easier to access
the driver data. Also add a helper function that simplifies drivers
access to their data.
Would it be possible to store wwan_netdev_priv at the end of priv data instead?
That would allow drivers to use the standard netdev_priv without any change.
And would also simplify forwarding to rmnet (in mhi_net) since rmnet
uses netdev_priv.
I do not think that mimicking something by one subsystem for another
is generally a good idea. This could look good in a short term, but
finally it will become a headache due to involvement of too many
entities.
IMHO, a suitable approach to share the rmnet library and data
structures among drivers is to make the rmnet interface more generic.
E.g. consider such netdev/rtnl specific function:
static int rmnet_foo_action(struct net_device *dev, ...)
{
struct rmnet_priv *rmdev = netdev_priv(dev);
<do a foo action here>
}
It could be split into a wrapper and an actual handler:
int __rmnet_foo_action(struct rmnet_priv *rmdev, ...)
{
<do a foo action here>
}
EXPORT_GPL(__rmnet_foo_action)
static int rmnet_foo_action(struct net_device *dev, ...)
{
struct rmnet_priv *rmdev = netdev_priv(dev);
return __rmnet_foo_action(rmdev, ...)
}
So a call from mhi_net to rmnet could looks like this:
static int mhi_net_foo_action(struct net_device *dev, ...)
{
struct rmnet_priv *rmdev = wwan_netdev_drvpriv(dev);
return __rmnet_foo_action(rmdev, ...)
}
In such a way, only the rmnet users know something special, while
other wwan core users and the core itself behave without any
surprises. E.g. any regular wwan core minidriver can access the
link_id field of the wwan common data by calling netdev_priv() without
further calculating the common data offset.
Yes, that would work, but it's an important refactoring since rmnet is
all built around the idea that netdev_priv is rmnet_priv, including rx
path (netdev_priv(skb->dev)).
My initial tests were based on this 'simple' change:
https://git.linaro.org/people/loic.poulain/linux.git/commit/?h=wwan_rmnet&id=6308d49790f10615bd33a38d56bc7f101646558f
Moreover, a driver like mhi_net also supports non WWAN local link
(called mhi_swip), which is a network link between the host and the
modem cpu (for modem hosted services...). This link is not managed by
the WWAN layer and is directly created by mhi_net. I could create a
different driver or set of handlers for this netdev, but it's
additional complexity.
quoted
quoted
At the moment we use the common WWAN private data to store the WWAN data
link (channel) id at the time the link is created, and report it back to
user using the .fill_info() RTNL callback. This should help the user to
be aware which network interface is binded to which WWAN device data
channel.
I wonder if it would not be simpler to store the link ID into
netdev->dev_port, it's after all a kind of virtual/logical port.
That would only postpone the introduction of a wwan_netdev_priv struct though.
Regards,
Loic
Hi Loic,
On Mon, Jun 21, 2021 at 9:44 AM Loic Poulain [off-list ref] wrote:
On Sun, 20 Jun 2021 at 15:51, Sergey Ryazanov [off-list ref] wrote:
quoted
On Tue, Jun 15, 2021 at 10:08 AM Loic Poulain [off-list ref] wrote:
quoted
On Tue, 15 Jun 2021 at 02:30, Sergey Ryazanov [off-list ref] wrote:
quoted
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(-)
@@ -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)gotoout_err;
@@ -392,55 +386,19 @@ const struct wwan_ops mhi_wwan_ops = {staticintmhi_net_probe(structmhi_device*mhi_dev,conststructmhi_device_id*id){-conststructmhi_device_info*info=(structmhi_device_info*)id->driver_data;structmhi_controller*cntrl=mhi_dev->mhi_cntrl;-structnet_device*ndev;-interr;--err=wwan_register_ops(&cntrl->mhi_dev->dev,&mhi_wwan_ops,mhi_dev,-WWAN_NO_DEFAULT_LINK);-if(err)-returnerr;--if(!create_default_iface)-return0;--/* Create a default interface which is used as either RMNET real-dev,-*MBIMlink0oriplink0)-*/-ndev=alloc_netdev(sizeof(structmhi_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.
Hi Loic,
On Mon, Jun 21, 2021 at 10:28 AM Loic Poulain [off-list ref] wrote:
On Sun, 20 Jun 2021 at 16:39, Sergey Ryazanov [off-list ref] wrote:
quoted
On Tue, Jun 15, 2021 at 10:24 AM Loic Poulain [off-list ref] wrote:
quoted
On Tue, 15 Jun 2021 at 02:30, Sergey Ryazanov [off-list ref] wrote:
quoted
The WWAN core not only multiplex the netdev configuration data, but
process it too, and needs some space to store its private data
associated with the netdev. Add a structure to keep common WWAN core
data. The structure will be stored inside the netdev private data before
WWAN driver private data and have a field to make it easier to access
the driver data. Also add a helper function that simplifies drivers
access to their data.
Would it be possible to store wwan_netdev_priv at the end of priv data instead?
That would allow drivers to use the standard netdev_priv without any change.
And would also simplify forwarding to rmnet (in mhi_net) since rmnet
uses netdev_priv.
I do not think that mimicking something by one subsystem for another
is generally a good idea. This could look good in a short term, but
finally it will become a headache due to involvement of too many
entities.
IMHO, a suitable approach to share the rmnet library and data
structures among drivers is to make the rmnet interface more generic.
E.g. consider such netdev/rtnl specific function:
static int rmnet_foo_action(struct net_device *dev, ...)
{
struct rmnet_priv *rmdev = netdev_priv(dev);
<do a foo action here>
}
It could be split into a wrapper and an actual handler:
int __rmnet_foo_action(struct rmnet_priv *rmdev, ...)
{
<do a foo action here>
}
EXPORT_GPL(__rmnet_foo_action)
static int rmnet_foo_action(struct net_device *dev, ...)
{
struct rmnet_priv *rmdev = netdev_priv(dev);
return __rmnet_foo_action(rmdev, ...)
}
So a call from mhi_net to rmnet could looks like this:
static int mhi_net_foo_action(struct net_device *dev, ...)
{
struct rmnet_priv *rmdev = wwan_netdev_drvpriv(dev);
return __rmnet_foo_action(rmdev, ...)
}
In such a way, only the rmnet users know something special, while
other wwan core users and the core itself behave without any
surprises. E.g. any regular wwan core minidriver can access the
link_id field of the wwan common data by calling netdev_priv() without
further calculating the common data offset.
Yes, that would work, but it's an important refactoring since rmnet is
all built around the idea that netdev_priv is rmnet_priv, including rx
path (netdev_priv(skb->dev)).
My initial tests were based on this 'simple' change:
https://git.linaro.org/people/loic.poulain/linux.git/commit/?h=wwan_rmnet&id=6308d49790f10615bd33a38d56bc7f101646558f
Moreover, a driver like mhi_net also supports non WWAN local link
(called mhi_swip), which is a network link between the host and the
modem cpu (for modem hosted services...). This link is not managed by
the WWAN layer and is directly created by mhi_net. I could create a
different driver or set of handlers for this netdev, but it's
additional complexity.
Correct me if I am wrong. I just checked the rmnet code and realized
that rmnet should work on top of mhi_net and not vice versa. mhi_net
should provide some kind of transportation for QMAP packets from a HW
device to rmnet. Then rmnet will perform demultiplexing, deaggregation
and decapsulation of QMAP packets to pure IP packets.
rmnet itself receives these QMAP packets via a network device. So any
driver that would like to provide the QMAP transport for rmnet should
create a network device for this task.
The main issue with the integration of mhi_net with the wwan is that
the mhi_net driver should pass its traffic through the rmnet demuxer.
While the network device that will be created by the rmnet demuxer
will not be a child of a MHI device or a wwan device. So to properly
integrate the mhi_net driver with the wwan core netdev capabilities,
you should begin to use rmnet not as an independent demux created on
top of a transport network interface, but as a library. Am I correctly
understanding?
Does the same issue appear when we begin a more tight integration of
the qmi_wwan USB driver with the wwan core?
I would like to say that one way or another, rmnet will be converted
to a quite abstract library that should avoid direct access to the
network device private data.
quoted
quoted
quoted
At the moment we use the common WWAN private data to store the WWAN data
link (channel) id at the time the link is created, and report it back to
user using the .fill_info() RTNL callback. This should help the user to
be aware which network interface is binded to which WWAN device data
channel.
I wonder if it would not be simpler to store the link ID into
netdev->dev_port, it's after all a kind of virtual/logical port.
That would only postpone the introduction of a wwan_netdev_priv struct though.
I like this idea. This is likely to solve the link id storage problem.
But only if we plan to never extend the wwan core private data.
Otherwise, as you mention, this only postpones the wwan data structure
introduction to a moment when we will need to rework a lot of drivers.
Looks like we have no absolutely good solution. Only a set of
proposals, each which has its own shortcomings :(
--
Sergey
On Mon, 21 Jun 2021 at 19:22, Sergey Ryazanov [off-list ref] wrote:
Hi Loic,
On Mon, Jun 21, 2021 at 10:28 AM Loic Poulain [off-list ref] wrote:
quoted
On Sun, 20 Jun 2021 at 16:39, Sergey Ryazanov [off-list ref] wrote:
quoted
On Tue, Jun 15, 2021 at 10:24 AM Loic Poulain [off-list ref] wrote:
quoted
On Tue, 15 Jun 2021 at 02:30, Sergey Ryazanov [off-list ref] wrote:
quoted
The WWAN core not only multiplex the netdev configuration data, but
process it too, and needs some space to store its private data
associated with the netdev. Add a structure to keep common WWAN core
data. The structure will be stored inside the netdev private data before
WWAN driver private data and have a field to make it easier to access
the driver data. Also add a helper function that simplifies drivers
access to their data.
Would it be possible to store wwan_netdev_priv at the end of priv data instead?
That would allow drivers to use the standard netdev_priv without any change.
And would also simplify forwarding to rmnet (in mhi_net) since rmnet
uses netdev_priv.
I do not think that mimicking something by one subsystem for another
is generally a good idea. This could look good in a short term, but
finally it will become a headache due to involvement of too many
entities.
IMHO, a suitable approach to share the rmnet library and data
structures among drivers is to make the rmnet interface more generic.
E.g. consider such netdev/rtnl specific function:
static int rmnet_foo_action(struct net_device *dev, ...)
{
struct rmnet_priv *rmdev = netdev_priv(dev);
<do a foo action here>
}
It could be split into a wrapper and an actual handler:
int __rmnet_foo_action(struct rmnet_priv *rmdev, ...)
{
<do a foo action here>
}
EXPORT_GPL(__rmnet_foo_action)
static int rmnet_foo_action(struct net_device *dev, ...)
{
struct rmnet_priv *rmdev = netdev_priv(dev);
return __rmnet_foo_action(rmdev, ...)
}
So a call from mhi_net to rmnet could looks like this:
static int mhi_net_foo_action(struct net_device *dev, ...)
{
struct rmnet_priv *rmdev = wwan_netdev_drvpriv(dev);
return __rmnet_foo_action(rmdev, ...)
}
In such a way, only the rmnet users know something special, while
other wwan core users and the core itself behave without any
surprises. E.g. any regular wwan core minidriver can access the
link_id field of the wwan common data by calling netdev_priv() without
further calculating the common data offset.
Yes, that would work, but it's an important refactoring since rmnet is
all built around the idea that netdev_priv is rmnet_priv, including rx
path (netdev_priv(skb->dev)).
My initial tests were based on this 'simple' change:
https://git.linaro.org/people/loic.poulain/linux.git/commit/?h=wwan_rmnet&id=6308d49790f10615bd33a38d56bc7f101646558f
Moreover, a driver like mhi_net also supports non WWAN local link
(called mhi_swip), which is a network link between the host and the
modem cpu (for modem hosted services...). This link is not managed by
the WWAN layer and is directly created by mhi_net. I could create a
different driver or set of handlers for this netdev, but it's
additional complexity.
Correct me if I am wrong. I just checked the rmnet code and realized
that rmnet should work on top of mhi_net and not vice versa. mhi_net
should provide some kind of transportation for QMAP packets from a HW
device to rmnet. Then rmnet will perform demultiplexing, deaggregation
and decapsulation of QMAP packets to pure IP packets.
Exact mhi_net act as the transport layer in that case.
rmnet itself receives these QMAP packets via a network device. So any
driver that would like to provide the QMAP transport for rmnet should
create a network device for this task.
Yes, this is what they call the 'real' device (or lower_dev).
The main issue with the integration of mhi_net with the wwan is that
the mhi_net driver should pass its traffic through the rmnet demuxer.
While the network device that will be created by the rmnet demuxer
will not be a child of a MHI device or a wwan device. So to properly
integrate the mhi_net driver with the wwan core netdev capabilities,
you should begin to use rmnet not as an independent demux created on
top of a transport network interface, but as a library. Am I correctly
understanding?
Once the link is created, packets received by the 'real' ndev are
automatically forwarded to the upper layer (in that case, rmnet). So
the 'transport' netdev doesn't even need to know about the upper layer
details.
Does the same issue appear when we begin a more tight integration of
the qmi_wwan USB driver with the wwan core?
That should be handled the same way as for mhi_net, indeed.
I would like to say that one way or another, rmnet will be converted
to a quite abstract library that should avoid direct access to the
network device private data.
quoted
quoted
quoted
quoted
At the moment we use the common WWAN private data to store the WWAN data
link (channel) id at the time the link is created, and report it back to
user using the .fill_info() RTNL callback. This should help the user to
be aware which network interface is binded to which WWAN device data
channel.
I wonder if it would not be simpler to store the link ID into
netdev->dev_port, it's after all a kind of virtual/logical port.
That would only postpone the introduction of a wwan_netdev_priv struct though.
I like this idea. This is likely to solve the link id storage problem.
But only if we plan to never extend the wwan core private data.
Otherwise, as you mention, this only postpones the wwan data structure
introduction to a moment when we will need to rework a lot of drivers.
Looks like we have no absolutely good solution. Only a set of
proposals, each which has its own shortcomings :(
--
Hi Chetan,
On Sun, 20 Jun 2021 at 17:42, Kumar, M Chetan [off-list ref] wrote:
Hi Sergey,
quoted
On Tue, Jun 15, 2021 at 3:30 AM Sergey Ryazanov
[off-list ref] wrote:
quoted
Since the last commit, the WWAN core will remove all our network
interfaces for us at the time of the WWAN netdev ops unregistering.
Therefore, we can safely drop the custom code that cleaning the list
of created netdevs. Anyway it no longer removes any netdev, since all
netdevs were removed earlier in the wwan_unregister_ops() call.
Are you Ok with this change? I plan to submit a next version of the series. If
you have any objections, I can address them in V2.
Changes looks fine.
quoted
BTW, if IOSM modems have a default data channel, I can add a separate
patch to the series to create a default network interface for IOSM if you tell
me which link id is used for the default data channel.
Link id 1 is always associated as default data channel.
Quick question, Isn't your driver use MBIM session IDs? with
session-ID 0 as the default one?
Regards,
Loic
From: Kumar, M Chetan <hidden> Date: 2021-06-29 14:56:33
Hi Loic,
On 6/29/2021 7:44 PM, Loic Poulain wrote:
quoted
quoted
BTW, if IOSM modems have a default data channel, I can add a separate
patch to the series to create a default network interface for IOSM if you tell
me which link id is used for the default data channel.
Link id 1 is always associated as default data channel.
Quick question, Isn't your driver use MBIM session IDs? with
session-ID 0 as the default one?
Link Id from 1 to 8 are treated as valid link ids. These ids are
decremented by 1 to match session id.
In this case link id 1 would be mapped to session id 0. So have
requested link id 1 to be set as default data channel.
Regards,
Chetan
Hi Chetan,
On Tue, 29 Jun 2021 at 16:56, Kumar, M Chetan [off-list ref] wrote:
Hi Loic,
On 6/29/2021 7:44 PM, Loic Poulain wrote:
quoted
quoted
quoted
BTW, if IOSM modems have a default data channel, I can add a separate
patch to the series to create a default network interface for IOSM if you tell
me which link id is used for the default data channel.
Link id 1 is always associated as default data channel.
Quick question, Isn't your driver use MBIM session IDs? with
session-ID 0 as the default one?
Link Id from 1 to 8 are treated as valid link ids. These ids are
decremented by 1 to match session id.
In this case link id 1 would be mapped to session id 0. So have
requested link id 1 to be set as default data channel.
Oh ok, but why? it seems quite confusing, that means a user creating a
MBIM session 0 has to create a link with ID 1?
It seems to be quite specific to your driver, can't you simply handle
ID 0 from user? to keep aligned with other drivers.
Regards,
Loic
From: Kumar, M Chetan <hidden> Date: 2021-06-30 05:11:55
Hi Loic,
On 6/29/2021 8:59 PM, Loic Poulain wrote:
Hi Chetan,
On Tue, 29 Jun 2021 at 16:56, Kumar, M Chetan [off-list ref] wrote:
quoted
Hi Loic,
On 6/29/2021 7:44 PM, Loic Poulain wrote:
quoted
quoted
quoted
BTW, if IOSM modems have a default data channel, I can add a separate
patch to the series to create a default network interface for IOSM if you tell
me which link id is used for the default data channel.
Link id 1 is always associated as default data channel.
Quick question, Isn't your driver use MBIM session IDs? with
session-ID 0 as the default one?
Link Id from 1 to 8 are treated as valid link ids. These ids are
decremented by 1 to match session id.
In this case link id 1 would be mapped to session id 0. So have
requested link id 1 to be set as default data channel.
Oh ok, but why? it seems quite confusing, that means a user creating a
MBIM session 0 has to create a link with ID 1?
It seems to be quite specific to your driver, can't you simply handle
ID 0 from user? to keep aligned with other drivers.
Thought link id 0 is not a valid id so had considered it from 1 :(
Sure, We will correct it to be intact with other drivers.
Regards,
Chetan