[PATCH 0/3] Add MHI Endpoint network driver

[PATCH 0/3] Add MHI Endpoint network driver

[Manivannan Sadhasivam]

Hi,

This series adds a network driver for the Modem Host Interface (MHI) endpoint
devices that provides network interfaces to the PCIe based Qualcomm endpoint
devices supporting MHI bus (like Modems). This driver allows the MHI endpoint
devices to establish IP communication with the host machines (x86, ARM64) over
MHI bus.

On the host side, the existing mhi_net driver provides the network connectivity
to the host.

- Mani

Manivannan Sadhasivam (3):
  net: Add MHI Endpoint network driver
  MAINTAINERS: Add entry for MHI networking drivers under MHI bus
  net: mhi: Increase the default MTU from 16K to 32K

 MAINTAINERS              |   1 +
 drivers/net/Kconfig      |   9 ++
 drivers/net/Makefile     |   1 +
 drivers/net/mhi_ep_net.c | 331 +++++++++++++++++++++++++++++++++++++++
 drivers/net/mhi_net.c    |   2 +-
 5 files changed, 343 insertions(+), 1 deletion(-)
 create mode 100644 drivers/net/mhi_ep_net.c


base-commit: ae91f7e436f8b631c47e244b892ecac62a4d9430
-- 
2.25.1

[PATCH 1/3] net: Add MHI Endpoint network driver

[Manivannan Sadhasivam]

Add a network driver for the Modem Host Interface (MHI) endpoint devices
that provides network interfaces to the PCIe based Qualcomm endpoint
devices supporting MHI bus. This driver allows the MHI endpoint devices to
establish IP communication with the host machines (x86, ARM64) over MHI
bus.

The driver currently supports only IP_SW0 MHI channel that can be used
to route IP traffic from the endpoint CPU to host machine.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/net/Kconfig      |   9 ++
 drivers/net/Makefile     |   1 +
 drivers/net/mhi_ep_net.c | 331 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 341 insertions(+)
 create mode 100644 drivers/net/mhi_ep_net.c

diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
index 368c6f5b327e..ec1382d7fcbd 100644
--- a/drivers/net/Kconfig
+++ b/drivers/net/Kconfig
@@ -452,6 +452,15 @@ config MHI_NET
 	  QCOM based WWAN modems for IP or QMAP/rmnet protocol (like SDX55).
 	  Say Y or M.
 
+config MHI_EP_NET
+	tristate "MHI Endpoint network driver"
+	depends on MHI_BUS_EP
+	help
+	  This is the network driver for MHI bus implementation in endpoint
+	  devices. It is used provide the network interface for QCOM modems
+	  such as SDX55.
+	  Say Y or M.
+
 endif # NET_CORE
 
 config SUNGEM_PHY
diff --git a/drivers/net/Makefile b/drivers/net/Makefile
index e26f98f897c5..b8e706a4150e 100644
--- a/drivers/net/Makefile
+++ b/drivers/net/Makefile
@@ -40,6 +40,7 @@ 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_EP_NET) += mhi_ep_net.o
 
 #
 # Networking Drivers
