[PATCH] brcmfmac: implement more accurate skb tracking

[PATCH] brcmfmac: implement more accurate skb tracking

[Rafał Miłecki]

From: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>

We need to track 802.1x packets to know if there are any pending ones
for transmission. This is required for performing key update in the
firmware.

Unfortunately our old tracking code wasn't very accurate. It was
treating skb as pending as soon as it was passed by the netif. Actual
handling packet to the firmware was happening later as brcmfmac
internally queues them and uses its own worker(s).

Other than that it was hard to handle freeing packets. Everytime we had
to determine (in more generic funcions) if packet was counted as pending
802.1x one or not. It was causing some problems, e.g. it wasn't clear if
brcmf_flowring_delete should free skb directly or not.

This patch introduces 2 separated functions for tracking skbs. This
simplifies logic, fixes brcmf_flowring_delete (maybe other hidden bugs
as well) and allows further simplifications. Thanks to better accuracy
is also increases time window for key update (and lowers timeout risk).

Signed-off-by: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
---
This was successfully tested with 4366b1. Can someone give it a try with
some USB/SDIO device, please?
---
 .../wireless/broadcom/brcm80211/brcmfmac/bcdc.c    | 11 +++++++
 .../wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c  | 12 +++++++-
 .../wireless/broadcom/brcm80211/brcmfmac/core.c    | 36 ++++++++++++++++------
 .../wireless/broadcom/brcm80211/brcmfmac/core.h    |  2 ++
 .../wireless/broadcom/brcm80211/brcmfmac/msgbuf.c  | 14 +++++++--
 .../wireless/broadcom/brcm80211/brcmfmac/proto.h   | 11 +++++++
 .../wireless/broadcom/brcm80211/brcmfmac/sdio.c    |  8 +++++
 .../net/wireless/broadcom/brcm80211/brcmfmac/usb.c | 10 ++++++
 8 files changed, 91 insertions(+), 13 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c
index d1bc51f..3e40244 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c
@@ -326,6 +326,16 @@ brcmf_proto_bcdc_hdrpull(struct brcmf_pub *drvr, bool do_fws,
 	return 0;
 }
 
+static int brcmf_proto_bcdc_hdr_get_ifidx(struct brcmf_pub *drvr,
+					  struct sk_buff *skb)
+{
+	struct brcmf_proto_bcdc_header *h;
+
+	h = (struct brcmf_proto_bcdc_header *)(skb->data);
+
+	return BCDC_GET_IF_IDX(h);
+}
+
 static int
 brcmf_proto_bcdc_txdata(struct brcmf_pub *drvr, int ifidx, u8 offset,
 			struct sk_buff *pktbuf)
@@ -373,6 +383,7 @@ int brcmf_proto_bcdc_attach(struct brcmf_pub *drvr)
 	}
 
 	drvr->proto->hdrpull = brcmf_proto_bcdc_hdrpull;
+	drvr->proto->hdr_get_ifidx = brcmf_proto_bcdc_hdr_get_ifidx;
 	drvr->proto->query_dcmd = brcmf_proto_bcdc_query_dcmd;
 	drvr->proto->set_dcmd = brcmf_proto_bcdc_set_dcmd;
 	drvr->proto->txdata = brcmf_proto_bcdc_txdata;
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
index 03404cb..fef9d02 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
@@ -43,6 +43,7 @@
 #include "chip.h"
 #include "bus.h"
 #include "debug.h"
+#include "proto.h"
 #include "sdio.h"
 #include "core.h"
 #include "common.h"
