Add hexdump tracepoints to be used bcmdhd-dissector:
https://github.com/kanstrup/bcmdhd-dissector
bcmdhd-dissector is a Wireshark LUA plugin dissector used to decode
protocol data between the brcmfmac driver and the wifi chip firmware.
This includes decoding firmware command requests and responses as
well as events and even tx/rx data interleaved if the data_dump
tracepoint is enabled.
Signed-off-by: Mikael Kanstrup <[email protected]>
---
v2:
- Add description of bcmdhd-dissector as suggested by Rafał
- Fix tx/rx data dump.
- Fix tx data dump on USB interface.
Also updated the github project page with some screenshots.
.../wireless/broadcom/brcm80211/brcmfmac/bcdc.c | 4 ++
.../wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c | 2 +
.../wireless/broadcom/brcm80211/brcmfmac/core.c | 1 +
.../wireless/broadcom/brcm80211/brcmfmac/debug.c | 54 ++++++++++++++++++++++
.../wireless/broadcom/brcm80211/brcmfmac/debug.h | 15 ++++++
.../wireless/broadcom/brcm80211/brcmfmac/fweh.c | 2 +
.../broadcom/brcm80211/brcmfmac/tracepoint.h | 33 +++++++++++++
.../net/wireless/broadcom/brcm80211/brcmfmac/usb.c | 3 ++
8 files changed, 114 insertions(+)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c
index 6af658e..197942c 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c
@@ -134,6 +134,8 @@ brcmf_proto_bcdc_msg(struct brcmf_pub *drvr, int
ifidx, uint cmd, void *buf,
if (len > BRCMF_TX_IOCTL_MAX_MSG_SIZE)
len = BRCMF_TX_IOCTL_MAX_MSG_SIZE;
+ brcmf_dbg_dissect_dump(BRCMF_DISSECT_IOCTL, 1, &bcdc->msg, len);
+
/* Send request */
return brcmf_bus_txctl(drvr->bus_if, (unsigned char *)&bcdc->msg, len);
}
@@ -152,6 +154,8 @@ static int brcmf_proto_bcdc_cmplt(struct brcmf_pub
*drvr, u32 id, u32 len)
break;
} while (BCDC_DCMD_ID(le32_to_cpu(bcdc->msg.flags)) != id);
+ brcmf_dbg_dissect_dump(BRCMF_DISSECT_IOCTL, 0, &bcdc->msg, len);
+
return ret;
}
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
index da0cdd3..1dc91ea 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
@@ -782,6 +782,8 @@ int brcmf_sdiod_send_pkt(struct brcmf_sdio_dev *sdiodev,
addr, skb);
if (err)
break;
+ brcmf_dbg_dissect_data_dump(skb->data + 0x1a,
+ skb->len - 0x1a);
}
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 ff825cd..3415170 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
@@ -548,6 +548,7 @@ void brcmf_rx_frame(struct device *dev, struct sk_buff *skb)
/* process and remove protocol-specific header */
ret = brcmf_proto_hdrpull(drvr, true, skb, &ifp);
+ brcmf_dbg_dissect_data_dump(skb->data, skb->len);
if (ret || !ifp || !ifp->ndev) {
if (ret != -ENODATA && ifp)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/debug.c
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/debug.c
index e64557c..d0b078a 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/debug.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/debug.c
@@ -24,6 +24,7 @@
#include "bus.h"
#include "fweh.h"
#include "debug.h"
+#include "tracepoint.h"
static struct dentry *root_folder;
@@ -109,3 +110,56 @@ int brcmf_debugfs_add_entry(struct brcmf_pub
*drvr, const char *fn,
drvr->dbgfs_dir, read_fn);
return PTR_ERR_OR_ZERO(e);
}
+
+static u8 brcmf_dbg_dump_buf[0x0e + BRCMF_DCMD_MAXLEN];
+
+int brcmf_dbg_dissect_dump(int type, int tx, void *data, int len)
+{
+ /* These are ethernet headers with ethertype BC01, BC02, BC03 */
+ static const u8 event_hdr[] = {
+ 0xfc, 0xff, 0xff, 0xff, 0xff, 0xff,
+ 0x00, 0x01, 0x01, 0x01, 0x01, 0x01,
+ 0xbc, 0x01 };
+ static const u8 ioctl_out_hdr[] = {
+ 0x00, 0x01, 0x01, 0x01, 0x01, 0x01,
+ 0xfc, 0xff, 0xff, 0xff, 0xff, 0xff,
+ 0xbc, 0x02 };
+ static const u8 ioctl_in_hdr[] = {
+ 0xfc, 0xff, 0xff, 0xff, 0xff, 0xff,
+ 0x00, 0x01, 0x01, 0x01, 0x01, 0x01,
+ 0xbc, 0x03 };
+ const u8 *hdr;
+
+ if (!trace_brcmf_dissect_hexdump_enabled())
+ return 0;
+
+ switch (type) {
+ case BRCMF_DISSECT_IOCTL:
+ hdr = tx ? ioctl_out_hdr : ioctl_in_hdr;
+ break;
+ case BRCMF_DISSECT_EVENT:
+ /* If data dump is enabled all events will be
+ * logged through data path so don't do it here
+ */
+ if (trace_brcmf_dissect_data_hexdump_enabled())
+ return 0;
+ hdr = event_hdr;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ memcpy(brcmf_dbg_dump_buf, hdr, 0x0e);
+ memcpy(brcmf_dbg_dump_buf + 0x0e, data, len);
+ trace_brcmf_dissect_hexdump(brcmf_dbg_dump_buf, len + 0x0e);
+ return 0;
+}
+
+int brcmf_dbg_dissect_data_dump(void *data, int len)
+{
+ if (!trace_brcmf_dissect_data_hexdump_enabled())
+ return 0;
+
+ trace_brcmf_dissect_data_hexdump(data, len);
+ return 0;
+}
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/debug.h
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/debug.h
index 6687812..b889db8 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/debug.h
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/debug.h
@@ -41,6 +41,9 @@
#define BRCMF_PCIE_VAL 0x00080000
#define BRCMF_FWCON_VAL 0x00100000
+#define BRCMF_DISSECT_IOCTL 0
+#define BRCMF_DISSECT_EVENT 1
+
/* set default print format */
#undef pr_fmt
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
@@ -116,6 +119,8 @@ void brcmf_debug_detach(struct brcmf_pub *drvr);
struct dentry *brcmf_debugfs_get_devdir(struct brcmf_pub *drvr);
int brcmf_debugfs_add_entry(struct brcmf_pub *drvr, const char *fn,
int (*read_fn)(struct seq_file *seq, void *data));
+int brcmf_dbg_dissect_dump(int type, int tx, void *data, int len);
+int brcmf_dbg_dissect_data_dump(void *data, int len);
#else
static inline void brcmf_debugfs_init(void)
{
@@ -136,6 +141,16 @@ int brcmf_debugfs_add_entry(struct brcmf_pub
*drvr, const char *fn,
{
return 0;
}
+static inline
+int brcmf_dbg_dissect_dump(int type, int tx, void *data, int len)
+{
+ return 0;
+}
+static inline
+int brcmf_dbg_dissect_data_dump(void *data, int len)
+{
+ return 0;
+}
#endif
#endif /* BRCMFMAC_DEBUG_H */
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c
index d414fbb..63fe061 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c
@@ -246,6 +246,8 @@ static void brcmf_fweh_event_worker(struct
work_struct *work)
emsg.ifidx = emsg_be->ifidx;
emsg.bsscfgidx = emsg_be->bsscfgidx;
+ brcmf_dbg_dissect_dump(BRCMF_DISSECT_EVENT, 0, &event->emsg,
+ sizeof(event->emsg));
brcmf_dbg(EVENT, " version %u flags %u status %u reason %u\n",
emsg.version, emsg.flags, emsg.status, emsg.reason);
brcmf_dbg_hex_dump(BRCMF_EVENT_ON(), event->data,
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/tracepoint.h
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/tracepoint.h
index 4d7d51f..04b0c34 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/tracepoint.h
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/tracepoint.h
@@ -110,6 +110,39 @@ TRACE_EVENT(brcmf_bcdchdr,
TP_printk("bcdc: prio=%d siglen=%d", __entry->prio, __entry->siglen)
);
+TRACE_EVENT(brcmf_dissect_hexdump,
+ TP_PROTO(void *data, size_t len),
+ TP_ARGS(data, len),
+ TP_STRUCT__entry(
+ __field(unsigned long, len)
+ __field(unsigned long, addr)
+ __dynamic_array(u8, hdata, len)
+ ),
+ TP_fast_assign(
+ __entry->len = len;
+ __entry->addr = (unsigned long)data;
+ memcpy(__get_dynamic_array(hdata), data, len);
+ ),
+ TP_printk("dissect [addr=%lx, length=%lu]", __entry->addr, __entry->len)
+);
+
+TRACE_EVENT(brcmf_dissect_data_hexdump,
+ TP_PROTO(void *data, size_t len),
+ TP_ARGS(data, len),
+ TP_STRUCT__entry(
+ __field(unsigned long, len)
+ __field(unsigned long, addr)
+ __dynamic_array(u8, hdata, len)
+ ),
+ TP_fast_assign(
+ __entry->len = len;
+ __entry->addr = (unsigned long)data;
+ memcpy(__get_dynamic_array(hdata), data, len);
+ ),
+ TP_printk("dissect_data [addr=%lx, length=%lu]",
+ __entry->addr, __entry->len)
+);
+
#ifndef SDPCM_RX
#define SDPCM_RX 0
#endif
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
index 869eb82..f3c42c7 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
@@ -620,6 +620,9 @@ static int brcmf_usb_tx(struct device *dev, struct
sk_buff *skb)
req->devinfo = devinfo;
usb_fill_bulk_urb(req->urb, devinfo->usbdev, devinfo->tx_pipe,
skb->data, skb->len, brcmf_usb_tx_complete, req);
+ brcmf_dbg_dissect_data_dump(skb->data + 0x04, skb->len - 0x04);
req->urb->transfer_flags |= URB_ZERO_PACKET;
brcmf_usb_enq(devinfo, &devinfo->tx_postq, req, NULL);
ret = usb_submit_urb(req->urb, GFP_ATOMIC);
--
2.6.1.213.ga838ae9
On 27-5-2016 16:24, Mikael Kanstrup wrote:
> Add hexdump tracepoints to be used bcmdhd-dissector:
> https://github.com/kanstrup/bcmdhd-dissector
>
> bcmdhd-dissector is a Wireshark LUA plugin dissector used to decode
> protocol data between the brcmfmac driver and the wifi chip firmware.
> This includes decoding firmware command requests and responses as
> well as events and even tx/rx data interleaved if the data_dump
> tracepoint is enabled.
I really don't know what to think of this. Adding tracepoints that do
perform a memcpy() makes me want to push back. Also there are already
tracepoints that capture bus protocol data. So I think it is better to
see if you can use those and use the trace-cmd plugin to add ether
header encapsulation. I have looked at your patch so there are some
comments below.
> Signed-off-by: Mikael Kanstrup <[email protected]>
> ---
> v2:
> - Add description of bcmdhd-dissector as suggested by Rafał
> - Fix tx/rx data dump.
> - Fix tx data dump on USB interface.
>
> Also updated the github project page with some screenshots.
>
> .../wireless/broadcom/brcm80211/brcmfmac/bcdc.c | 4 ++
> .../wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c | 2 +
> .../wireless/broadcom/brcm80211/brcmfmac/core.c | 1 +
> .../wireless/broadcom/brcm80211/brcmfmac/debug.c | 54 ++++++++++++++++++++++
> .../wireless/broadcom/brcm80211/brcmfmac/debug.h | 15 ++++++
> .../wireless/broadcom/brcm80211/brcmfmac/fweh.c | 2 +
> .../broadcom/brcm80211/brcmfmac/tracepoint.h | 33 +++++++++++++
> .../net/wireless/broadcom/brcm80211/brcmfmac/usb.c | 3 ++
> 8 files changed, 114 insertions(+)
>
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c
> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c
> index 6af658e..197942c 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c
> @@ -134,6 +134,8 @@ brcmf_proto_bcdc_msg(struct brcmf_pub *drvr, int
> ifidx, uint cmd, void *buf,
> if (len > BRCMF_TX_IOCTL_MAX_MSG_SIZE)
> len = BRCMF_TX_IOCTL_MAX_MSG_SIZE;
>
> + brcmf_dbg_dissect_dump(BRCMF_DISSECT_IOCTL, 1, &bcdc->msg, len);
> +
Not sure what happened but your indentation has gone bonkers. Better fix it.
> /* Send request */
> return brcmf_bus_txctl(drvr->bus_if, (unsigned char *)&bcdc->msg, len);
> }
> @@ -152,6 +154,8 @@ static int brcmf_proto_bcdc_cmplt(struct brcmf_pub
> *drvr, u32 id, u32 len)
> break;
> } while (BCDC_DCMD_ID(le32_to_cpu(bcdc->msg.flags)) != id);
>
> + brcmf_dbg_dissect_dump(BRCMF_DISSECT_IOCTL, 0, &bcdc->msg, len);
> +
> return ret;
> }
>
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> index da0cdd3..1dc91ea 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> @@ -782,6 +782,8 @@ int brcmf_sdiod_send_pkt(struct brcmf_sdio_dev *sdiodev,
> addr, skb);
> if (err)
> break;
> + brcmf_dbg_dissect_data_dump(skb->data + 0x1a,
> + skb->len - 0x1a);
Better come up with a reasonable define for that 0x1a value. Just seems
like the wrong place to put the trace. Why not in core.c in the
.start_xmit() callback or can this call sleep?
> }
> 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 ff825cd..3415170 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> @@ -548,6 +548,7 @@ void brcmf_rx_frame(struct device *dev, struct sk_buff *skb)
>
> /* process and remove protocol-specific header */
> ret = brcmf_proto_hdrpull(drvr, true, skb, &ifp);
> + brcmf_dbg_dissect_data_dump(skb->data, skb->len);
Again the wrong place. Better put it just before netif_rx() call.
> if (ret || !ifp || !ifp->ndev) {
> if (ret != -ENODATA && ifp)
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/debug.c
> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/debug.c
> index e64557c..d0b078a 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/debug.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/debug.c
> @@ -24,6 +24,7 @@
> #include "bus.h"
> #include "fweh.h"
> #include "debug.h"
> +#include "tracepoint.h"
>
> static struct dentry *root_folder;
>
> @@ -109,3 +110,56 @@ int brcmf_debugfs_add_entry(struct brcmf_pub
> *drvr, const char *fn,
> drvr->dbgfs_dir, read_fn);
> return PTR_ERR_OR_ZERO(e);
> }
> +
> +static u8 brcmf_dbg_dump_buf[0x0e + BRCMF_DCMD_MAXLEN];
Don't use a global. There are people out there who have more than one
brcmfmac device running on their system.
> +int brcmf_dbg_dissect_dump(int type, int tx, void *data, int len)
just make this function void as you only return 0 below in every
possible code path.
> +{
> + /* These are ethernet headers with ethertype BC01, BC02, BC03 */
> + static const u8 event_hdr[] = {
> + 0xfc, 0xff, 0xff, 0xff, 0xff, 0xff,
> + 0x00, 0x01, 0x01, 0x01, 0x01, 0x01,
> + 0xbc, 0x01 };
> + static const u8 ioctl_out_hdr[] = {
> + 0x00, 0x01, 0x01, 0x01, 0x01, 0x01,
> + 0xfc, 0xff, 0xff, 0xff, 0xff, 0xff,
> + 0xbc, 0x02 };
> + static const u8 ioctl_in_hdr[] = {
> + 0xfc, 0xff, 0xff, 0xff, 0xff, 0xff,
> + 0x00, 0x01, 0x01, 0x01, 0x01, 0x01,
> + 0xbc, 0x03 };
> + const u8 *hdr;
> +
> + if (!trace_brcmf_dissect_hexdump_enabled())
> + return 0;
> +
> + switch (type) {
> + case BRCMF_DISSECT_IOCTL:
> + hdr = tx ? ioctl_out_hdr : ioctl_in_hdr;
> + break;
> + case BRCMF_DISSECT_EVENT:
> + /* If data dump is enabled all events will be
> + * logged through data path so don't do it here
> + */
> + if (trace_brcmf_dissect_data_hexdump_enabled())
> + return 0;
> + hdr = event_hdr;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + memcpy(brcmf_dbg_dump_buf, hdr, 0x0e);
Is 0x0e maybe same as ETH_HLEN?
> + memcpy(brcmf_dbg_dump_buf + 0x0e, data, len);
> + trace_brcmf_dissect_hexdump(brcmf_dbg_dump_buf, len + 0x0e);
> + return 0;
> +}
> +
> +int brcmf_dbg_dissect_data_dump(void *data, int len)
make it void.
> +{
> + if (!trace_brcmf_dissect_data_hexdump_enabled())
> + return 0;
> +
> + trace_brcmf_dissect_data_hexdump(data, len);
> + return 0;
> +}
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/debug.h
> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/debug.h
> index 6687812..b889db8 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/debug.h
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/debug.h
> @@ -41,6 +41,9 @@
> #define BRCMF_PCIE_VAL 0x00080000
> #define BRCMF_FWCON_VAL 0x00100000
>
> +#define BRCMF_DISSECT_IOCTL 0
> +#define BRCMF_DISSECT_EVENT 1
> +
> /* set default print format */
> #undef pr_fmt
> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> @@ -116,6 +119,8 @@ void brcmf_debug_detach(struct brcmf_pub *drvr);
> struct dentry *brcmf_debugfs_get_devdir(struct brcmf_pub *drvr);
> int brcmf_debugfs_add_entry(struct brcmf_pub *drvr, const char *fn,
> int (*read_fn)(struct seq_file *seq, void *data));
> +int brcmf_dbg_dissect_dump(int type, int tx, void *data, int len);
> +int brcmf_dbg_dissect_data_dump(void *data, int len);
just make it void functions.
Regards,
Arend
Mikael Kanstrup <[email protected]> writes:
> Add hexdump tracepoints to be used bcmdhd-dissector:
> https://github.com/kanstrup/bcmdhd-dissector
>
> bcmdhd-dissector is a Wireshark LUA plugin dissector used to decode
> protocol data between the brcmfmac driver and the wifi chip firmware.
> This includes decoding firmware command requests and responses as
> well as events and even tx/rx data interleaved if the data_dump
> tracepoint is enabled.
>
> Signed-off-by: Mikael Kanstrup <[email protected]>
> ---
> v2:
> - Add description of bcmdhd-dissector as suggested by Rafał
> - Fix tx/rx data dump.
> - Fix tx data dump on USB interface.
>
> Also updated the github project page with some screenshots.
The patch looks corrupted, did you use git-send-email?
$ git am -s -3 mikael.mbox
Applying: brcmfmac: Add tracepoints for bcmdhd-dissector tool
fatal: corrupt patch at line 31
You can test this yourself by downloading the mbox patchwork (or mailing
it to you directly) and trying apply it to your tree:
https://patchwork.kernel.org/patch/9138515/
--
Kalle Valo