diff --git a/drivers/net/mhi_ep_net.c b/drivers/net/mhi_ep_net.c
new file mode 100644
index 000000000000..b3960814c879
--- /dev/null
+++ b/drivers/net/mhi_ep_net.c
@@ -0,0 +1,331 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * MHI Endpoint Network driver
+ *
+ * Based on drivers/net/mhi_net.c
+ *
+ * Copyright (c) 2023, Linaro Ltd.
+ * Author: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
+ */
+
+#include <linux/if_arp.h>
+#include <linux/mhi_ep.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
+
+struct mhi_ep_net_stats {
+	u64_stats_t rx_packets;
+	u64_stats_t rx_bytes;
+	u64_stats_t rx_errors;
+	u64_stats_t tx_packets;
+	u64_stats_t tx_bytes;
+	u64_stats_t tx_errors;
+	u64_stats_t tx_dropped;
+	struct u64_stats_sync tx_syncp;
+	struct u64_stats_sync rx_syncp;
+};
+
+struct mhi_ep_net_dev {
+	struct mhi_ep_device *mdev;
+	struct net_device *ndev;
+	struct mhi_ep_net_stats stats;
+	struct workqueue_struct *xmit_wq;
+	struct work_struct xmit_work;
+	struct sk_buff_head tx_buffers;
+	spinlock_t tx_lock; /* Lock for protecting tx_buffers */
+	u32 mru;
+};
+
+static void mhi_ep_net_dev_process_queue_packets(struct work_struct *work)
+{
+	struct mhi_ep_net_dev *mhi_ep_netdev = container_of(work,
+			struct mhi_ep_net_dev, xmit_work);
+	struct mhi_ep_device *mdev = mhi_ep_netdev->mdev;
+	struct sk_buff_head q;
+	struct sk_buff *skb;
+	int ret;
+
+	if (mhi_ep_queue_is_empty(mdev, DMA_FROM_DEVICE)) {
+		netif_stop_queue(mhi_ep_netdev->ndev);
+		return;
+	}
+
+	__skb_queue_head_init(&q);
+
+	spin_lock_bh(&mhi_ep_netdev->tx_lock);
+	skb_queue_splice_init(&mhi_ep_netdev->tx_buffers, &q);
+	spin_unlock_bh(&mhi_ep_netdev->tx_lock);
+
+	while ((skb = __skb_dequeue(&q))) {
+		ret = mhi_ep_queue_skb(mdev, skb);
+		if (ret) {
+			kfree(skb);
+			goto exit_drop;
+		}
+
+		u64_stats_update_begin(&mhi_ep_netdev->stats.tx_syncp);
+		u64_stats_inc(&mhi_ep_netdev->stats.tx_packets);
+		u64_stats_add(&mhi_ep_netdev->stats.tx_bytes, skb->len);
+		u64_stats_update_end(&mhi_ep_netdev->stats.tx_syncp);
+
+		/* Check if queue is empty */
+		if (mhi_ep_queue_is_empty(mdev, DMA_FROM_DEVICE)) {
+			netif_stop_queue(mhi_ep_netdev->ndev);
+			break;
+		}
+
+		consume_skb(skb);
+		cond_resched();
+	}
+
+	return;
+
+exit_drop:
+	u64_stats_update_begin(&mhi_ep_netdev->stats.tx_syncp);
+	u64_stats_inc(&mhi_ep_netdev->stats.tx_dropped);
+	u64_stats_update_end(&mhi_ep_netdev->stats.tx_syncp);
+}
+
+static int mhi_ndo_open(struct net_device *ndev)
+{
+	netif_carrier_on(ndev);
+	netif_start_queue(ndev);
+
+	return 0;
+}
+
+static int mhi_ndo_stop(struct net_device *ndev)
+{
+	netif_stop_queue(ndev);
+	netif_carrier_off(ndev);
+
+	return 0;
+}
+
+static netdev_tx_t mhi_ndo_xmit(struct sk_buff *skb, struct net_device *ndev)
+{
+	struct mhi_ep_net_dev *mhi_ep_netdev = netdev_priv(ndev);
+
+	spin_lock(&mhi_ep_netdev->tx_lock);
+	skb_queue_tail(&mhi_ep_netdev->tx_buffers, skb);
+	spin_unlock(&mhi_ep_netdev->tx_lock);
+
+	queue_work(mhi_ep_netdev->xmit_wq, &mhi_ep_netdev->xmit_work);
+
+	return NETDEV_TX_OK;
+}
+
+static void mhi_ndo_get_stats64(struct net_device *ndev,
+				struct rtnl_link_stats64 *stats)
+{
+	struct mhi_ep_net_dev *mhi_ep_netdev = netdev_priv(ndev);
+	unsigned int start;
+
+	do {
+		start = u64_stats_fetch_begin(&mhi_ep_netdev->stats.rx_syncp);
+		stats->rx_packets = u64_stats_read(&mhi_ep_netdev->stats.rx_packets);
+		stats->rx_bytes = u64_stats_read(&mhi_ep_netdev->stats.rx_bytes);
+		stats->rx_errors = u64_stats_read(&mhi_ep_netdev->stats.rx_errors);
+	} while (u64_stats_fetch_retry(&mhi_ep_netdev->stats.rx_syncp, start));
+
+	do {
+		start = u64_stats_fetch_begin(&mhi_ep_netdev->stats.tx_syncp);
+		stats->tx_packets = u64_stats_read(&mhi_ep_netdev->stats.tx_packets);
+		stats->tx_bytes = u64_stats_read(&mhi_ep_netdev->stats.tx_bytes);
+		stats->tx_errors = u64_stats_read(&mhi_ep_netdev->stats.tx_errors);
+		stats->tx_dropped = u64_stats_read(&mhi_ep_netdev->stats.tx_dropped);
+	} while (u64_stats_fetch_retry(&mhi_ep_netdev->stats.tx_syncp, start));
+}
+
+static const struct net_device_ops mhi_ep_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_ep_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_ep_netdev_ops;
+	ndev->mtu = MHI_EP_DEFAULT_MTU;
+	ndev->min_mtu = MHI_NET_MIN_MTU;
+	ndev->max_mtu = MHI_NET_MAX_MTU;
+	ndev->tx_queue_len = 1000;
+}
+
+static void mhi_ep_net_ul_callback(struct mhi_ep_device *mhi_dev,
+				   struct mhi_result *mhi_res)
+{
+	struct mhi_ep_net_dev *mhi_ep_netdev = dev_get_drvdata(&mhi_dev->dev);
+	struct net_device *ndev = mhi_ep_netdev->ndev;
+	struct sk_buff *skb;
+	size_t size;
+
+	size = mhi_ep_netdev->mru ? mhi_ep_netdev->mru : READ_ONCE(ndev->mtu);
+
+	skb = netdev_alloc_skb(ndev, size);
+	if (unlikely(!skb)) {
+		u64_stats_update_begin(&mhi_ep_netdev->stats.rx_syncp);
+		u64_stats_inc(&mhi_ep_netdev->stats.rx_errors);
+		u64_stats_update_end(&mhi_ep_netdev->stats.rx_syncp);
+		return;
+	}
+
+	skb_copy_to_linear_data(skb, mhi_res->buf_addr, mhi_res->bytes_xferd);
+	skb->len = mhi_res->bytes_xferd;
+	skb->dev = mhi_ep_netdev->ndev;
+
+	if (unlikely(mhi_res->transaction_status)) {
+		switch (mhi_res->transaction_status) {
+		case -ENOTCONN:
+			/* MHI layer stopping/resetting the UL channel */
+			dev_kfree_skb_any(skb);
+			return;
+		default:
+			/* Unknown error, simply drop */
+			dev_kfree_skb_any(skb);
+			u64_stats_update_begin(&mhi_ep_netdev->stats.rx_syncp);
+			u64_stats_inc(&mhi_ep_netdev->stats.rx_errors);
+			u64_stats_update_end(&mhi_ep_netdev->stats.rx_syncp);
+		}
+	} else {
+		skb_put(skb, mhi_res->bytes_xferd);
+
+		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;
+		}
+
+		u64_stats_update_begin(&mhi_ep_netdev->stats.rx_syncp);
+		u64_stats_inc(&mhi_ep_netdev->stats.rx_packets);
+		u64_stats_add(&mhi_ep_netdev->stats.rx_bytes, skb->len);
+		u64_stats_update_end(&mhi_ep_netdev->stats.rx_syncp);
+		netif_rx(skb);
+	}
+}
+
+static void mhi_ep_net_dl_callback(struct mhi_ep_device *mhi_dev,
+				   struct mhi_result *mhi_res)
+{
+	struct mhi_ep_net_dev *mhi_ep_netdev = dev_get_drvdata(&mhi_dev->dev);
+
+	if (unlikely(mhi_res->transaction_status == -ENOTCONN))
+		return;
+
+	/* Since we got enough buffers to queue, wake the queue if stopped */
+	if (netif_queue_stopped(mhi_ep_netdev->ndev)) {
+		netif_wake_queue(mhi_ep_netdev->ndev);
+		queue_work(mhi_ep_netdev->xmit_wq, &mhi_ep_netdev->xmit_work);
+	}
+}
+
+static int mhi_ep_net_newlink(struct mhi_ep_device *mhi_dev, struct net_device *ndev)
+{
+	struct mhi_ep_net_dev *mhi_ep_netdev;
+	int ret;
+
+	mhi_ep_netdev = netdev_priv(ndev);
+
+	dev_set_drvdata(&mhi_dev->dev, mhi_ep_netdev);
+	mhi_ep_netdev->ndev = ndev;
+	mhi_ep_netdev->mdev = mhi_dev;
+	mhi_ep_netdev->mru = mhi_dev->mhi_cntrl->mru;
+
+	skb_queue_head_init(&mhi_ep_netdev->tx_buffers);
+	spin_lock_init(&mhi_ep_netdev->tx_lock);
+
+	u64_stats_init(&mhi_ep_netdev->stats.rx_syncp);
+	u64_stats_init(&mhi_ep_netdev->stats.tx_syncp);
+
+	mhi_ep_netdev->xmit_wq = alloc_workqueue("mhi_ep_net_xmit_wq", 0, WQ_HIGHPRI);
+	INIT_WORK(&mhi_ep_netdev->xmit_work, mhi_ep_net_dev_process_queue_packets);
+
+	ret = register_netdev(ndev);
+	if (ret) {
+		destroy_workqueue(mhi_ep_netdev->xmit_wq);
+		return ret;
+	}
+
+	return 0;
+}
+
+static void mhi_ep_net_dellink(struct mhi_ep_device *mhi_dev, struct net_device *ndev)
+{
+	struct mhi_ep_net_dev *mhi_ep_netdev = netdev_priv(ndev);
+
+	destroy_workqueue(mhi_ep_netdev->xmit_wq);
+	unregister_netdev(ndev);
+	free_netdev(ndev);
+	dev_set_drvdata(&mhi_dev->dev, NULL);
+}
+
+static int mhi_ep_net_probe(struct mhi_ep_device *mhi_dev, const struct mhi_device_id *id)
+{
+	struct net_device *ndev;
+	int ret;
+
+	ndev = alloc_netdev(sizeof(struct mhi_ep_net_dev), (const char *)id->driver_data,
+			    NET_NAME_PREDICTABLE, mhi_ep_net_setup);
+	if (!ndev)
+		return -ENOMEM;
+
+	SET_NETDEV_DEV(ndev, &mhi_dev->dev);
+
+	ret = mhi_ep_net_newlink(mhi_dev, ndev);
+	if (ret) {
+		free_netdev(ndev);
+		return ret;
+	}
+
+	return 0;
+}
+
+static void mhi_ep_net_remove(struct mhi_ep_device *mhi_dev)
+{
+	struct mhi_ep_net_dev *mhi_ep_netdev = dev_get_drvdata(&mhi_dev->dev);
+
+	mhi_ep_net_dellink(mhi_dev, mhi_ep_netdev->ndev);
+}
+
+static const struct mhi_device_id mhi_ep_net_id_table[] = {
+	/* Software data PATH (from endpoint CPU) */
+	{ .chan = "IP_SW0", .driver_data = (kernel_ulong_t)"mhi_swip%d" },
+	{}
+};
+MODULE_DEVICE_TABLE(mhi, mhi_ep_net_id_table);
+
+static struct mhi_ep_driver mhi_ep_net_driver = {
+	.probe = mhi_ep_net_probe,
+	.remove = mhi_ep_net_remove,
+	.dl_xfer_cb = mhi_ep_net_dl_callback,
+	.ul_xfer_cb = mhi_ep_net_ul_callback,
+	.id_table = mhi_ep_net_id_table,
+	.driver = {
+		.name = "mhi_ep_net",
+		.owner = THIS_MODULE,
+	},
+};
+
+module_mhi_ep_driver(mhi_ep_net_driver);
+
+MODULE_AUTHOR("Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>");
+MODULE_DESCRIPTION("MHI Endpoint Network driver");
+MODULE_LICENSE("GPL");
-- 
2.25.1

