[PATCH 0/3] caif: CAIF for CDC NCM USB

[PATCH 0/3] caif: CAIF for CDC NCM USB

[Sjur Brændeland]

The following patches enables sending and receive CAIF packets
over a cdc_ncm usb interface.

The patches has been tested on real HW and we have been running with this
for more than 6 months now.

NCM 1.0 does not support anything but Ethernet framing, hence
CAIF payload will be put into Ethernet frames. This it's a little
quirky. The long term solution for NCM should be based on
MBIM 1.0 specification.

Thanks,
Sjur Brændeland


Sjur Brændeland (3):
  if_ether.h: Add IEEE 802.1 Local Experimental Ethertype 1.
  caif: Add support for flow-control on device's tx-queue
  caif: Add support for CAIF over CDC NCM USB interface

 include/linux/if_ether.h  |    1 +
 include/net/caif/cfusbl.h |   13 +++
 net/caif/Kconfig          |   11 +++
 net/caif/Makefile         |    4 +-
 net/caif/caif_dev.c       |   55 +++++++++++++
 net/caif/cfusbl.c         |  199 +++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 282 insertions(+), 1 deletions(-)
 create mode 100644 include/net/caif/cfusbl.h
 create mode 100644 net/caif/cfusbl.c

[PATCH 1/3] if_ether.h: Add IEEE 802.1 Local Experimental Ethertype 1.

[Sjur Brændeland]

Add EthType 0x88b5.
This Ethertype value is available for public use for prototype and
vendor-specific protocol development,as defined in Amendment 802a
to IEEE Std 802.

Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
---
 include/linux/if_ether.h |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/include/linux/if_ether.h b/include/linux/if_ether.h
index e473003..56d907a 100644
--- a/include/linux/if_ether.h
+++ b/include/linux/if_ether.h
@@ -79,6 +79,7 @@
 #define ETH_P_PAE	0x888E		/* Port Access Entity (IEEE 802.1X) */
 #define ETH_P_AOE	0x88A2		/* ATA over Ethernet		*/
 #define ETH_P_8021AD	0x88A8          /* 802.1ad Service VLAN		*/
+#define ETH_P_802_EX1	0x88B5		/* 802.1 Local Experimental 1.  */
 #define ETH_P_TIPC	0x88CA		/* TIPC 			*/
 #define ETH_P_8021AH	0x88E7          /* 802.1ah Backbone Service Tag */
 #define ETH_P_1588	0x88F7		/* IEEE 1588 Timesync */
-- 
1.7.0.4

[PATCH 2/3] caif: Add support for flow-control on device's tx-queue

[Sjur Brændeland]

Flow control is implemented by inspecting the qdisc queue length
in order to detect potential overflow on the TX queue. When a threshold
is reached flow-off is sent upwards in the CAIF stack. At the same time
the skb->destructor is hi-jacked in order to detect when the last packet
put on queue is consumed. When this "hi-jacked" packet is consumed, flow-on
is sent upwards in the CAIF stack.

Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
---
 net/caif/caif_dev.c |   48 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 48 insertions(+), 0 deletions(-)

