diff mbox series

[v4] cdx: Replace custom mcdi logging with print_hex_dump_debug()

Message ID 20230613065231.19932-1-abhijit.gangurde@amd.com
State New
Headers
Series [v4] cdx: Replace custom mcdi logging with print_hex_dump_debug() |

Commit Message

Gangurde, Abhijit June 13, 2023, 6:52 a.m. UTC 
  Replace custom mcdi logging for send and receive requests
with dynamic debug method print_hex_dump_debug().

Fixes: eb96b740192b ("cdx: add MCDI protocol interface for firmware interaction")
Signed-off-by: Abhijit Gangurde <abhijit.gangurde@amd.com>
Closes: https://lore.kernel.org/lkml/CAMuHMdWTCdQagFFANygMgA8D0sWaoGxWv2AjibC3vwSd0UxuRw@mail.gmail.com/
---
 Changes v3->v4:
 - Custom logging under macro CONFIG_MCDI_LOGGING is replaced with dynamic debug

 Changes v2->v3:
 - Dropped sysfs entry changes from v2

 Changes v1->v2:
 - Moved CONFIG_CDX_MCDI_LOGGING flag to header file
 - Added sysfs entry to enable/disable mcdi logging

 drivers/cdx/controller/Kconfig | 10 ----
 drivers/cdx/controller/mcdi.c  | 86 ++++------------------------------
 drivers/cdx/controller/mcdi.h  |  6 ---
 3 files changed, 9 insertions(+), 93 deletions(-)

Comments

Greg KH June 13, 2023, 7:40 a.m. UTC | #1 
On Tue, Jun 13, 2023 at 12:22:31PM +0530, Abhijit Gangurde wrote:
> Replace custom mcdi logging for send and receive requests
> with dynamic debug method print_hex_dump_debug().
> 
> Fixes: eb96b740192b ("cdx: add MCDI protocol interface for firmware interaction")

This isn't a "fix", right?

> Signed-off-by: Abhijit Gangurde <abhijit.gangurde@amd.com>
> Closes: https://lore.kernel.org/lkml/CAMuHMdWTCdQagFFANygMgA8D0sWaoGxWv2AjibC3vwSd0UxuRw@mail.gmail.com/

This wasn't a "bug report" this was a "the code should be changed to not
do this" type of thing.  Part of normal development.

Other than those nits, looks good, want to fix this up and resend a v5?

thanks,

greg k-h
diff mbox series

Patch

diff --git a/drivers/cdx/controller/Kconfig b/drivers/cdx/controller/Kconfig
index c3e3b9ff8dfe..61bf17fbe433 100644
--- a/drivers/cdx/controller/Kconfig
+++ b/drivers/cdx/controller/Kconfig
@@ -18,14 +18,4 @@  config CDX_CONTROLLER
 
 	  If unsure, say N.
 
-config MCDI_LOGGING
-	bool "MCDI Logging for the CDX controller"
-	depends on CDX_CONTROLLER
-	help
-	  Enable MCDI Logging for
-	  the CDX Controller for debug
-	  purpose.
-
-	  If unsure, say N.
-
 endif
diff --git a/drivers/cdx/controller/mcdi.c b/drivers/cdx/controller/mcdi.c
index a211a2ca762e..1eedc5eeb315 100644
--- a/drivers/cdx/controller/mcdi.c
+++ b/drivers/cdx/controller/mcdi.c
@@ -31,10 +31,6 @@  struct cdx_mcdi_copy_buffer {
 	struct cdx_dword buffer[DIV_ROUND_UP(MCDI_CTL_SDU_LEN_MAX, 4)];
 };
 
