Skip to contentSkip to conversation

sfc: add MCDI tracing

PATCH Superseded Edward Cree 2 participants 8 messages May 20, 2015 – May 22, 2015

Subsystems (3)

v2 of 4 Superseded Latest is v4 →

lore
Share & tools
lore
#0 lore raw

v2 cover sfc: add MCDI tracing

This patchset adds support for logging MCDI (Management-Controller-to-
Driver Interface) interactions between the sfc driver and a bound device, to aid in debugging.
Solarflare has a tool to decode the resulting traces and will look to
open-source this if there is any external interest, but the protocol is already detailed in drivers/net/ethernet/sfc/mcdi_pcol.h.

--
v2: avoid long lines in cover letter; fix multiline comment style

Edward Cree (3):
sfc: add tracing of MCDI commands
sfc: add sysfs entry to control MCDI tracing
sfc: add module parameter to enable MCDI logging on new functions

Jon Cooper (1):
sfc: Initialise MCDI buffers to 0 on declaration.

 drivers/net/ethernet/sfc/Kconfig |   9 ++++
 drivers/net/ethernet/sfc/ef10.c  |  27 ++++++----
 drivers/net/ethernet/sfc/efx.c   |  49 +++++++++++++++---
 drivers/net/ethernet/sfc/mcdi.c  | 108 ++++++++++++++++++++++++++++++++++++---
 drivers/net/ethernet/sfc/mcdi.h  |  14 +++--
 drivers/net/ethernet/sfc/ptp.c   |   6 +--
 6 files changed, 181 insertions(+), 32 deletions(-)
#1 lore raw

PATCH v2 1/4 sfc: add tracing of MCDI commands

MCDI tracing is conditional on CONFIG_SFC_MCDI_LOGGING, which is enabled
by default.

Each MCDI command will produce a console line like
sfc dom:bus:dev:fn ifname: MCDI RPC REQ: xxxxxxxx [yyyyyyyy...]
where xxxxxxxx etc. are the raw MCDI payload in 32-bit hex chunks. The response will then produce a similar line with "RESP" instead of "REQ",
and containing the MCDI response payload (if any).

---
 drivers/net/ethernet/sfc/Kconfig |  8 ++++
 drivers/net/ethernet/sfc/ef10.c  |  3 ++
 drivers/net/ethernet/sfc/mcdi.c  | 91 ++++++++++++++++++++++++++++++++++++++--
 drivers/net/ethernet/sfc/mcdi.h  |  4 ++
 4 files changed, 102 insertions(+), 4 deletions(-)
drivers/net/ethernet/sfc/Kconfig +8 -0
index 0889212..c4ba42b 100644
@@ -36,3 +36,11 @@ config SFC_SRIOV
 	  This enables support for the SFC9000 I/O Virtualization
 	  features, allowing accelerated network performance in
 	  virtualized environments.
+config SFC_MCDI_LOGGING
+	bool "Solarflare SFC9000/SFC9100-family MCDI logging support"
+	depends on SFC
+	default y
+	---help---
+	  This enables support for tracing of MCDI (Management-Controller-to-
+	  Driver-Interface) commands and responses, allowing debugging of
+	  driver/firmware interaction.
drivers/net/ethernet/sfc/ef10.c +3 -0
index 882117a..2e8e5de 100644
@@ -219,6 +219,9 @@ static int efx_ef10_probe(struct efx_nic *efx)
 		return -ENOMEM;
 	efx->nic_data = nic_data;
 
+	/* we assume later that we can copy from this buffer in dwords */
+	BUILD_BUG_ON(MCDI_CTL_SDU_LEN_MAX_V2 % 4);
+
 	rc = efx_nic_alloc_buffer(efx, &nic_data->mcdi_buf,
 				  8 + MCDI_CTL_SDU_LEN_MAX_V2, GFP_KERNEL);
 	if (rc)
drivers/net/ethernet/sfc/mcdi.c +87 -4

151 lines not on this page · raw message