@@ -772,6 +773,7 @@ int brcmf_sdiod_send_buf(struct brcmf_sdio_dev *sdiodev, u8 *buf, uint nbytes)
 int brcmf_sdiod_send_pkt(struct brcmf_sdio_dev *sdiodev,
 			 struct sk_buff_head *pktq)
 {
+	struct brcmf_pub *pub = sdiodev->bus_if->drvr;
 	struct sk_buff *skb;
 	u32 addr = sdiodev->sbwad;
 	int err;
@@ -784,10 +786,18 @@ int brcmf_sdiod_send_pkt(struct brcmf_sdio_dev *sdiodev,
 
 	if (pktq->qlen == 1 || !sdiodev->sg_support)
 		skb_queue_walk(pktq, skb) {
+			struct brcmf_if *ifp;
+			int ifidx;
+
+			ifidx = brcmf_proto_hdr_get_ifidx(pub, skb);
+			ifp = brcmf_get_ifp(pub, ifidx);
+			brcmf_tx_passing_skb(ifp, skb);
 			err = brcmf_sdiod_buffrw(sdiodev, SDIO_FUNC_2, true,
 						 addr, skb);
-			if (err)
+			if (err) {
+				brcmf_tx_regained_skb(ifp, skb);
 				break;
+			}
 		}
 	else
 		err = brcmf_sdiod_sglist_rw(sdiodev, SDIO_FUNC_2, true, addr,
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
index bc3d8ab..7cdc1f6 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
@@ -247,9 +247,6 @@ static netdev_tx_t brcmf_netdev_start_xmit(struct sk_buff *skb,
 		goto done;
 	}
 
-	if (eh->h_proto == htons(ETH_P_PAE))
-		atomic_inc(&ifp->pend_8021x_cnt);
-
 	/* determine the priority */
 	if (skb->priority == 0 || skb->priority > 7)
 		skb->priority = cfg80211_classify8021d(skb, NULL);
@@ -388,20 +385,41 @@ void brcmf_rx_event(struct device *dev, struct sk_buff *skb)
 	brcmu_pkt_buf_free_skb(skb);
 }
 
-void brcmf_txfinalize(struct brcmf_if *ifp, struct sk_buff *txp, bool success)
+/**
+ * brcmf_tx_passing_skb - Let core know skb is being passed to the firmware
+ *
+ * Core code needs to track state of some skbs. This function should be called
+ * every time skb is going to be passed to the firmware for transmitting.
+ */
+void brcmf_tx_passing_skb(struct brcmf_if *ifp, struct sk_buff *skb)
 {
-	struct ethhdr *eh;
-	u16 type;
+	struct ethhdr *eh = (struct ethhdr *)(skb->data);
 
-	eh = (struct ethhdr *)(txp->data);
-	type = ntohs(eh->h_proto);
+	if (eh->h_proto == htons(ETH_P_PAE))
+		atomic_inc(&ifp->pend_8021x_cnt);
+}
 
-	if (type == ETH_P_PAE) {
+/**
+ * brcmf_tx_regained_skb - Let core know skb is not being fw processed anymore
+ *
+ * This function should be called every time skb is returned from the firmware
+ * processing for whatever reason. It usually happens after successful
+ * transmission but may be also due to some error.
+ */
+void brcmf_tx_regained_skb(struct brcmf_if *ifp, struct sk_buff *skb)
+{
+	struct ethhdr *eh = (struct ethhdr *)(skb->data);
+
+	if (eh->h_proto == htons(ETH_P_PAE)) {
 		atomic_dec(&ifp->pend_8021x_cnt);
+
 		if (waitqueue_active(&ifp->pend_8021x_wait))
 			wake_up(&ifp->pend_8021x_wait);
 	}
+}
 
+void brcmf_txfinalize(struct brcmf_if *ifp, struct sk_buff *txp, bool success)
+{
 	if (!success)
 		ifp->stats.tx_errors++;
 
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h
index f16cfc9..80478b5 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h
@@ -215,6 +215,8 @@ struct brcmf_if *brcmf_add_if(struct brcmf_pub *drvr, s32 bsscfgidx, s32 ifidx,
 void brcmf_remove_interface(struct brcmf_if *ifp, bool rtnl_locked);
 void brcmf_txflowblock_if(struct brcmf_if *ifp,
 			  enum brcmf_netif_stop_reason reason, bool state);
+void brcmf_tx_passing_skb(struct brcmf_if *ifp, struct sk_buff *skb);
+void brcmf_tx_regained_skb(struct brcmf_if *ifp, struct sk_buff *skb);
 void brcmf_txfinalize(struct brcmf_if *ifp, struct sk_buff *txp, bool success);
 void brcmf_netif_rx(struct brcmf_if *ifp, struct sk_buff *skb);
 void brcmf_net_setcarrier(struct brcmf_if *ifp, bool on);
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/msgbuf.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/msgbuf.c
index 2b9a2bc..2a25eea 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/msgbuf.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/msgbuf.c
@@ -701,17 +701,22 @@ static void brcmf_msgbuf_txflow(struct brcmf_msgbuf *msgbuf, u16 flowid)
 
 	count = BRCMF_MSGBUF_TX_FLUSH_CNT2 - BRCMF_MSGBUF_TX_FLUSH_CNT1;
 	while (brcmf_flowring_qlen(flow, flowid)) {
+		u8 ifidx = brcmf_flowring_ifidx_get(flow, flowid);
+		struct brcmf_if *ifp = brcmf_get_ifp(msgbuf->drvr, ifidx);
+
 		skb = brcmf_flowring_dequeue(flow, flowid);
 		if (skb == NULL) {
 			brcmf_err("No SKB, but qlen %d\n",
 				  brcmf_flowring_qlen(flow, flowid));
 			break;
 		}
+		brcmf_tx_passing_skb(ifp, skb);
 		skb_orphan(skb);
 		if (brcmf_msgbuf_alloc_pktid(msgbuf->drvr->bus_if->dev,
 					     msgbuf->tx_pktids, skb, ETH_HLEN,
 					     &physaddr, &pktid)) {
 			brcmf_flowring_reinsert(flow, flowid, skb);
+			brcmf_tx_regained_skb(ifp, skb);
 			brcmf_err("No PKTID available !!\n");
 			break;
 		}
@@ -720,6 +725,7 @@ static void brcmf_msgbuf_txflow(struct brcmf_msgbuf *msgbuf, u16 flowid)
 			brcmf_msgbuf_get_pktid(msgbuf->drvr->bus_if->dev,
 					       msgbuf->tx_pktids, pktid);
 			brcmf_flowring_reinsert(flow, flowid, skb);
+			brcmf_tx_regained_skb(ifp, skb);
 			break;
 		}
 		count++;
@@ -728,7 +734,7 @@ static void brcmf_msgbuf_txflow(struct brcmf_msgbuf *msgbuf, u16 flowid)
 
 		tx_msghdr->msg.msgtype = MSGBUF_TYPE_TX_POST;
 		tx_msghdr->msg.request_id = cpu_to_le32(pktid);
-		tx_msghdr->msg.ifidx = brcmf_flowring_ifidx_get(flow, flowid);
+		tx_msghdr->msg.ifidx = ifidx;
 		tx_msghdr->flags = BRCMF_MSGBUF_PKT_FLAGS_FRAME_802_3;
 		tx_msghdr->flags |= (skb->priority & 0x07) <<
 				    BRCMF_MSGBUF_PKT_FLAGS_PRIO_SHIFT;
@@ -857,6 +863,7 @@ brcmf_msgbuf_process_ioctl_complete(struct brcmf_msgbuf *msgbuf, void *buf)
 static void
 brcmf_msgbuf_process_txstatus(struct brcmf_msgbuf *msgbuf, void *buf)
 {
+	struct brcmf_if *ifp;
 	struct brcmf_commonring *commonring;
 	struct msgbuf_tx_status *tx_status;
 	u32 idx;
@@ -876,8 +883,9 @@ brcmf_msgbuf_process_txstatus(struct brcmf_msgbuf *msgbuf, void *buf)
 	commonring = msgbuf->flowrings[flowid];
 	atomic_dec(&commonring->outstanding_tx);
 
-	brcmf_txfinalize(brcmf_get_ifp(msgbuf->drvr, tx_status->msg.ifidx),
-			 skb, true);
+	ifp = brcmf_get_ifp(msgbuf->drvr, tx_status->msg.ifidx);
+	brcmf_tx_regained_skb(ifp, skb);
+	brcmf_txfinalize(ifp, skb, true);
 }
 
 
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/proto.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/proto.h
index 57531f4..453cc5a 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/proto.h
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/proto.h
@@ -16,6 +16,7 @@
 #ifndef BRCMFMAC_PROTO_H
 #define BRCMFMAC_PROTO_H
 
+#include "core.h"
 
 enum proto_addr_mode {
 	ADDR_INDIRECT	= 0,
@@ -29,6 +30,7 @@ struct brcmf_skb_reorder_data {
 struct brcmf_proto {
 	int (*hdrpull)(struct brcmf_pub *drvr, bool do_fws,
 		       struct sk_buff *skb, struct brcmf_if **ifp);
+	int (*hdr_get_ifidx)(struct brcmf_pub *drvr, struct sk_buff *skb);
 	int (*query_dcmd)(struct brcmf_pub *drvr, int ifidx, uint cmd,
 			  void *buf, uint len);
 	int (*set_dcmd)(struct brcmf_pub *drvr, int ifidx, uint cmd, void *buf,
@@ -64,6 +66,15 @@ static inline int brcmf_proto_hdrpull(struct brcmf_pub *drvr, bool do_fws,
 		ifp = &tmp;
 	return drvr->proto->hdrpull(drvr, do_fws, skb, ifp);
 }
+
+static inline int brcmf_proto_hdr_get_ifidx(struct brcmf_pub *drvr,
+					    struct sk_buff *skb)
+{
+	if (!drvr->proto->hdr_get_ifidx)
+		return -ENOTSUPP;
+	return drvr->proto->hdr_get_ifidx(drvr, skb);
+}
+
 static inline int brcmf_proto_query_dcmd(struct brcmf_pub *drvr, int ifidx,
 					 uint cmd, void *buf, uint len)
 {
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
index b892dac..4dc96bd 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
@@ -42,6 +42,7 @@
 #include "sdio.h"
 #include "chip.h"
 #include "firmware.h"
+#include "proto.h"
 #include "core.h"
 #include "common.h"
 
@@ -2240,6 +2241,7 @@ brcmf_sdio_txpkt_postp(struct brcmf_sdio *bus, struct sk_buff_head *pktq)
 static int brcmf_sdio_txpkt(struct brcmf_sdio *bus, struct sk_buff_head *pktq,
 			    uint chan)
 {
+	struct brcmf_pub *pub = bus->sdiodev->bus_if->drvr;
 	int ret;
 	struct sk_buff *pkt_next, *tmp;
 
@@ -2263,7 +2265,13 @@ done:
 	if (ret == 0)
 		bus->tx_seq = (bus->tx_seq + pktq->qlen) % SDPCM_SEQ_WRAP;
 	skb_queue_walk_safe(pktq, pkt_next, tmp) {
+		struct brcmf_if *ifp;
+		int ifidx;
+
 		__skb_unlink(pkt_next, pktq);
+		ifidx = brcmf_proto_hdr_get_ifidx(pub, pkt_next);
+		ifp = brcmf_get_ifp(pub, ifidx);
+		brcmf_tx_regained_skb(ifp, pkt_next);
 		brcmf_txcomplete(bus->sdiodev->dev, pkt_next, ret == 0);
 	}
 	return ret;
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
index 2f978a3..d2f81c7 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
@@ -26,6 +26,7 @@
 #include "bus.h"
 #include "debug.h"
 #include "firmware.h"
+#include "proto.h"
 #include "usb.h"
 #include "core.h"
 #include "common.h"
@@ -476,12 +477,16 @@ static void brcmf_usb_tx_complete(struct urb *urb)
 {
 	struct brcmf_usbreq *req = (struct brcmf_usbreq *)urb->context;
 	struct brcmf_usbdev_info *devinfo = req->devinfo;
+	struct brcmf_pub *pub = devinfo->bus_pub.bus->drvr;
+	struct brcmf_if *ifp;
 	unsigned long flags;
 
 	brcmf_dbg(USB, "Enter, urb->status=%d, skb=%p\n", urb->status,
 		  req->skb);
 	brcmf_usb_del_fromq(devinfo, req);
 
+	ifp = brcmf_get_ifp(pub, brcmf_proto_hdr_get_ifidx(pub, req->skb));
+	brcmf_tx_regained_skb(ifp, req->skb);
 	brcmf_txcomplete(devinfo->dev, req->skb, urb->status == 0);
 	req->skb = NULL;
 	brcmf_usb_enq(devinfo, &devinfo->tx_freeq, req, &devinfo->tx_freecount);
@@ -598,7 +603,9 @@ brcmf_usb_state_change(struct brcmf_usbdev_info *devinfo, int state)
 static int brcmf_usb_tx(struct device *dev, struct sk_buff *skb)
 {
 	struct brcmf_usbdev_info *devinfo = brcmf_usb_get_businfo(dev);
+	struct brcmf_pub *pub = devinfo->bus_pub.bus->drvr;
 	struct brcmf_usbreq  *req;
+	struct brcmf_if *ifp;
 	int ret;
 	unsigned long flags;
 
@@ -622,6 +629,8 @@ static int brcmf_usb_tx(struct device *dev, struct sk_buff *skb)
 			  skb->data, skb->len, brcmf_usb_tx_complete, req);
 	req->urb->transfer_flags |= URB_ZERO_PACKET;
 	brcmf_usb_enq(devinfo, &devinfo->tx_postq, req, NULL);
+	ifp = brcmf_get_ifp(pub, brcmf_proto_hdr_get_ifidx(pub, skb));
+	brcmf_tx_passing_skb(ifp, skb);
 	ret = usb_submit_urb(req->urb, GFP_ATOMIC);
 	if (ret) {
 		brcmf_err("brcmf_usb_tx usb_submit_urb FAILED\n");
@@ -629,6 +638,7 @@ static int brcmf_usb_tx(struct device *dev, struct sk_buff *skb)
 		req->skb = NULL;
 		brcmf_usb_enq(devinfo, &devinfo->tx_freeq, req,
 			      &devinfo->tx_freecount);
+		brcmf_tx_regained_skb(ifp, skb);
 		goto fail;
 	}
 
-- 
2.9.3

Re: [PATCH] brcmfmac: implement more accurate skb tracking

[Arend Van Spriel]

On 26-9-2016 12:23, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> We need to track 802.1x packets to know if there are any pending ones
> for transmission. This is required for performing key update in the
> firmware.

The problem we are trying to solve is a pretty old one. The problem is
that wpa_supplicant uses two separate code paths: EAPOL messaging
through data path and key configuration though nl80211.

> Unfortunately our old tracking code wasn't very accurate. It was
> treating skb as pending as soon as it was passed by the netif. Actual
> handling packet to the firmware was happening later as brcmfmac
> internally queues them and uses its own worker(s).

That does not seem right. As soon as we get a 1x packet we need to wait
with key configuration regardless whether it is still in the driver or
handed over to firmware already.

Regards,
Arend

> Other than that it was hard to handle freeing packets. Everytime we had
> to determine (in more generic funcions) if packet was counted as pending
> 802.1x one or not. It was causing some problems, e.g. it wasn't clear if
> brcmf_flowring_delete should free skb directly or not.
> 
> This patch introduces 2 separated functions for tracking skbs. This
> simplifies logic, fixes brcmf_flowring_delete (maybe other hidden bugs
> as well) and allows further simplifications. Thanks to better accuracy
> is also increases time window for key update (and lowers timeout risk).
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
> This was successfully tested with 4366b1. Can someone give it a try with
> some USB/SDIO device, please?
> ---
>  .../wireless/broadcom/brcm80211/brcmfmac/bcdc.c    | 11 +++++++
>  .../wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c  | 12 +++++++-
>  .../wireless/broadcom/brcm80211/brcmfmac/core.c    | 36 ++++++++++++++++------
>  .../wireless/broadcom/brcm80211/brcmfmac/core.h    |  2 ++
>  .../wireless/broadcom/brcm80211/brcmfmac/msgbuf.c  | 14 +++++++--
>  .../wireless/broadcom/brcm80211/brcmfmac/proto.h   | 11 +++++++
>  .../wireless/broadcom/brcm80211/brcmfmac/sdio.c    |  8 +++++
>  .../net/wireless/broadcom/brcm80211/brcmfmac/usb.c | 10 ++++++
>  8 files changed, 91 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c
> index d1bc51f..3e40244 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c
> @@ -326,6 +326,16 @@ brcmf_proto_bcdc_hdrpull(struct brcmf_pub *drvr, bool do_fws,
>  	return 0;
>  }
>  
> +static int brcmf_proto_bcdc_hdr_get_ifidx(struct brcmf_pub *drvr,
> +					  struct sk_buff *skb)
> +{
> +	struct brcmf_proto_bcdc_header *h;
> +
> +	h = (struct brcmf_proto_bcdc_header *)(skb->data);
> +
> +	return BCDC_GET_IF_IDX(h);
> +}
> +
>  static int
>  brcmf_proto_bcdc_txdata(struct brcmf_pub *drvr, int ifidx, u8 offset,
>  			struct sk_buff *pktbuf)
> @@ -373,6 +383,7 @@ int brcmf_proto_bcdc_attach(struct brcmf_pub *drvr)
>  	}
>  
>  	drvr->proto->hdrpull = brcmf_proto_bcdc_hdrpull;
> +	drvr->proto->hdr_get_ifidx = brcmf_proto_bcdc_hdr_get_ifidx;
>  	drvr->proto->query_dcmd = brcmf_proto_bcdc_query_dcmd;
>  	drvr->proto->set_dcmd = brcmf_proto_bcdc_set_dcmd;
>  	drvr->proto->txdata = brcmf_proto_bcdc_txdata;
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> index 03404cb..fef9d02 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> @@ -43,6 +43,7 @@
>  #include "chip.h"
>  #include "bus.h"
>  #include "debug.h"
> +#include "proto.h"
>  #include "sdio.h"
>  #include "core.h"
>  #include "common.h"
> @@ -772,6 +773,7 @@ int brcmf_sdiod_send_buf(struct brcmf_sdio_dev *sdiodev, u8 *buf, uint nbytes)
>  int brcmf_sdiod_send_pkt(struct brcmf_sdio_dev *sdiodev,
>  			 struct sk_buff_head *pktq)
>  {
> +	struct brcmf_pub *pub = sdiodev->bus_if->drvr;
>  	struct sk_buff *skb;
>  	u32 addr = sdiodev->sbwad;
>  	int err;
> @@ -784,10 +786,18 @@ int brcmf_sdiod_send_pkt(struct brcmf_sdio_dev *sdiodev,
>  
>  	if (pktq->qlen == 1 || !sdiodev->sg_support)
>  		skb_queue_walk(pktq, skb) {
> +			struct brcmf_if *ifp;
> +			int ifidx;
> +
> +			ifidx = brcmf_proto_hdr_get_ifidx(pub, skb);
> +			ifp = brcmf_get_ifp(pub, ifidx);
> +			brcmf_tx_passing_skb(ifp, skb);
>  			err = brcmf_sdiod_buffrw(sdiodev, SDIO_FUNC_2, true,
>  						 addr, skb);
> -			if (err)
> +			if (err) {
> +				brcmf_tx_regained_skb(ifp, skb);
>  				break;
> +			}
>  		}
>  	else
>  		err = brcmf_sdiod_sglist_rw(sdiodev, SDIO_FUNC_2, true, addr,
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> index bc3d8ab..7cdc1f6 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> @@ -247,9 +247,6 @@ static netdev_tx_t brcmf_netdev_start_xmit(struct sk_buff *skb,
>  		goto done;
>  	}
>  
> -	if (eh->h_proto == htons(ETH_P_PAE))
> -		atomic_inc(&ifp->pend_8021x_cnt);
> -
>  	/* determine the priority */
>  	if (skb->priority == 0 || skb->priority > 7)
>  		skb->priority = cfg80211_classify8021d(skb, NULL);
> @@ -388,20 +385,41 @@ void brcmf_rx_event(struct device *dev, struct sk_buff *skb)
>  	brcmu_pkt_buf_free_skb(skb);
>  }
>  
> -void brcmf_txfinalize(struct brcmf_if *ifp, struct sk_buff *txp, bool success)
> +/**
> + * brcmf_tx_passing_skb - Let core know skb is being passed to the firmware
> + *
> + * Core code needs to track state of some skbs. This function should be called
> + * every time skb is going to be passed to the firmware for transmitting.
> + */
> +void brcmf_tx_passing_skb(struct brcmf_if *ifp, struct sk_buff *skb)
>  {
> -	struct ethhdr *eh;
> -	u16 type;
> +	struct ethhdr *eh = (struct ethhdr *)(skb->data);
>  
> -	eh = (struct ethhdr *)(txp->data);
> -	type = ntohs(eh->h_proto);
> +	if (eh->h_proto == htons(ETH_P_PAE))
> +		atomic_inc(&ifp->pend_8021x_cnt);
> +}
>  
> -	if (type == ETH_P_PAE) {
> +/**
> + * brcmf_tx_regained_skb - Let core know skb is not being fw processed anymore
> + *
> + * This function should be called every time skb is returned from the firmware
> + * processing for whatever reason. It usually happens after successful
> + * transmission but may be also due to some error.
> + */
> +void brcmf_tx_regained_skb(struct brcmf_if *ifp, struct sk_buff *skb)
> +{
> +	struct ethhdr *eh = (struct ethhdr *)(skb->data);
> +
> +	if (eh->h_proto == htons(ETH_P_PAE)) {
>  		atomic_dec(&ifp->pend_8021x_cnt);
> +
>  		if (waitqueue_active(&ifp->pend_8021x_wait))
>  			wake_up(&ifp->pend_8021x_wait);
>  	}
> +}
>  
> +void brcmf_txfinalize(struct brcmf_if *ifp, struct sk_buff *txp, bool success)
> +{
>  	if (!success)
>  		ifp->stats.tx_errors++;
>  
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h
> index f16cfc9..80478b5 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h
> @@ -215,6 +215,8 @@ struct brcmf_if *brcmf_add_if(struct brcmf_pub *drvr, s32 bsscfgidx, s32 ifidx,
>  void brcmf_remove_interface(struct brcmf_if *ifp, bool rtnl_locked);
>  void brcmf_txflowblock_if(struct brcmf_if *ifp,
>  			  enum brcmf_netif_stop_reason reason, bool state);
> +void brcmf_tx_passing_skb(struct brcmf_if *ifp, struct sk_buff *skb);
> +void brcmf_tx_regained_skb(struct brcmf_if *ifp, struct sk_buff *skb);
>  void brcmf_txfinalize(struct brcmf_if *ifp, struct sk_buff *txp, bool success);
>  void brcmf_netif_rx(struct brcmf_if *ifp, struct sk_buff *skb);
>  void brcmf_net_setcarrier(struct brcmf_if *ifp, bool on);
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/msgbuf.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/msgbuf.c
> index 2b9a2bc..2a25eea 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/msgbuf.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/msgbuf.c
> @@ -701,17 +701,22 @@ static void brcmf_msgbuf_txflow(struct brcmf_msgbuf *msgbuf, u16 flowid)
>  
>  	count = BRCMF_MSGBUF_TX_FLUSH_CNT2 - BRCMF_MSGBUF_TX_FLUSH_CNT1;
>  	while (brcmf_flowring_qlen(flow, flowid)) {
> +		u8 ifidx = brcmf_flowring_ifidx_get(flow, flowid);
> +		struct brcmf_if *ifp = brcmf_get_ifp(msgbuf->drvr, ifidx);
> +
>  		skb = brcmf_flowring_dequeue(flow, flowid);
>  		if (skb == NULL) {
>  			brcmf_err("No SKB, but qlen %d\n",
>  				  brcmf_flowring_qlen(flow, flowid));
>  			break;
>  		}
> +		brcmf_tx_passing_skb(ifp, skb);
>  		skb_orphan(skb);
>  		if (brcmf_msgbuf_alloc_pktid(msgbuf->drvr->bus_if->dev,
>  					     msgbuf->tx_pktids, skb, ETH_HLEN,
>  					     &physaddr, &pktid)) {
>  			brcmf_flowring_reinsert(flow, flowid, skb);
> +			brcmf_tx_regained_skb(ifp, skb);
>  			brcmf_err("No PKTID available !!\n");
>  			break;
>  		}
> @@ -720,6 +725,7 @@ static void brcmf_msgbuf_txflow(struct brcmf_msgbuf *msgbuf, u16 flowid)
>  			brcmf_msgbuf_get_pktid(msgbuf->drvr->bus_if->dev,
>  					       msgbuf->tx_pktids, pktid);
>  			brcmf_flowring_reinsert(flow, flowid, skb);
> +			brcmf_tx_regained_skb(ifp, skb);
>  			break;
>  		}
>  		count++;
> @@ -728,7 +734,7 @@ static void brcmf_msgbuf_txflow(struct brcmf_msgbuf *msgbuf, u16 flowid)
>  
>  		tx_msghdr->msg.msgtype = MSGBUF_TYPE_TX_POST;
>  		tx_msghdr->msg.request_id = cpu_to_le32(pktid);
> -		tx_msghdr->msg.ifidx = brcmf_flowring_ifidx_get(flow, flowid);
> +		tx_msghdr->msg.ifidx = ifidx;
>  		tx_msghdr->flags = BRCMF_MSGBUF_PKT_FLAGS_FRAME_802_3;
>  		tx_msghdr->flags |= (skb->priority & 0x07) <<
>  				    BRCMF_MSGBUF_PKT_FLAGS_PRIO_SHIFT;
> @@ -857,6 +863,7 @@ brcmf_msgbuf_process_ioctl_complete(struct brcmf_msgbuf *msgbuf, void *buf)
>  static void
>  brcmf_msgbuf_process_txstatus(struct brcmf_msgbuf *msgbuf, void *buf)
>  {
> +	struct brcmf_if *ifp;
>  	struct brcmf_commonring *commonring;
>  	struct msgbuf_tx_status *tx_status;
>  	u32 idx;
> @@ -876,8 +883,9 @@ brcmf_msgbuf_process_txstatus(struct brcmf_msgbuf *msgbuf, void *buf)
>  	commonring = msgbuf->flowrings[flowid];
>  	atomic_dec(&commonring->outstanding_tx);
>  
> -	brcmf_txfinalize(brcmf_get_ifp(msgbuf->drvr, tx_status->msg.ifidx),
> -			 skb, true);
> +	ifp = brcmf_get_ifp(msgbuf->drvr, tx_status->msg.ifidx);
> +	brcmf_tx_regained_skb(ifp, skb);
> +	brcmf_txfinalize(ifp, skb, true);
>  }
>  
>  
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/proto.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/proto.h
> index 57531f4..453cc5a 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/proto.h
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/proto.h
> @@ -16,6 +16,7 @@
>  #ifndef BRCMFMAC_PROTO_H
>  #define BRCMFMAC_PROTO_H
>  
> +#include "core.h"
>  
>  enum proto_addr_mode {
>  	ADDR_INDIRECT	= 0,
> @@ -29,6 +30,7 @@ struct brcmf_skb_reorder_data {
>  struct brcmf_proto {
>  	int (*hdrpull)(struct brcmf_pub *drvr, bool do_fws,
>  		       struct sk_buff *skb, struct brcmf_if **ifp);
> +	int (*hdr_get_ifidx)(struct brcmf_pub *drvr, struct sk_buff *skb);
>  	int (*query_dcmd)(struct brcmf_pub *drvr, int ifidx, uint cmd,
>  			  void *buf, uint len);
>  	int (*set_dcmd)(struct brcmf_pub *drvr, int ifidx, uint cmd, void *buf,
> @@ -64,6 +66,15 @@ static inline int brcmf_proto_hdrpull(struct brcmf_pub *drvr, bool do_fws,
>  		ifp = &tmp;
>  	return drvr->proto->hdrpull(drvr, do_fws, skb, ifp);
>  }
> +
> +static inline int brcmf_proto_hdr_get_ifidx(struct brcmf_pub *drvr,
> +					    struct sk_buff *skb)
> +{
> +	if (!drvr->proto->hdr_get_ifidx)
> +		return -ENOTSUPP;
> +	return drvr->proto->hdr_get_ifidx(drvr, skb);
> +}
> +
>  static inline int brcmf_proto_query_dcmd(struct brcmf_pub *drvr, int ifidx,
>  					 uint cmd, void *buf, uint len)
>  {
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> index b892dac..4dc96bd 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> @@ -42,6 +42,7 @@
>  #include "sdio.h"
>  #include "chip.h"
>  #include "firmware.h"
> +#include "proto.h"
>  #include "core.h"
>  #include "common.h"
>  
> @@ -2240,6 +2241,7 @@ brcmf_sdio_txpkt_postp(struct brcmf_sdio *bus, struct sk_buff_head *pktq)
>  static int brcmf_sdio_txpkt(struct brcmf_sdio *bus, struct sk_buff_head *pktq,
>  			    uint chan)
>  {
> +	struct brcmf_pub *pub = bus->sdiodev->bus_if->drvr;
>  	int ret;
>  	struct sk_buff *pkt_next, *tmp;
>  
> @@ -2263,7 +2265,13 @@ done:
>  	if (ret == 0)
>  		bus->tx_seq = (bus->tx_seq + pktq->qlen) % SDPCM_SEQ_WRAP;
>  	skb_queue_walk_safe(pktq, pkt_next, tmp) {
> +		struct brcmf_if *ifp;
> +		int ifidx;
> +
>  		__skb_unlink(pkt_next, pktq);
> +		ifidx = brcmf_proto_hdr_get_ifidx(pub, pkt_next);
> +		ifp = brcmf_get_ifp(pub, ifidx);
> +		brcmf_tx_regained_skb(ifp, pkt_next);
>  		brcmf_txcomplete(bus->sdiodev->dev, pkt_next, ret == 0);
>  	}
>  	return ret;
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
> index 2f978a3..d2f81c7 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
> @@ -26,6 +26,7 @@
>  #include "bus.h"
>  #include "debug.h"
>  #include "firmware.h"
> +#include "proto.h"
>  #include "usb.h"
>  #include "core.h"
>  #include "common.h"
> @@ -476,12 +477,16 @@ static void brcmf_usb_tx_complete(struct urb *urb)
>  {
>  	struct brcmf_usbreq *req = (struct brcmf_usbreq *)urb->context;
>  	struct brcmf_usbdev_info *devinfo = req->devinfo;
> +	struct brcmf_pub *pub = devinfo->bus_pub.bus->drvr;
> +	struct brcmf_if *ifp;
>  	unsigned long flags;
>  
>  	brcmf_dbg(USB, "Enter, urb->status=%d, skb=%p\n", urb->status,
>  		  req->skb);
>  	brcmf_usb_del_fromq(devinfo, req);
>  
> +	ifp = brcmf_get_ifp(pub, brcmf_proto_hdr_get_ifidx(pub, req->skb));
> +	brcmf_tx_regained_skb(ifp, req->skb);
>  	brcmf_txcomplete(devinfo->dev, req->skb, urb->status == 0);
>  	req->skb = NULL;
>  	brcmf_usb_enq(devinfo, &devinfo->tx_freeq, req, &devinfo->tx_freecount);
> @@ -598,7 +603,9 @@ brcmf_usb_state_change(struct brcmf_usbdev_info *devinfo, int state)
>  static int brcmf_usb_tx(struct device *dev, struct sk_buff *skb)
>  {
>  	struct brcmf_usbdev_info *devinfo = brcmf_usb_get_businfo(dev);
> +	struct brcmf_pub *pub = devinfo->bus_pub.bus->drvr;
>  	struct brcmf_usbreq  *req;
> +	struct brcmf_if *ifp;
>  	int ret;
>  	unsigned long flags;
>  
> @@ -622,6 +629,8 @@ static int brcmf_usb_tx(struct device *dev, struct sk_buff *skb)
>  			  skb->data, skb->len, brcmf_usb_tx_complete, req);
>  	req->urb->transfer_flags |= URB_ZERO_PACKET;
>  	brcmf_usb_enq(devinfo, &devinfo->tx_postq, req, NULL);
> +	ifp = brcmf_get_ifp(pub, brcmf_proto_hdr_get_ifidx(pub, skb));
> +	brcmf_tx_passing_skb(ifp, skb);
>  	ret = usb_submit_urb(req->urb, GFP_ATOMIC);
>  	if (ret) {
>  		brcmf_err("brcmf_usb_tx usb_submit_urb FAILED\n");
> @@ -629,6 +638,7 @@ static int brcmf_usb_tx(struct device *dev, struct sk_buff *skb)
>  		req->skb = NULL;
>  		brcmf_usb_enq(devinfo, &devinfo->tx_freeq, req,
>  			      &devinfo->tx_freecount);
> +		brcmf_tx_regained_skb(ifp, skb);
>  		goto fail;
>  	}
>  
> 

Re: [PATCH] brcmfmac: implement more accurate skb tracking

[Rafał Miłecki]

On 26 September 2016 at 13:46, Arend Van Spriel
<arend.vanspriel@broadcom.com> wrote:
> On 26-9-2016 12:23, Rafał Miłecki wrote:
>> From: Rafał Miłecki <rafal@milecki.pl>
>>
>> We need to track 802.1x packets to know if there are any pending ones
>> for transmission. This is required for performing key update in the
>> firmware.
>
> The problem we are trying to solve is a pretty old one. The problem is
> that wpa_supplicant uses two separate code paths: EAPOL messaging
> through data path and key configuration though nl80211.

Can I find it described/reported somewhere?


>> Unfortunately our old tracking code wasn't very accurate. It was
>> treating skb as pending as soon as it was passed by the netif. Actual
>> handling packet to the firmware was happening later as brcmfmac
>> internally queues them and uses its own worker(s).
>
> That does not seem right. As soon as we get a 1x packet we need to wait
> with key configuration regardless whether it is still in the driver or
> handed over to firmware already.

OK, thanks.

Re: [PATCH] brcmfmac: implement more accurate skb tracking

[Arend Van Spriel]

On 26-9-2016 14:13, Rafał Miłecki wrote:
> On 26 September 2016 at 13:46, Arend Van Spriel
> <arend.vanspriel-dY08KVG/lbpWk0Htik3J/w@public.gmane.org> wrote:
>> On 26-9-2016 12:23, Rafał Miłecki wrote:
>>> From: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
>>>
>>> We need to track 802.1x packets to know if there are any pending ones
>>> for transmission. This is required for performing key update in the
>>> firmware.
>>
>> The problem we are trying to solve is a pretty old one. The problem is
>> that wpa_supplicant uses two separate code paths: EAPOL messaging
>> through data path and key configuration though nl80211.
> 
> Can I find it described/reported somewhere?

Not sure. It is something that I recall from working at Intersil so back
in the prism days.

Regards,
Arend

>>> Unfortunately our old tracking code wasn't very accurate. It was
>>> treating skb as pending as soon as it was passed by the netif. Actual
>>> handling packet to the firmware was happening later as brcmfmac
>>> internally queues them and uses its own worker(s).
>>
>> That does not seem right. As soon as we get a 1x packet we need to wait
>> with key configuration regardless whether it is still in the driver or
>> handed over to firmware already.
> 
> OK, thanks.
> 

Re: [PATCH] brcmfmac: implement more accurate skb tracking

[Rafał Miłecki]

On 26 September 2016 at 14:13, Rafał Miłecki <zajec5@gmail.com> wrote:
> On 26 September 2016 at 13:46, Arend Van Spriel
> <arend.vanspriel@broadcom.com> wrote:
>> On 26-9-2016 12:23, Rafał Miłecki wrote:
>>> From: Rafał Miłecki <rafal@milecki.pl>
>>>
>>> We need to track 802.1x packets to know if there are any pending ones
>>> for transmission. This is required for performing key update in the
>>> firmware.
>>
>> The problem we are trying to solve is a pretty old one. The problem is
>> that wpa_supplicant uses two separate code paths: EAPOL messaging
>> through data path and key configuration though nl80211.
>
> Can I find it described/reported somewhere?
>
>
>>> Unfortunately our old tracking code wasn't very accurate. It was
>>> treating skb as pending as soon as it was passed by the netif. Actual
>>> handling packet to the firmware was happening later as brcmfmac
>>> internally queues them and uses its own worker(s).
>>
>> That does not seem right. As soon as we get a 1x packet we need to wait
>> with key configuration regardless whether it is still in the driver or
>> handed over to firmware already.
>
> OK, thanks.

Actually, it's not OK. I was trying to report/describe/discuss this
problem for over a week. I couldn't get much of answer from you.

I had to come with a patch I worked on for quite some time. Only then
you decided to react and reply with a reason for a nack. I see this
patch may be wrong (but it's still hard to know what's going wrong
without a proper hostapd bug report). I'd expect you to somehow work &
communicate with open source community.

-- 
Rafał

Re: [PATCH] brcmfmac: implement more accurate skb tracking

[Arend Van Spriel]

On 26-9-2016 14:38, Rafał Miłecki wrote:
> On 26 September 2016 at 14:13, Rafał Miłecki <zajec5@gmail.com> wrote:
>> On 26 September 2016 at 13:46, Arend Van Spriel
>> <arend.vanspriel@broadcom.com> wrote:
>>> On 26-9-2016 12:23, Rafał Miłecki wrote:
>>>> From: Rafał Miłecki <rafal@milecki.pl>
>>>>
>>>> We need to track 802.1x packets to know if there are any pending ones
>>>> for transmission. This is required for performing key update in the
>>>> firmware.
>>>
>>> The problem we are trying to solve is a pretty old one. The problem is
>>> that wpa_supplicant uses two separate code paths: EAPOL messaging
>>> through data path and key configuration though nl80211.
>>
>> Can I find it described/reported somewhere?
>>
>>
>>>> Unfortunately our old tracking code wasn't very accurate. It was
>>>> treating skb as pending as soon as it was passed by the netif. Actual
>>>> handling packet to the firmware was happening later as brcmfmac
>>>> internally queues them and uses its own worker(s).
>>>
>>> That does not seem right. As soon as we get a 1x packet we need to wait
>>> with key configuration regardless whether it is still in the driver or
>>> handed over to firmware already.
>>
>> OK, thanks.
> 
> Actually, it's not OK. I was trying to report/describe/discuss this
> problem for over a week. I couldn't get much of answer from you.
> 
> I had to come with a patch I worked on for quite some time. Only then
> you decided to react and reply with a reason for a nack. I see this
> patch may be wrong (but it's still hard to know what's going wrong
> without a proper hostapd bug report). I'd expect you to somehow work &
> communicate with open source community.

We do or at least make an honest attempt, but there is more on our plate
so responses may be delayed. It also does not help when you get anal and
preachy when we do respond. Also not OK. In this case the delay is
caused because I had to pick up the thread(s) as Hante is on vacation
(he needed a break :-p ). However, you started sending patches so I
decided to look at and respond to those. Sorry if you felt like we left
you hanging to dry.

Regards,
Arend

Re: [PATCH] brcmfmac: implement more accurate skb tracking

[Rafał Miłecki]

On 27 September 2016 at 11:24, Arend Van Spriel
<arend.vanspriel-dY08KVG/lbpWk0Htik3J/w@public.gmane.org> wrote:
> On 26-9-2016 14:38, Rafał Miłecki wrote:
>> On 26 September 2016 at 14:13, Rafał Miłecki <zajec5-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>> On 26 September 2016 at 13:46, Arend Van Spriel
>>> <arend.vanspriel-dY08KVG/lbpWk0Htik3J/w@public.gmane.org> wrote:
>>>> On 26-9-2016 12:23, Rafał Miłecki wrote:
>>>>> From: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
>>>>>
>>>>> We need to track 802.1x packets to know if there are any pending ones
>>>>> for transmission. This is required for performing key update in the
>>>>> firmware.
>>>>
>>>> The problem we are trying to solve is a pretty old one. The problem is
>>>> that wpa_supplicant uses two separate code paths: EAPOL messaging
>>>> through data path and key configuration though nl80211.
>>>
>>> Can I find it described/reported somewhere?
>>>
>>>
>>>>> Unfortunately our old tracking code wasn't very accurate. It was
>>>>> treating skb as pending as soon as it was passed by the netif. Actual
>>>>> handling packet to the firmware was happening later as brcmfmac
>>>>> internally queues them and uses its own worker(s).
>>>>
>>>> That does not seem right. As soon as we get a 1x packet we need to wait
>>>> with key configuration regardless whether it is still in the driver or
>>>> handed over to firmware already.
>>>
>>> OK, thanks.
>>
>> Actually, it's not OK. I was trying to report/describe/discuss this
>> problem for over a week. I couldn't get much of answer from you.
>>
>> I had to come with a patch I worked on for quite some time. Only then
>> you decided to react and reply with a reason for a nack. I see this
>> patch may be wrong (but it's still hard to know what's going wrong
>> without a proper hostapd bug report). I'd expect you to somehow work &
>> communicate with open source community.
>
> We do or at least make an honest attempt, but there is more on our plate
> so responses may be delayed. It also does not help when you get anal and
> preachy when we do respond. Also not OK. In this case the delay is
> caused because I had to pick up the thread(s) as Hante is on vacation
> (he needed a break :-p ). However, you started sending patches so I
> decided to look at and respond to those. Sorry if you felt like we left
> you hanging to dry.

I believe I get easily irritated due to my communication experience I
got so far :(


Over a year ago I reported brcmfmac can't recover from failed
register_netdev(ice). This bug remains unfixed.

In 2014 I reported problem with 80 MHz support. I didn't have hardware
to fix & test it on my own (you weren't able/allowed to send me one of
your PCIe cards). In remained broken until I fixed it year later.

You missed my crash bug report about caused by missing eth_type_trans
and came with patch on your own a month later.

Earlier this year I reported you problem with BCM4366 and multiple
interfaces. I didn't get much help. 3 months later I came with patch
to workaround the problem but you said there's a better way to do
this. It took me 2 weeks to figure out a new wlioctl API for that
while all I needed was a simple hint on "interface_remove".

Right now I'm waiting to get any answer from you about 4366c0
firmware. It's still less than 2 weeks since I asked for it, but a
simple ETA would be nice. I'm actually not sure if I should report
more problems to you to don't distract you from pending things.

Problems with brcmf_netdev_wait_pend8021x were reported multiples
times for last few months. When I finally got time for that it took me
a week to debug them.


As you can see, it takes me months to get help on some things. And in
few cases I never got much help at all. Yes, I was hoping to have you
more involved into brcmfmac development and problems solving. I guess
things didn't meet my expectations and I got grumpy & preachy.

-- 
Rafał

Re: [PATCH] brcmfmac: implement more accurate skb tracking

[Arend Van Spriel]

On 29-9-2016 23:57, Rafał Miłecki wrote:
> On 27 September 2016 at 11:24, Arend Van Spriel
> <arend.vanspriel-dY08KVG/lbpWk0Htik3J/w@public.gmane.org> wrote:
>> On 26-9-2016 14:38, Rafał Miłecki wrote:
>>> On 26 September 2016 at 14:13, Rafał Miłecki <zajec5-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>> On 26 September 2016 at 13:46, Arend Van Spriel
>>>> <arend.vanspriel-dY08KVG/lbpWk0Htik3J/w@public.gmane.org> wrote:
>>>>> On 26-9-2016 12:23, Rafał Miłecki wrote:
>>>>>> From: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
>>>>>>
>>>>>> We need to track 802.1x packets to know if there are any pending ones
>>>>>> for transmission. This is required for performing key update in the
>>>>>> firmware.
>>>>>
>>>>> The problem we are trying to solve is a pretty old one. The problem is
>>>>> that wpa_supplicant uses two separate code paths: EAPOL messaging
>>>>> through data path and key configuration though nl80211.
>>>>
>>>> Can I find it described/reported somewhere?
>>>>
>>>>
>>>>>> Unfortunately our old tracking code wasn't very accurate. It was
>>>>>> treating skb as pending as soon as it was passed by the netif. Actual
>>>>>> handling packet to the firmware was happening later as brcmfmac
>>>>>> internally queues them and uses its own worker(s).
>>>>>
>>>>> That does not seem right. As soon as we get a 1x packet we need to wait
>>>>> with key configuration regardless whether it is still in the driver or
>>>>> handed over to firmware already.
>>>>
>>>> OK, thanks.
>>>
>>> Actually, it's not OK. I was trying to report/describe/discuss this
>>> problem for over a week. I couldn't get much of answer from you.
>>>
>>> I had to come with a patch I worked on for quite some time. Only then
>>> you decided to react and reply with a reason for a nack. I see this
>>> patch may be wrong (but it's still hard to know what's going wrong
>>> without a proper hostapd bug report). I'd expect you to somehow work &
>>> communicate with open source community.
>>
>> We do or at least make an honest attempt, but there is more on our plate
>> so responses may be delayed. It also does not help when you get anal and
>> preachy when we do respond. Also not OK. In this case the delay is
>> caused because I had to pick up the thread(s) as Hante is on vacation
>> (he needed a break :-p ). However, you started sending patches so I
>> decided to look at and respond to those. Sorry if you felt like we left
>> you hanging to dry.
> 
> I believe I get easily irritated due to my communication experience I
> got so far :(
> 
> 
> Over a year ago I reported brcmfmac can't recover from failed
> register_netdev(ice). This bug remains unfixed.
> 
> In 2014 I reported problem with 80 MHz support. I didn't have hardware
> to fix & test it on my own (you weren't able/allowed to send me one of
> your PCIe cards). In remained broken until I fixed it year later.
> 
> You missed my crash bug report about caused by missing eth_type_trans
> and came with patch on your own a month later.
> 
> Earlier this year I reported you problem with BCM4366 and multiple
> interfaces. I didn't get much help. 3 months later I came with patch
> to workaround the problem but you said there's a better way to do
> this. It took me 2 weeks to figure out a new wlioctl API for that
> while all I needed was a simple hint on "interface_remove".
> 
> Right now I'm waiting to get any answer from you about 4366c0
> firmware. It's still less than 2 weeks since I asked for it, but a
> simple ETA would be nice. I'm actually not sure if I should report
> more problems to you to don't distract you from pending things.

This is a difficult question. All upstream firmware releases for router
chips are put on hold until further notice. Some decisions have been
made, but I have not seen a detailed plan to give an ETA.

> Problems with brcmf_netdev_wait_pend8021x were reported multiples
> times for last few months. When I finally got time for that it took me
> a week to debug them.

For the pend8021x you were sending a number of messages showing debug
progress so not sure whether you wanted our feedback on that. If so a
ping might have done it.

> As you can see, it takes me months to get help on some things. And in
> few cases I never got much help at all. Yes, I was hoping to have you
> more involved into brcmfmac development and problems solving. I guess
> things didn't meet my expectations and I got grumpy & preachy.

Thanks for listing all our failures. Somehow we are very good at getting
each other grumpy. When we provide a patch and you break it up and
submit that to Kalle, we get grumpy and it all piles up to the point
where we have this kind of conversation. As long as it helps to get
things of our chest I can live with that. Hope you can too. We strive to
give support to the community, but the priority is low as it is not
full-time activity.

Regards,
Arend

Re: [PATCH] brcmfmac: implement more accurate skb tracking

[Dan Williams]

On Mon, 2016-09-26 at 14:13 +0200, Rafał Miłecki wrote:
> On 26 September 2016 at 13:46, Arend Van Spriel
> <arend.vanspriel@broadcom.com> wrote:
> > 
> > On 26-9-2016 12:23, Rafał Miłecki wrote:
> > > 
> > > From: Rafał Miłecki <rafal@milecki.pl>
> > > 
> > > We need to track 802.1x packets to know if there are any pending
> > > ones
> > > for transmission. This is required for performing key update in
> > > the
> > > firmware.
> > 
> > The problem we are trying to solve is a pretty old one. The problem
> > is
> > that wpa_supplicant uses two separate code paths: EAPOL messaging
> > through data path and key configuration though nl80211.
> 
> Can I find it described/reported somewhere?

If I understand the issue correctly, you can find all this in the
supplicant code.  Once the supplicant has done whatever it wants to do
with the data frames that just happen to be EAPOL it then sends the
keys down to the driver with nl80211.

But it sounds like, instead of sniffing EAPOL frames in the driver skb
tracking and sniffing ETH_P_PAE, you should probably implement support
for NL80211_CMD_CRIT_PROTOCOL_START/NL80211_CMD_CRIT_PROTOCOL_STOP and
key off the passed-in NL80211_CRIT_PROTO_EAPOL.  At least at the
beginning of connection setup only EAPOL packets will be allowed
anyway.

It doesn't seem like the supplicant uses NL80211_CRIT_PROTO_EAPOL yet,
but that should also be fixed in the supplicant itself.  You should
probably get some comments from Jouni on how he'd like to see all this
work.  But generally the less specific sniffing of frames in drivers,
likely the better.

Dan

> 
> > 
> > > 
> > > Unfortunately our old tracking code wasn't very accurate. It was
> > > treating skb as pending as soon as it was passed by the netif.
> > > Actual
> > > handling packet to the firmware was happening later as brcmfmac
> > > internally queues them and uses its own worker(s).
> > 
> > That does not seem right. As soon as we get a 1x packet we need to
> > wait
> > with key configuration regardless whether it is still in the driver
> > or
> > handed over to firmware already.
> 
> OK, thanks.

Re: [PATCH] brcmfmac: implement more accurate skb tracking

[Arend Van Spriel]

On 26-9-2016 16:59, Dan Williams wrote:
> On Mon, 2016-09-26 at 14:13 +0200, Rafał Miłecki wrote:
>> On 26 September 2016 at 13:46, Arend Van Spriel
>> <arend.vanspriel-dY08KVG/lbpWk0Htik3J/w@public.gmane.org> wrote:
>>>
>>> On 26-9-2016 12:23, Rafał Miłecki wrote:
>>>>
>>>> From: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
>>>>
>>>> We need to track 802.1x packets to know if there are any pending
>>>> ones
>>>> for transmission. This is required for performing key update in
>>>> the
>>>> firmware.
>>>
>>> The problem we are trying to solve is a pretty old one. The problem
>>> is
>>> that wpa_supplicant uses two separate code paths: EAPOL messaging
>>> through data path and key configuration though nl80211.
>>
>> Can I find it described/reported somewhere?
> 
> If I understand the issue correctly, you can find all this in the
> supplicant code.  Once the supplicant has done whatever it wants to do
> with the data frames that just happen to be EAPOL it then sends the
> keys down to the driver with nl80211.

Indeed. EAPOL packets are simply data packets as far as the 802.11 stack
is concerned. The arrival of those in the driver is not predictable
hence we hold off the key configuration until those have been passed
over to firmware.

> But it sounds like, instead of sniffing EAPOL frames in the driver skb
> tracking and sniffing ETH_P_PAE, you should probably implement support
> for NL80211_CMD_CRIT_PROTOCOL_START/NL80211_CMD_CRIT_PROTOCOL_STOP and
> key off the passed-in NL80211_CRIT_PROTO_EAPOL.  At least at the
> beginning of connection setup only EAPOL packets will be allowed
> anyway.
> 
> It doesn't seem like the supplicant uses NL80211_CRIT_PROTO_EAPOL yet,
> but that should also be fixed in the supplicant itself.  You should
> probably get some comments from Jouni on how he'd like to see all this
> work.  But generally the less specific sniffing of frames in drivers,
> likely the better.

Indeed. That was the main motivation to introduce the CRIT_PROTO api. If
I recall correctly it was considered the task of the network manager to
issue the START/STOP. Recently noticed the use of CRIT_PROTO_DHCP on
some target system, which we already support in brcmfmac. From your
response I guess you consider CRIT_PROTO_EAPOL to be issued by the
supplicant.

Regards,
Arend

> Dan
> 
>>
>>>
>>>>
>>>> Unfortunately our old tracking code wasn't very accurate. It was
>>>> treating skb as pending as soon as it was passed by the netif.
>>>> Actual
>>>> handling packet to the firmware was happening later as brcmfmac
>>>> internally queues them and uses its own worker(s).
>>>
>>> That does not seem right. As soon as we get a 1x packet we need to
>>> wait
>>> with key configuration regardless whether it is still in the driver
>>> or
>>> handed over to firmware already.
>>
>> OK, thanks.