[PATCH 2/3] MAINTAINERS: Add entry for MHI networking drivers under MHI bus

[Manivannan Sadhasivam]

The host MHI net driver was not listed earlier. So let's add both host and
endpoint MHI net drivers under MHI bus.

Cc: Loic Poulain <loic.poulain@linaro.org>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 081eb65ef865..14d9e15ae360 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -13632,6 +13632,7 @@ T:	git git://git.kernel.org/pub/scm/linux/kernel/git/mani/mhi.git
 F:	Documentation/ABI/stable/sysfs-bus-mhi
 F:	Documentation/mhi/
 F:	drivers/bus/mhi/
+F:	drivers/net/mhi_*
 F:	include/linux/mhi.h
 
 MICROBLAZE ARCHITECTURE
-- 
2.25.1

[PATCH 3/3] net: mhi: Increase the default MTU from 16K to 32K

[Manivannan Sadhasivam]

Most of the Qualcomm endpoint devices are supporting 32K MTU for the
UL (Uplink) and DL (Downlink) channels. So let's use the same value
in the MHI NET driver also. This gives almost 2x increase in the throughput
for the UL channel.

Below is the comparision:

iperf on the UL channel with 16K MTU:

[ ID] Interval       Transfer     Bandwidth
[  3]  0.0-10.0 sec   353 MBytes   296 Mbits/sec