-#ifdef CONFIG_MCDI_LOGGING
-#define LOG_LINE_MAX		(1024 - 32)
-#endif
-
 static void cdx_mcdi_cancel_cmd(struct cdx_mcdi *cdx, struct cdx_mcdi_cmd *cmd);
 static void cdx_mcdi_wait_for_cleanup(struct cdx_mcdi *cdx);
 static int cdx_mcdi_rpc_async_internal(struct cdx_mcdi *cdx,
@@ -119,14 +115,9 @@  int cdx_mcdi_init(struct cdx_mcdi *cdx)
 	mcdi = cdx_mcdi_if(cdx);
 	mcdi->cdx = cdx;
 
-#ifdef CONFIG_MCDI_LOGGING
-	mcdi->logging_buffer = kmalloc(LOG_LINE_MAX, GFP_KERNEL);
-	if (!mcdi->logging_buffer)
-		goto fail2;
-#endif
 	mcdi->workqueue = alloc_ordered_workqueue("mcdi_wq", 0);
 	if (!mcdi->workqueue)
-		goto fail3;
+		goto fail2;
 	mutex_init(&mcdi->iface_lock);
 	mcdi->mode = MCDI_MODE_EVENTS;
 	INIT_LIST_HEAD(&mcdi->cmd_list);
@@ -135,11 +126,7 @@  int cdx_mcdi_init(struct cdx_mcdi *cdx)
 	mcdi->new_epoch = true;
 
 	return 0;
-fail3:
-#ifdef CONFIG_MCDI_LOGGING
-	kfree(mcdi->logging_buffer);
 fail2:
-#endif
 	kfree(cdx->mcdi);
 	cdx->mcdi = NULL;
 fail:
@@ -156,10 +143,6 @@  void cdx_mcdi_finish(struct cdx_mcdi *cdx)
 
 	cdx_mcdi_wait_for_cleanup(cdx);
 
-#ifdef CONFIG_MCDI_LOGGING
-	kfree(mcdi->logging_buffer);
-#endif
-
 	destroy_workqueue(mcdi->workqueue);
 	kfree(cdx->mcdi);
 	cdx->mcdi = NULL;
@@ -246,15 +229,9 @@  static void cdx_mcdi_send_request(struct cdx_mcdi *cdx,
 	size_t hdr_len;
 	bool not_epoch;
 	u32 xflags;
-#ifdef CONFIG_MCDI_LOGGING
-	char *buf;
-#endif
 
 	if (!mcdi)
 		return;
-#ifdef CONFIG_MCDI_LOGGING
-	buf = mcdi->logging_buffer; /* page-sized */
-#endif
 
 	mcdi->prev_seq = cmd->seq;
 	mcdi->seq_held_by[cmd->seq] = cmd;
@@ -281,39 +258,12 @@  static void cdx_mcdi_send_request(struct cdx_mcdi *cdx,
 			     MC_CMD_V2_EXTN_IN_MCDI_MESSAGE_TYPE_PLATFORM);
 	hdr_len = 8;
 
-#ifdef CONFIG_MCDI_LOGGING
-	if (!WARN_ON_ONCE(!buf)) {
-		const struct cdx_dword *frags[] = { hdr, inbuf };
-		const size_t frag_len[] = { hdr_len, round_up(inlen, 4) };
-		int bytes = 0;
-		int i, j;
-
-		for (j = 0; j < ARRAY_SIZE(frags); j++) {
-			const struct cdx_dword *frag;
-
-			frag = frags[j];
-			for (i = 0;
-			     i < frag_len[j] / 4;
-			     i++) {
-				/*
-				 * Do not exceed the internal printk limit.
-				 * The string before that is just over 70 bytes.
-				 */
-				if ((bytes + 75) > LOG_LINE_MAX) {
-					pr_info("MCDI RPC REQ:%s \\\n", buf);
-					bytes = 0;
-				}
-				bytes += snprintf(buf + bytes,
-						  LOG_LINE_MAX - bytes, " %08x",
-						  le32_to_cpu(frag[i].cdx_u32));
-			}
-		}
-
-		pr_info("MCDI RPC REQ:%s\n", buf);
-	}
-#endif
 	hdr[0].cdx_u32 |= (__force __le32)(cdx_mcdi_payload_csum(hdr, hdr_len, inbuf, inlen) <<
 			 MCDI_HEADER_XFLAGS_LBN);
+
+	print_hex_dump_debug("MCDI REQ HEADER: ", DUMP_PREFIX_NONE, 32, 4, hdr, hdr_len, false);
+	print_hex_dump_debug("MCDI REQ PAYLOAD: ", DUMP_PREFIX_NONE, 32, 4, inbuf, inlen, false);
+
 	cdx->mcdi_ops->mcdi_request(cdx, hdr, hdr_len, inbuf, inlen);
 
 	mcdi->new_epoch = false;
@@ -700,28 +650,10 @@  static bool cdx_mcdi_complete_cmd(struct cdx_mcdi_iface *mcdi,
 		resp_data_len = 0;
 	}
 
-#ifdef CONFIG_MCDI_LOGGING
-	if (!WARN_ON_ONCE(!mcdi->logging_buffer)) {
-		char *log = mcdi->logging_buffer;
-		int i, bytes = 0;
-		size_t rlen;
-
-		WARN_ON_ONCE(resp_hdr_len % 4);
-
-		rlen = resp_hdr_len / 4 + DIV_ROUND_UP(resp_data_len, 4);
-
-		for (i = 0; i < rlen; i++) {
-			if ((bytes + 75) > LOG_LINE_MAX) {
-				pr_info("MCDI RPC RESP:%s \\\n", log);
-				bytes = 0;
-			}
-			bytes += snprintf(log + bytes, LOG_LINE_MAX - bytes,
-					  " %08x", le32_to_cpu(outbuf[i].cdx_u32));
-		}
-
-		pr_info("MCDI RPC RESP:%s\n", log);
-	}
-#endif
+	print_hex_dump_debug("MCDI RESP HEADER: ", DUMP_PREFIX_NONE, 32, 4,
+			     outbuf, resp_hdr_len, false);
+	print_hex_dump_debug("MCDI RESP PAYLOAD: ", DUMP_PREFIX_NONE, 32, 4,
+			     outbuf + (resp_hdr_len / 4), resp_data_len, false);
 
 	if (error && resp_data_len == 0) {
 		/* MC rebooted during command */
diff --git a/drivers/cdx/controller/mcdi.h b/drivers/cdx/controller/mcdi.h
index 0bfbeab04e43..54a65e9760ae 100644
--- a/drivers/cdx/controller/mcdi.h
+++ b/drivers/cdx/controller/mcdi.h
@@ -153,8 +153,6 @@  struct cdx_mcdi_cmd {
  * @mode: Poll for mcdi completion, or wait for an mcdi_event
  * @prev_seq: The last used sequence number
  * @new_epoch: Indicates start of day or start of MC reboot recovery
- * @logging_buffer: Buffer that may be used to build MCDI tracing messages
- * @logging_enabled: Whether to trace MCDI
  */
 struct cdx_mcdi_iface {
 	struct cdx_mcdi *cdx;
@@ -170,10 +168,6 @@  struct cdx_mcdi_iface {
 	enum cdx_mcdi_mode mode;
 	u8 prev_seq;
 	bool new_epoch;
-#ifdef CONFIG_MCDI_LOGGING
-	bool logging_enabled;
-	char *logging_buffer;
-#endif
 };
 
 /**