drivers/net/ethernet/sfc/mcdi.h +4 -0
index 5df1e98..2f6f032 100644
@@ -58,6 +58,7 @@ enum efx_mcdi_mode {
  *	enabled
  * @async_list: Queue of asynchronous requests
  * @async_timer: Timer for asynchronous request timeout
+ * @logging_buffer: buffer that may be used to build MCDI tracing messages
  */
 struct efx_mcdi_iface {
 	struct efx_nic *efx;
@@ -74,6 +75,9 @@ struct efx_mcdi_iface {
 	spinlock_t async_lock;
 	struct list_head async_list;
 	struct timer_list async_timer;
+#ifdef CONFIG_SFC_MCDI_LOGGING
+	char *logging_buffer;
+#endif
 };
 
 struct efx_mcdi_mon {
#2 lore raw

PATCH v2 2/4 sfc: add sysfs entry to control MCDI tracing

In reply to Edward Cree

MCDI tracing is enabled per-function with a sysfs file
/sys/class/net/<NET_DEV>/device/mcdi_logging

---
 drivers/net/ethernet/sfc/Kconfig |  3 ++-
 drivers/net/ethernet/sfc/efx.c   | 49 ++++++++++++++++++++++++++++++++++------
 drivers/net/ethernet/sfc/mcdi.c  |  4 ++--
 drivers/net/ethernet/sfc/mcdi.h  |  2 ++
 4 files changed, 48 insertions(+), 10 deletions(-)
drivers/net/ethernet/sfc/Kconfig +2 -1
index c4ba42b..4dd92b7 100644
@@ -43,4 +43,5 @@ config SFC_MCDI_LOGGING
 	---help---
 	  This enables support for tracing of MCDI (Management-Controller-to-
 	  Driver-Interface) commands and responses, allowing debugging of
-	  driver/firmware interaction.
+	  driver/firmware interaction.  The tracing is actually enabled by
+	  a sysfs file 'mcdi_logging' under the PCI device.
drivers/net/ethernet/sfc/efx.c +42 -7

70 lines not on this page · raw message

drivers/net/ethernet/sfc/mcdi.c +2 -2
index 1400ed4..3e4137c 100644
@@ -188,7 +188,7 @@ static void efx_mcdi_send_request(struct efx_nic *efx, unsigned cmd,
 	}
 
 #ifdef CONFIG_SFC_MCDI_LOGGING
-	if (!WARN_ON_ONCE(!buf)) {
+	if (mcdi->logging_enabled && !WARN_ON_ONCE(!buf)) {
 		int bytes = 0;
 		int i;
 		/* Lengths should always be a whole number of dwords, so scream
@@ -274,7 +274,7 @@ static void efx_mcdi_read_response_header(struct efx_nic *efx)
 	}
 
 #ifdef CONFIG_SFC_MCDI_LOGGING
-	if (!WARN_ON_ONCE(!buf)) {
+	if (mcdi->logging_enabled && !WARN_ON_ONCE(!buf)) {
 		size_t hdr_len, data_len;
 		int bytes = 0;
 		int i;
drivers/net/ethernet/sfc/mcdi.h +2 -0
index 2f6f032..47035a0 100644
@@ -59,6 +59,7 @@ enum efx_mcdi_mode {
  * @async_list: Queue of asynchronous requests
  * @async_timer: Timer for asynchronous request timeout
  * @logging_buffer: buffer that may be used to build MCDI tracing messages
+ * @logging_enabled: whether to trace MCDI
  */
 struct efx_mcdi_iface {
 	struct efx_nic *efx;
@@ -77,6 +78,7 @@ struct efx_mcdi_iface {
 	struct timer_list async_timer;
 #ifdef CONFIG_SFC_MCDI_LOGGING
 	char *logging_buffer;
+	bool logging_enabled;
 #endif
 };
 
#3 lore raw

PATCH v2 3/4 sfc: add module parameter to enable MCDI logging on new functions

In reply to Edward Cree

As many issues are encountered at probe time, where MCDI logging can't be
enabled through the sysfs node, this change adds a module parameter 'mcdi_logging_default', which defaults to false. When set to true, newly- probed functions will have MCDI logging enabled. The setting can subsequently be changed as normal through the sysfs node.

---
 drivers/net/ethernet/sfc/mcdi.c | 9 +++++++++
 1 file changed, 9 insertions(+)
drivers/net/ethernet/sfc/mcdi.c +9 -0
index 3e4137c..1f7153d 100644
@@ -8,6 +8,7 @@
  */
 
 #include <linux/delay.h>
+#include <linux/moduleparam.h>
 #include <asm/cmpxchg.h>
 #include "net_driver.h"
 #include "nic.h"
@@ -54,6 +55,13 @@ static int efx_mcdi_drv_attach(struct efx_nic *efx, bool driver_operating,
 static bool efx_mcdi_poll_once(struct efx_nic *efx);
 static void efx_mcdi_abandon(struct efx_nic *efx);
 
+#ifdef CONFIG_SFC_MCDI_LOGGING
+static bool mcdi_logging_default;
+module_param(mcdi_logging_default, bool, 0644);
+MODULE_PARM_DESC(mcdi_logging_default,
+		 "Enable MCDI logging on newly-probed functions");
+#endif
+
 int efx_mcdi_init(struct efx_nic *efx)
 {
 	struct efx_mcdi_iface *mcdi;
@@ -71,6 +79,7 @@ int efx_mcdi_init(struct efx_nic *efx)
 	mcdi->logging_buffer = (char *)__get_free_page(GFP_KERNEL);
 	if (!mcdi->logging_buffer)
 		goto fail1;
+	mcdi->logging_enabled = mcdi_logging_default;
 #endif
 	init_waitqueue_head(&mcdi->wq);
 	spin_lock_init(&mcdi->iface_lock);
#4 lore raw

PATCH v2 4/4 sfc: Initialise MCDI buffers to 0 on declaration.

In reply to Edward Cree

From: Jon Cooper <[email protected]>

---
 drivers/net/ethernet/sfc/ef10.c | 24 +++++++++++++-----------
 drivers/net/ethernet/sfc/mcdi.c |  8 ++++----
 drivers/net/ethernet/sfc/mcdi.h |  8 +++++---
 drivers/net/ethernet/sfc/ptp.c  |  6 +++---
 4 files changed, 25 insertions(+), 21 deletions(-)
drivers/net/ethernet/sfc/ef10.c +13 -11
index 2e8e5de..ed0ec88 100644
@@ -350,7 +350,7 @@ static int efx_ef10_probe_vf(struct efx_nic *efx __attribute__ ((unused)))
 
 static int efx_ef10_free_vis(struct efx_nic *efx)
 {
-	MCDI_DECLARE_BUF_OUT_OR_ERR(outbuf, 0);
+	MCDI_DECLARE_BUF_ERR(outbuf);
 	size_t outlen;
 	int rc = efx_mcdi_rpc_quiet(efx, MC_CMD_FREE_VIS, NULL, 0,
 				    outbuf, sizeof(outbuf), &outlen);
@@ -421,7 +421,7 @@ static int efx_ef10_alloc_piobufs(struct efx_nic *efx, unsigned int n)
 static int efx_ef10_link_piobufs(struct efx_nic *efx)
 {
 	struct efx_ef10_nic_data *nic_data = efx->nic_data;
-	MCDI_DECLARE_BUF(inbuf,
+	_MCDI_DECLARE_BUF(inbuf,
 			 max(MC_CMD_LINK_PIOBUF_IN_LEN,
 			     MC_CMD_UNLINK_PIOBUF_IN_LEN));
 	struct efx_channel *channel;
@@ -432,6 +432,8 @@ static int efx_ef10_link_piobufs(struct efx_nic *efx)
 	BUILD_BUG_ON(MC_CMD_LINK_PIOBUF_OUT_LEN != 0);
 	BUILD_BUG_ON(MC_CMD_UNLINK_PIOBUF_OUT_LEN != 0);
 
+	memset(inbuf, 0, sizeof(inbuf));
+
 	/* Link a buffer to each VI in the write-combining mapping */
 	for (index = 0; index < nic_data->n_piobufs; ++index) {
 		MCDI_SET_DWORD(inbuf, LINK_PIOBUF_IN_PIOBUF_HANDLE,
@@ -1315,17 +1317,17 @@ static void efx_ef10_tx_init(struct efx_tx_queue *tx_queue)
 {
 	MCDI_DECLARE_BUF(inbuf, MC_CMD_INIT_TXQ_IN_LEN(EFX_MAX_DMAQ_SIZE * 8 /
 						       EFX_BUF_SIZE));
-	MCDI_DECLARE_BUF(outbuf, MC_CMD_INIT_TXQ_OUT_LEN);
 	bool csum_offload = tx_queue->queue & EFX_TXQ_TYPE_OFFLOAD;
 	size_t entries = tx_queue->txd.buf.len / EFX_BUF_SIZE;
 	struct efx_channel *channel = tx_queue->channel;
 	struct efx_nic *efx = tx_queue->efx;
 	struct efx_ef10_nic_data *nic_data = efx->nic_data;
-	size_t inlen, outlen;
+	size_t inlen;
 	dma_addr_t dma_addr;
 	efx_qword_t *txd;
 	int rc;
 	int i;
+	BUILD_BUG_ON(MC_CMD_INIT_TXQ_OUT_LEN != 0);
 
 	MCDI_SET_DWORD(inbuf, INIT_TXQ_IN_SIZE, tx_queue->ptr_mask + 1);
 	MCDI_SET_DWORD(inbuf, INIT_TXQ_IN_TARGET_EVQ, channel->channel);
@@ -1350,7 +1352,7 @@ static void efx_ef10_tx_init(struct efx_tx_queue *tx_queue)
 	inlen = MC_CMD_INIT_TXQ_IN_LEN(entries);
 
 	rc = efx_mcdi_rpc(efx, MC_CMD_INIT_TXQ, inbuf, inlen,
-			  outbuf, sizeof(outbuf), &outlen);
+			  NULL, 0, NULL);
 	if (rc)
 		goto fail;
 
@@ -1383,7 +1385,7 @@ fail:
 static void efx_ef10_tx_fini(struct efx_tx_queue *tx_queue)
 {
 	MCDI_DECLARE_BUF(inbuf, MC_CMD_FINI_TXQ_IN_LEN);
-	MCDI_DECLARE_BUF(outbuf, MC_CMD_FINI_TXQ_OUT_LEN);
+	MCDI_DECLARE_BUF_ERR(outbuf);
 	struct efx_nic *efx = tx_queue->efx;
 	size_t outlen;
 	int rc;
@@ -1690,15 +1692,15 @@ static void efx_ef10_rx_init(struct efx_rx_queue *rx_queue)
 	MCDI_DECLARE_BUF(inbuf,
 			 MC_CMD_INIT_RXQ_IN_LEN(EFX_MAX_DMAQ_SIZE * 8 /
 						EFX_BUF_SIZE));
-	MCDI_DECLARE_BUF(outbuf, MC_CMD_INIT_RXQ_OUT_LEN);
 	struct efx_channel *channel = efx_rx_queue_channel(rx_queue);
 	size_t entries = rx_queue->rxd.buf.len / EFX_BUF_SIZE;
 	struct efx_nic *efx = rx_queue->efx;
 	struct efx_ef10_nic_data *nic_data = efx->nic_data;
-	size_t inlen, outlen;
+	size_t inlen;
 	dma_addr_t dma_addr;
 	int rc;
 	int i;
+	BUILD_BUG_ON(MC_CMD_INIT_RXQ_OUT_LEN != 0);
 
 	rx_queue->scatter_n = 0;
 	rx_queue->scatter_len = 0;
@@ -1727,7 +1729,7 @@ static void efx_ef10_rx_init(struct efx_rx_queue *rx_queue)
 	inlen = MC_CMD_INIT_RXQ_IN_LEN(entries);
 
 	rc = efx_mcdi_rpc(efx, MC_CMD_INIT_RXQ, inbuf, inlen,
-			  outbuf, sizeof(outbuf), &outlen);
+			  NULL, 0, NULL);
 	if (rc)
 		netdev_WARN(efx->net_dev, "failed to initialise RXQ %d\n",
 			    efx_rx_queue_index(rx_queue));
@@ -1736,7 +1738,7 @@ static void efx_ef10_rx_init(struct efx_rx_queue *rx_queue)
 static void efx_ef10_rx_fini(struct efx_rx_queue *rx_queue)
 {
 	MCDI_DECLARE_BUF(inbuf, MC_CMD_FINI_RXQ_IN_LEN);
-	MCDI_DECLARE_BUF(outbuf, MC_CMD_FINI_RXQ_OUT_LEN);
+	MCDI_DECLARE_BUF_ERR(outbuf);
 	struct efx_nic *efx = rx_queue->efx;
 	size_t outlen;
 	int rc;
@@ -1898,7 +1900,7 @@ static int efx_ef10_ev_init(struct efx_channel *channel)
 static void efx_ef10_ev_fini(struct efx_channel *channel)
 {
 	MCDI_DECLARE_BUF(inbuf, MC_CMD_FINI_EVQ_IN_LEN);
-	MCDI_DECLARE_BUF(outbuf, MC_CMD_FINI_EVQ_OUT_LEN);
+	MCDI_DECLARE_BUF_ERR(outbuf);
 	struct efx_nic *efx = channel->efx;
 	size_t outlen;
 	int rc;
drivers/net/ethernet/sfc/mcdi.c +4 -4
index 1f7153d..612f3752 100644
@@ -498,7 +498,7 @@ static bool efx_mcdi_complete_async(struct efx_mcdi_iface *mcdi, bool timeout)
 	struct efx_mcdi_async_param *async;
 	size_t hdr_len, data_len, err_len;
 	efx_dword_t *outbuf;
-	MCDI_DECLARE_BUF_OUT_OR_ERR(errbuf, 0);
+	MCDI_DECLARE_BUF_ERR(errbuf);
 	int rc;
 
 	if (cmpxchg(&mcdi->state,
@@ -626,7 +626,7 @@ static int _efx_mcdi_rpc_finish(struct efx_nic *efx, unsigned cmd, size_t inlen,
 				size_t *outlen_actual, bool quiet)
 {
 	struct efx_mcdi_iface *mcdi = efx_mcdi(efx);
-	MCDI_DECLARE_BUF_OUT_OR_ERR(errbuf, 0);
+	MCDI_DECLARE_BUF_ERR(errbuf);
 	int rc;
 
 	if (mcdi->mode == MCDI_MODE_POLL)
@@ -1481,7 +1481,7 @@ fail1:
 static int efx_mcdi_read_assertion(struct efx_nic *efx)
 {
 	MCDI_DECLARE_BUF(inbuf, MC_CMD_GET_ASSERTS_IN_LEN);
-	MCDI_DECLARE_BUF_OUT_OR_ERR(outbuf, MC_CMD_GET_ASSERTS_OUT_LEN);
+	MCDI_DECLARE_BUF(outbuf, MC_CMD_GET_ASSERTS_OUT_LEN);
 	unsigned int flags, index;
 	const char *reason;
 	size_t outlen;
@@ -1791,7 +1791,7 @@ int efx_mcdi_set_workaround(struct efx_nic *efx, u32 type, bool enabled)
 int efx_mcdi_get_workarounds(struct efx_nic *efx, unsigned int *impl_out,
 			     unsigned int *enabled_out)
 {
-	MCDI_DECLARE_BUF_OUT_OR_ERR(outbuf, MC_CMD_GET_WORKAROUNDS_OUT_LEN);
+	MCDI_DECLARE_BUF(outbuf, MC_CMD_GET_WORKAROUNDS_OUT_LEN);
 	size_t outlen;
 	int rc;
 
drivers/net/ethernet/sfc/mcdi.h +5 -3
index 47035a0..1838afe 100644
@@ -182,10 +182,12 @@ void efx_mcdi_sensor_event(struct efx_nic *efx, efx_qword_t *ev);
  * 32-bit-aligned.  Also, on Siena we must copy to the MC shared
  * memory strictly 32 bits at a time, so add any necessary padding.
  */
-#define MCDI_DECLARE_BUF(_name, _len)					\
+#define _MCDI_DECLARE_BUF(_name, _len)					\
 	efx_dword_t _name[DIV_ROUND_UP(_len, 4)]
-#define MCDI_DECLARE_BUF_OUT_OR_ERR(_name, _len)			\
-	MCDI_DECLARE_BUF(_name, max_t(size_t, _len, 8))
+#define MCDI_DECLARE_BUF(_name, _len)					\
+	_MCDI_DECLARE_BUF(_name, _len) = {{{0}}}
+#define MCDI_DECLARE_BUF_ERR(_name)					\
+	MCDI_DECLARE_BUF(_name, 8)
 #define _MCDI_PTR(_buf, _offset)					\
 	((u8 *)(_buf) + (_offset))
 #define MCDI_PTR(_buf, _field)						\
drivers/net/ethernet/sfc/ptp.c +3 -3
index 5578c54..ad62615 100644
@@ -306,7 +306,7 @@ struct efx_ptp_data {
 	struct work_struct pps_work;
 	struct workqueue_struct *pps_workwq;
 	bool nic_ts_enabled;
-	MCDI_DECLARE_BUF(txbuf, MC_CMD_PTP_IN_TRANSMIT_LENMAX);
+	_MCDI_DECLARE_BUF(txbuf, MC_CMD_PTP_IN_TRANSMIT_LENMAX);
 
 	unsigned int good_syncs;
 	unsigned int fast_syncs;
@@ -573,7 +573,7 @@ static int efx_ptp_get_timestamp_corrections(struct efx_nic *efx)
 static int efx_ptp_enable(struct efx_nic *efx)
 {
 	MCDI_DECLARE_BUF(inbuf, MC_CMD_PTP_IN_ENABLE_LEN);
-	MCDI_DECLARE_BUF_OUT_OR_ERR(outbuf, 0);
+	MCDI_DECLARE_BUF_ERR(outbuf);
 	int rc;
 
 	MCDI_SET_DWORD(inbuf, PTP_IN_OP, MC_CMD_PTP_OP_ENABLE);
@@ -601,7 +601,7 @@ static int efx_ptp_enable(struct efx_nic *efx)
 static int efx_ptp_disable(struct efx_nic *efx)
 {
 	MCDI_DECLARE_BUF(inbuf, MC_CMD_PTP_IN_DISABLE_LEN);
-	MCDI_DECLARE_BUF_OUT_OR_ERR(outbuf, 0);
+	MCDI_DECLARE_BUF_ERR(outbuf);
 	int rc;
 
 	MCDI_SET_DWORD(inbuf, PTP_IN_OP, MC_CMD_PTP_OP_DISABLE);
maintainer
#5 lore raw
In reply to Edward Cree

From: Edward Cree <[email protected]>
Date: Wed, 20 May 2015 16:49:09 +0100

Quoted 8 lines

This patchset adds support for logging MCDI (Management-Controller-to-
Driver Interface) interactions between the sfc driver and a bound device, to aid in debugging.
Solarflare has a tool to decode the resulting traces and will look to
open-source this if there is any external interest, but the protocol is already detailed in drivers/net/ethernet/sfc/mcdi_pcol.h.

v2: avoid long lines in cover letter; fix multiline comment style

All of this work to allocate the buffer, add messages to it, and dump it to some user obtainable location duplicates existing infrastructure in the kernel.

Please do not do this, and instead use the kernel's existing tracing infrastructure to implement this.

I do not care how much time and effort you've already invested in your existing tool, you should have thought about that when designing this in the first place.

#6 lore raw
Quoted 5 lines · David Miller

All of this work to allocate the buffer, add messages to it, and dump it to some user obtainable location duplicates existing infrastructure in the kernel.
Please do not do this, and instead use the kernel's existing tracing infrastructure to implement this.

The logging_buffer we allocate isn't there to store _messages_ for later dumping, it's a work area for constructing a _single_ message, which is then merely written to the system log with netif_info. Thus, I don't see how it would make much difference if we used (say) ftrace instead of netif_info to ship the events to userspace.
The reason the buffer is long-lived is simply to avoid the overhead of allocating and freeing it every MCDI call, since MCDIs are already known to be serialised for other reasons.
Can you please further explain your objection and what you'd prefer us to do?

maintainer
#7 lore raw

From: Edward Cree <[email protected]>
Date: Fri, 22 May 2015 15:49:46 +0100

Quoted 7 lines · David Miller
Quoted 5 lines

All of this work to allocate the buffer, add messages to it, and dump it to some user obtainable location duplicates existing infrastructure in the kernel.
Please do not do this, and instead use the kernel's existing tracing infrastructure to implement this.

Quoted 10 lines

The logging_buffer we allocate isn't there to store _messages_ for later dumping, it's a work area for constructing a _single_ message, which is then merely written to the system log with netif_info. Thus, I don't see how it would make much difference if we used (say) ftrace instead of netif_info to ship the events to userspace.
The reason the buffer is long-lived is simply to avoid the overhead of allocating and freeing it every MCDI call, since MCDIs are already known to be serialised for other reasons.
Can you please further explain your objection and what you'd prefer us to do?

Ok fair enough, please repost your series, thanks.