iperf on the UL channel with 32K MTU:

[ ID] Interval       Transfer     Bandwidth
[  3]  0.0-10.0 sec   695 MBytes   583 Mbits/sec

Cc: Loic Poulain <loic.poulain@linaro.org>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/net/mhi_net.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/mhi_net.c b/drivers/net/mhi_net.c
index 3d322ac4f6a5..eddc2c701da4 100644
--- a/drivers/net/mhi_net.c
+++ b/drivers/net/mhi_net.c
@@ -14,7 +14,7 @@
 
 #define MHI_NET_MIN_MTU		ETH_MIN_MTU
 #define MHI_NET_MAX_MTU		0xffff
-#define MHI_NET_DEFAULT_MTU	0x4000
+#define MHI_NET_DEFAULT_MTU	0x8000
 
 struct mhi_net_stats {
 	u64_stats_t rx_packets;
-- 
2.25.1

Re: [PATCH 0/3] Add MHI Endpoint network driver

[Andrew Lunn]

On Tue, Jun 06, 2023 at 06:01:16PM +0530, Manivannan Sadhasivam wrote:
> Hi,
> 
> This series adds a network driver for the Modem Host Interface (MHI) endpoint
> devices that provides network interfaces to the PCIe based Qualcomm endpoint
> devices supporting MHI bus (like Modems). This driver allows the MHI endpoint
> devices to establish IP communication with the host machines (x86, ARM64) over
> MHI bus.
> 
> On the host side, the existing mhi_net driver provides the network connectivity
> to the host.
> 
> - Mani
> 
> Manivannan Sadhasivam (3):
>   net: Add MHI Endpoint network driver
>   MAINTAINERS: Add entry for MHI networking drivers under MHI bus
>   net: mhi: Increase the default MTU from 16K to 32K
> 
>  MAINTAINERS              |   1 +
>  drivers/net/Kconfig      |   9 ++
>  drivers/net/Makefile     |   1 +
>  drivers/net/mhi_ep_net.c | 331 +++++++++++++++++++++++++++++++++++++++
>  drivers/net/mhi_net.c    |   2 +-

Should we add a drivers/net/modem directory? Maybe modem is too
generic, we want something which represents GSM, LTE, UMTS, 3G, 4G,
5G, ... XG etc.

    Andrew

Re: [PATCH 3/3] net: mhi: Increase the default MTU from 16K to 32K

[Jeffrey Hugo]

On 6/6/2023 6:31 AM, Manivannan Sadhasivam wrote:
> Most of the Qualcomm endpoint devices are supporting 32K MTU for the
> UL (Uplink) and DL (Downlink) channels. So let's use the same value
> in the MHI NET driver also. This gives almost 2x increase in the throughput
> for the UL channel.
> 
> Below is the comparision:
> 
> iperf on the UL channel with 16K MTU:
> 
> [ ID] Interval       Transfer     Bandwidth
> [  3]  0.0-10.0 sec   353 MBytes   296 Mbits/sec
> 
> iperf on the UL channel with 32K MTU:
> 
> [ ID] Interval       Transfer     Bandwidth
> [  3]  0.0-10.0 sec   695 MBytes   583 Mbits/sec
> 
> Cc: Loic Poulain <loic.poulain@linaro.org>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>   drivers/net/mhi_net.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/mhi_net.c b/drivers/net/mhi_net.c
> index 3d322ac4f6a5..eddc2c701da4 100644
> --- a/drivers/net/mhi_net.c
> +++ b/drivers/net/mhi_net.c
> @@ -14,7 +14,7 @@
>   
>   #define MHI_NET_MIN_MTU		ETH_MIN_MTU
>   #define MHI_NET_MAX_MTU		0xffff
> -#define MHI_NET_DEFAULT_MTU	0x4000
> +#define MHI_NET_DEFAULT_MTU	0x8000

Why not SZ_32K?

Re: [PATCH 0/3] Add MHI Endpoint network driver

[Jakub Kicinski]

On Tue,  6 Jun 2023 18:01:16 +0530 Manivannan Sadhasivam wrote:
> This series adds a network driver for the Modem Host Interface (MHI) endpoint
> devices that provides network interfaces to the PCIe based Qualcomm endpoint
> devices supporting MHI bus (like Modems). This driver allows the MHI endpoint
> devices to establish IP communication with the host machines (x86, ARM64) over
> MHI bus.
> 
> On the host side, the existing mhi_net driver provides the network connectivity
> to the host.

So the host can talk to the firmware over IP?

Re: [PATCH 0/3] Add MHI Endpoint network driver

[Manivannan Sadhasivam]

On Tue, Jun 06, 2023 at 02:59:00PM +0200, Andrew Lunn wrote:
> On Tue, Jun 06, 2023 at 06:01:16PM +0530, Manivannan Sadhasivam wrote:
> > Hi,
> > 
> > This series adds a network driver for the Modem Host Interface (MHI) endpoint
> > devices that provides network interfaces to the PCIe based Qualcomm endpoint
> > devices supporting MHI bus (like Modems). This driver allows the MHI endpoint
> > devices to establish IP communication with the host machines (x86, ARM64) over
> > MHI bus.
> > 
> > On the host side, the existing mhi_net driver provides the network connectivity
> > to the host.
> > 
> > - Mani
> > 
> > Manivannan Sadhasivam (3):
> >   net: Add MHI Endpoint network driver
> >   MAINTAINERS: Add entry for MHI networking drivers under MHI bus
> >   net: mhi: Increase the default MTU from 16K to 32K
> > 
> >  MAINTAINERS              |   1 +
> >  drivers/net/Kconfig      |   9 ++
> >  drivers/net/Makefile     |   1 +
> >  drivers/net/mhi_ep_net.c | 331 +++++++++++++++++++++++++++++++++++++++
> >  drivers/net/mhi_net.c    |   2 +-
> 
> Should we add a drivers/net/modem directory? Maybe modem is too
> generic, we want something which represents GSM, LTE, UMTS, 3G, 4G,
> 5G, ... XG etc.
> 

The generic modem hierarchy sounds good to me because most of the times a
single driver handles multiple technologies. The existing drivers supporting
modems are already under different hierarchy like usb, wwan etc... So unifying
them makes sense. But someone from networking community should take a call.

- Mani

>     Andrew

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH 3/3] net: mhi: Increase the default MTU from 16K to 32K

