Here's a respin of the series to create netdevs through the WWAN
framework. I haven't tested it at all, since I don't have any
such hardware, so I guess there will be some bugs ... It'd be
best if somebody else takes over here, Loic, maybe I can talk
you into getting the generic bits done if you have a test case? :)
This applies on top of the IOSM driver series posted here:
https://lore.kernel.org/r/[email protected]
I've included the first bugfix patch only so it actually all
can apply properly, not really needed for review.
johannes
From: Johannes Berg <[email protected]>
Signed-off-by: Johannes Berg <[email protected]>
---
drivers/net/wwan/iosm/iosm_ipc_wwan.c | 25 +++++++++++++++++--------
1 file changed, 17 insertions(+), 8 deletions(-)
diff --git a/drivers/net/wwan/iosm/iosm_ipc_wwan.c b/drivers/net/wwan/iosm/iosm_ipc_wwan.c
index 02c35bc86674..719c88d9b2e9 100644
--- a/drivers/net/wwan/iosm/iosm_ipc_wwan.c
+++ b/drivers/net/wwan/iosm/iosm_ipc_wwan.c
@@ -312,7 +312,8 @@ int ipc_wwan_receive(struct iosm_wwan *ipc_wwan, struct sk_buff *skb_arg,
bool dss, int if_id)
{
struct sk_buff *skb = skb_arg;
- struct net_device_stats stats;
+ struct net_device_stats *stats;
+ struct iosm_net_link *priv;
int ret;
if ((skb->data[0] & IOSM_IP_TYPE_MASK) == IOSM_IP_TYPE_IPV4)
@@ -325,19 +326,27 @@ int ipc_wwan_receive(struct iosm_wwan *ipc_wwan, struct sk_buff *skb_arg,
if (if_id < (IP_MUX_SESSION_START - 1) ||
if_id > (IP_MUX_SESSION_END - 1)) {
- dev_kfree_skb(skb);
- return -EINVAL;
+ ret = -EINVAL;
+ goto free;
}
rcu_read_lock();
- skb->dev = rcu_dereference(ipc_wwan->sub_netlist[if_id])->netdev;
- stats = rcu_dereference(ipc_wwan->sub_netlist[if_id])->netdev->stats;
- stats.rx_packets++;
- stats.rx_bytes += skb->len;
+ priv = rcu_dereference(ipc_wwan->sub_netlist[if_id]);
+ if (!priv) {
+ ret = -EINVAL;
+ goto unlock;
+ }
+ skb->dev = priv->netdev;
+ stats = &priv->netdev->stats;
+ stats->rx_packets++;
+ stats->rx_bytes += skb->len;
ret = netif_rx(skb);
+ skb = NULL;
+unlock:
rcu_read_unlock();
-
+free:
+ dev_kfree_skb(skb);
return ret;
}
--
2.31.1
From: Johannes Berg <[email protected]>
In order to make rtnetlink ops that can create different
kinds of devices, like what we want to add to the WWAN
framework, the priv_size and setup parameters aren't quite
sufficient. Make this easier to manage by allowing ops to
allocate their own netdev via an @alloc method that gets
both the tb and data.
Signed-off-by: Johannes Berg <[email protected]>
---
drivers/net/bareudp.c | 2 +-
drivers/net/can/vxcan.c | 2 +-
drivers/net/geneve.c | 2 +-
drivers/net/veth.c | 2 +-
drivers/net/vxlan.c | 2 +-
include/net/rtnetlink.h | 10 ++++++++++
net/core/rtnetlink.c | 17 ++++++++++++++---
net/ipv4/ip_gre.c | 2 +-
8 files changed, 30 insertions(+), 9 deletions(-)
diff --git a/drivers/net/bareudp.c b/drivers/net/bareudp.c
index edfad93e7b68..a694f6d8eb21 100644
--- a/drivers/net/bareudp.c
+++ b/drivers/net/bareudp.c
@@ -727,7 +727,7 @@ struct net_device *bareudp_dev_create(struct net *net, const char *name,
memset(tb, 0, sizeof(tb));
dev = rtnl_create_link(net, name, name_assign_type,
- &bareudp_link_ops, tb, NULL);
+ &bareudp_link_ops, tb, NULL, NULL);
if (IS_ERR(dev))
return dev;
diff --git a/drivers/net/can/vxcan.c b/drivers/net/can/vxcan.c
index 8861a7d875e7..f904c78b4c23 100644
--- a/drivers/net/can/vxcan.c
+++ b/drivers/net/can/vxcan.c
@@ -204,7 +204,7 @@ static int vxcan_newlink(struct net *net, struct net_device *dev,
return PTR_ERR(peer_net);
peer = rtnl_create_link(peer_net, ifname, name_assign_type,
- &vxcan_link_ops, tbp, extack);
+ &vxcan_link_ops, tbp, NULL, extack);
if (IS_ERR(peer)) {
put_net(peer_net);
return PTR_ERR(peer);
diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
index 1ab94b5f9bbf..130397110623 100644
--- a/drivers/net/geneve.c
+++ b/drivers/net/geneve.c
@@ -1840,7 +1840,7 @@ struct net_device *geneve_dev_create_fb(struct net *net, const char *name,
memset(tb, 0, sizeof(tb));
dev = rtnl_create_link(net, name, name_assign_type,
- &geneve_link_ops, tb, NULL);
+ &geneve_link_ops, tb, NULL, NULL);
if (IS_ERR(dev))
return dev;
diff --git a/drivers/net/veth.c b/drivers/net/veth.c
index bdb7ce3cb054..788faa8faa98 100644
--- a/drivers/net/veth.c
+++ b/drivers/net/veth.c
@@ -1498,7 +1498,7 @@ static int veth_newlink(struct net *src_net, struct net_device *dev,
return PTR_ERR(net);
peer = rtnl_create_link(net, ifname, name_assign_type,
- &veth_link_ops, tbp, extack);
+ &veth_link_ops, tbp, NULL, extack);
if (IS_ERR(peer)) {
put_net(net);
return PTR_ERR(peer);
diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index 02a14f1b938a..b8a63a8bcb73 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -4483,7 +4483,7 @@ struct net_device *vxlan_dev_create(struct net *net, const char *name,
memset(&tb, 0, sizeof(tb));
dev = rtnl_create_link(net, name, name_assign_type,
- &vxlan_link_ops, tb, NULL);
+ &vxlan_link_ops, tb, NULL, NULL);
if (IS_ERR(dev))
return dev;
diff --git a/include/net/rtnetlink.h b/include/net/rtnetlink.h
index 479f60ef54c0..75426ad5a05b 100644
--- a/include/net/rtnetlink.h
+++ b/include/net/rtnetlink.h
@@ -37,6 +37,9 @@ static inline int rtnl_msg_family(const struct nlmsghdr *nlh)
* @maxtype: Highest device specific netlink attribute number
* @policy: Netlink policy for device specific attribute validation
* @validate: Optional validation function for netlink/changelink parameters
+ * @alloc: netdev allocation function, can be %NULL and is then used
+ * in place of alloc_netdev_mqs(), in this case @priv_size
+ * and @setup are unused. Returns a netdev or ERR_PTR().
* @priv_size: sizeof net_device private space
* @setup: net_device setup function
* @newlink: Function for configuring and registering a new device
@@ -63,6 +66,12 @@ struct rtnl_link_ops {
const char *kind;
size_t priv_size;
+ struct net_device *(*alloc)(struct nlattr *tb[],
+ struct nlattr *data[],
+ const char *ifname,
+ unsigned char name_assign_type,
+ unsigned int num_tx_queues,
+ unsigned int num_rx_queues);
void (*setup)(struct net_device *dev);
bool netns_refund;
@@ -162,6 +171,7 @@ struct net_device *rtnl_create_link(struct net *net, const char *ifname,
unsigned char name_assign_type,
const struct rtnl_link_ops *ops,
struct nlattr *tb[],
+ struct nlattr *data[],
struct netlink_ext_ack *extack);
int rtnl_delete_link(struct net_device *dev);
int rtnl_configure_link(struct net_device *dev, const struct ifinfomsg *ifm);
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 714d5fa38546..9da38639f088 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -3151,6 +3151,7 @@ struct net_device *rtnl_create_link(struct net *net, const char *ifname,
unsigned char name_assign_type,
const struct rtnl_link_ops *ops,
struct nlattr *tb[],
+ struct nlattr *data[],
struct netlink_ext_ack *extack)
{
struct net_device *dev;
@@ -3177,8 +3178,17 @@ struct net_device *rtnl_create_link(struct net *net, const char *ifname,
return ERR_PTR(-EINVAL);
}
- dev = alloc_netdev_mqs(ops->priv_size, ifname, name_assign_type,
- ops->setup, num_tx_queues, num_rx_queues);
+ if (ops->alloc) {
+ dev = ops->alloc(tb, data, ifname, name_assign_type,
+ num_tx_queues, num_rx_queues);
+ if (IS_ERR(dev))
+ return dev;
+ } else {
+ dev = alloc_netdev_mqs(ops->priv_size, ifname,
+ name_assign_type, ops->setup,
+ num_tx_queues, num_rx_queues);
+ }
+
if (!dev)
return ERR_PTR(-ENOMEM);
@@ -3440,7 +3450,8 @@ static int __rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,
}
dev = rtnl_create_link(link_net ? : dest_net, ifname,
- name_assign_type, ops, tb, extack);
+ name_assign_type, ops, tb, data,
+ extack);
if (IS_ERR(dev)) {
err = PTR_ERR(dev);
goto out;
diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
index a68bf4c6fe9b..7680f5b87f61 100644
--- a/net/ipv4/ip_gre.c
+++ b/net/ipv4/ip_gre.c
@@ -1637,7 +1637,7 @@ struct net_device *gretap_fb_dev_create(struct net *net, const char *name,
memset(&tb, 0, sizeof(tb));
dev = rtnl_create_link(net, name, name_assign_type,
- &ipgre_tap_ops, tb, NULL);
+ &ipgre_tap_ops, tb, NULL, NULL);
if (IS_ERR(dev))
return dev;
--
2.31.1
From: Johannes Berg <[email protected]>
Signed-off-by: Johannes Berg <[email protected]>
---
drivers/net/wwan/iosm/iosm_ipc_imem_ops.h | 1 -
drivers/net/wwan/iosm/iosm_ipc_pcie.c | 7 -
drivers/net/wwan/iosm/iosm_ipc_pcie.h | 2 -
drivers/net/wwan/iosm/iosm_ipc_wwan.c | 310 +++++++---------------
include/uapi/linux/if_link.h | 10 -
tools/include/uapi/linux/if_link.h | 10 -
6 files changed, 103 insertions(+), 237 deletions(-)
diff --git a/drivers/net/wwan/iosm/iosm_ipc_imem_ops.h b/drivers/net/wwan/iosm/iosm_ipc_imem_ops.h
index 6677a82be77b..84087cf33329 100644
--- a/drivers/net/wwan/iosm/iosm_ipc_imem_ops.h
+++ b/drivers/net/wwan/iosm/iosm_ipc_imem_ops.h
@@ -29,7 +29,6 @@
/* IP MUX channel range */
#define IP_MUX_SESSION_START 1
#define IP_MUX_SESSION_END 8
-#define MAX_IP_MUX_SESSION IP_MUX_SESSION_END
/**
* ipc_imem_sys_port_open - Open a port link to CP.
diff --git a/drivers/net/wwan/iosm/iosm_ipc_pcie.c b/drivers/net/wwan/iosm/iosm_ipc_pcie.c
index 0c26047ebc1c..ac6baddfde61 100644
--- a/drivers/net/wwan/iosm/iosm_ipc_pcie.c
+++ b/drivers/net/wwan/iosm/iosm_ipc_pcie.c
@@ -567,18 +567,11 @@ static int __init iosm_ipc_driver_init(void)
return -1;
}
- if (rtnl_link_register(&iosm_netlink)) {
- pr_err("IOSM RTNL register failed");
- pci_unregister_driver(&iosm_ipc_driver);
- return -1;
- }
-
return 0;
}
static void __exit iosm_ipc_driver_exit(void)
{
- rtnl_link_unregister(&iosm_netlink);
pci_unregister_driver(&iosm_ipc_driver);
}
diff --git a/drivers/net/wwan/iosm/iosm_ipc_pcie.h b/drivers/net/wwan/iosm/iosm_ipc_pcie.h
index 839809fee3dd..7d1f0cd7364c 100644
--- a/drivers/net/wwan/iosm/iosm_ipc_pcie.h
+++ b/drivers/net/wwan/iosm/iosm_ipc_pcie.h
@@ -12,8 +12,6 @@
#include "iosm_ipc_irq.h"
-extern struct rtnl_link_ops iosm_netlink;
-
/* Device ID */
#define INTEL_CP_DEVICE_7560_ID 0x7560
diff --git a/drivers/net/wwan/iosm/iosm_ipc_wwan.c b/drivers/net/wwan/iosm/iosm_ipc_wwan.c
index 719c88d9b2e9..daf3d36edc4e 100644
--- a/drivers/net/wwan/iosm/iosm_ipc_wwan.c
+++ b/drivers/net/wwan/iosm/iosm_ipc_wwan.c
@@ -6,8 +6,7 @@
#include <linux/etherdevice.h>
#include <linux/if_arp.h>
#include <linux/if_link.h>
-#include <net/rtnetlink.h>
-
+#include <linux/wwan.h>
#include "iosm_ipc_chnl_cfg.h"
#include "iosm_ipc_imem_ops.h"
#include "iosm_ipc_wwan.h"
@@ -18,65 +17,52 @@
#define IOSM_IF_ID_PAYLOAD 2
-static struct device_type wwan_type = { .name = "wwan" };
-
-static const struct nla_policy ipc_wwan_policy[IFLA_IOSM_MAX + 1] = {
- [IFLA_IOSM_IF_ID] = { .type = NLA_U16 },
-};
-
/**
- * struct iosm_net_link - This structure includes information about interface
- * dev.
+ * struct iosm_netdev_priv - netdev private data
* @if_id: Interface id for device.
* @ch_id: IPC channel number for which interface device is created.
- * @netdev: Pointer to network interface device structure
* @ipc_wwan: Pointer to iosm_wwan struct
*/
-
-struct iosm_net_link {
+struct iosm_netdev_priv {
+ struct iosm_wwan *ipc_wwan;
+ struct net_device *netdev;
int if_id;
int ch_id;
- struct net_device *netdev;
- struct iosm_wwan *ipc_wwan;
};
/**
* struct iosm_wwan - This structure contains information about WWAN root device
- * and interface to the IPC layer.
- * @netdev: Pointer to network interface device structure.
- * @sub_netlist: List of netlink interfaces
+ * and interface to the IPC layer.
* @ipc_imem: Pointer to imem data-struct
+ * @sub_netlist: List of active netdevs
* @dev: Pointer device structure
* @if_mutex: Mutex used for add and remove interface id
- * @is_registered: Registration status with netdev
*/
struct iosm_wwan {
- struct net_device *netdev;
- struct iosm_net_link __rcu *sub_netlist[MAX_IP_MUX_SESSION];
struct iosm_imem *ipc_imem;
+ struct iosm_netdev_priv __rcu *sub_netlist[IP_MUX_SESSION_END + 1];
struct device *dev;
struct mutex if_mutex; /* Mutex used for add and remove interface id */
- u8 is_registered:1;
};
/* Bring-up the wwan net link */
static int ipc_wwan_link_open(struct net_device *netdev)
{
- struct iosm_net_link *netlink = netdev_priv(netdev);
- struct iosm_wwan *ipc_wwan = netlink->ipc_wwan;
- int if_id = netlink->if_id;
- int ret = -EINVAL;
+ struct iosm_netdev_priv *priv = netdev_priv(netdev);
+ struct iosm_wwan *ipc_wwan = priv->ipc_wwan;
+ int if_id = priv->if_id;
+ int ret;
- if (if_id < IP_MUX_SESSION_START || if_id > IP_MUX_SESSION_END)
- return ret;
+ if (if_id < IP_MUX_SESSION_START ||
+ if_id >= ARRAY_SIZE(ipc_wwan->sub_netlist))
+ return -EINVAL;
mutex_lock(&ipc_wwan->if_mutex);
/* get channel id */
- netlink->ch_id =
- ipc_imem_sys_wwan_open(ipc_wwan->ipc_imem, if_id);
+ priv->ch_id = ipc_imem_sys_wwan_open(ipc_wwan->ipc_imem, if_id);
- if (netlink->ch_id < 0) {
+ if (priv->ch_id < 0) {
dev_err(ipc_wwan->dev,
"cannot connect wwan0 & id %d to the IPC mem layer",
if_id);
@@ -88,7 +74,7 @@ static int ipc_wwan_link_open(struct net_device *netdev)
netif_start_queue(netdev);
dev_dbg(ipc_wwan->dev, "Channel id %d allocated to if_id %d",
- netlink->ch_id, netlink->if_id);
+ priv->ch_id, priv->if_id);
ret = 0;
out:
@@ -99,14 +85,15 @@ static int ipc_wwan_link_open(struct net_device *netdev)
/* Bring-down the wwan net link */
static int ipc_wwan_link_stop(struct net_device *netdev)
{
- struct iosm_net_link *netlink = netdev_priv(netdev);
+ struct iosm_netdev_priv *priv = netdev_priv(netdev);
netif_stop_queue(netdev);
- mutex_lock(&netlink->ipc_wwan->if_mutex);
- ipc_imem_sys_wwan_close(netlink->ipc_wwan->ipc_imem, netlink->if_id,
- netlink->ch_id);
- mutex_unlock(&netlink->ipc_wwan->if_mutex);
+ mutex_lock(&priv->ipc_wwan->if_mutex);
+ ipc_imem_sys_wwan_close(priv->ipc_wwan->ipc_imem, priv->if_id,
+ priv->ch_id);
+ priv->ch_id = -1;
+ mutex_unlock(&priv->ipc_wwan->if_mutex);
return 0;
}
@@ -115,20 +102,21 @@ static int ipc_wwan_link_stop(struct net_device *netdev)
static int ipc_wwan_link_transmit(struct sk_buff *skb,
struct net_device *netdev)
{
- struct iosm_net_link *netlink = netdev_priv(netdev);
- struct iosm_wwan *ipc_wwan = netlink->ipc_wwan;
- int if_id = netlink->if_id;
- int ret = -EINVAL;
+ struct iosm_netdev_priv *priv = netdev_priv(netdev);
+ struct iosm_wwan *ipc_wwan = priv->ipc_wwan;
+ int if_id = priv->if_id;
+ int ret;
/* Interface IDs from 1 to 8 are for IP data
* & from 257 to 261 are for non-IP data
*/
- if (if_id < IP_MUX_SESSION_START || if_id > IP_MUX_SESSION_END)
- goto exit;
+ if (if_id < IP_MUX_SESSION_START ||
+ if_id >= ARRAY_SIZE(ipc_wwan->sub_netlist))
+ return -EINVAL;
/* Send the SKB to device for transmission */
ret = ipc_imem_sys_wwan_transmit(ipc_wwan->ipc_imem,
- if_id, netlink->ch_id, skb);
+ if_id, priv->ch_id, skb);
/* Return code of zero is success */
if (ret == 0) {
@@ -175,137 +163,72 @@ static void ipc_wwan_setup(struct net_device *iosm_dev)
iosm_dev->netdev_ops = &ipc_inm_ops;
}
-static struct device_type inm_type = { .name = "iosmdev" };
-
/* Create new wwan net link */
-static int ipc_wwan_newlink(struct net *src_net, struct net_device *dev,
- struct nlattr *tb[], struct nlattr *data[],
- struct netlink_ext_ack *extack)
+static int ipc_wwan_newlink(void *ctxt, struct net_device *dev,
+ u32 if_id, struct netlink_ext_ack *extack)
{
- struct iosm_net_link *netlink = netdev_priv(dev);
- struct iosm_wwan *ipc_wwan;
- struct net_device *netdev;
- int err = -EINVAL;
- int index;
-
- if (!data[IFLA_IOSM_IF_ID]) {
- pr_err("Interface ID not specified");
- goto out;
- }
-
- if (!tb[IFLA_LINK]) {
- pr_err("Link not specified");
- goto out;
- }
-
- netlink->netdev = dev;
-
- netdev = __dev_get_by_index(src_net, nla_get_u32(tb[IFLA_LINK]));
-
- netlink->ipc_wwan = netdev_priv(netdev);
-
- ipc_wwan = netlink->ipc_wwan;
+ struct iosm_wwan *ipc_wwan = ctxt;
+ struct iosm_netdev_priv *priv;
+ int err;
- if (ipc_wwan->netdev != netdev)
- goto out;
-
- netlink->if_id = nla_get_u16(data[IFLA_IOSM_IF_ID]);
- index = netlink->if_id;
-
- /* Complete all memory stores before this point */
- smp_mb();
- if (index < IP_MUX_SESSION_START || index > IP_MUX_SESSION_END)
- goto out;
+ if (if_id < IP_MUX_SESSION_START ||
+ if_id >= ARRAY_SIZE(ipc_wwan->sub_netlist))
+ return -EINVAL;
- rcu_read_lock();
+ priv = netdev_priv(dev);
+ priv->if_id = if_id;
- if (rcu_access_pointer(ipc_wwan->sub_netlist[index - 1])) {
- pr_err("IOSM interface ID already in use");
- goto out_free_lock;
+ mutex_lock(&ipc_wwan->if_mutex);
+ if (rcu_access_pointer(ipc_wwan->sub_netlist[if_id])) {
+ err = -EBUSY;
+ goto out_unlock;
}
- SET_NETDEV_DEVTYPE(dev, &inm_type);
-
- eth_hw_addr_random(dev);
err = register_netdevice(dev);
- if (err) {
- dev_err(ipc_wwan->dev, "register netlink failed.\n");
- goto out_free_lock;
- }
-
- err = netdev_upper_dev_link(ipc_wwan->netdev, dev, extack);
+ if (err)
+ goto out_unlock;
- if (err) {
- dev_err(ipc_wwan->dev, "netdev linking with parent failed.\n");
- goto netlink_err;
- }
+ rcu_assign_pointer(ipc_wwan->sub_netlist[if_id], priv);
+ mutex_unlock(&ipc_wwan->if_mutex);
- rcu_assign_pointer(ipc_wwan->sub_netlist[index - 1], netlink);
netif_device_attach(dev);
- rcu_read_unlock();
return 0;
-netlink_err:
- unregister_netdevice(dev);
-out_free_lock:
- rcu_read_unlock();
-out:
+out_unlock:
+ mutex_unlock(&ipc_wwan->if_mutex);
return err;
}
-/* Delete new wwan net link */
-static void ipc_wwan_dellink(struct net_device *dev, struct list_head *head)
+static void ipc_wwan_dellink(void *ctxt, struct net_device *dev,
+ struct list_head *head)
{
- struct iosm_net_link *netlink = netdev_priv(dev);
- u16 index = netlink->if_id;
-
- netdev_upper_dev_unlink(netlink->ipc_wwan->netdev, dev);
- unregister_netdevice(dev);
+ struct iosm_wwan *ipc_wwan = ctxt;
+ struct iosm_netdev_priv *priv = netdev_priv(dev);
+ int if_id = priv->if_id;
- mutex_lock(&netlink->ipc_wwan->if_mutex);
- rcu_assign_pointer(netlink->ipc_wwan->sub_netlist[index - 1], NULL);
- mutex_unlock(&netlink->ipc_wwan->if_mutex);
-}
+ if (WARN_ON(if_id < IP_MUX_SESSION_START ||
+ if_id >= ARRAY_SIZE(ipc_wwan->sub_netlist)))
+ return;
-/* Get size for iosm net link payload*/
-static size_t ipc_wwan_get_size(const struct net_device *dev)
-{
- return nla_total_size(IOSM_IF_ID_PAYLOAD);
-}
-
-/* Validate the input parameters for wwan net link */
-static int ipc_wwan_validate(struct nlattr *tb[], struct nlattr *data[],
- struct netlink_ext_ack *extack)
-{
- u16 if_id;
-
- if (!data || !data[IFLA_IOSM_IF_ID]) {
- NL_SET_ERR_MSG_MOD(extack, "IF ID not specified");
- return -EINVAL;
- }
+ mutex_lock(&ipc_wwan->if_mutex);
- if_id = nla_get_u16(data[IFLA_IOSM_IF_ID]);
+ if (WARN_ON(rcu_access_pointer(ipc_wwan->sub_netlist[if_id]) != priv))
+ goto unlock;
- if (if_id < IP_MUX_SESSION_START || if_id > IP_MUX_SESSION_END) {
- NL_SET_ERR_MSG_MOD(extack, "Invalid Interface");
- return -ERANGE;
- }
+ RCU_INIT_POINTER(ipc_wwan->sub_netlist[if_id], NULL);
+ /* unregistering includes synchronize_net() */
+ unregister_netdevice(dev);
- return 0;
+unlock:
+ mutex_unlock(&ipc_wwan->if_mutex);
}
-/* RT Net link ops structure for new wwan net link */
-struct rtnl_link_ops iosm_netlink __read_mostly = {
- .kind = "iosm",
- .maxtype = __IFLA_IOSM_MAX,
- .priv_size = sizeof(struct iosm_net_link),
- .setup = ipc_wwan_setup,
- .validate = ipc_wwan_validate,
- .newlink = ipc_wwan_newlink,
- .dellink = ipc_wwan_dellink,
- .get_size = ipc_wwan_get_size,
- .policy = ipc_wwan_policy,
+static const struct wwan_ops iosm_wwan_ops = {
+ .priv_size = sizeof(struct iosm_netdev_priv),
+ .setup = ipc_wwan_setup,
+ .newlink = ipc_wwan_newlink,
+ .dellink = ipc_wwan_dellink,
};
int ipc_wwan_receive(struct iosm_wwan *ipc_wwan, struct sk_buff *skb_arg,
@@ -313,7 +236,7 @@ int ipc_wwan_receive(struct iosm_wwan *ipc_wwan, struct sk_buff *skb_arg,
{
struct sk_buff *skb = skb_arg;
struct net_device_stats *stats;
- struct iosm_net_link *priv;
+ struct iosm_netdev_priv *priv;
int ret;
if ((skb->data[0] & IOSM_IP_TYPE_MASK) == IOSM_IP_TYPE_IPV4)
@@ -353,10 +276,17 @@ int ipc_wwan_receive(struct iosm_wwan *ipc_wwan, struct sk_buff *skb_arg,
void ipc_wwan_tx_flowctrl(struct iosm_wwan *ipc_wwan, int if_id, bool on)
{
struct net_device *netdev;
+ struct iosm_netdev_priv *priv;
bool is_tx_blk;
rcu_read_lock();
- netdev = rcu_dereference(ipc_wwan->sub_netlist[if_id])->netdev;
+ priv = rcu_dereference(ipc_wwan->sub_netlist[if_id]);
+ if (!priv) {
+ rcu_read_unlock();
+ return;
+ }
+
+ netdev = priv->netdev;
is_tx_blk = netif_queue_stopped(netdev);
@@ -371,78 +301,44 @@ void ipc_wwan_tx_flowctrl(struct iosm_wwan *ipc_wwan, int if_id, bool on)
rcu_read_unlock();
}
-static void ipc_netdev_setup(struct net_device *dev) {}
-
struct iosm_wwan *ipc_wwan_init(struct iosm_imem *ipc_imem, struct device *dev)
{
- static const struct net_device_ops iosm_wwandev_ops = {};
struct iosm_wwan *ipc_wwan;
- struct net_device *netdev;
- netdev = alloc_netdev(sizeof(*ipc_wwan), "wwan%d", NET_NAME_ENUM,
- ipc_netdev_setup);
-
- if (!netdev)
+ ipc_wwan = kzalloc(sizeof(*ipc_wwan), GFP_KERNEL);
+ if (!ipc_wwan)
return NULL;
- ipc_wwan = netdev_priv(netdev);
-
ipc_wwan->dev = dev;
- ipc_wwan->netdev = netdev;
- ipc_wwan->is_registered = false;
-
ipc_wwan->ipc_imem = ipc_imem;
- mutex_init(&ipc_wwan->if_mutex);
-
- /* allocate random ethernet address */
- eth_random_addr(netdev->dev_addr);
- netdev->addr_assign_type = NET_ADDR_RANDOM;
-
- netdev->netdev_ops = &iosm_wwandev_ops;
- netdev->flags |= IFF_NOARP;
-
- SET_NETDEV_DEVTYPE(netdev, &wwan_type);
-
- if (register_netdev(netdev)) {
- dev_err(ipc_wwan->dev, "register_netdev failed");
- goto reg_fail;
+ if (wwan_register_ops(ipc_wwan->dev, &iosm_wwan_ops, ipc_wwan)) {
+ kfree(ipc_wwan);
+ return NULL;
}
- ipc_wwan->is_registered = true;
-
- netif_device_attach(netdev);
+ mutex_init(&ipc_wwan->if_mutex);
return ipc_wwan;
-
-reg_fail:
- free_netdev(netdev);
- return NULL;
}
void ipc_wwan_deinit(struct iosm_wwan *ipc_wwan)
{
- struct iosm_net_link *netlink;
- int i;
-
- if (ipc_wwan->is_registered) {
- rcu_read_lock();
- for (i = IP_MUX_SESSION_START; i <= IP_MUX_SESSION_END; i++) {
- if (rcu_access_pointer(ipc_wwan->sub_netlist[i - 1])) {
- netlink =
- rcu_dereference(ipc_wwan->sub_netlist[i - 1]);
- rtnl_lock();
- netdev_upper_dev_unlink(ipc_wwan->netdev,
- netlink->netdev);
- unregister_netdevice(netlink->netdev);
- rtnl_unlock();
- rcu_assign_pointer(ipc_wwan->sub_netlist[i - 1],
- NULL);
- }
- }
- rcu_read_unlock();
+ int if_id;
- unregister_netdev(ipc_wwan->netdev);
- free_netdev(ipc_wwan->netdev);
+ wwan_unregister_ops(ipc_wwan->dev);
+
+ for (if_id = 0; if_id < ARRAY_SIZE(ipc_wwan->sub_netlist); if_id++) {
+ struct iosm_netdev_priv *priv;
+
+ priv = rcu_access_pointer(ipc_wwan->sub_netlist[if_id]);
+ if (!priv)
+ continue;
+
+ ipc_wwan_dellink(ipc_wwan, priv->netdev, NULL);
}
+
+ mutex_destroy(&ipc_wwan->if_mutex);
+
+ kfree(ipc_wwan);
}
diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index f7c3beebb074..cd5b382a4138 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -1251,14 +1251,4 @@ struct ifla_rmnet_flags {
__u32 mask;
};
-/* IOSM Section */
-
-enum {
- IFLA_IOSM_UNSPEC,
- IFLA_IOSM_IF_ID,
- __IFLA_IOSM_MAX,
-};
-
-#define IFLA_IOSM_MAX (__IFLA_IOSM_MAX - 1)
-
#endif /* _UAPI_LINUX_IF_LINK_H */
diff --git a/tools/include/uapi/linux/if_link.h b/tools/include/uapi/linux/if_link.h
index cb496d0de39e..d208b2af697f 100644
--- a/tools/include/uapi/linux/if_link.h
+++ b/tools/include/uapi/linux/if_link.h
@@ -1046,14 +1046,4 @@ struct ifla_rmnet_flags {
__u32 mask;
};
-/* IOSM Section */
-
-enum {
- IFLA_IOSM_UNSPEC,
- IFLA_IOSM_IF_ID,
- __IFLA_IOSM_MAX,
-};
-
-#define IFLA_IOSM_MAX (__IFLA_IOSM_MAX - 1)
-
#endif /* _UAPI_LINUX_IF_LINK_H */
--
2.31.1
From: Johannes Berg <[email protected]>
Add support to create (and destroy) interfaces via a new
rtnetlink kind "wwan". The responsible driver has to use
the new wwan_register_ops() to make this possible.
Signed-off-by: Johannes Berg <[email protected]>
---
drivers/net/wwan/wwan_core.c | 219 ++++++++++++++++++++++++++++++++++-
include/linux/wwan.h | 36 ++++++
include/uapi/linux/wwan.h | 17 +++
3 files changed, 267 insertions(+), 5 deletions(-)
create mode 100644 include/uapi/linux/wwan.h
diff --git a/drivers/net/wwan/wwan_core.c b/drivers/net/wwan/wwan_core.c
index cff04e532c1e..b1ad78f386bc 100644
--- a/drivers/net/wwan/wwan_core.c
+++ b/drivers/net/wwan/wwan_core.c
@@ -13,6 +13,8 @@
#include <linux/slab.h>
#include <linux/types.h>
#include <linux/wwan.h>
+#include <net/rtnetlink.h>
+#include <uapi/linux/wwan.h>
#define WWAN_MAX_MINORS 256 /* 256 minors allowed with register_chrdev() */
@@ -524,24 +526,231 @@ static const struct file_operations wwan_port_fops = {
.llseek = noop_llseek,
};
+struct wwan_dev_reg {
+ struct list_head list;
+ struct device *dev;
+ const struct wwan_ops *ops;
+ void *ctxt;
+};
+
+static DEFINE_MUTEX(wwan_mtx);
+static LIST_HEAD(wwan_devs);
+
+int wwan_register_ops(struct device *parent, const struct wwan_ops *ops,
+ void *ctxt)
+{
+ struct wwan_dev_reg *reg;
+ int ret;
+
+ if (WARN_ON(!parent || !ops))
+ return -EINVAL;
+
+ mutex_lock(&wwan_mtx);
+ list_for_each_entry(reg, &wwan_devs, list) {
+ if (WARN_ON(reg->dev == parent)) {
+ ret = -EBUSY;
+ goto out;
+ }
+ }
+
+ reg = kzalloc(sizeof(*reg), GFP_KERNEL);
+ if (!reg) {
+ ret = -ENOMEM;
+ goto out;
+ }
+
+ reg->dev = parent;
+ reg->ops = ops;
+ reg->ctxt = ctxt;
+ list_add_tail(®->list, &wwan_devs);
+
+ ret = 0;
+
+out:
+ mutex_unlock(&wwan_mtx);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(wwan_register_ops);
+
+void wwan_unregister_ops(struct device *parent)
+{
+ struct wwan_dev_reg *tmp;
+
+ mutex_lock(&wwan_mtx);
+ list_for_each_entry(tmp, &wwan_devs, list) {
+ if (tmp->dev == parent) {
+ list_del(&tmp->list);
+ break;
+ }
+ }
+ mutex_unlock(&wwan_mtx);
+}
+EXPORT_SYMBOL_GPL(wwan_unregister_ops);
+
+static struct wwan_dev_reg *wwan_find_by_name(const char *name)
+{
+ struct wwan_dev_reg *tmp;
+
+ lockdep_assert_held(&wwan_mtx);
+
+ list_for_each_entry(tmp, &wwan_devs, list) {
+ if (strcmp(name, dev_name(tmp->dev)) == 0)
+ return tmp;
+ }
+
+ return NULL;
+}
+
+static struct wwan_dev_reg *wwan_find_by_dev(struct device *dev)
+{
+ struct wwan_dev_reg *tmp;
+
+ lockdep_assert_held(&wwan_mtx);
+
+ list_for_each_entry(tmp, &wwan_devs, list) {
+ if (tmp->dev == dev)
+ return tmp;
+ }
+
+ return NULL;
+}
+
+static int wwan_rtnl_validate(struct nlattr *tb[], struct nlattr *data[],
+ struct netlink_ext_ack *extack)
+{
+ if (!data)
+ return -EINVAL;
+
+ if (!data[IFLA_WWAN_LINK_ID] || !data[IFLA_WWAN_DEV_NAME])
+ return -EINVAL;
+
+ return 0;
+}
+
+static struct device_type wwan_type = { .name = "wwan" };
+
+static struct net_device *wwan_rtnl_alloc(struct nlattr *tb[],
+ struct nlattr *data[],
+ const char *ifname,
+ unsigned char name_assign_type,
+ unsigned int num_tx_queues,
+ unsigned int num_rx_queues)
+{
+ const char *devname = nla_data(data[IFLA_WWAN_DEV_NAME]);
+ struct wwan_dev_reg *reg;
+ struct net_device *dev;
+
+ mutex_lock(&wwan_mtx);
+ reg = wwan_find_by_name(devname);
+ if (!reg) {
+ mutex_unlock(&wwan_mtx);
+ return ERR_PTR(-EINVAL);
+ }
+
+ dev = alloc_netdev_mqs(reg->ops->priv_size, ifname, name_assign_type,
+ reg->ops->setup, num_tx_queues, num_rx_queues);
+
+ mutex_unlock(&wwan_mtx);
+
+ if (dev) {
+ SET_NETDEV_DEV(dev, reg->dev);
+ SET_NETDEV_DEVTYPE(dev, &wwan_type);
+ }
+
+ return dev;
+}
+
+static int wwan_rtnl_newlink(struct net *src_net, struct net_device *dev,
+ struct nlattr *tb[], struct nlattr *data[],
+ struct netlink_ext_ack *extack)
+{
+ struct wwan_dev_reg *reg;
+ u32 link_id = nla_get_u32(data[IFLA_WWAN_LINK_ID]);
+ int ret;
+
+ mutex_lock(&wwan_mtx);
+ reg = wwan_find_by_dev(dev->dev.parent);
+ if (!reg) {
+ mutex_unlock(&wwan_mtx);
+ return -EINVAL;
+ }
+
+ if (reg->ops->newlink)
+ ret = reg->ops->newlink(reg->ctxt, dev, link_id, extack);
+ else
+ ret = register_netdevice(dev);
+
+ mutex_unlock(&wwan_mtx);
+
+ return ret;
+}
+
+static void wwan_rtnl_dellink(struct net_device *dev, struct list_head *head)
+{
+ struct wwan_dev_reg *reg;
+
+ mutex_lock(&wwan_mtx);
+ reg = wwan_find_by_dev(dev->dev.parent);
+ if (!reg) {
+ mutex_unlock(&wwan_mtx);
+ return;
+ }
+
+ if (reg->ops->dellink)
+ reg->ops->dellink(reg->ctxt, dev, head);
+ else
+ unregister_netdevice(dev);
+
+ mutex_unlock(&wwan_mtx);
+}
+
+static const struct nla_policy wwan_rtnl_policy[IFLA_WWAN_MAX + 1] = {
+ [IFLA_WWAN_DEV_NAME] = { .type = NLA_NUL_STRING },
+ [IFLA_WWAN_LINK_ID] = { .type = NLA_U32 },
+};
+
+static struct rtnl_link_ops wwan_rtnl_link_ops __read_mostly = {
+ .kind = "wwan",
+ .maxtype = __IFLA_WWAN_MAX,
+ .alloc = wwan_rtnl_alloc,
+ .validate = wwan_rtnl_validate,
+ .newlink = wwan_rtnl_newlink,
+ .dellink = wwan_rtnl_dellink,
+ .policy = wwan_rtnl_policy,
+};
+
static int __init wwan_init(void)
{
+ int err;
+
+ err = rtnl_link_register(&wwan_rtnl_link_ops);
+ if (err)
+ return err;
+
wwan_class = class_create(THIS_MODULE, "wwan");
- if (IS_ERR(wwan_class))
- return PTR_ERR(wwan_class);
+ if (IS_ERR(wwan_class)) {
+ err = PTR_ERR(wwan_class);
+ goto unregister;
+ }
/* chrdev used for wwan ports */
wwan_major = register_chrdev(0, "wwan_port", &wwan_port_fops);
if (wwan_major < 0) {
- class_destroy(wwan_class);
- return wwan_major;
+ err = wwan_major;
+ goto destroy;
}
- return 0;
+ err = 0;
+destroy:
+ class_destroy(wwan_class);
+unregister:
+ rtnl_link_unregister(&wwan_rtnl_link_ops);
+ return err;
}
static void __exit wwan_exit(void)
{
+ rtnl_link_unregister(&wwan_rtnl_link_ops);
unregister_chrdev(wwan_major, "wwan_port");
class_destroy(wwan_class);
}
diff --git a/include/linux/wwan.h b/include/linux/wwan.h
index aa05a253dcf9..d07301962ff7 100644
--- a/include/linux/wwan.h
+++ b/include/linux/wwan.h
@@ -7,6 +7,7 @@
#include <linux/device.h>
#include <linux/kernel.h>
#include <linux/skbuff.h>
+#include <linux/netlink.h>
/**
* enum wwan_port_type - WWAN port types
@@ -108,4 +109,39 @@ void wwan_port_txon(struct wwan_port *port);
*/
void *wwan_port_get_drvdata(struct wwan_port *port);
+/**
+ * struct wwan_ops - WWAN device ops
+ * @priv_size: size of private netdev data area
+ * @setup: set up a new netdev
+ * @newlink: register the new netdev
+ * @dellink: remove the given netdev
+ */
+struct wwan_ops {
+ unsigned int priv_size;
+ void (*setup)(struct net_device *dev);
+ int (*newlink)(void *ctxt, struct net_device *dev,
+ u32 if_id, struct netlink_ext_ack *extack);
+ void (*dellink)(void *ctxt, struct net_device *dev,
+ struct list_head *head);
+};
+
+/**
+ * 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
+ *
+ * Returns: 0 on success, a negative error code on failure
+ */
+int wwan_register_ops(struct device *parent, const struct wwan_ops *ops,
+ void *ctxt);
+
+/**
+ * wwan_unregister_ops - remove WWAN device ops
+ * @parent: Device to use as parent and shared by all WWAN ports and
+ * created netdevs
+ */
+void wwan_unregister_ops(struct device *parent);
+
#endif /* __WWAN_H */
diff --git a/include/uapi/linux/wwan.h b/include/uapi/linux/wwan.h
new file mode 100644
index 000000000000..a134c823cfbd
--- /dev/null
+++ b/include/uapi/linux/wwan.h
@@ -0,0 +1,17 @@
+/* SPDX-License-Identifier: GPL-2.0-only WITH Linux-syscall-note */
+/*
+ * Copyright (C) 2021 Intel Corporation.
+ */
+#ifndef _UAPI_WWAN_H_
+#define _UAPI_WWAN_H_
+
+enum {
+ IFLA_WWAN_UNSPEC,
+ IFLA_WWAN_DEV_NAME, /* nul-string */
+ IFLA_WWAN_LINK_ID, /* u32 */
+
+ __IFLA_WWAN_MAX
+};
+#define IFLA_WWAN_MAX (__IFLA_WWAN_MAX - 1)
+
+#endif /* _UAPI_WWAN_H_ */
--
2.31.1
Hi Johannes,
On Tue, 1 Jun 2021 at 10:05, Johannes Berg <[email protected]> wrote:
>
> From: Johannes Berg <[email protected]>
>
> Add support to create (and destroy) interfaces via a new
> rtnetlink kind "wwan". The responsible driver has to use
> the new wwan_register_ops() to make this possible.
>
> Signed-off-by: Johannes Berg <[email protected]>
> ---
> drivers/net/wwan/wwan_core.c | 219 ++++++++++++++++++++++++++++++++++-
> include/linux/wwan.h | 36 ++++++
> include/uapi/linux/wwan.h | 17 +++
> 3 files changed, 267 insertions(+), 5 deletions(-)
> create mode 100644 include/uapi/linux/wwan.h
>
> diff --git a/drivers/net/wwan/wwan_core.c b/drivers/net/wwan/wwan_core.c
> index cff04e532c1e..b1ad78f386bc 100644
> --- a/drivers/net/wwan/wwan_core.c
> +++ b/drivers/net/wwan/wwan_core.c
> @@ -13,6 +13,8 @@
> #include <linux/slab.h>
> #include <linux/types.h>
> #include <linux/wwan.h>
> +#include <net/rtnetlink.h>
> +#include <uapi/linux/wwan.h>
>
> #define WWAN_MAX_MINORS 256 /* 256 minors allowed with register_chrdev() */
>
> @@ -524,24 +526,231 @@ static const struct file_operations wwan_port_fops = {
> .llseek = noop_llseek,
> };
>
> +struct wwan_dev_reg {
> + struct list_head list;
> + struct device *dev;
> + const struct wwan_ops *ops;
> + void *ctxt;
> +};
> +
> +static DEFINE_MUTEX(wwan_mtx);
> +static LIST_HEAD(wwan_devs);
> +
> +int wwan_register_ops(struct device *parent, const struct wwan_ops *ops,
> + void *ctxt)
> +{
> + struct wwan_dev_reg *reg;
> + int ret;
> +
> + if (WARN_ON(!parent || !ops))
> + return -EINVAL;
> +
> + mutex_lock(&wwan_mtx);
> + list_for_each_entry(reg, &wwan_devs, list) {
> + if (WARN_ON(reg->dev == parent)) {
> + ret = -EBUSY;
> + goto out;
> + }
> + }
Thanks for this, overall it looks good to me, but just checking why
you're not using the wwan_dev internally to create-or-pick wwan_dev
(wwan_dev_create) and register ops to it, instead of having a global
new wwan_devs list.
> +
> + reg = kzalloc(sizeof(*reg), GFP_KERNEL);
> + if (!reg) {
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> + reg->dev = parent;
> + reg->ops = ops;
> + reg->ctxt = ctxt;
> + list_add_tail(®->list, &wwan_devs);
> +
> + ret = 0;
> +
> +out:
> + mutex_unlock(&wwan_mtx);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(wwan_register_ops);
Regards,
Loic
Hi,
> > +int wwan_register_ops(struct device *parent, const struct wwan_ops *ops,
> > + void *ctxt)
> > +{
> > + struct wwan_dev_reg *reg;
> > + int ret;
> > +
> > + if (WARN_ON(!parent || !ops))
> > + return -EINVAL;
> > +
> > + mutex_lock(&wwan_mtx);
> > + list_for_each_entry(reg, &wwan_devs, list) {
> > + if (WARN_ON(reg->dev == parent)) {
> > + ret = -EBUSY;
> > + goto out;
> > + }
> > + }
>
> Thanks for this, overall it looks good to me, but just checking why
> you're not using the wwan_dev internally to create-or-pick wwan_dev
> (wwan_dev_create) and register ops to it, instead of having a global
> new wwan_devs list.
Uh, no good reason. I just missed that all that infrastructure is
already there, oops.
johannes
Hello Johannes and Loic,
On Tue, Jun 1, 2021 at 11:07 AM Johannes Berg <[email protected]> wrote:
> Add support to create (and destroy) interfaces via a new
> rtnetlink kind "wwan". The responsible driver has to use
> the new wwan_register_ops() to make this possible.
Wow, this is a perfect solution! I just could not help but praise this
proposal :)
When researching the latest WWAN device drivers and related
discussions, I began to assume that implementing the netdev management
API without the dummy (no-op) netdev is only possible using genetlink.
But this usage of a regular device specified by its name as a parent
for netdev creation is so natural and clear that I believe in RTNL
again.
Let me place my 2 cents. Maybe the parent device attribute should be
made generic? E.g. call it IFLA_PARENT_DEV_NAME, with usage semantics
similar to the IFLA_LINK attribute for VLAN interfaces. The case when
a user needs to create a netdev on behalf of a regular device is not
WWAN specific, IMHO. So, other drivers could benefit from such
attribute too. To be honest, I can not recall any driver that could
immediately start using such attribute, but the situation with the
need for such attribute seems to be quite common.
--
Sergey
On Tue, 1 Jun 2021 at 12:35, Johannes Berg <[email protected]> wrote:
>
> Hi,
>
> > > +int wwan_register_ops(struct device *parent, const struct wwan_ops *ops,
> > > + void *ctxt)
> > > +{
> > > + struct wwan_dev_reg *reg;
> > > + int ret;
> > > +
> > > + if (WARN_ON(!parent || !ops))
> > > + return -EINVAL;
> > > +
> > > + mutex_lock(&wwan_mtx);
> > > + list_for_each_entry(reg, &wwan_devs, list) {
> > > + if (WARN_ON(reg->dev == parent)) {
> > > + ret = -EBUSY;
> > > + goto out;
> > > + }
> > > + }
> >
> > Thanks for this, overall it looks good to me, but just checking why
> > you're not using the wwan_dev internally to create-or-pick wwan_dev
> > (wwan_dev_create) and register ops to it, instead of having a global
> > new wwan_devs list.
>
> Uh, no good reason. I just missed that all that infrastructure is
> already there, oops.
OK no prob ;-), are you going to resubmit something or do you want I
take care of this?
Regards,
Loic
Hi Sergey,
> Wow, this is a perfect solution! I just could not help but praise this
> proposal :)
Heh.
> When researching the latest WWAN device drivers and related
> discussions, I began to assume that implementing the netdev management
> API without the dummy (no-op) netdev is only possible using genetlink.
> But this usage of a regular device specified by its name as a parent
> for netdev creation is so natural and clear that I believe in RTNL
> again.
>
> Let me place my 2 cents. Maybe the parent device attribute should be
> made generic? E.g. call it IFLA_PARENT_DEV_NAME, with usage semantics
> similar to the IFLA_LINK attribute for VLAN interfaces. The case when
> a user needs to create a netdev on behalf of a regular device is not
> WWAN specific, IMHO. So, other drivers could benefit from such
> attribute too. To be honest, I can not recall any driver that could
> immediately start using such attribute, but the situation with the
> need for such attribute seems to be quite common.
That's a good question/thought.
I mean, in principle this is trivial, right? Just add a
IFLA_PARENT_DEV_NAME like you say, and use it instead of
IFLA_WWAN_DEV_NAME.
It'd come out of tb[] instead of data[] and in this case would remove
the need to add the additional data[] argument to rtnl_create_link() in
my patch, since it's in tb[] then.
The only thing I'd be worried about is that different implementations
use it for different meanings, but I guess that's not that big a deal?
johannes
On Wed, 2021-06-02 at 08:52 +0200, Loic Poulain wrote:
>
> OK no prob ;-), are you going to resubmit something or do you want I
> take care of this?
I just respun a v2, but I'm still not able to test any of this (I'm in a
completely different group than Chetan, just have been helping/advising
them, so I don't even have their HW).
So if you want to take over at some point and are able to test it, I'd
much appreciate it.
johannes
Hello Johannes,
On Wed, Jun 2, 2021 at 10:38 AM Johannes Berg <[email protected]> wrote:
>> Wow, this is a perfect solution! I just could not help but praise this
>> proposal :)
>
> Heh.
>
>> When researching the latest WWAN device drivers and related
>> discussions, I began to assume that implementing the netdev management
>> API without the dummy (no-op) netdev is only possible using genetlink.
>> But this usage of a regular device specified by its name as a parent
>> for netdev creation is so natural and clear that I believe in RTNL
>> again.
>>
>> Let me place my 2 cents. Maybe the parent device attribute should be
>> made generic? E.g. call it IFLA_PARENT_DEV_NAME, with usage semantics
>> similar to the IFLA_LINK attribute for VLAN interfaces. The case when
>> a user needs to create a netdev on behalf of a regular device is not
>> WWAN specific, IMHO. So, other drivers could benefit from such
>> attribute too. To be honest, I can not recall any driver that could
>> immediately start using such attribute, but the situation with the
>> need for such attribute seems to be quite common.
>
> That's a good question/thought.
>
> I mean, in principle this is trivial, right? Just add a
> IFLA_PARENT_DEV_NAME like you say, and use it instead of
> IFLA_WWAN_DEV_NAME.
>
> It'd come out of tb[] instead of data[] and in this case would remove
> the need to add the additional data[] argument to rtnl_create_link() in
> my patch, since it's in tb[] then.
Yep, exactly.
> The only thing I'd be worried about is that different implementations
> use it for different meanings, but I guess that's not that big a deal?
The spectrum of sane use of the IFLA_PARENT_DEV_NAME attribute by
various subsystems and (or) drivers will be quite narrow. It should do
exactly what its name says - identify a parent device.
We can not handle the attribute in the common rtnetlink code since
rtnetlink does not know the HW configuration details. That is why
IFLA_PARENT_DEV_NAME should be handled by the RTNL ->newlink()
callback. But after all the processing, the device that is identified
by the IFLA_PARENT_DEV_NAME attribute should appear in the
netdev->dev.parent field with help of SET_NETDEV_DEV(). Eventually
RTNL will be able to fill IFLA_PARENT_DEV_NAME during the netdevs dump
on its own, taking data from netdev->dev.parent.
I assume that IFLA_PARENT_DEV_NAME could replace the IFLA_LINK
attribute usage in such drivers as MBIM and RMNET. But the best way to
evolve these drivers is to make them WWAN-subsystem-aware using the
WWAN interface configuration API from your proposal, IMHO.
--
Sergey
Hi Sergey,
> > The only thing I'd be worried about is that different implementations
> > use it for different meanings, but I guess that's not that big a deal?
>
> The spectrum of sane use of the IFLA_PARENT_DEV_NAME attribute by
> various subsystems and (or) drivers will be quite narrow. It should do
> exactly what its name says - identify a parent device.
Sure, I was more worried there could be multiple interpretations as to
what "a parent device" is, since userspace does nothing but pass a
string in. But we can say it should be a 'struct device' in the kernel.
> We can not handle the attribute in the common rtnetlink code since
> rtnetlink does not know the HW configuration details. That is why
> IFLA_PARENT_DEV_NAME should be handled by the RTNL ->newlink()
> callback. But after all the processing, the device that is identified
> by the IFLA_PARENT_DEV_NAME attribute should appear in the
> netdev->dev.parent field with help of SET_NETDEV_DEV(). Eventually
> RTNL will be able to fill IFLA_PARENT_DEV_NAME during the netdevs dump
> on its own, taking data from netdev->dev.parent.
I didn't do that second part, but I guess that makes sense.
Want to send a follow-up patch to my other patch? I guess you should've
gotten it, but if not the new series is here:
https://lore.kernel.org/netdev/[email protected]/T/#t
> I assume that IFLA_PARENT_DEV_NAME could replace the IFLA_LINK
> attribute usage in such drivers as MBIM and RMNET. But the best way to
> evolve these drivers is to make them WWAN-subsystem-aware using the
> WWAN interface configuration API from your proposal, IMHO.
Right.
johannes
On Wed, Jun 2, 2021 at 3:56 PM Johannes Berg <[email protected]> wrote:
>>> The only thing I'd be worried about is that different implementations
>>> use it for different meanings, but I guess that's not that big a deal?
>>
>> The spectrum of sane use of the IFLA_PARENT_DEV_NAME attribute by
>> various subsystems and (or) drivers will be quite narrow. It should do
>> exactly what its name says - identify a parent device.
>
> Sure, I was more worried there could be multiple interpretations as to
> what "a parent device" is, since userspace does nothing but pass a
> string in. But we can say it should be a 'struct device' in the kernel.
>
>> We can not handle the attribute in the common rtnetlink code since
>> rtnetlink does not know the HW configuration details. That is why
>> IFLA_PARENT_DEV_NAME should be handled by the RTNL ->newlink()
>> callback. But after all the processing, the device that is identified
>> by the IFLA_PARENT_DEV_NAME attribute should appear in the
>> netdev->dev.parent field with help of SET_NETDEV_DEV(). Eventually
>> RTNL will be able to fill IFLA_PARENT_DEV_NAME during the netdevs dump
>> on its own, taking data from netdev->dev.parent.
>
> I didn't do that second part, but I guess that makes sense.
>
> Want to send a follow-up patch to my other patch? I guess you should've
> gotten it, but if not the new series is here:
>
> https://lore.kernel.org/netdev/[email protected]/T/#t
Yes, I saw the second version of your RFC and even attempted to
provide a full picture of why this attribute should be generic.
I will send a follow-up series tonight with parent device exporting
support and with some usage examples.
>> I assume that IFLA_PARENT_DEV_NAME could replace the IFLA_LINK
>> attribute usage in such drivers as MBIM and RMNET. But the best way to
>> evolve these drivers is to make them WWAN-subsystem-aware using the
>> WWAN interface configuration API from your proposal, IMHO.
>
> Right.
--
Sergey
On Wed, 2 Jun 2021 at 10:29, Johannes Berg <[email protected]> wrote:
>
> On Wed, 2021-06-02 at 08:52 +0200, Loic Poulain wrote:
> >
> > OK no prob ;-), are you going to resubmit something or do you want I
> > take care of this?
>
> I just respun a v2, but I'm still not able to test any of this (I'm in a
> completely different group than Chetan, just have been helping/advising
> them, so I don't even have their HW).
>
> So if you want to take over at some point and are able to test it, I'd
> much appreciate it.
Thanks for this work, yes I can try testing this with mhi_net.
Chetan, would you be able to test that as well? basically with the two
kernel series (Johannes, Sergey) applied on top of your IOSM one + the
iproute2 changes for the userspace side (Sergey), that should work,
but let us know if any issues.
Regards,
Loic
Hi Loic,
> > > OK no prob ;-), are you going to resubmit something or do you want I
> > > take care of this?
> >
> > I just respun a v2, but I'm still not able to test any of this (I'm in
> > a completely different group than Chetan, just have been
> > helping/advising them, so I don't even have their HW).
> >
> > So if you want to take over at some point and are able to test it, I'd
> > much appreciate it.
>
> Thanks for this work, yes I can try testing this with mhi_net.
>
> Chetan, would you be able to test that as well? basically with the two kernel
> series (Johannes, Sergey) applied on top of your IOSM one + the
> iproute2 changes for the userspace side (Sergey), that should work, but let us
> know if any issues.
Sure. I will test these changes with the hardware and share my observation.
Regards,
Chetan