diff --git a/net/caif/caif_dev.c b/net/caif/caif_dev.c
index f7e8c70..415353e 100644
--- a/net/caif/caif_dev.c
+++ b/net/caif/caif_dev.c
@@ -34,6 +34,7 @@ struct caif_device_entry {
 	struct list_head list;
 	struct net_device *netdev;
 	int __percpu *pcpu_refcnt;
+	bool xoff;
 };
 
 struct caif_device_entry_list {
@@ -48,6 +49,7 @@ struct caif_net {
 };
 
 static int caif_net_id;
+static int q_high = 50; /* Percent */
 
 struct cfcnfg *get_cfcnfg(struct net *net)
 {
@@ -126,9 +128,28 @@ static struct caif_device_entry *caif_get(struct net_device *dev)
 	return NULL;
 }
 
+void caif_flow_cb(struct sk_buff *skb)
+{
+	struct caif_device_entry *caifd;
+	WARN_ON(skb->dev == NULL);
+
+	rcu_read_lock();
+	caifd = caif_get(skb->dev);
+	caifd->xoff = 0;
+	caifd_hold(caifd);
+	rcu_read_unlock();
+
+	caifd->layer.up->
+		ctrlcmd(caifd->layer.up,
+			_CAIF_CTRLCMD_PHYIF_FLOW_ON_IND,
+			caifd->layer.id);
+	caifd_put(caifd);
+}
+
 static int transmit(struct cflayer *layer, struct cfpkt *pkt)
 {
 	int err;
+	struct caif_dev_common *caifdev;
 	struct caif_device_entry *caifd =
 	    container_of(layer, struct caif_device_entry, layer);
 	struct sk_buff *skb;
@@ -137,6 +158,33 @@ static int transmit(struct cflayer *layer, struct cfpkt *pkt)
 	skb->dev = caifd->netdev;
 	skb_reset_network_header(skb);
 	skb->protocol = htons(ETH_P_CAIF);
+	caifdev = netdev_priv(caifd->netdev);
+
+	if (caifdev->flowctrl == NULL && caifd->netdev->tx_queue_len > 0 &&
+			!caifd->xoff) {
+		struct netdev_queue *txq;
+		int high;
+
+		txq = netdev_get_tx_queue(skb->dev, 0);
+		high = (caifd->netdev->tx_queue_len * q_high) / 100;
+
+		/* If we run with a TX queue, check if the queue is too long*/
+		if (netif_queue_stopped(caifd->netdev) ||
+			qdisc_qlen(txq->qdisc) > high) {
+
+			pr_debug("queue stop(%d) or full (%d>%d) - XOFF\n",
+					netif_queue_stopped(caifd->netdev),
+					qdisc_qlen(txq->qdisc), high);
+			caifd->xoff = 1;
+			/* Hijack this skb free callback function. */
+			skb_orphan(skb);
+			skb->destructor = caif_flow_cb;
+			caifd->layer.up->
+				ctrlcmd(caifd->layer.up,
+					_CAIF_CTRLCMD_PHYIF_FLOW_OFF_IND,
+					caifd->layer.id);
+		}
+	}
 
 	err = dev_queue_xmit(skb);
 	if (err > 0)
-- 
1.7.0.4

[PATCH 3/3] caif: Add support for CAIF over CDC NCM USB interface

[Sjur Brændeland]

NCM 1.0 does not support anything but Ethernet framing, hence
CAIF payload will be put into Ethernet frames.

Discovery is based on fixed USB vendor 0x04cc (ST-Ericsson),
product-id 0x230f (NCM). In this variant only CAIF payload is sent over
the NCM interface.

The CAIF stack (cfusbl.c) will when USB registers first check if
we got a CDC NCM USB interface with the right VID, PID.
It will then read the device's Ethernet address and create a 'template'
Ethernet TX header, using a broadcast address as the destination address,
and EthType 0x88b5 (802.1 Local Experimental - vendor specific).

A protocol handler for 0x88b5 is setup for reception of CAIF frames from
the CDC NCM USB interface.

Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
---
 include/net/caif/cfusbl.h |   13 +++
 net/caif/Kconfig          |   11 +++
 net/caif/Makefile         |    4 +-
 net/caif/caif_dev.c       |    7 ++
 net/caif/cfusbl.c         |  199 +++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 233 insertions(+), 1 deletions(-)
 create mode 100644 include/net/caif/cfusbl.h
 create mode 100644 net/caif/cfusbl.c

diff --git a/include/net/caif/cfusbl.h b/include/net/caif/cfusbl.h
new file mode 100644
index 0000000..7ef2499
--- /dev/null
+++ b/include/net/caif/cfusbl.h
@@ -0,0 +1,13 @@
+/*
+ * Copyright (C) ST-Ericsson AB 2010
+ * Author:	Sjur Brendeland/sjur.brandeland@stericsson.com
+ * License terms: GNU General Public License (GPL) version 2
+ */
+
+#ifndef CFUSBL_H_
+#define CFUSBL_H_
+
+void cfusbl_init(void);
+void cfusbl_exit(void);
+
+#endif				/* CFUSBL_H_ */
diff --git a/net/caif/Kconfig b/net/caif/Kconfig
index 529750d..43cd844 100644
--- a/net/caif/Kconfig
+++ b/net/caif/Kconfig
@@ -40,3 +40,14 @@ config CAIF_NETDEV
 	If you select to build it as a built-in then the main CAIF device must
 	also be a built-in.
 	If unsure say Y.
+
+config CAIF_USB
+	bool "CAIF USB support"
+	depends on CAIF
+	default CAIF
+	---help---
+	Say Y if you are using CAIF over USB CDC NCM.
+	This can be either built-in or a loadable module,
+	If you select to build it as a built-in then the main CAIF device must
+	also be a built-in.
+	If unsure say N.
diff --git a/net/caif/Makefile b/net/caif/Makefile
index ebcd4e7..3965f73 100644
--- a/net/caif/Makefile
+++ b/net/caif/Makefile
@@ -1,4 +1,5 @@
-ccflags-$(CONFIG_CAIF_DEBUG)     :=      -DDEBUG
+ccflags-$(CONFIG_CAIF_DEBUG) += -DDEBUG
+ccflags-$(CONFIG_CAIF_USB) += -DCAIF_USB
 
 caif-y := caif_dev.o \
 	cfcnfg.o cfmuxl.o cfctrl.o  \
@@ -6,6 +7,7 @@ caif-y := caif_dev.o \
 	cfserl.o cfdgml.o  \
 	cfrfml.o cfvidl.o cfutill.o \
 	cfsrvl.o cfpkt_skbuff.o
+caif-$(CONFIG_CAIF_USB) += cfusbl.o
 
 obj-$(CONFIG_CAIF) += caif.o
 obj-$(CONFIG_CAIF_NETDEV) += chnl_net.o
diff --git a/net/caif/caif_dev.c b/net/caif/caif_dev.c
index 415353e..6ef0e90 100644
--- a/net/caif/caif_dev.c
+++ b/net/caif/caif_dev.c
@@ -25,6 +25,7 @@
 #include <net/caif/cfpkt.h>
 #include <net/caif/cfcnfg.h>
 #include <net/caif/cfserl.h>
+#include <net/caif/cfusbl.h>
 
 MODULE_LICENSE("GPL");
 
@@ -506,6 +507,9 @@ static int __init caif_device_init(void)
 	if (result)
 		return result;
 
+#ifdef CAIF_USB
+	cfusbl_init();
+#endif
 	register_netdevice_notifier(&caif_device_notifier);
 	dev_add_pack(&caif_packet_type);
 
@@ -517,6 +521,9 @@ static void __exit caif_device_exit(void)
 	unregister_pernet_device(&caif_net_ops);
 	unregister_netdevice_notifier(&caif_device_notifier);
 	dev_remove_pack(&caif_packet_type);
+#ifdef CAIF_USB
+	cfusbl_exit();
+#endif
 }
 
 module_init(caif_device_init);
diff --git a/net/caif/cfusbl.c b/net/caif/cfusbl.c
new file mode 100644
index 0000000..8353a55
--- /dev/null
+++ b/net/caif/cfusbl.c
@@ -0,0 +1,199 @@
+/*
+ * CAIF USB handler
+ * Copyright (C) ST-Ericsson AB 2011
+ * Author:	Sjur Brendeland/sjur.brandeland@stericsson.com
+ * License terms: GNU General Public License (GPL) version 2
+ *
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ":%s(): " fmt, __func__
+
+#include <linux/netdevice.h>
+#include <linux/slab.h>
+#include <linux/netdevice.h>
+#include <linux/mii.h>
+#include <linux/usb.h>
+#include <linux/usb/usbnet.h>
+#include <net/netns/generic.h>
+#include <net/caif/caif_dev.h>
+#include <net/caif/caif_layer.h>
+#include <net/caif/cfpkt.h>
+#include <net/caif/cfcnfg.h>
+
+MODULE_LICENSE("GPL");
+
+#define CFUSB_PAD_DESCR_SZ 1	/* Alignment descriptor length */
+#define CFUSB_ALIGNMENT 4	/* Number of bytes to align. */
+#define CFUSB_MAX_HEADLEN (CFUSB_PAD_DESCR_SZ + CFUSB_ALIGNMENT-1)
+#define STE_USB_VID 0x04cc	/* USB Product ID for ST-Ericsson */
+#define STE_USB_PID_CAIF 0x2306	/* Product id for CAIF Modems */
+
+struct cfusbl {
+	struct cflayer layer;
+	u8 tx_eth_hdr[ETH_HLEN];
+};
+
+static bool pack_added;
+
+static int cfusbl_receive(struct cflayer *layr, struct cfpkt *pkt)
+{
+	u8 hpad;
+
+	/* Remove padding. */
+	cfpkt_extr_head(pkt, &hpad, 1);
+	cfpkt_extr_head(pkt, NULL, hpad);
+	return layr->up->receive(layr->up, pkt);
+}
+
+static int cfusbl_transmit(struct cflayer *layr, struct cfpkt *pkt)
+{
+	struct caif_payload_info *info;
+	u8 hpad;
+	u8 zeros[CFUSB_ALIGNMENT];
+	struct sk_buff *skb;
+	struct cfusbl *usbl = container_of(layr, struct cfusbl, layer);
+
+	skb = cfpkt_tonative(pkt);
+
+	skb_reset_network_header(skb);
+	skb->protocol = htons(ETH_P_IP);
+
+	info = cfpkt_info(pkt);
+	hpad = (info->hdr_len + CFUSB_PAD_DESCR_SZ) & (CFUSB_ALIGNMENT - 1);
+
+	if (skb_headroom(skb) < ETH_HLEN + CFUSB_PAD_DESCR_SZ + hpad) {
+		pr_warn("Headroom to small\n");
+		kfree_skb(skb);
+		return -EIO;
+	}
+	memset(zeros, 0, hpad);
+
+	cfpkt_add_head(pkt, zeros, hpad);
+	cfpkt_add_head(pkt, &hpad, 1);
+	cfpkt_add_head(pkt, usbl->tx_eth_hdr, sizeof(usbl->tx_eth_hdr));
+	return layr->dn->transmit(layr->dn, pkt);
+}
+
+static void cfusbl_ctrlcmd(struct cflayer *layr, enum caif_ctrlcmd ctrl,
+					int phyid)
+{
+	if (layr->up && layr->up->ctrlcmd)
+		layr->up->ctrlcmd(layr->up, ctrl, layr->id);
+}
+
+struct cflayer *cfusbl_create(int phyid, u8 ethaddr[ETH_ALEN],
+					u8 braddr[ETH_ALEN])
+{
+	struct cfusbl *this = kmalloc(sizeof(struct cfusbl), GFP_ATOMIC);
+
+	if (!this) {
+		pr_warn("Out of memory\n");
+		return NULL;
+	}
+	caif_assert(offsetof(struct cfusbl, layer) == 0);
+
+	memset(this, 0, sizeof(struct cflayer));
+	this->layer.receive = cfusbl_receive;
+	this->layer.transmit = cfusbl_transmit;
+	this->layer.ctrlcmd = cfusbl_ctrlcmd;
+	snprintf(this->layer.name, CAIF_LAYER_NAME_SZ, "usb%d", phyid);
+	this->layer.id = phyid;
+
+	/*
+	 * Construct TX ethernet header:
+	 *	0-5	destination address
+	 *	5-11	source address
+	 *	12-13	protocol type
+	 */
+	memcpy(&this->tx_eth_hdr[ETH_ALEN], braddr, ETH_ALEN);
+	memcpy(&this->tx_eth_hdr[ETH_ALEN], ethaddr, ETH_ALEN);
+	this->tx_eth_hdr[12] = cpu_to_be16(ETH_P_802_EX1) & 0xff;
+	this->tx_eth_hdr[13] = (cpu_to_be16(ETH_P_802_EX1) >> 8) & 0xff;
+	pr_debug("caif ethernet TX-header dst:%pM src:%pM type:%02x%02x\n",
+			this->tx_eth_hdr, this->tx_eth_hdr + ETH_ALEN,
+			this->tx_eth_hdr[12], this->tx_eth_hdr[13]);
+
+	return (struct cflayer *) this;
+}
+
+static struct packet_type caif_usb_type __read_mostly = {
+	.type = cpu_to_be16(ETH_P_802_EX1),
+};
+
+static int cfusbl_device_notify(struct notifier_block *me, unsigned long what,
+			      void *arg)
+{
+	struct net_device *dev = arg;
+	struct caif_dev_common common;
+	struct cflayer *layer, *link_support;
+	struct usbnet	*usbnet = netdev_priv(dev);
+	struct usb_device	*usbdev = usbnet->udev;
+	struct ethtool_drvinfo drvinfo;
+
+	if (what != NETDEV_REGISTER)
+		return 0;
+	/*
+	 * Quirks: High-jack ethtool to find if we have a NCM device,
+	 * and find it's VID/PID.
+	 */
+	if (dev->ethtool_ops == NULL || dev->ethtool_ops->get_drvinfo == NULL)
+		return 0;
+
+	dev->ethtool_ops->get_drvinfo(dev, &drvinfo);
+	if (strncmp(drvinfo.driver, "cdc_ncm", 7) != 0)
+		return 0;
+
+	pr_debug("USB CDC NCM device VID:0x%4x PID:0x%4x\n",
+		le16_to_cpu(usbdev->descriptor.idVendor),
+		le16_to_cpu(usbdev->descriptor.idProduct));
+
+	/* Check for VID/PID that supports CAIF */
+	if (!(le16_to_cpu(usbdev->descriptor.idVendor) == STE_USB_VID &&
+		le16_to_cpu(usbdev->descriptor.idProduct) == STE_USB_PID_CAIF))
+		return 0;
+
+	memset(&common, 0, sizeof(common));
+	common.use_frag = false;
+	common.use_fcs = false;
+	common.use_stx = false;
+	common.link_select = CAIF_LINK_HIGH_BANDW;
+	common.flowctrl = NULL;
+
+	link_support = cfusbl_create(dev->ifindex, dev->dev_addr,
+					dev->broadcast);
+	if (!link_support) {
+		pr_warn("Out of memory\n");
+		return -ENOMEM;
+	}
+
+	if (dev->num_tx_queues > 1)
+		pr_warn("USB device uses more than one tx queue\n");
+
+	caif_enroll_dev(dev, &common, link_support, CFUSB_MAX_HEADLEN,
+			&layer, &caif_usb_type.func);
+	if (!pack_added)
+		dev_add_pack(&caif_usb_type);
+	pack_added = 1;
+
+	strncpy(layer->name, dev->name,
+			sizeof(layer->name) - 1);
+	layer->name[sizeof(layer->name) - 1] = 0;
+
+	return 0;
+}
+
+static struct notifier_block caif_device_notifier = {
+	.notifier_call = cfusbl_device_notify,
+	.priority = 0,
+};
+
+void cfusbl_init(void)
+{
+	register_netdevice_notifier(&caif_device_notifier);
+}
+
+void cfusbl_exit(void)
+{
+	unregister_netdevice_notifier(&caif_device_notifier);
+	dev_remove_pack(&caif_usb_type);
+}
-- 
1.7.0.4

Re: [PATCH 2/3] caif: Add support for flow-control on device's tx-queue

[Eric Dumazet]

Le vendredi 02 décembre 2011 à 15:06 +0100, Sjur Brændeland a écrit :
> Flow control is implemented by inspecting the qdisc queue length
> in order to detect potential overflow on the TX queue. When a threshold
> is reached flow-off is sent upwards in the CAIF stack. At the same time
> the skb->destructor is hi-jacked in order to detect when the last packet
> put on queue is consumed. When this "hi-jacked" packet is consumed, flow-on
> is sent upwards in the CAIF stack.
> 
> Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
> ---
>  net/caif/caif_dev.c |   48 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 48 insertions(+), 0 deletions(-)
> 
> diff --git a/net/caif/caif_dev.c b/net/caif/caif_dev.c
> index f7e8c70..415353e 100644
> --- a/net/caif/caif_dev.c
> +++ b/net/caif/caif_dev.c
> @@ -34,6 +34,7 @@ struct caif_device_entry {
>  	struct list_head list;
>  	struct net_device *netdev;
>  	int __percpu *pcpu_refcnt;
> +	bool xoff;
>  };
>  
>  struct caif_device_entry_list {
> @@ -48,6 +49,7 @@ struct caif_net {
>  };
>  
>  static int caif_net_id;
> +static int q_high = 50; /* Percent */
>  
>  struct cfcnfg *get_cfcnfg(struct net *net)
>  {
> @@ -126,9 +128,28 @@ static struct caif_device_entry *caif_get(struct net_device *dev)
>  	return NULL;
>  }
>  
> +void caif_flow_cb(struct sk_buff *skb)
> +{
> +	struct caif_device_entry *caifd;
> +	WARN_ON(skb->dev == NULL);
> +
> +	rcu_read_lock();
> +	caifd = caif_get(skb->dev);
> +	caifd->xoff = 0;
> +	caifd_hold(caifd);
> +	rcu_read_unlock();
> +
> +	caifd->layer.up->
> +		ctrlcmd(caifd->layer.up,
> +			_CAIF_CTRLCMD_PHYIF_FLOW_ON_IND,
> +			caifd->layer.id);
> +	caifd_put(caifd);
> +}
> +
>  static int transmit(struct cflayer *layer, struct cfpkt *pkt)
>  {
>  	int err;
> +	struct caif_dev_common *caifdev;
>  	struct caif_device_entry *caifd =
>  	    container_of(layer, struct caif_device_entry, layer);
>  	struct sk_buff *skb;
> @@ -137,6 +158,33 @@ static int transmit(struct cflayer *layer, struct cfpkt *pkt)
>  	skb->dev = caifd->netdev;
>  	skb_reset_network_header(skb);
>  	skb->protocol = htons(ETH_P_CAIF);
> +	caifdev = netdev_priv(caifd->netdev);
> +
> +	if (caifdev->flowctrl == NULL && caifd->netdev->tx_queue_len > 0 &&
> +			!caifd->xoff) {
> +		struct netdev_queue *txq;
> +		int high;
> +
> +		txq = netdev_get_tx_queue(skb->dev, 0);

Why queue 0 and not another one ?

> +		high = (caifd->netdev->tx_queue_len * q_high) / 100;
> +
> +		/* If we run with a TX queue, check if the queue is too long*/

Are you sure only this cpu can run here ? any lock is held ?

> +		if (netif_queue_stopped(caifd->netdev) ||
> +			qdisc_qlen(txq->qdisc) > high) {
> +
> +			pr_debug("queue stop(%d) or full (%d>%d) - XOFF\n",
> +					netif_queue_stopped(caifd->netdev),
> +					qdisc_qlen(txq->qdisc), high);
> +			caifd->xoff = 1;
> +			/* Hijack this skb free callback function. */
> +			skb_orphan(skb);
> +			skb->destructor = caif_flow_cb;
> +			caifd->layer.up->
> +				ctrlcmd(caifd->layer.up,
> +					_CAIF_CTRLCMD_PHYIF_FLOW_OFF_IND,
> +					caifd->layer.id);
> +		}
> +	}
>  
>  	err = dev_queue_xmit(skb);
>  	if (err > 0)

What prevents dev_queue_xmit() to early orphan skb ?

Re: [PATCH 2/3] caif: Add support for flow-control on device's tx-queue

[Sjur Brændeland]

Hi Eric,

> >  static int transmit(struct cflayer *layer, struct cfpkt *pkt)
> >  {
> >  	int err;
> > +	struct caif_dev_common *caifdev;
> >  	struct caif_device_entry *caifd =
> >  	    container_of(layer, struct caif_device_entry, layer);
> >  	struct sk_buff *skb;
> > @@ -137,6 +158,33 @@ static int transmit(struct cflayer *layer,
> struct cfpkt *pkt)
> >  	skb->dev = caifd->netdev;
> >  	skb_reset_network_header(skb);
> >  	skb->protocol = htons(ETH_P_CAIF);
> > +	caifdev = netdev_priv(caifd->netdev);
> > +
> > +	if (caifdev->flowctrl == NULL && caifd->netdev->tx_queue_len > 0
> &&
> > +			!caifd->xoff) {
> > +		struct netdev_queue *txq;
> > +		int high;
> > +
> > +		txq = netdev_get_tx_queue(skb->dev, 0);
>
> Why queue 0 and not another one ?

The purpose of flow-control here is to try to avoid loosing
"control" traffic such as AT-commands sent to the modem.

So far we have (too my knowledge) never configured
multiple queues for any CAIF interface. So for current
setup just queue 0 should work just fine.

But in future it might make sense to configure more than one queue:
One queue for control and others for IP traffic or debug.
I think the sensible setup then would be to have control traffic
on queue-0 with flow control, but just ignore overflow other queues
carrying IP and Debug traffic. (For proper IP queue management
this is done in the modem anyway - the radio-link normally
slower than the CAIF link so queues would normally build
in the modem not at the CAIF link interface).

In any case flow control on queue-0 would do.


> > +		high = (caifd->netdev->tx_queue_len * q_high) / 100;
> > +
> > +		/* If we run with a TX queue, check if the queue is too
> long*/
>
> Are you sure only this cpu can run here ? any lock is held ?

I guess there are multiple issues here.
1) Access to tx_queue: I see in net/core/dev.c access to qdisc is RCU
	protected. I believe I should add RCU locking here.

2) I don't believe I need to hold spin_lock for the detection of overflow.
	If I we have races here the worst thing that can
	happened is that Flow-off is a bit off in timing.

3) I think I'll add a spin_lock when accessing caifd->xoff to avoid
	multiple flow-off indications.

I'll make a new patch fixing these issues.

> > +		if (netif_queue_stopped(caifd->netdev) ||
> > +			qdisc_qlen(txq->qdisc) > high) {
> > +
> > +			pr_debug("queue stop(%d) or full (%d>%d) - XOFF\n",
> > +					netif_queue_stopped(caifd->netdev),
> > +					qdisc_qlen(txq->qdisc), high);
> > +			caifd->xoff = 1;
> > +			/* Hijack this skb free callback function. */
> > +			skb_orphan(skb);
> > +			skb->destructor = caif_flow_cb;
> > +			caifd->layer.up->
> > +				ctrlcmd(caifd->layer.up,
> > +					_CAIF_CTRLCMD_PHYIF_FLOW_OFF_IND,
> > +					caifd->layer.id);
> > +		}
> > +	}
> >
> >  	err = dev_queue_xmit(skb);
> >  	if (err > 0)
>
> What prevents dev_queue_xmit() to early orphan skb ?

My understanding is that skb destructor primarily is used for socket
memory accounting. In this case we're fooling the socket accounting,
but it should only happen when hitting the flow-off queue length
threshold. If we have a tx queue on 1000 it would happen at most
every 500 packet, in reality much more seldom.

However I think I can, if I do locking properly, stash away the destructor
and call it when the flow callback is called... what do you think?


Thank you for reviewing this Eric, further feedback welcome.

Regards,
Sjur