[PATCH net-next 1/3] net: mhi: Add RX/TX fixup callbacks

[PATCH net-next 1/3] net: mhi: Add RX/TX fixup callbacks

[Loic Poulain]

MHI can transport different protocols, some are handled at upper level,
like IP and QMAP(rmnet/netlink), but others will need to be inside MHI
net driver, like mbim. This change adds support for protocol rx/tx
fixup callbacks registration, that can be used to encode/decode the
targeted protocol.

Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
---
 drivers/net/mhi_net.c | 70 ++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 58 insertions(+), 12 deletions(-)

diff --git a/drivers/net/mhi_net.c b/drivers/net/mhi_net.c
index a5a214d..aa3a5e0 100644
--- a/drivers/net/mhi_net.c
+++ b/drivers/net/mhi_net.c
@@ -33,11 +33,24 @@ struct mhi_net_stats {
 struct mhi_net_dev {
 	struct mhi_device *mdev;
 	struct net_device *ndev;
+	const struct mhi_net_proto *proto;
+	void *proto_data;
 	struct delayed_work rx_refill;
 	struct mhi_net_stats stats;
 	u32 rx_queue_sz;
 };
 
+struct mhi_net_proto {
+	int (*init)(struct mhi_net_dev *dev);
+	struct sk_buff * (*tx_fixup)(struct net_device *ndev, struct sk_buff *skb);
+	int (*rx_fixup)(struct net_device *ndev, struct sk_buff *skb);
+};
+
+struct mhi_device_info {
+	const char *netname;
+	const struct mhi_net_proto *proto;
+};
+
 static int mhi_ndo_open(struct net_device *ndev)
 {
 	struct mhi_net_dev *mhi_netdev = netdev_priv(ndev);
@@ -67,26 +80,35 @@ static int mhi_ndo_stop(struct net_device *ndev)
 static int mhi_ndo_xmit(struct sk_buff *skb, struct net_device *ndev)
 {
 	struct mhi_net_dev *mhi_netdev = netdev_priv(ndev);
+	const struct mhi_net_proto *proto = mhi_netdev->proto;
 	struct mhi_device *mdev = mhi_netdev->mdev;
 	int err;
 
+	if (proto && proto->tx_fixup) {
+		skb = proto->tx_fixup(mhi_netdev->ndev, skb);
+		if (unlikely(!skb))
+			goto exit_drop;
+	}
+
 	err = mhi_queue_skb(mdev, DMA_TO_DEVICE, skb, skb->len, MHI_EOT);
 	if (unlikely(err)) {
 		net_err_ratelimited("%s: Failed to queue TX buf (%d)\n",
 				    ndev->name, err);
-
-		u64_stats_update_begin(&mhi_netdev->stats.tx_syncp);
-		u64_stats_inc(&mhi_netdev->stats.tx_dropped);
-		u64_stats_update_end(&mhi_netdev->stats.tx_syncp);
-
-		/* drop the packet */
 		dev_kfree_skb_any(skb);
+		goto exit_drop;
 	}
 
 	if (mhi_queue_is_full(mdev, DMA_TO_DEVICE))
 		netif_stop_queue(ndev);
 
 	return NETDEV_TX_OK;
+
+exit_drop:
+	u64_stats_update_begin(&mhi_netdev->stats.tx_syncp);
+	u64_stats_inc(&mhi_netdev->stats.tx_dropped);
+	u64_stats_update_end(&mhi_netdev->stats.tx_syncp);
+
+	return NETDEV_TX_OK;
 }
 
 static void mhi_ndo_get_stats64(struct net_device *ndev,
@@ -137,6 +159,7 @@ static void mhi_net_dl_callback(struct mhi_device *mhi_dev,
 				struct mhi_result *mhi_res)
 {
 	struct mhi_net_dev *mhi_netdev = dev_get_drvdata(&mhi_dev->dev);
+	const struct mhi_net_proto *proto = mhi_netdev->proto;
 	struct sk_buff *skb = mhi_res->buf_addr;
 	int remaining;
 
@@ -171,7 +194,11 @@ static void mhi_net_dl_callback(struct mhi_device *mhi_dev,
 		}
 
 		skb_put(skb, mhi_res->bytes_xferd);
-		netif_rx(skb);
+
+		if (proto && proto->rx_fixup)
+			proto->rx_fixup(mhi_netdev->ndev, skb);
+		else
+			netif_rx(skb);
 	}
 
 	/* Refill if RX buffers queue becomes low */
@@ -255,14 +282,14 @@ static struct device_type wwan_type = {
 static int mhi_net_probe(struct mhi_device *mhi_dev,
 			 const struct mhi_device_id *id)
 {
-	const char *netname = (char *)id->driver_data;
+	const struct mhi_device_info *info = (struct mhi_device_info *)id->driver_data;
 	struct device *dev = &mhi_dev->dev;
 	struct mhi_net_dev *mhi_netdev;
 	struct net_device *ndev;
 	int err;
 
-	ndev = alloc_netdev(sizeof(*mhi_netdev), netname, NET_NAME_PREDICTABLE,
-			    mhi_net_setup);
+	ndev = alloc_netdev(sizeof(*mhi_netdev), info->netname,
+			    NET_NAME_PREDICTABLE, mhi_net_setup);
 	if (!ndev)
 		return -ENOMEM;
 
@@ -270,6 +297,7 @@ static int mhi_net_probe(struct mhi_device *mhi_dev,
 	dev_set_drvdata(dev, mhi_netdev);
 	mhi_netdev->ndev = ndev;
 	mhi_netdev->mdev = mhi_dev;
+	mhi_netdev->proto = info->proto;
 	SET_NETDEV_DEV(ndev, &mhi_dev->dev);
 	SET_NETDEV_DEVTYPE(ndev, &wwan_type);
 
@@ -289,8 +317,16 @@ static int mhi_net_probe(struct mhi_device *mhi_dev,
 	if (err)
 		goto out_err;
 
+	if (mhi_netdev->proto) {
+		err = mhi_netdev->proto->init(mhi_netdev);
+		if (err)
+			goto out_err_proto;
+	}
+
 	return 0;
 
+out_err_proto:
+	unregister_netdev(ndev);
 out_err:
 	free_netdev(ndev);
 	return err;
@@ -307,9 +343,19 @@ static void mhi_net_remove(struct mhi_device *mhi_dev)
 	free_netdev(mhi_netdev->ndev);
 }
 
+static const struct mhi_device_info mhi_hwip0 = {
+	.netname = "mhi_hwip%d",
+};
+
+static const struct mhi_device_info mhi_swip0 = {
+	.netname = "mhi_swip%d",
+};
+
 static const struct mhi_device_id mhi_net_id_table[] = {
-	{ .chan = "IP_HW0", .driver_data = (kernel_ulong_t)"mhi_hwip%d" },
-	{ .chan = "IP_SW0", .driver_data = (kernel_ulong_t)"mhi_swip%d" },
+	/* Hardware accelerated data PATH (to modem IPA), protocol agnostic */
+	{ .chan = "IP_HW0", .driver_data = (kernel_ulong_t)&mhi_hwip0 },
+	/* Software data PATH (to modem CPU) */
+	{ .chan = "IP_SW0", .driver_data = (kernel_ulong_t)&mhi_swip0 },
 	{}
 };
 MODULE_DEVICE_TABLE(mhi, mhi_net_id_table);
-- 
2.7.4

[PATCH net-next 2/3] net: mhi: Add dedicated folder

[Loic Poulain]

Create a dedicated mhi directory for mhi-net, mhi-net is going to
be split into differente files (for additional protocol support).

Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
---
 drivers/net/Makefile     |   2 +-
 drivers/net/mhi/Makefile |   3 +
 drivers/net/mhi/net.c    | 379 +++++++++++++++++++++++++++++++++++++++++++++++
 drivers/net/mhi_net.c    | 379 -----------------------------------------------
 4 files changed, 383 insertions(+), 380 deletions(-)
 create mode 100644 drivers/net/mhi/Makefile
 create mode 100644 drivers/net/mhi/net.c
 delete mode 100644 drivers/net/mhi_net.c

diff --git a/drivers/net/Makefile b/drivers/net/Makefile
index 36e2e41..f4990ff 100644
--- a/drivers/net/Makefile
+++ b/drivers/net/Makefile
@@ -36,7 +36,7 @@ obj-$(CONFIG_GTP) += gtp.o
 obj-$(CONFIG_NLMON) += nlmon.o
 obj-$(CONFIG_NET_VRF) += vrf.o
 obj-$(CONFIG_VSOCKMON) += vsockmon.o
-obj-$(CONFIG_MHI_NET) += mhi_net.o
+obj-$(CONFIG_MHI_NET) += mhi/
 
 #
 # Networking Drivers
diff --git a/drivers/net/mhi/Makefile b/drivers/net/mhi/Makefile
new file mode 100644
index 0000000..0acf989
--- /dev/null
+++ b/drivers/net/mhi/Makefile
@@ -0,0 +1,3 @@
+obj-$(CONFIG_MHI_NET) += mhi_net.o
+
+mhi_net-y := net.o
diff --git a/drivers/net/mhi/net.c b/drivers/net/mhi/net.c
new file mode 100644
index 0000000..aa3a5e0
--- /dev/null
+++ b/drivers/net/mhi/net.c
@@ -0,0 +1,379 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/* MHI Network driver - Network over MHI bus
+ *
+ * Copyright (C) 2020 Linaro Ltd <loic.poulain@linaro.org>
+ */
+
+#include <linux/if_arp.h>
+#include <linux/mhi.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/netdevice.h>
+#include <linux/skbuff.h>
+#include <linux/u64_stats_sync.h>
+
+#define MHI_NET_MIN_MTU		ETH_MIN_MTU
+#define MHI_NET_MAX_MTU		0xffff
+#define MHI_NET_DEFAULT_MTU	0x4000
+
+struct mhi_net_stats {
+	u64_stats_t rx_packets;
+	u64_stats_t rx_bytes;
+	u64_stats_t rx_errors;
+	u64_stats_t rx_dropped;
+	u64_stats_t tx_packets;
+	u64_stats_t tx_bytes;
+	u64_stats_t tx_errors;
+	u64_stats_t tx_dropped;
+	atomic_t rx_queued;
+	struct u64_stats_sync tx_syncp;
+	struct u64_stats_sync rx_syncp;
+};
+
+struct mhi_net_dev {
+	struct mhi_device *mdev;
+	struct net_device *ndev;
+	const struct mhi_net_proto *proto;
+	void *proto_data;
+	struct delayed_work rx_refill;
+	struct mhi_net_stats stats;
+	u32 rx_queue_sz;
+};
+
+struct mhi_net_proto {
+	int (*init)(struct mhi_net_dev *dev);
+	struct sk_buff * (*tx_fixup)(struct net_device *ndev, struct sk_buff *skb);
+	int (*rx_fixup)(struct net_device *ndev, struct sk_buff *skb);
+};
+
+struct mhi_device_info {
+	const char *netname;
+	const struct mhi_net_proto *proto;
+};
+
+static int mhi_ndo_open(struct net_device *ndev)
+{
+	struct mhi_net_dev *mhi_netdev = netdev_priv(ndev);
+
+	/* Feed the rx buffer pool */
+	schedule_delayed_work(&mhi_netdev->rx_refill, 0);
+
+	/* Carrier is established via out-of-band channel (e.g. qmi) */
+	netif_carrier_on(ndev);
+
+	netif_start_queue(ndev);
+
+	return 0;
+}
+
+static int mhi_ndo_stop(struct net_device *ndev)
+{
+	struct mhi_net_dev *mhi_netdev = netdev_priv(ndev);
+
+	netif_stop_queue(ndev);
+	netif_carrier_off(ndev);
+	cancel_delayed_work_sync(&mhi_netdev->rx_refill);
+
+	return 0;
+}
+
+static int mhi_ndo_xmit(struct sk_buff *skb, struct net_device *ndev)
+{
+	struct mhi_net_dev *mhi_netdev = netdev_priv(ndev);
+	const struct mhi_net_proto *proto = mhi_netdev->proto;
+	struct mhi_device *mdev = mhi_netdev->mdev;
+	int err;
+
+	if (proto && proto->tx_fixup) {
+		skb = proto->tx_fixup(mhi_netdev->ndev, skb);
+		if (unlikely(!skb))
+			goto exit_drop;
+	}
+
+	err = mhi_queue_skb(mdev, DMA_TO_DEVICE, skb, skb->len, MHI_EOT);
+	if (unlikely(err)) {
+		net_err_ratelimited("%s: Failed to queue TX buf (%d)\n",
+				    ndev->name, err);
+		dev_kfree_skb_any(skb);
+		goto exit_drop;
+	}
+
+	if (mhi_queue_is_full(mdev, DMA_TO_DEVICE))
+		netif_stop_queue(ndev);
+
+	return NETDEV_TX_OK;
+
+exit_drop:
+	u64_stats_update_begin(&mhi_netdev->stats.tx_syncp);
+	u64_stats_inc(&mhi_netdev->stats.tx_dropped);
+	u64_stats_update_end(&mhi_netdev->stats.tx_syncp);
+
+	return NETDEV_TX_OK;
+}
+
+static void mhi_ndo_get_stats64(struct net_device *ndev,
+				struct rtnl_link_stats64 *stats)
+{
+	struct mhi_net_dev *mhi_netdev = netdev_priv(ndev);
+	unsigned int start;
+
+	do {
+		start = u64_stats_fetch_begin_irq(&mhi_netdev->stats.rx_syncp);
+		stats->rx_packets = u64_stats_read(&mhi_netdev->stats.rx_packets);
+		stats->rx_bytes = u64_stats_read(&mhi_netdev->stats.rx_bytes);
+		stats->rx_errors = u64_stats_read(&mhi_netdev->stats.rx_errors);
+		stats->rx_dropped = u64_stats_read(&mhi_netdev->stats.rx_dropped);
+	} while (u64_stats_fetch_retry_irq(&mhi_netdev->stats.rx_syncp, start));
+
+	do {
+		start = u64_stats_fetch_begin_irq(&mhi_netdev->stats.tx_syncp);
+		stats->tx_packets = u64_stats_read(&mhi_netdev->stats.tx_packets);
+		stats->tx_bytes = u64_stats_read(&mhi_netdev->stats.tx_bytes);
+		stats->tx_errors = u64_stats_read(&mhi_netdev->stats.tx_errors);
+		stats->tx_dropped = u64_stats_read(&mhi_netdev->stats.tx_dropped);
+	} while (u64_stats_fetch_retry_irq(&mhi_netdev->stats.tx_syncp, start));
+}
+
+static const struct net_device_ops mhi_netdev_ops = {
+	.ndo_open               = mhi_ndo_open,
+	.ndo_stop               = mhi_ndo_stop,
+	.ndo_start_xmit         = mhi_ndo_xmit,
+	.ndo_get_stats64	= mhi_ndo_get_stats64,
+};
+
+static void mhi_net_setup(struct net_device *ndev)
+{
+	ndev->header_ops = NULL;  /* No header */
+	ndev->type = ARPHRD_RAWIP;
+	ndev->hard_header_len = 0;
+	ndev->addr_len = 0;
+	ndev->flags = IFF_POINTOPOINT | IFF_NOARP;
+	ndev->netdev_ops = &mhi_netdev_ops;
+	ndev->mtu = MHI_NET_DEFAULT_MTU;
+	ndev->min_mtu = MHI_NET_MIN_MTU;
+	ndev->max_mtu = MHI_NET_MAX_MTU;
+	ndev->tx_queue_len = 1000;
+}
+
+static void mhi_net_dl_callback(struct mhi_device *mhi_dev,
+				struct mhi_result *mhi_res)
+{
+	struct mhi_net_dev *mhi_netdev = dev_get_drvdata(&mhi_dev->dev);
+	const struct mhi_net_proto *proto = mhi_netdev->proto;
+	struct sk_buff *skb = mhi_res->buf_addr;
+	int remaining;
+
+	remaining = atomic_dec_return(&mhi_netdev->stats.rx_queued);
+
+	if (unlikely(mhi_res->transaction_status)) {
+		dev_kfree_skb_any(skb);
+
+		/* MHI layer stopping/resetting the DL channel */
+		if (mhi_res->transaction_status == -ENOTCONN)
+			return;
+
+		u64_stats_update_begin(&mhi_netdev->stats.rx_syncp);
+		u64_stats_inc(&mhi_netdev->stats.rx_errors);
+		u64_stats_update_end(&mhi_netdev->stats.rx_syncp);
+	} else {
+		u64_stats_update_begin(&mhi_netdev->stats.rx_syncp);
+		u64_stats_inc(&mhi_netdev->stats.rx_packets);
+		u64_stats_add(&mhi_netdev->stats.rx_bytes, mhi_res->bytes_xferd);
+		u64_stats_update_end(&mhi_netdev->stats.rx_syncp);
+
+		switch (skb->data[0] & 0xf0) {
+		case 0x40:
+			skb->protocol = htons(ETH_P_IP);
+			break;
+		case 0x60:
+			skb->protocol = htons(ETH_P_IPV6);
+			break;
+		default:
+			skb->protocol = htons(ETH_P_MAP);
+			break;
+		}
+
+		skb_put(skb, mhi_res->bytes_xferd);
+
+		if (proto && proto->rx_fixup)
+			proto->rx_fixup(mhi_netdev->ndev, skb);
+		else
+			netif_rx(skb);
+	}
+
+	/* Refill if RX buffers queue becomes low */
+	if (remaining <= mhi_netdev->rx_queue_sz / 2)
+		schedule_delayed_work(&mhi_netdev->rx_refill, 0);
+}
+
+static void mhi_net_ul_callback(struct mhi_device *mhi_dev,
+				struct mhi_result *mhi_res)
+{
+	struct mhi_net_dev *mhi_netdev = dev_get_drvdata(&mhi_dev->dev);
+	struct net_device *ndev = mhi_netdev->ndev;
+	struct mhi_device *mdev = mhi_netdev->mdev;
+	struct sk_buff *skb = mhi_res->buf_addr;
+
+	/* Hardware has consumed the buffer, so free the skb (which is not
+	 * freed by the MHI stack) and perform accounting.
+	 */
+	dev_consume_skb_any(skb);
+
+	u64_stats_update_begin(&mhi_netdev->stats.tx_syncp);
+	if (unlikely(mhi_res->transaction_status)) {
+
+		/* MHI layer stopping/resetting the UL channel */
+		if (mhi_res->transaction_status == -ENOTCONN) {
+			u64_stats_update_end(&mhi_netdev->stats.tx_syncp);
+			return;
+		}
+
+		u64_stats_inc(&mhi_netdev->stats.tx_errors);
+	} else {
+		u64_stats_inc(&mhi_netdev->stats.tx_packets);
+		u64_stats_add(&mhi_netdev->stats.tx_bytes, mhi_res->bytes_xferd);
+	}
+	u64_stats_update_end(&mhi_netdev->stats.tx_syncp);
+
+	if (netif_queue_stopped(ndev) && !mhi_queue_is_full(mdev, DMA_TO_DEVICE))
+		netif_wake_queue(ndev);
+}
+
+static void mhi_net_rx_refill_work(struct work_struct *work)
+{
+	struct mhi_net_dev *mhi_netdev = container_of(work, struct mhi_net_dev,
+						      rx_refill.work);
+	struct net_device *ndev = mhi_netdev->ndev;
+	struct mhi_device *mdev = mhi_netdev->mdev;
+	int size = READ_ONCE(ndev->mtu);
+	struct sk_buff *skb;
+	int err;
+
+	while (atomic_read(&mhi_netdev->stats.rx_queued) < mhi_netdev->rx_queue_sz) {
+		skb = netdev_alloc_skb(ndev, size);
+		if (unlikely(!skb))
+			break;
+
+		err = mhi_queue_skb(mdev, DMA_FROM_DEVICE, skb, size, MHI_EOT);
+		if (unlikely(err)) {
+			net_err_ratelimited("%s: Failed to queue RX buf (%d)\n",
+					    ndev->name, err);
+			kfree_skb(skb);
+			break;
+		}
+
+		atomic_inc(&mhi_netdev->stats.rx_queued);
+
+		/* Do not hog the CPU if rx buffers are consumed faster than
+		 * queued (unlikely).
+		 */
+		cond_resched();
+	}
+
+	/* If we're still starved of rx buffers, reschedule later */
+	if (unlikely(!atomic_read(&mhi_netdev->stats.rx_queued)))
+		schedule_delayed_work(&mhi_netdev->rx_refill, HZ / 2);
+}
+
+static struct device_type wwan_type = {
+	.name = "wwan",
+};
+
+static int mhi_net_probe(struct mhi_device *mhi_dev,
+			 const struct mhi_device_id *id)
+{
+	const struct mhi_device_info *info = (struct mhi_device_info *)id->driver_data;
+	struct device *dev = &mhi_dev->dev;
+	struct mhi_net_dev *mhi_netdev;
+	struct net_device *ndev;
+	int err;
+
+	ndev = alloc_netdev(sizeof(*mhi_netdev), info->netname,
+			    NET_NAME_PREDICTABLE, mhi_net_setup);
+	if (!ndev)
+		return -ENOMEM;
+
+	mhi_netdev = netdev_priv(ndev);
+	dev_set_drvdata(dev, mhi_netdev);
+	mhi_netdev->ndev = ndev;
+	mhi_netdev->mdev = mhi_dev;
+	mhi_netdev->proto = info->proto;
+	SET_NETDEV_DEV(ndev, &mhi_dev->dev);
+	SET_NETDEV_DEVTYPE(ndev, &wwan_type);
+
+	/* All MHI net channels have 128 ring elements (at least for now) */
+	mhi_netdev->rx_queue_sz = 128;
+
+	INIT_DELAYED_WORK(&mhi_netdev->rx_refill, mhi_net_rx_refill_work);
+	u64_stats_init(&mhi_netdev->stats.rx_syncp);
+	u64_stats_init(&mhi_netdev->stats.tx_syncp);
+
+	/* Start MHI channels */
+	err = mhi_prepare_for_transfer(mhi_dev);
+	if (err)
+		goto out_err;
+
+	err = register_netdev(ndev);
+	if (err)
+		goto out_err;
+
+	if (mhi_netdev->proto) {
+		err = mhi_netdev->proto->init(mhi_netdev);
+		if (err)
+			goto out_err_proto;
+	}
+
+	return 0;
+
+out_err_proto:
+	unregister_netdev(ndev);
+out_err:
+	free_netdev(ndev);
+	return err;
+}
+
+static void mhi_net_remove(struct mhi_device *mhi_dev)
+{
+	struct mhi_net_dev *mhi_netdev = dev_get_drvdata(&mhi_dev->dev);
+
+	unregister_netdev(mhi_netdev->ndev);
+
+	mhi_unprepare_from_transfer(mhi_netdev->mdev);
+
+	free_netdev(mhi_netdev->ndev);
+}
+
+static const struct mhi_device_info mhi_hwip0 = {
+	.netname = "mhi_hwip%d",
+};
+
+static const struct mhi_device_info mhi_swip0 = {
+	.netname = "mhi_swip%d",
+};
+
+static const struct mhi_device_id mhi_net_id_table[] = {
+	/* Hardware accelerated data PATH (to modem IPA), protocol agnostic */
+	{ .chan = "IP_HW0", .driver_data = (kernel_ulong_t)&mhi_hwip0 },
+	/* Software data PATH (to modem CPU) */
+	{ .chan = "IP_SW0", .driver_data = (kernel_ulong_t)&mhi_swip0 },
+	{}
+};
+MODULE_DEVICE_TABLE(mhi, mhi_net_id_table);
+
+static struct mhi_driver mhi_net_driver = {
+	.probe = mhi_net_probe,
+	.remove = mhi_net_remove,
+	.dl_xfer_cb = mhi_net_dl_callback,
+	.ul_xfer_cb = mhi_net_ul_callback,
+	.id_table = mhi_net_id_table,
+	.driver = {
+		.name = "mhi_net",
+		.owner = THIS_MODULE,
+	},
+};
+
+module_mhi_driver(mhi_net_driver);
+
+MODULE_AUTHOR("Loic Poulain <loic.poulain@linaro.org>");
+MODULE_DESCRIPTION("Network over MHI");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/net/mhi_net.c b/drivers/net/mhi_net.c
deleted file mode 100644
index aa3a5e0..0000000
--- a/drivers/net/mhi_net.c
+++ /dev/null
@@ -1,379 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0-or-later
-/* MHI Network driver - Network over MHI bus
- *
- * Copyright (C) 2020 Linaro Ltd <loic.poulain@linaro.org>
- */
-
-#include <linux/if_arp.h>
-#include <linux/mhi.h>
-#include <linux/mod_devicetable.h>
-#include <linux/module.h>
-#include <linux/netdevice.h>
-#include <linux/skbuff.h>
-#include <linux/u64_stats_sync.h>
-
-#define MHI_NET_MIN_MTU		ETH_MIN_MTU
-#define MHI_NET_MAX_MTU		0xffff
-#define MHI_NET_DEFAULT_MTU	0x4000
-
-struct mhi_net_stats {
-	u64_stats_t rx_packets;
-	u64_stats_t rx_bytes;
-	u64_stats_t rx_errors;
-	u64_stats_t rx_dropped;
-	u64_stats_t tx_packets;
-	u64_stats_t tx_bytes;
-	u64_stats_t tx_errors;
-	u64_stats_t tx_dropped;
-	atomic_t rx_queued;
-	struct u64_stats_sync tx_syncp;
-	struct u64_stats_sync rx_syncp;
-};
-
-struct mhi_net_dev {
-	struct mhi_device *mdev;
-	struct net_device *ndev;
-	const struct mhi_net_proto *proto;
-	void *proto_data;
-	struct delayed_work rx_refill;
-	struct mhi_net_stats stats;
-	u32 rx_queue_sz;
-};
-
-struct mhi_net_proto {
-	int (*init)(struct mhi_net_dev *dev);
-	struct sk_buff * (*tx_fixup)(struct net_device *ndev, struct sk_buff *skb);
-	int (*rx_fixup)(struct net_device *ndev, struct sk_buff *skb);
-};
-
-struct mhi_device_info {
-	const char *netname;
-	const struct mhi_net_proto *proto;
-};
-
-static int mhi_ndo_open(struct net_device *ndev)
-{
-	struct mhi_net_dev *mhi_netdev = netdev_priv(ndev);
-
-	/* Feed the rx buffer pool */
-	schedule_delayed_work(&mhi_netdev->rx_refill, 0);
-
-	/* Carrier is established via out-of-band channel (e.g. qmi) */
-	netif_carrier_on(ndev);
-
-	netif_start_queue(ndev);
-
-	return 0;
-}
-
-static int mhi_ndo_stop(struct net_device *ndev)
-{
-	struct mhi_net_dev *mhi_netdev = netdev_priv(ndev);
-
-	netif_stop_queue(ndev);
-	netif_carrier_off(ndev);
-	cancel_delayed_work_sync(&mhi_netdev->rx_refill);
-
-	return 0;
-}
-
-static int mhi_ndo_xmit(struct sk_buff *skb, struct net_device *ndev)
-{
-	struct mhi_net_dev *mhi_netdev = netdev_priv(ndev);
-	const struct mhi_net_proto *proto = mhi_netdev->proto;
-	struct mhi_device *mdev = mhi_netdev->mdev;
-	int err;
-
-	if (proto && proto->tx_fixup) {
-		skb = proto->tx_fixup(mhi_netdev->ndev, skb);
-		if (unlikely(!skb))
-			goto exit_drop;
-	}
-
-	err = mhi_queue_skb(mdev, DMA_TO_DEVICE, skb, skb->len, MHI_EOT);
-	if (unlikely(err)) {
-		net_err_ratelimited("%s: Failed to queue TX buf (%d)\n",
-				    ndev->name, err);
-		dev_kfree_skb_any(skb);
-		goto exit_drop;
-	}
-
-	if (mhi_queue_is_full(mdev, DMA_TO_DEVICE))
-		netif_stop_queue(ndev);
-
-	return NETDEV_TX_OK;
-
-exit_drop:
-	u64_stats_update_begin(&mhi_netdev->stats.tx_syncp);
-	u64_stats_inc(&mhi_netdev->stats.tx_dropped);
-	u64_stats_update_end(&mhi_netdev->stats.tx_syncp);
-
-	return NETDEV_TX_OK;
-}
-
-static void mhi_ndo_get_stats64(struct net_device *ndev,
-				struct rtnl_link_stats64 *stats)
-{
-	struct mhi_net_dev *mhi_netdev = netdev_priv(ndev);
-	unsigned int start;
-
-	do {
-		start = u64_stats_fetch_begin_irq(&mhi_netdev->stats.rx_syncp);
-		stats->rx_packets = u64_stats_read(&mhi_netdev->stats.rx_packets);
-		stats->rx_bytes = u64_stats_read(&mhi_netdev->stats.rx_bytes);
-		stats->rx_errors = u64_stats_read(&mhi_netdev->stats.rx_errors);
-		stats->rx_dropped = u64_stats_read(&mhi_netdev->stats.rx_dropped);
-	} while (u64_stats_fetch_retry_irq(&mhi_netdev->stats.rx_syncp, start));
-
-	do {
-		start = u64_stats_fetch_begin_irq(&mhi_netdev->stats.tx_syncp);
-		stats->tx_packets = u64_stats_read(&mhi_netdev->stats.tx_packets);
-		stats->tx_bytes = u64_stats_read(&mhi_netdev->stats.tx_bytes);
-		stats->tx_errors = u64_stats_read(&mhi_netdev->stats.tx_errors);
-		stats->tx_dropped = u64_stats_read(&mhi_netdev->stats.tx_dropped);
-	} while (u64_stats_fetch_retry_irq(&mhi_netdev->stats.tx_syncp, start));
-}
-
-static const struct net_device_ops mhi_netdev_ops = {
-	.ndo_open               = mhi_ndo_open,
-	.ndo_stop               = mhi_ndo_stop,
-	.ndo_start_xmit         = mhi_ndo_xmit,
-	.ndo_get_stats64	= mhi_ndo_get_stats64,
-};
-
-static void mhi_net_setup(struct net_device *ndev)
-{
-	ndev->header_ops = NULL;  /* No header */
-	ndev->type = ARPHRD_RAWIP;
-	ndev->hard_header_len = 0;
-	ndev->addr_len = 0;
-	ndev->flags = IFF_POINTOPOINT | IFF_NOARP;
-	ndev->netdev_ops = &mhi_netdev_ops;
-	ndev->mtu = MHI_NET_DEFAULT_MTU;
-	ndev->min_mtu = MHI_NET_MIN_MTU;
-	ndev->max_mtu = MHI_NET_MAX_MTU;
-	ndev->tx_queue_len = 1000;
-}
-
-static void mhi_net_dl_callback(struct mhi_device *mhi_dev,
-				struct mhi_result *mhi_res)
-{
-	struct mhi_net_dev *mhi_netdev = dev_get_drvdata(&mhi_dev->dev);
-	const struct mhi_net_proto *proto = mhi_netdev->proto;
-	struct sk_buff *skb = mhi_res->buf_addr;
-	int remaining;
-
-	remaining = atomic_dec_return(&mhi_netdev->stats.rx_queued);
-
-	if (unlikely(mhi_res->transaction_status)) {
-		dev_kfree_skb_any(skb);
-
-		/* MHI layer stopping/resetting the DL channel */
-		if (mhi_res->transaction_status == -ENOTCONN)
-			return;
-
-		u64_stats_update_begin(&mhi_netdev->stats.rx_syncp);
-		u64_stats_inc(&mhi_netdev->stats.rx_errors);
-		u64_stats_update_end(&mhi_netdev->stats.rx_syncp);
-	} else {
-		u64_stats_update_begin(&mhi_netdev->stats.rx_syncp);
-		u64_stats_inc(&mhi_netdev->stats.rx_packets);
-		u64_stats_add(&mhi_netdev->stats.rx_bytes, mhi_res->bytes_xferd);
-		u64_stats_update_end(&mhi_netdev->stats.rx_syncp);
-
-		switch (skb->data[0] & 0xf0) {
-		case 0x40:
-			skb->protocol = htons(ETH_P_IP);
-			break;
-		case 0x60:
-			skb->protocol = htons(ETH_P_IPV6);
-			break;
-		default:
-			skb->protocol = htons(ETH_P_MAP);
-			break;
-		}
-
-		skb_put(skb, mhi_res->bytes_xferd);
-
-		if (proto && proto->rx_fixup)
-			proto->rx_fixup(mhi_netdev->ndev, skb);
-		else
-			netif_rx(skb);
-	}
-
-	/* Refill if RX buffers queue becomes low */
-	if (remaining <= mhi_netdev->rx_queue_sz / 2)
-		schedule_delayed_work(&mhi_netdev->rx_refill, 0);
-}
-
-static void mhi_net_ul_callback(struct mhi_device *mhi_dev,
-				struct mhi_result *mhi_res)
-{
-	struct mhi_net_dev *mhi_netdev = dev_get_drvdata(&mhi_dev->dev);
-	struct net_device *ndev = mhi_netdev->ndev;
-	struct mhi_device *mdev = mhi_netdev->mdev;
-	struct sk_buff *skb = mhi_res->buf_addr;
-
-	/* Hardware has consumed the buffer, so free the skb (which is not
-	 * freed by the MHI stack) and perform accounting.
-	 */
-	dev_consume_skb_any(skb);
-
-	u64_stats_update_begin(&mhi_netdev->stats.tx_syncp);
-	if (unlikely(mhi_res->transaction_status)) {
-
-		/* MHI layer stopping/resetting the UL channel */
-		if (mhi_res->transaction_status == -ENOTCONN) {
-			u64_stats_update_end(&mhi_netdev->stats.tx_syncp);
-			return;
-		}
-
-		u64_stats_inc(&mhi_netdev->stats.tx_errors);
-	} else {
-		u64_stats_inc(&mhi_netdev->stats.tx_packets);
-		u64_stats_add(&mhi_netdev->stats.tx_bytes, mhi_res->bytes_xferd);
-	}
-	u64_stats_update_end(&mhi_netdev->stats.tx_syncp);
-
-	if (netif_queue_stopped(ndev) && !mhi_queue_is_full(mdev, DMA_TO_DEVICE))
-		netif_wake_queue(ndev);
-}
-
-static void mhi_net_rx_refill_work(struct work_struct *work)
-{
-	struct mhi_net_dev *mhi_netdev = container_of(work, struct mhi_net_dev,
-						      rx_refill.work);
-	struct net_device *ndev = mhi_netdev->ndev;
-	struct mhi_device *mdev = mhi_netdev->mdev;
-	int size = READ_ONCE(ndev->mtu);
-	struct sk_buff *skb;
-	int err;
-
-	while (atomic_read(&mhi_netdev->stats.rx_queued) < mhi_netdev->rx_queue_sz) {
-		skb = netdev_alloc_skb(ndev, size);
-		if (unlikely(!skb))
-			break;
-
-		err = mhi_queue_skb(mdev, DMA_FROM_DEVICE, skb, size, MHI_EOT);
-		if (unlikely(err)) {
-			net_err_ratelimited("%s: Failed to queue RX buf (%d)\n",
-					    ndev->name, err);
-			kfree_skb(skb);
-			break;
-		}
-
-		atomic_inc(&mhi_netdev->stats.rx_queued);
-
-		/* Do not hog the CPU if rx buffers are consumed faster than
-		 * queued (unlikely).
-		 */
-		cond_resched();
-	}
-
-	/* If we're still starved of rx buffers, reschedule later */
-	if (unlikely(!atomic_read(&mhi_netdev->stats.rx_queued)))
-		schedule_delayed_work(&mhi_netdev->rx_refill, HZ / 2);
-}
-
-static struct device_type wwan_type = {
-	.name = "wwan",
-};
-
-static int mhi_net_probe(struct mhi_device *mhi_dev,
-			 const struct mhi_device_id *id)
-{
-	const struct mhi_device_info *info = (struct mhi_device_info *)id->driver_data;
-	struct device *dev = &mhi_dev->dev;
-	struct mhi_net_dev *mhi_netdev;
-	struct net_device *ndev;
-	int err;
-
-	ndev = alloc_netdev(sizeof(*mhi_netdev), info->netname,
-			    NET_NAME_PREDICTABLE, mhi_net_setup);
-	if (!ndev)
-		return -ENOMEM;
-
-	mhi_netdev = netdev_priv(ndev);
-	dev_set_drvdata(dev, mhi_netdev);
-	mhi_netdev->ndev = ndev;
-	mhi_netdev->mdev = mhi_dev;
-	mhi_netdev->proto = info->proto;
-	SET_NETDEV_DEV(ndev, &mhi_dev->dev);
-	SET_NETDEV_DEVTYPE(ndev, &wwan_type);
-
-	/* All MHI net channels have 128 ring elements (at least for now) */
-	mhi_netdev->rx_queue_sz = 128;
-
-	INIT_DELAYED_WORK(&mhi_netdev->rx_refill, mhi_net_rx_refill_work);
-	u64_stats_init(&mhi_netdev->stats.rx_syncp);
-	u64_stats_init(&mhi_netdev->stats.tx_syncp);
-
-	/* Start MHI channels */
-	err = mhi_prepare_for_transfer(mhi_dev);
-	if (err)
-		goto out_err;
-
-	err = register_netdev(ndev);
-	if (err)
-		goto out_err;
-
-	if (mhi_netdev->proto) {
-		err = mhi_netdev->proto->init(mhi_netdev);
-		if (err)
-			goto out_err_proto;
-	}
-
-	return 0;
-
-out_err_proto:
-	unregister_netdev(ndev);
-out_err:
-	free_netdev(ndev);
-	return err;
-}
-
-static void mhi_net_remove(struct mhi_device *mhi_dev)
-{
-	struct mhi_net_dev *mhi_netdev = dev_get_drvdata(&mhi_dev->dev);
-
-	unregister_netdev(mhi_netdev->ndev);
-
-	mhi_unprepare_from_transfer(mhi_netdev->mdev);
-
-	free_netdev(mhi_netdev->ndev);
-}
-
-static const struct mhi_device_info mhi_hwip0 = {
-	.netname = "mhi_hwip%d",
-};
-
-static const struct mhi_device_info mhi_swip0 = {
-	.netname = "mhi_swip%d",
-};
-
-static const struct mhi_device_id mhi_net_id_table[] = {
-	/* Hardware accelerated data PATH (to modem IPA), protocol agnostic */
-	{ .chan = "IP_HW0", .driver_data = (kernel_ulong_t)&mhi_hwip0 },
-	/* Software data PATH (to modem CPU) */
-	{ .chan = "IP_SW0", .driver_data = (kernel_ulong_t)&mhi_swip0 },
-	{}
-};
-MODULE_DEVICE_TABLE(mhi, mhi_net_id_table);
-
-static struct mhi_driver mhi_net_driver = {
-	.probe = mhi_net_probe,
-	.remove = mhi_net_remove,
-	.dl_xfer_cb = mhi_net_dl_callback,
-	.ul_xfer_cb = mhi_net_ul_callback,
-	.id_table = mhi_net_id_table,
-	.driver = {
-		.name = "mhi_net",
-		.owner = THIS_MODULE,
-	},
-};
-
-module_mhi_driver(mhi_net_driver);
-
-MODULE_AUTHOR("Loic Poulain <loic.poulain@linaro.org>");
-MODULE_DESCRIPTION("Network over MHI");
-MODULE_LICENSE("GPL v2");
-- 
2.7.4

[PATCH net-next 3/3] net: mhi: Add mbim proto

[Loic Poulain]

MBIM has initially been specified by USB-IF for transporting data (IP)
between a modem and a host over USB. However some modern modems also
support MBIM over PCIe (via MHI). In the same way as QMAP(rmnet), it
allows to aggregate IP packets and to perform context multiplexing.

This change adds minimal MBIM support to MHI, allowing to support MBIM
only modems. MBIM being based on USB NCM, it reuses some helpers from
the USB stack, but the cdc-mbim driver is too USB coupled to be reused.

At some point it would be interesting to move on a factorized solution,
having a generic MBIM network lib or dedicated MBIM netlink virtual
interface support.

This code has been highly inspired from the mhi_mbim downstream driver
(Carl Yin <carl.yin@quectel.com>).

Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
---
 drivers/net/mhi/Makefile     |   2 +-
 drivers/net/mhi/mhi.h        |  39 ++++++++
 drivers/net/mhi/net.c        |  41 ++------
 drivers/net/mhi/proto_mbim.c | 220 +++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 270 insertions(+), 32 deletions(-)
 create mode 100644 drivers/net/mhi/mhi.h
 create mode 100644 drivers/net/mhi/proto_mbim.c

diff --git a/drivers/net/mhi/Makefile b/drivers/net/mhi/Makefile
index 0acf989..f71b9f8 100644
--- a/drivers/net/mhi/Makefile
+++ b/drivers/net/mhi/Makefile
@@ -1,3 +1,3 @@
 obj-$(CONFIG_MHI_NET) += mhi_net.o
 
-mhi_net-y := net.o
+mhi_net-y := net.o proto_mbim.o
diff --git a/drivers/net/mhi/mhi.h b/drivers/net/mhi/mhi.h
new file mode 100644
index 0000000..e7f7246
--- /dev/null
+++ b/drivers/net/mhi/mhi.h
@@ -0,0 +1,39 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+/* MHI Network driver - Network over MHI bus
+ *
+ * Copyright (C) 2021 Linaro Ltd <loic.poulain@linaro.org>
+ */
+
+struct mhi_net_stats {
+	u64_stats_t rx_packets;
+	u64_stats_t rx_bytes;
+	u64_stats_t rx_errors;
+	u64_stats_t rx_dropped;
+	u64_stats_t tx_packets;
+	u64_stats_t tx_bytes;
+	u64_stats_t tx_errors;
+	u64_stats_t tx_dropped;
+	atomic_t rx_queued;
+	struct u64_stats_sync tx_syncp;
+	struct u64_stats_sync rx_syncp;
+};
+
+struct mhi_net_dev {
+	struct mhi_device *mdev;
+	struct net_device *ndev;
+	const struct mhi_device_info *info;
+	const struct mhi_net_proto *proto;
+	void *proto_data;
+	struct delayed_work rx_refill;
+	struct mhi_net_stats stats;
+	u32 rx_queue_sz;
+};
+
+struct mhi_net_proto {
+	int (*init)(struct mhi_net_dev *dev);
+	struct sk_buff * (*tx_fixup)(struct net_device *ndev, struct sk_buff *skb);
+	int (*rx_fixup)(struct net_device *ndev, struct sk_buff *skb);
+};
+
+extern const struct mhi_net_proto proto_mbim;
diff --git a/drivers/net/mhi/net.c b/drivers/net/mhi/net.c
index aa3a5e0..e8e458f 100644
--- a/drivers/net/mhi/net.c
+++ b/drivers/net/mhi/net.c
@@ -12,40 +12,12 @@
 #include <linux/skbuff.h>
 #include <linux/u64_stats_sync.h>
 
+#include "mhi.h"
+
 #define MHI_NET_MIN_MTU		ETH_MIN_MTU
 #define MHI_NET_MAX_MTU		0xffff
 #define MHI_NET_DEFAULT_MTU	0x4000
 
-struct mhi_net_stats {
-	u64_stats_t rx_packets;
-	u64_stats_t rx_bytes;
-	u64_stats_t rx_errors;
-	u64_stats_t rx_dropped;
-	u64_stats_t tx_packets;
-	u64_stats_t tx_bytes;
-	u64_stats_t tx_errors;
-	u64_stats_t tx_dropped;
-	atomic_t rx_queued;
-	struct u64_stats_sync tx_syncp;
-	struct u64_stats_sync rx_syncp;
-};
-
-struct mhi_net_dev {
-	struct mhi_device *mdev;
-	struct net_device *ndev;
-	const struct mhi_net_proto *proto;
-	void *proto_data;
-	struct delayed_work rx_refill;
-	struct mhi_net_stats stats;
-	u32 rx_queue_sz;
-};
-
-struct mhi_net_proto {
-	int (*init)(struct mhi_net_dev *dev);
-	struct sk_buff * (*tx_fixup)(struct net_device *ndev, struct sk_buff *skb);
-	int (*rx_fixup)(struct net_device *ndev, struct sk_buff *skb);
-};
-
 struct mhi_device_info {
 	const char *netname;
 	const struct mhi_net_proto *proto;
@@ -351,12 +323,19 @@ static const struct mhi_device_info mhi_swip0 = {
 	.netname = "mhi_swip%d",
 };
 
+static const struct mhi_device_info mhi_hwip0_mbim = {
+	.netname = "mhi_mbim%d",
+	.proto = &proto_mbim,
+};
+
 static const struct mhi_device_id mhi_net_id_table[] = {
 	/* Hardware accelerated data PATH (to modem IPA), protocol agnostic */
 	{ .chan = "IP_HW0", .driver_data = (kernel_ulong_t)&mhi_hwip0 },
 	/* Software data PATH (to modem CPU) */
 	{ .chan = "IP_SW0", .driver_data = (kernel_ulong_t)&mhi_swip0 },
-	{}
+	/* Hardware accelerated data PATH (to modem IPA), MBIM protocol */
+	{ .chan = "IP_HW0_MBIM", .driver_data = (kernel_ulong_t)&mhi_hwip0_mbim },
+	{ }
 };
 MODULE_DEVICE_TABLE(mhi, mhi_net_id_table);
 
diff --git a/drivers/net/mhi/proto_mbim.c b/drivers/net/mhi/proto_mbim.c
new file mode 100644
index 0000000..b568078c
--- /dev/null
+++ b/drivers/net/mhi/proto_mbim.c
@@ -0,0 +1,220 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/* MHI Network driver - Network over MHI bus
+ *
+ * Copyright (C) 2021 Linaro Ltd <loic.poulain@linaro.org>
+ */
+
+#include <linux/ethtool.h>
+#include <linux/if_vlan.h>
+#include <linux/ip.h>
+#include <linux/mii.h>
+#include <linux/netdevice.h>
+#include <linux/skbuff.h>
+#include <linux/usb.h>
+#include <linux/usb/cdc.h>
+#include <linux/usb/usbnet.h>
+#include <linux/usb/cdc_ncm.h>
+
+#include "mhi.h"
+
+struct mbim_context {
+	u16 rx_seq;
+};
+
+static int mbim_rx_verify_nth16(struct sk_buff *skb)
+{
+	struct usb_cdc_ncm_nth16 *nth16;
+	int ret = -EINVAL;
+
+	if (skb->len < (sizeof(struct usb_cdc_ncm_nth16) +
+			sizeof(struct usb_cdc_ncm_ndp16))) {
+		goto error;
+	}
+
+	nth16 = (struct usb_cdc_ncm_nth16 *)skb->data;
+
+	if (nth16->dwSignature != cpu_to_le32(USB_CDC_NCM_NTH16_SIGN))
+		goto error;
+
+	ret = le16_to_cpu(nth16->wNdpIndex);
+error:
+	return ret;
+}
+
+static int mbim_rx_verify_ndp16(struct sk_buff *skb, int ndpoffset)
+{
+	struct usb_cdc_ncm_ndp16 *ndp16;
+	int ret = -EINVAL;
+
+	if ((ndpoffset + sizeof(struct usb_cdc_ncm_ndp16)) > skb->len)
+		goto error;
+
+	ndp16 = (struct usb_cdc_ncm_ndp16 *)(skb->data + ndpoffset);
+
+	if (le16_to_cpu(ndp16->wLength) < USB_CDC_NCM_NDP16_LENGTH_MIN)
+		goto error;
+
+	ret = ((le16_to_cpu(ndp16->wLength) -
+					sizeof(struct usb_cdc_ncm_ndp16)) /
+					sizeof(struct usb_cdc_ncm_dpe16));
+	ret--; /* Last entry is always a NULL terminator */
+
+	if ((sizeof(struct usb_cdc_ncm_ndp16) +
+	     ret * (sizeof(struct usb_cdc_ncm_dpe16))) > skb->len) {
+		ret = -EINVAL;
+	}
+error:
+	return ret;
+}
+
+static int mbim_rx_fixup(struct net_device *ndev, struct sk_buff *skb)
+{
+	int ndpoffset;
+
+	/* Check NTB header signature and retrieve first NDP offset */
+	ndpoffset = mbim_rx_verify_nth16(skb);
+	if (ndpoffset < 0) {
+		netdev_err(ndev, "MBIM: Incorrect NTB header\n");
+		goto error;
+	}
+
+	/* Process each NDP */
+	while (1) {
+		struct usb_cdc_ncm_ndp16 *ndp16;
+		struct usb_cdc_ncm_dpe16 *dpe16;
+		int nframes, n;
+
+		/* Check NDP header and retrieve number of datagrams */
+		nframes = mbim_rx_verify_ndp16(skb, ndpoffset);
+		if (nframes < 0) {
+			netdev_err(ndev, "MBIM: Incorrect NDP16\n");
+			goto error;
+		}
+
+		/* Only support the IPS session 0 for now */
+		ndp16 = (struct usb_cdc_ncm_ndp16 *)(skb->data + ndpoffset);
+		switch (ndp16->dwSignature & cpu_to_le32(0x00ffffff)) {
+		case cpu_to_le32(USB_CDC_MBIM_NDP16_IPS_SIGN):
+			break;
+		default:
+			netdev_err(ndev, "MBIM: Unsupported NDP type\n");
+			goto next_ndp;
+		}
+
+		/* de-aggregate and deliver IP packets */
+		dpe16 = ndp16->dpe16;
+		for (n = 0; n < nframes; n++, dpe16++) {
+			u16 dgram_offset = le16_to_cpu(dpe16->wDatagramIndex);
+			u16 dgram_len = le16_to_cpu(dpe16->wDatagramLength);
+			struct sk_buff *skbn;
+
+			if (!dgram_offset || !dgram_len)
+				break; /* null terminator */
+
+			skbn = netdev_alloc_skb(ndev, dgram_len);
+			if (!skbn)
+				continue;
+
+			skb_put(skbn, dgram_len);
+			memcpy(skbn->data, skb->data + dgram_offset, dgram_len);
+
+			switch (skbn->data[0] & 0xf0) {
+			case 0x40:
+				skbn->protocol = htons(ETH_P_IP);
+				break;
+			case 0x60:
+				skbn->protocol = htons(ETH_P_IPV6);
+				break;
+			default:
+				netdev_err(ndev, "MBIM: unknown protocol\n");
+				continue;
+			}
+
+			netif_rx(skbn);
+		}
+next_ndp:
+		/* Other NDP to process? */
+		ndpoffset = le16_to_cpu(ndp16->wNextNdpIndex);
+		if (!ndpoffset)
+			break;
+	}
+
+	/* free skb */
+	dev_consume_skb_any(skb);
+	return 0;
+error:
+	dev_kfree_skb_any(skb);
+	return -EIO;
+}
+
+struct mbim_tx_hdr {
+	struct usb_cdc_ncm_nth16 nth16;
+	struct usb_cdc_ncm_ndp16 ndp16;
+	struct usb_cdc_ncm_dpe16 dpe16[2];
+} __packed;
+
+static struct sk_buff *mbim_tx_fixup(struct net_device *ndev, struct sk_buff *skb)
+{
+	struct mbim_tx_hdr *mbim_hdr;
+	struct usb_cdc_ncm_nth16 *nth16;
+	struct usb_cdc_ncm_ndp16 *ndp16;
+	unsigned int dgram_size = skb->len;
+	static int seq;
+
+	/* For now, this is a partial implementation of CDC MBIM, only one NDP
+	 * is sent, containing the IP packet (no aggregation).
+	 */
+
+	if (skb_headroom(skb) < sizeof(struct mbim_tx_hdr)) {
+		dev_kfree_skb_any(skb);
+		return NULL;
+	}
+
+	mbim_hdr = skb_push(skb, sizeof(struct mbim_tx_hdr));
+
+	/* Fill NTB header */
+	nth16 = &mbim_hdr->nth16;
+	nth16->dwSignature = cpu_to_le32(USB_CDC_NCM_NTH16_SIGN);
+	nth16->wHeaderLength = cpu_to_le16(sizeof(struct usb_cdc_ncm_nth16));
+	nth16->wSequence = cpu_to_le16(seq++);
+	nth16->wBlockLength = cpu_to_le16(skb->len);
+	nth16->wNdpIndex = cpu_to_le16(sizeof(struct usb_cdc_ncm_nth16));
+
+	/* Fill the unique NDP */
+	ndp16 = &mbim_hdr->ndp16;
+	ndp16->dwSignature = cpu_to_le32(USB_CDC_MBIM_NDP16_IPS_SIGN);
+	ndp16->wLength = cpu_to_le16(sizeof(struct usb_cdc_ncm_ndp16)
+					+ sizeof(struct usb_cdc_ncm_dpe16) * 2);
+	ndp16->wNextNdpIndex = 0;
+
+	/* Datagram follows the mbim header */
+	ndp16->dpe16[0].wDatagramIndex = cpu_to_le16(sizeof(struct mbim_tx_hdr));
+	ndp16->dpe16[0].wDatagramLength = cpu_to_le16(dgram_size);
+
+	/* null termination */
+	ndp16->dpe16[1].wDatagramIndex = 0;
+	ndp16->dpe16[1].wDatagramLength = 0;
+
+	return skb;
+}
+
+static int mbim_init(struct mhi_net_dev *mhi_netdev)
+{
+	struct net_device *ndev = mhi_netdev->ndev;
+
+	mhi_netdev->proto_data = devm_kzalloc(&ndev->dev,
+					      sizeof(struct mbim_context),
+					      GFP_KERNEL);
+	if (!mhi_netdev->proto_data)
+		return -ENOMEM;
+
+	ndev->needed_headroom = sizeof(struct mbim_tx_hdr);
+
+	return 0;
+}
+
+const struct mhi_net_proto proto_mbim = {
+	.init = mbim_init,
+	.rx_fixup = mbim_rx_fixup,
+	.tx_fixup = mbim_tx_fixup,
+};
-- 
2.7.4

Re: [PATCH net-next 3/3] net: mhi: Add mbim proto

[Jakub Kicinski]

On Wed, 27 Jan 2021 18:01:17 +0100 Loic Poulain wrote:
> MBIM has initially been specified by USB-IF for transporting data (IP)
> between a modem and a host over USB. However some modern modems also
> support MBIM over PCIe (via MHI). In the same way as QMAP(rmnet), it
> allows to aggregate IP packets and to perform context multiplexing.
> 
> This change adds minimal MBIM support to MHI, allowing to support MBIM
> only modems. MBIM being based on USB NCM, it reuses some helpers from
> the USB stack, but the cdc-mbim driver is too USB coupled to be reused.
> 
> At some point it would be interesting to move on a factorized solution,
> having a generic MBIM network lib or dedicated MBIM netlink virtual
> interface support.
> 
> This code has been highly inspired from the mhi_mbim downstream driver
> (Carl Yin <carl.yin@quectel.com>).
> 
> Signed-off-by: Loic Poulain <loic.poulain@linaro.org>

Does the existing MBIM over USB NCM also show up as a netdev?

Let's CC Dan and Bjorn on MBIM-related code, they may have opinions.

Re: [PATCH net-next 3/3] net: mhi: Add mbim proto

[Dan Williams]

On Fri, 2021-01-29 at 18:21 -0800, Jakub Kicinski wrote:
> On Wed, 27 Jan 2021 18:01:17 +0100 Loic Poulain wrote:
> > MBIM has initially been specified by USB-IF for transporting data
> > (IP)
> > between a modem and a host over USB. However some modern modems
> > also
> > support MBIM over PCIe (via MHI). In the same way as QMAP(rmnet),
> > it
> > allows to aggregate IP packets and to perform context multiplexing.
> > 
> > This change adds minimal MBIM support to MHI, allowing to support
> > MBIM
> > only modems. MBIM being based on USB NCM, it reuses some helpers
> > from
> > the USB stack, but the cdc-mbim driver is too USB coupled to be
> > reused.
> > 
> > At some point it would be interesting to move on a factorized
> > solution,
> > having a generic MBIM network lib or dedicated MBIM netlink virtual
> > interface support.

What would a kernel-side MBIM netlink interface do?  Just data-plane
stuff (like channel setup to create new netdevs), or are you thinking
about control-plane stuff like APN definition, radio scans, etc?

Dan

> > This code has been highly inspired from the mhi_mbim downstream
> > driver
> > (Carl Yin <carl.yin@quectel.com>).
> > 
> > Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
> 
> Does the existing MBIM over USB NCM also show up as a netdev?
> 
> Let's CC Dan and Bjorn on MBIM-related code, they may have opinions.
> 

Re: [PATCH net-next 3/3] net: mhi: Add mbim proto

[Loic Poulain]

On Mon, 1 Feb 2021 at 19:17, Dan Williams <dcbw@redhat.com> wrote:
>
> On Fri, 2021-01-29 at 18:21 -0800, Jakub Kicinski wrote:
> > On Wed, 27 Jan 2021 18:01:17 +0100 Loic Poulain wrote:
> > > MBIM has initially been specified by USB-IF for transporting data
> > > (IP)
> > > between a modem and a host over USB. However some modern modems
> > > also
> > > support MBIM over PCIe (via MHI). In the same way as QMAP(rmnet),
> > > it
> > > allows to aggregate IP packets and to perform context multiplexing.
> > >
> > > This change adds minimal MBIM support to MHI, allowing to support
> > > MBIM
> > > only modems. MBIM being based on USB NCM, it reuses some helpers
> > > from
> > > the USB stack, but the cdc-mbim driver is too USB coupled to be
> > > reused.
> > >
> > > At some point it would be interesting to move on a factorized
> > > solution,
> > > having a generic MBIM network lib or dedicated MBIM netlink virtual
> > > interface support.
>
> What would a kernel-side MBIM netlink interface do?  Just data-plane
> stuff (like channel setup to create new netdevs), or are you thinking
> about control-plane stuff like APN definition, radio scans, etc?

Just the data-plane (mbim encoding/decoding/muxing).

Loic

Re: [PATCH net-next 3/3] net: mhi: Add mbim proto

[Dan Williams]

On Mon, 2021-02-01 at 19:27 +0100, Loic Poulain wrote:
> On Mon, 1 Feb 2021 at 19:17, Dan Williams <dcbw@redhat.com> wrote:
> > On Fri, 2021-01-29 at 18:21 -0800, Jakub Kicinski wrote:
> > > On Wed, 27 Jan 2021 18:01:17 +0100 Loic Poulain wrote:
> > > > MBIM has initially been specified by USB-IF for transporting
> > > > data
> > > > (IP)
> > > > between a modem and a host over USB. However some modern modems
> > > > also
> > > > support MBIM over PCIe (via MHI). In the same way as
> > > > QMAP(rmnet),
> > > > it
> > > > allows to aggregate IP packets and to perform context
> > > > multiplexing.
> > > > 
> > > > This change adds minimal MBIM support to MHI, allowing to
> > > > support
> > > > MBIM
> > > > only modems. MBIM being based on USB NCM, it reuses some
> > > > helpers
> > > > from
> > > > the USB stack, but the cdc-mbim driver is too USB coupled to be
> > > > reused.
> > > > 
> > > > At some point it would be interesting to move on a factorized
> > > > solution,
> > > > having a generic MBIM network lib or dedicated MBIM netlink
> > > > virtual
> > > > interface support.
> > 
> > What would a kernel-side MBIM netlink interface do?  Just data-
> > plane
> > stuff (like channel setup to create new netdevs), or are you
> > thinking
> > about control-plane stuff like APN definition, radio scans, etc?
> 
> Just the data-plane (mbim encoding/decoding/muxing).

Ah yes :) If so, then fully agree.

But is that really specific to MBIM? eg, same kinds of things happen
for QMI. Johannes referred to a more generic WWAN framework that we had
discussed 1.5+ years ago to address these issues. Might be worth
restarting that, perhaps simplifying, and figuring out the minimal set
of generic bits needed to describe/add/delete a data channel for WWAN
control protocols.
Dan

Re: [PATCH net-next 3/3] net: mhi: Add mbim proto

[Loic Poulain]

On Mon, 1 Feb 2021 at 19:53, Dan Williams <dcbw@redhat.com> wrote:
>
> On Mon, 2021-02-01 at 19:27 +0100, Loic Poulain wrote:
> > On Mon, 1 Feb 2021 at 19:17, Dan Williams <dcbw@redhat.com> wrote:
> > > On Fri, 2021-01-29 at 18:21 -0800, Jakub Kicinski wrote:
> > > > On Wed, 27 Jan 2021 18:01:17 +0100 Loic Poulain wrote:
> > > > > MBIM has initially been specified by USB-IF for transporting
> > > > > data
> > > > > (IP)
> > > > > between a modem and a host over USB. However some modern modems
> > > > > also
> > > > > support MBIM over PCIe (via MHI). In the same way as
> > > > > QMAP(rmnet),
> > > > > it
> > > > > allows to aggregate IP packets and to perform context
> > > > > multiplexing.
> > > > >
> > > > > This change adds minimal MBIM support to MHI, allowing to
> > > > > support
> > > > > MBIM
> > > > > only modems. MBIM being based on USB NCM, it reuses some
> > > > > helpers
> > > > > from
> > > > > the USB stack, but the cdc-mbim driver is too USB coupled to be
> > > > > reused.
> > > > >
> > > > > At some point it would be interesting to move on a factorized
> > > > > solution,
> > > > > having a generic MBIM network lib or dedicated MBIM netlink
> > > > > virtual
> > > > > interface support.
> > >
> > > What would a kernel-side MBIM netlink interface do?  Just data-
> > > plane
> > > stuff (like channel setup to create new netdevs), or are you
> > > thinking
> > > about control-plane stuff like APN definition, radio scans, etc?
> >
> > Just the data-plane (mbim encoding/decoding/muxing).
>
> Ah yes :) If so, then fully agree.
>
> But is that really specific to MBIM? eg, same kinds of things happen
> for QMI. Johannes referred to a more generic WWAN framework that we had
> discussed 1.5+ years ago to address these issues. Might be worth
> restarting that, perhaps simplifying, and figuring out the minimal set
> of generic bits needed to describe/add/delete a data channel for WWAN
> control protocols.
> Dan
>

Right, it's not specific to MBIM, it would be just about decoupling
protocol from transport (though MBIM is originally specified with USB
transport). Having a WWAN framework/subsystem would indeed make sense,
at least to structure the way all modems services/channels are exposed
and grouped (today we have netdev, chardev, tty, etc) but it's far
beyond this series, I'm not against restarting the discussion though.

Regards,
Loic

Re: [PATCH net-next 3/3] net: mhi: Add mbim proto

[Bjørn Mork]

Loic Poulain <loic.poulain@linaro.org> writes:

> MBIM has initially been specified by USB-IF for transporting data (IP)
> between a modem and a host over USB. However some modern modems also
> support MBIM over PCIe (via MHI). In the same way as QMAP(rmnet), it
> allows to aggregate IP packets and to perform context multiplexing.
>
> This change adds minimal MBIM support to MHI, allowing to support MBIM
> only modems. MBIM being based on USB NCM, it reuses some helpers from
> the USB stack, but the cdc-mbim driver is too USB coupled to be reused.

Sure, the guts of the MBIM protocol is in cdc_ncm. But you did copy most
of cdc_mbim_rx_fixup() from cdc_mbim.c so this comment doesn't make
much sense...

> At some point it would be interesting to move on a factorized solution,
> having a generic MBIM network lib or dedicated MBIM netlink virtual
> interface support.

I believe that is now or never.  Sorry.  No one is going to fix it
later.

> +static int mbim_rx_verify_nth16(struct sk_buff *skb)
> +{
> +	struct usb_cdc_ncm_nth16 *nth16;
> +	int ret = -EINVAL;
> +
> +	if (skb->len < (sizeof(struct usb_cdc_ncm_nth16) +
> +			sizeof(struct usb_cdc_ncm_ndp16))) {
> +		goto error;
> +	}
> +
> +	nth16 = (struct usb_cdc_ncm_nth16 *)skb->data;
> +
> +	if (nth16->dwSignature != cpu_to_le32(USB_CDC_NCM_NTH16_SIGN))
> +		goto error;
> +
> +	ret = le16_to_cpu(nth16->wNdpIndex);
> +error:
> +	return ret;
> +}


This is a copy of  cdc_ncm_rx_verify_nth16() except that you've dropped
the debug messages and verification of wBlockLength and wSequence.  It's
unclear to me why you don't need to verify those fields?

This function could easily be shared with cdc_ncm instead of duplicating
it.

> +static int mbim_rx_verify_ndp16(struct sk_buff *skb, int ndpoffset)
> +{
> +	struct usb_cdc_ncm_ndp16 *ndp16;
> +	int ret = -EINVAL;
> +
> +	if ((ndpoffset + sizeof(struct usb_cdc_ncm_ndp16)) > skb->len)
> +		goto error;
> +
> +	ndp16 = (struct usb_cdc_ncm_ndp16 *)(skb->data + ndpoffset);
> +
> +	if (le16_to_cpu(ndp16->wLength) < USB_CDC_NCM_NDP16_LENGTH_MIN)
> +		goto error;
> +
> +	ret = ((le16_to_cpu(ndp16->wLength) -
> +					sizeof(struct usb_cdc_ncm_ndp16)) /
> +					sizeof(struct usb_cdc_ncm_dpe16));
> +	ret--; /* Last entry is always a NULL terminator */
> +
> +	if ((sizeof(struct usb_cdc_ncm_ndp16) +
> +	     ret * (sizeof(struct usb_cdc_ncm_dpe16))) > skb->len) {
> +		ret = -EINVAL;
> +	}
> +error:
> +	return ret;
> +}

This is an exact replica of cdc_ncm_rx_verify_ndp16() AFAICS, except for
the removed debug messages.   You do know that netif_dbg() is
conditional? There is nothing to be saved by removing those lines.

FWIW, you will have to fix the copyright attribution of this file if you
want to keep this copy here.  Otherwise it just looks like you are
stealing. And I'll wonder where the rest of the code came from and
whether you have the right to license that as GPL. Better be clear about
where you found this and who owns the copyright.  There is no question
about the rights to use, given the GPL license of the original.

> +static int mbim_rx_fixup(struct net_device *ndev, struct sk_buff *skb)
> +{
> +	int ndpoffset;
> +
> +	/* Check NTB header signature and retrieve first NDP offset */
> +	ndpoffset = mbim_rx_verify_nth16(skb);
> +	if (ndpoffset < 0) {
> +		netdev_err(ndev, "MBIM: Incorrect NTB header\n");
> +		goto error;
> +	}
> +
> +	/* Process each NDP */
> +	while (1) {
> +		struct usb_cdc_ncm_ndp16 *ndp16;
> +		struct usb_cdc_ncm_dpe16 *dpe16;
> +		int nframes, n;
> +
> +		/* Check NDP header and retrieve number of datagrams */
> +		nframes = mbim_rx_verify_ndp16(skb, ndpoffset);
> +		if (nframes < 0) {
> +			netdev_err(ndev, "MBIM: Incorrect NDP16\n");
> +			goto error;
> +		}
> +
> +		/* Only support the IPS session 0 for now */
> +		ndp16 = (struct usb_cdc_ncm_ndp16 *)(skb->data + ndpoffset);
> +		switch (ndp16->dwSignature & cpu_to_le32(0x00ffffff)) {
> +		case cpu_to_le32(USB_CDC_MBIM_NDP16_IPS_SIGN):
> +			break;
> +		default:
> +			netdev_err(ndev, "MBIM: Unsupported NDP type\n");
> +			goto next_ndp;
> +		}

You don't support DSS?  Why?  That's mandatory in the MBIM spec, isn't
it?  Can we have an MBIM driver without that support?  And if so, should
completely valid MBIM frames cause an error message?

And IP multiplexing isn't supported either?  And you simply ignore the
session ID?  How is that intended to work?  What happens here when the
driver receives IP packets from two different APNs?

But please, just implement the IP multiplexing.  You do that for rmnet,
right?

At least provide some plan on how you want to add it. Don't paint
yourself into a corner.  Userspace will need a way to manage the MBIM
transport and the multiplexed IP sessions independently.  E.g. take down
the netdev associated with IPS session 0 without breaking IPS session 1.

Locking this netdev to one session will be a problem.  I know, because
I've made that mistake.

> +
> +		/* de-aggregate and deliver IP packets */
> +		dpe16 = ndp16->dpe16;
> +		for (n = 0; n < nframes; n++, dpe16++) {
> +			u16 dgram_offset = le16_to_cpu(dpe16->wDatagramIndex);
> +			u16 dgram_len = le16_to_cpu(dpe16->wDatagramLength);
> +			struct sk_buff *skbn;
> +
> +			if (!dgram_offset || !dgram_len)
> +				break; /* null terminator */
> +
> +			skbn = netdev_alloc_skb(ndev, dgram_len);
> +			if (!skbn)
> +				continue;
> +
> +			skb_put(skbn, dgram_len);
> +			memcpy(skbn->data, skb->data + dgram_offset, dgram_len);
> +
> +			switch (skbn->data[0] & 0xf0) {
> +			case 0x40:
> +				skbn->protocol = htons(ETH_P_IP);
> +				break;
> +			case 0x60:
> +				skbn->protocol = htons(ETH_P_IPV6);
> +				break;
> +			default:
> +				netdev_err(ndev, "MBIM: unknown protocol\n");
> +				continue;
> +			}
> +
> +			netif_rx(skbn);
> +		}
> +next_ndp:
> +		/* Other NDP to process? */
> +		ndpoffset = le16_to_cpu(ndp16->wNextNdpIndex);
> +		if (!ndpoffset)
> +			break;
> +	}
> +
> +	/* free skb */
> +	dev_consume_skb_any(skb);
> +	return 0;
> +error:
> +	dev_kfree_skb_any(skb);
> +	return -EIO;
> +}


Except for the missing feature, this is still mostly a copy
cdc_mbim_rx_fixup(). Please respect the copyright on code you are
copying.  You are obviously free to use this under the GPL, but the
original author still retains copyright on it.

FWIW, I can understand why you want to use a slightly modified copy in
this case, since the original is tied both to usbnet and to the weird
VLAN mapping.  So that's fine with me.


Bjørn

Re: [PATCH net-next 3/3] net: mhi: Add mbim proto

[Loic Poulain]

On Sat, 30 Jan 2021 at 15:42, Bjørn Mork <bjorn@mork.no> wrote:
>
> Loic Poulain <loic.poulain@linaro.org> writes:
>
> > MBIM has initially been specified by USB-IF for transporting data (IP)
> > between a modem and a host over USB. However some modern modems also
> > support MBIM over PCIe (via MHI). In the same way as QMAP(rmnet), it
> > allows to aggregate IP packets and to perform context multiplexing.
> >
> > This change adds minimal MBIM support to MHI, allowing to support MBIM
> > only modems. MBIM being based on USB NCM, it reuses some helpers from
> > the USB stack, but the cdc-mbim driver is too USB coupled to be reused.
>
> Sure, the guts of the MBIM protocol is in cdc_ncm. But you did copy most
> of cdc_mbim_rx_fixup() from cdc_mbim.c so this comment doesn't make
> much sense...

Yes, just wanted to say that I have to duplicate mbim functions here
because of the USB coupling of the cdc_mbim.c version.

>
> > At some point it would be interesting to move on a factorized solution,
> > having a generic MBIM network lib or dedicated MBIM netlink virtual
> > interface support.
>
> I believe that is now or never.  Sorry.  No one is going to fix it
> later.
>
> > +static int mbim_rx_verify_nth16(struct sk_buff *skb)
> > +{
> > +     struct usb_cdc_ncm_nth16 *nth16;
> > +     int ret = -EINVAL;
> > +
> > +     if (skb->len < (sizeof(struct usb_cdc_ncm_nth16) +
> > +                     sizeof(struct usb_cdc_ncm_ndp16))) {
> > +             goto error;
> > +     }
> > +
> > +     nth16 = (struct usb_cdc_ncm_nth16 *)skb->data;
> > +
> > +     if (nth16->dwSignature != cpu_to_le32(USB_CDC_NCM_NTH16_SIGN))
> > +             goto error;
> > +
> > +     ret = le16_to_cpu(nth16->wNdpIndex);
> > +error:
> > +     return ret;
> > +}
>
>
> This is a copy of  cdc_ncm_rx_verify_nth16() except that you've dropped
> the debug messages and verification of wBlockLength and wSequence.  It's
> unclear to me why you don't need to verify those fields?

Yes, I can probably re-introduce that, to be aligned with USB version
(I removed that because I initially had no context for mbim).

>
> This function could easily be shared with cdc_ncm instead of duplicating
> it.

Yes but that would request cdc_mbim changes since this function takes
a USB mbim context (cdc_ncm_ctx).

>
> > +static int mbim_rx_verify_ndp16(struct sk_buff *skb, int ndpoffset)
> > +{
> > +     struct usb_cdc_ncm_ndp16 *ndp16;
> > +     int ret = -EINVAL;
> > +
> > +     if ((ndpoffset + sizeof(struct usb_cdc_ncm_ndp16)) > skb->len)
> > +             goto error;
> > +
> > +     ndp16 = (struct usb_cdc_ncm_ndp16 *)(skb->data + ndpoffset);
> > +
> > +     if (le16_to_cpu(ndp16->wLength) < USB_CDC_NCM_NDP16_LENGTH_MIN)
> > +             goto error;
> > +
> > +     ret = ((le16_to_cpu(ndp16->wLength) -
> > +                                     sizeof(struct usb_cdc_ncm_ndp16)) /
> > +                                     sizeof(struct usb_cdc_ncm_dpe16));
> > +     ret--; /* Last entry is always a NULL terminator */
> > +
> > +     if ((sizeof(struct usb_cdc_ncm_ndp16) +
> > +          ret * (sizeof(struct usb_cdc_ncm_dpe16))) > skb->len) {
> > +             ret = -EINVAL;
> > +     }
> > +error:
> > +     return ret;
> > +}
>
> This is an exact replica of cdc_ncm_rx_verify_ndp16() AFAICS, except for
> the removed debug messages.   You do know that netif_dbg() is
> conditional? There is nothing to be saved by removing those lines.

Yes, I tried to make this function generic, it only takes the
skb/offset as parameters, and lets the caller handling error as
desired. but I can certainly re-introduce dbg messages, along with a
ndev parameter (for the print context).

>
> FWIW, you will have to fix the copyright attribution of this file if you
> want to keep this copy here.  Otherwise it just looks like you are
> stealing. And I'll wonder where the rest of the code came from and
> whether you have the right to license that as GPL. Better be clear about
> where you found this and who owns the copyright.  There is no question
> about the rights to use, given the GPL license of the original.

I'll add the proper copyright from cdc-mbim since it's clearly a
partial-copy of it (and no will to hide that).

> > +static int mbim_rx_fixup(struct net_device *ndev, struct sk_buff *skb)
> > +{
> > +     int ndpoffset;
> > +
> > +     /* Check NTB header signature and retrieve first NDP offset */
> > +     ndpoffset = mbim_rx_verify_nth16(skb);
> > +     if (ndpoffset < 0) {
> > +             netdev_err(ndev, "MBIM: Incorrect NTB header\n");
> > +             goto error;
> > +     }
> > +
> > +     /* Process each NDP */
> > +     while (1) {
> > +             struct usb_cdc_ncm_ndp16 *ndp16;
> > +             struct usb_cdc_ncm_dpe16 *dpe16;
> > +             int nframes, n;
> > +
> > +             /* Check NDP header and retrieve number of datagrams */
> > +             nframes = mbim_rx_verify_ndp16(skb, ndpoffset);
> > +             if (nframes < 0) {
> > +                     netdev_err(ndev, "MBIM: Incorrect NDP16\n");
> > +                     goto error;
> > +             }
> > +
> > +             /* Only support the IPS session 0 for now */
> > +             ndp16 = (struct usb_cdc_ncm_ndp16 *)(skb->data + ndpoffset);
> > +             switch (ndp16->dwSignature & cpu_to_le32(0x00ffffff)) {
> > +             case cpu_to_le32(USB_CDC_MBIM_NDP16_IPS_SIGN):
> > +                     break;
> > +             default:
> > +                     netdev_err(ndev, "MBIM: Unsupported NDP type\n");
> > +                     goto next_ndp;
> > +             }
>
> You don't support DSS?  Why?  That's mandatory in the MBIM spec, isn't
> it?  Can we have an MBIM driver without that support?  And if so, should
> completely valid MBIM frames cause an error message?

Well, this is a subset of MBIM, since a lot of the MBIM/NCM does not
apply for MHI/PCIe. In MHI context, the IP channel is used for network
data transport only, and MBIM simply brings aggregation and
multiplexing. Other modem functions/services are exposed via other
dedicated MHI channels.

>
> And IP multiplexing isn't supported either?  And you simply ignore the
> session ID?  How is that intended to work?  What happens here when the
> driver receives IP packets from two different APNs?

You're Right, this is on purpose, I would like to keep the initial
implementation simple, working for the main use case. So multi-pdn
context is simply not supported in that series. Moreover, I can not
test multi-context for now, but I plan to add that in a follow-up
series/patch.

> But please, just implement the IP multiplexing.  You do that for rmnet,
> right?

I especially would like to discuss the implementation since the
architecture is quite different from rmnet netlink (do we want to
create additional ifaces or to align with the VLAN cdc-mbim trick...).

>
> At least provide some plan on how you want to add it. Don't paint
> yourself into a corner.  Userspace will need a way to manage the MBIM
> transport and the multiplexed IP sessions independently.  E.g. take down
> the netdev associated with IPS session 0 without breaking IPS session 1.
>
> Locking this netdev to one session will be a problem.  I know, because
> I've made that mistake.

I think it makes sense to align with cdc-mbim behavior here, for
compatiblity, so the lower transport interface will have to stay up
for the additional session/context interfaces. Let me know we should
do that in another way.

>
> > +
> > +             /* de-aggregate and deliver IP packets */
> > +             dpe16 = ndp16->dpe16;
> > +             for (n = 0; n < nframes; n++, dpe16++) {
> > +                     u16 dgram_offset = le16_to_cpu(dpe16->wDatagramIndex);
> > +                     u16 dgram_len = le16_to_cpu(dpe16->wDatagramLength);
> > +                     struct sk_buff *skbn;
> > +
> > +                     if (!dgram_offset || !dgram_len)
> > +                             break; /* null terminator */
> > +
> > +                     skbn = netdev_alloc_skb(ndev, dgram_len);
> > +                     if (!skbn)
> > +                             continue;
> > +
> > +                     skb_put(skbn, dgram_len);
> > +                     memcpy(skbn->data, skb->data + dgram_offset, dgram_len);
> > +
> > +                     switch (skbn->data[0] & 0xf0) {
> > +                     case 0x40:
> > +                             skbn->protocol = htons(ETH_P_IP);
> > +                             break;
> > +                     case 0x60:
> > +                             skbn->protocol = htons(ETH_P_IPV6);
> > +                             break;
> > +                     default:
> > +                             netdev_err(ndev, "MBIM: unknown protocol\n");
> > +                             continue;
> > +                     }
> > +
> > +                     netif_rx(skbn);
> > +             }
> > +next_ndp:
> > +             /* Other NDP to process? */
> > +             ndpoffset = le16_to_cpu(ndp16->wNextNdpIndex);
> > +             if (!ndpoffset)
> > +                     break;
> > +     }
> > +
> > +     /* free skb */
> > +     dev_consume_skb_any(skb);
> > +     return 0;
> > +error:
> > +     dev_kfree_skb_any(skb);
> > +     return -EIO;
> > +}
>
>
> Except for the missing feature, this is still mostly a copy
> cdc_mbim_rx_fixup(). Please respect the copyright on code you are
> copying.  You are obviously free to use this under the GPL, but the
> original author still retains copyright on it.

For sure, will do.

>
> FWIW, I can understand why you want to use a slightly modified copy in
> this case, since the original is tied both to usbnet and to the weird
> VLAN mapping.  So that's fine with me.
>
>
> Bjørn

Thanks,
Loic