[Manivannan Sadhasivam]

On Tue, Jun 06, 2023 at 07:50:23AM -0600, Jeffrey Hugo wrote:
> On 6/6/2023 6:31 AM, Manivannan Sadhasivam wrote:
> > Most of the Qualcomm endpoint devices are supporting 32K MTU for the
> > UL (Uplink) and DL (Downlink) channels. So let's use the same value
> > in the MHI NET driver also. This gives almost 2x increase in the throughput
> > for the UL channel.
> > 
> > Below is the comparision:
> > 
> > iperf on the UL channel with 16K MTU:
> > 
> > [ ID] Interval       Transfer     Bandwidth
> > [  3]  0.0-10.0 sec   353 MBytes   296 Mbits/sec
> > 
> > iperf on the UL channel with 32K MTU:
> > 
> > [ ID] Interval       Transfer     Bandwidth
> > [  3]  0.0-10.0 sec   695 MBytes   583 Mbits/sec
> > 
> > Cc: Loic Poulain <loic.poulain@linaro.org>
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >   drivers/net/mhi_net.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/mhi_net.c b/drivers/net/mhi_net.c
> > index 3d322ac4f6a5..eddc2c701da4 100644
> > --- a/drivers/net/mhi_net.c
> > +++ b/drivers/net/mhi_net.c
> > @@ -14,7 +14,7 @@
> >   #define MHI_NET_MIN_MTU		ETH_MIN_MTU
> >   #define MHI_NET_MAX_MTU		0xffff
> > -#define MHI_NET_DEFAULT_MTU	0x4000
> > +#define MHI_NET_DEFAULT_MTU	0x8000
> 
> Why not SZ_32K?

Makes sense. Will change it in next iteration.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH 0/3] Add MHI Endpoint network driver

[Manivannan Sadhasivam]

