2021-06-01 08:06:17

by Johannes Berg

[permalink] [raw]
Subject: [RFC 0/4] wwan framework netdev creation

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



2021-06-01 08:06:19

by Johannes Berg

[permalink] [raw]
Subject: [RFC 1/4] iosm: fix stats and RCU bugs in RX

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

2021-06-01 08:06:24

by Johannes Berg

[permalink] [raw]
Subject: [RFC 2/4] rtnetlink: add alloc() method to rtnl_link_ops

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

2021-06-01 08:06:27

by Johannes Berg

[permalink] [raw]
Subject: [RFC 4/4] iosm: convert to generic wwan ops

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

2021-06-01 08:08:46

by Johannes Berg

[permalink] [raw]
Subject: [RFC 3/4] wwan: add interface creation support

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(&reg->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

2021-06-01 09:29:15

by Loic Poulain

[permalink] [raw]
Subject: Re: [RFC 3/4] wwan: add interface creation support

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(&reg->list, &wwan_devs);
> +
> + ret = 0;
> +
> +out:
> + mutex_unlock(&wwan_mtx);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(wwan_register_ops);

Regards,
Loic

2021-06-01 10:36:18

by Johannes Berg

[permalink] [raw]
Subject: Re: [RFC 3/4] wwan: add interface creation support

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


2021-06-02 02:14:16

by Sergey Ryazanov

[permalink] [raw]
Subject: Re: [RFC 3/4] wwan: add interface creation support

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

2021-06-02 06:50:10

by Loic Poulain

[permalink] [raw]
Subject: Re: [RFC 3/4] wwan: add interface creation support

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

2021-06-02 08:59:31

by Johannes Berg

[permalink] [raw]
Subject: Re: [RFC 3/4] wwan: add interface creation support

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

2021-06-02 09:45:31

by Johannes Berg

[permalink] [raw]
Subject: Re: [RFC 3/4] wwan: add interface creation support

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

2021-06-02 12:49:17

by Sergey Ryazanov

[permalink] [raw]
Subject: Re: [RFC 3/4] wwan: add interface creation support

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

2021-06-02 12:59:30

by Johannes Berg

[permalink] [raw]
Subject: Re: [RFC 3/4] wwan: add interface creation support

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

2021-06-02 15:41:36

by Sergey Ryazanov

[permalink] [raw]
Subject: Re: [RFC 3/4] wwan: add interface creation support

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

2021-06-03 06:53:33

by Loic Poulain

[permalink] [raw]
Subject: Re: [RFC 3/4] wwan: add interface creation support

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

2021-06-03 07:02:55

by M Chetan Kumar

[permalink] [raw]
Subject: RE: [RFC 3/4] wwan: add interface creation support

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