On Tue, Jun 06, 2023 at 02:22:27PM -0700, Jakub Kicinski wrote:
> On Tue,  6 Jun 2023 18:01:16 +0530 Manivannan Sadhasivam wrote:
> > This series adds a network driver for the Modem Host Interface (MHI) endpoint
> > devices that provides network interfaces to the PCIe based Qualcomm endpoint
> > devices supporting MHI bus (like Modems). This driver allows the MHI endpoint
> > devices to establish IP communication with the host machines (x86, ARM64) over
> > MHI bus.
> > 
> > On the host side, the existing mhi_net driver provides the network connectivity
> > to the host.
> 
> So the host can talk to the firmware over IP?

That's the typical usecase of these PCIe based modems. On the host, mhi_net
driver creates the network interface that communicates with the endpoint over
MHI host stack. On the endpoint, mhi_ep_net driver creates the network interface
that communicates with the host over MHI endpoint stack.

These drivers work on top of MHI channels like IP_SW0, IP_HW0 etc... IP_SW0
channel represents the IP communication between host and modem CPUs while IP_HW0
represents IP communication between host and modem DSP.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH 0/3] Add MHI Endpoint network driver

[Loic Poulain]

On Wed, 7 Jun 2023 at 08:56, Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> wrote:
>
> On Tue, Jun 06, 2023 at 02:59:00PM +0200, Andrew Lunn wrote:
> > On Tue, Jun 06, 2023 at 06:01:16PM +0530, Manivannan Sadhasivam wrote:
> > > Hi,
> > >
> > > This series adds a network driver for the Modem Host Interface (MHI) endpoint
> > > devices that provides network interfaces to the PCIe based Qualcomm endpoint
> > > devices supporting MHI bus (like Modems). This driver allows the MHI endpoint
> > > devices to establish IP communication with the host machines (x86, ARM64) over
> > > MHI bus.
> > >
> > > On the host side, the existing mhi_net driver provides the network connectivity
> > > to the host.
> > >
> > > - Mani
> > >
> > > Manivannan Sadhasivam (3):
> > >   net: Add MHI Endpoint network driver
> > >   MAINTAINERS: Add entry for MHI networking drivers under MHI bus
> > >   net: mhi: Increase the default MTU from 16K to 32K
> > >
> > >  MAINTAINERS              |   1 +
> > >  drivers/net/Kconfig      |   9 ++
> > >  drivers/net/Makefile     |   1 +
> > >  drivers/net/mhi_ep_net.c | 331 +++++++++++++++++++++++++++++++++++++++
> > >  drivers/net/mhi_net.c    |   2 +-
> >
> > Should we add a drivers/net/modem directory? Maybe modem is too
> > generic, we want something which represents GSM, LTE, UMTS, 3G, 4G,
> > 5G, ... XG etc.
> >
>
> The generic modem hierarchy sounds good to me because most of the times a
> single driver handles multiple technologies. The existing drivers supporting
> modems are already under different hierarchy like usb, wwan etc... So unifying
> them makes sense. But someone from networking community should take a call.


Yes, so there is already a drivers/net/wwan directory for this, in
which there are drivers for control and data path, that together
represent a given 'wwan' (modem) entity. So the generic mhi_net could
be moved there, but the point is AFAIU, that MHI, despite his name, is
not (more) used only for modem, but as a generic memory sharing based
transport protocol, such as virtio. It would then not be necessarily
true that a peripheral exposing MHI net channel is actually a modem?

Regards,
Loic

Re: [PATCH 0/3] Add MHI Endpoint network driver

[Manivannan Sadhasivam]

On Wed, Jun 07, 2023 at 09:12:00AM +0200, Loic Poulain wrote:
> On Wed, 7 Jun 2023 at 08:56, Manivannan Sadhasivam
> <manivannan.sadhasivam@linaro.org> wrote:
> >
> > On Tue, Jun 06, 2023 at 02:59:00PM +0200, Andrew Lunn wrote:
> > > On Tue, Jun 06, 2023 at 06:01:16PM +0530, Manivannan Sadhasivam wrote:
> > > > Hi,
> > > >
> > > > This series adds a network driver for the Modem Host Interface (MHI) endpoint
> > > > devices that provides network interfaces to the PCIe based Qualcomm endpoint
> > > > devices supporting MHI bus (like Modems). This driver allows the MHI endpoint
> > > > devices to establish IP communication with the host machines (x86, ARM64) over
> > > > MHI bus.
> > > >
> > > > On the host side, the existing mhi_net driver provides the network connectivity
> > > > to the host.
> > > >
> > > > - Mani
> > > >
> > > > Manivannan Sadhasivam (3):
> > > >   net: Add MHI Endpoint network driver
> > > >   MAINTAINERS: Add entry for MHI networking drivers under MHI bus
> > > >   net: mhi: Increase the default MTU from 16K to 32K
> > > >
> > > >  MAINTAINERS              |   1 +
> > > >  drivers/net/Kconfig      |   9 ++
> > > >  drivers/net/Makefile     |   1 +
> > > >  drivers/net/mhi_ep_net.c | 331 +++++++++++++++++++++++++++++++++++++++
> > > >  drivers/net/mhi_net.c    |   2 +-
> > >
> > > Should we add a drivers/net/modem directory? Maybe modem is too
> > > generic, we want something which represents GSM, LTE, UMTS, 3G, 4G,
> > > 5G, ... XG etc.
> > >
> >
> > The generic modem hierarchy sounds good to me because most of the times a
> > single driver handles multiple technologies. The existing drivers supporting
> > modems are already under different hierarchy like usb, wwan etc... So unifying
> > them makes sense. But someone from networking community should take a call.
> 
> 
> Yes, so there is already a drivers/net/wwan directory for this, in
> which there are drivers for control and data path, that together
> represent a given 'wwan' (modem) entity. So the generic mhi_net could
> be moved there, but the point is AFAIU, that MHI, despite his name, is
> not (more) used only for modem, but as a generic memory sharing based
> transport protocol, such as virtio. It would then not be necessarily
> true that a peripheral exposing MHI net channel is actually a modem?
> 

Agree, mhi_*_net drivers can be used by non-modem devices too as long as they
support MHI protocol.

- Mani

> Regards,
> Loic

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH 1/3] net: Add MHI Endpoint network driver

[Simon Horman]

On Tue, Jun 06, 2023 at 06:01:17PM +0530, Manivannan Sadhasivam wrote:

...

> +static void mhi_ep_net_dev_process_queue_packets(struct work_struct *work)
> +{
> +	struct mhi_ep_net_dev *mhi_ep_netdev = container_of(work,
> +			struct mhi_ep_net_dev, xmit_work);
> +	struct mhi_ep_device *mdev = mhi_ep_netdev->mdev;
> +	struct sk_buff_head q;
> +	struct sk_buff *skb;
> +	int ret;
> +
> +	if (mhi_ep_queue_is_empty(mdev, DMA_FROM_DEVICE)) {
> +		netif_stop_queue(mhi_ep_netdev->ndev);
> +		return;
> +	}
> +
> +	__skb_queue_head_init(&q);
> +
> +	spin_lock_bh(&mhi_ep_netdev->tx_lock);
> +	skb_queue_splice_init(&mhi_ep_netdev->tx_buffers, &q);
> +	spin_unlock_bh(&mhi_ep_netdev->tx_lock);
> +
> +	while ((skb = __skb_dequeue(&q))) {
> +		ret = mhi_ep_queue_skb(mdev, skb);
> +		if (ret) {

Hi Manivannan,

I wonder if this should be kfree_skb(skb);

> +			kfree(skb);
> +			goto exit_drop;
> +		}

...

Re: [PATCH 3/3] net: mhi: Increase the default MTU from 16K to 32K

[Simon Horman]

On Tue, Jun 06, 2023 at 06:01:19PM +0530, Manivannan Sadhasivam wrote:
> Most of the Qualcomm endpoint devices are supporting 32K MTU for the
> UL (Uplink) and DL (Downlink) channels. So let's use the same value
> in the MHI NET driver also. This gives almost 2x increase in the throughput
> for the UL channel.
> 
> Below is the comparision:

Hi Manivannan,

as it looks like there will be a v2: comparision -> comparison

...

Re: [PATCH 1/3] net: Add MHI Endpoint network driver

[Manivannan Sadhasivam]

On Wed, Jun 07, 2023 at 10:20:39AM +0200, Simon Horman wrote:
> On Tue, Jun 06, 2023 at 06:01:17PM +0530, Manivannan Sadhasivam wrote:
> 
> ...
> 
> > +static void mhi_ep_net_dev_process_queue_packets(struct work_struct *work)
> > +{
> > +	struct mhi_ep_net_dev *mhi_ep_netdev = container_of(work,
> > +			struct mhi_ep_net_dev, xmit_work);
> > +	struct mhi_ep_device *mdev = mhi_ep_netdev->mdev;
> > +	struct sk_buff_head q;
> > +	struct sk_buff *skb;
> > +	int ret;
> > +
> > +	if (mhi_ep_queue_is_empty(mdev, DMA_FROM_DEVICE)) {
> > +		netif_stop_queue(mhi_ep_netdev->ndev);
> > +		return;
> > +	}
> > +
> > +	__skb_queue_head_init(&q);
> > +
> > +	spin_lock_bh(&mhi_ep_netdev->tx_lock);
> > +	skb_queue_splice_init(&mhi_ep_netdev->tx_buffers, &q);
> > +	spin_unlock_bh(&mhi_ep_netdev->tx_lock);
> > +
> > +	while ((skb = __skb_dequeue(&q))) {
> > +		ret = mhi_ep_queue_skb(mdev, skb);
> > +		if (ret) {
> 
> Hi Manivannan,
> 
> I wonder if this should be kfree_skb(skb);
> 

Good catch! Will fix it.

- Mani

> > +			kfree(skb);
> > +			goto exit_drop;
> > +		}
> 
> ...

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH 3/3] net: mhi: Increase the default MTU from 16K to 32K

[Andrew Lunn]

On Wed, Jun 07, 2023 at 12:28:09PM +0530, Manivannan Sadhasivam wrote:
> On Tue, Jun 06, 2023 at 07:50:23AM -0600, Jeffrey Hugo wrote:
> > On 6/6/2023 6:31 AM, Manivannan Sadhasivam wrote:
> > > Most of the Qualcomm endpoint devices are supporting 32K MTU for the
> > > UL (Uplink) and DL (Downlink) channels. So let's use the same value
> > > in the MHI NET driver also. This gives almost 2x increase in the throughput
> > > for the UL channel.

You say here 'Most'. What happens on those which do not support 32K?
Do the packets get dropped and it turns into a black hole?

   Andrew

Re: [PATCH 0/3] Add MHI Endpoint network driver

[Andrew Lunn]

> > Yes, so there is already a drivers/net/wwan directory for this, in
> > which there are drivers for control and data path, that together
> > represent a given 'wwan' (modem) entity. So the generic mhi_net could
> > be moved there, but the point is AFAIU, that MHI, despite his name, is
> > not (more) used only for modem, but as a generic memory sharing based
> > transport protocol, such as virtio. It would then not be necessarily
> > true that a peripheral exposing MHI net channel is actually a modem?
> > 
> 
> Agree, mhi_*_net drivers can be used by non-modem devices too as long as they
> support MHI protocol.

O.K. I was just trying to avoid cluttering up the directory. But if
this is shared code, not actual drivers, this is fine.

Are there more features yet to be implemented? Would it make sense to
create a mhi directory?

       Andrew

Re: [PATCH 0/3] Add MHI Endpoint network driver

[Jeffrey Hugo]

On 6/7/2023 1:41 AM, Manivannan Sadhasivam wrote:
> On Wed, Jun 07, 2023 at 09:12:00AM +0200, Loic Poulain wrote:
>> On Wed, 7 Jun 2023 at 08:56, Manivannan Sadhasivam
>> <manivannan.sadhasivam@linaro.org> wrote:
>>>
>>> On Tue, Jun 06, 2023 at 02:59:00PM +0200, Andrew Lunn wrote:
>>>> On Tue, Jun 06, 2023 at 06:01:16PM +0530, Manivannan Sadhasivam wrote:
>>>>> Hi,
>>>>>
>>>>> This series adds a network driver for the Modem Host Interface (MHI) endpoint
>>>>> devices that provides network interfaces to the PCIe based Qualcomm endpoint
>>>>> devices supporting MHI bus (like Modems). This driver allows the MHI endpoint
>>>>> devices to establish IP communication with the host machines (x86, ARM64) over
>>>>> MHI bus.
>>>>>
>>>>> On the host side, the existing mhi_net driver provides the network connectivity
>>>>> to the host.
>>>>>
>>>>> - Mani
>>>>>
>>>>> Manivannan Sadhasivam (3):
>>>>>    net: Add MHI Endpoint network driver
>>>>>    MAINTAINERS: Add entry for MHI networking drivers under MHI bus
>>>>>    net: mhi: Increase the default MTU from 16K to 32K
>>>>>
>>>>>   MAINTAINERS              |   1 +
>>>>>   drivers/net/Kconfig      |   9 ++
>>>>>   drivers/net/Makefile     |   1 +
>>>>>   drivers/net/mhi_ep_net.c | 331 +++++++++++++++++++++++++++++++++++++++
>>>>>   drivers/net/mhi_net.c    |   2 +-
>>>>
>>>> Should we add a drivers/net/modem directory? Maybe modem is too
>>>> generic, we want something which represents GSM, LTE, UMTS, 3G, 4G,
>>>> 5G, ... XG etc.
>>>>
>>>
>>> The generic modem hierarchy sounds good to me because most of the times a
>>> single driver handles multiple technologies. The existing drivers supporting
>>> modems are already under different hierarchy like usb, wwan etc... So unifying
>>> them makes sense. But someone from networking community should take a call.
>>
>>
>> Yes, so there is already a drivers/net/wwan directory for this, in
>> which there are drivers for control and data path, that together
>> represent a given 'wwan' (modem) entity. So the generic mhi_net could
>> be moved there, but the point is AFAIU, that MHI, despite his name, is
>> not (more) used only for modem, but as a generic memory sharing based
>> transport protocol, such as virtio. It would then not be necessarily
>> true that a peripheral exposing MHI net channel is actually a modem?
>>
> 
> Agree, mhi_*_net drivers can be used by non-modem devices too as long as they
> support MHI protocol.

I know of at-least 1 non-modem product in development that would benefit 
from these drivers.

Re: [PATCH 3/3] net: mhi: Increase the default MTU from 16K to 32K

[Manivannan Sadhasivam]

On Wed, Jun 07, 2023 at 02:25:50PM +0200, Andrew Lunn wrote:
> On Wed, Jun 07, 2023 at 12:28:09PM +0530, Manivannan Sadhasivam wrote:
> > On Tue, Jun 06, 2023 at 07:50:23AM -0600, Jeffrey Hugo wrote:
> > > On 6/6/2023 6:31 AM, Manivannan Sadhasivam wrote:
> > > > Most of the Qualcomm endpoint devices are supporting 32K MTU for the
> > > > UL (Uplink) and DL (Downlink) channels. So let's use the same value
> > > > in the MHI NET driver also. This gives almost 2x increase in the throughput
> > > > for the UL channel.
> 
> You say here 'Most'. What happens on those which do not support 32K?
> Do the packets get dropped and it turns into a black hole?
> 

Yeah, and the host has to retransmit. But I checked again with Qcom on the MTU
size and got a different answer that forced me to change "most" to "few". So
this patch is not needed for now. I'll drop it.

- Mani

>    Andrew
> 

-- 
மணிவண்ணன் சதாசிவம்