[net-next] liquidio: update debug console logging mechanism

Message ID 20170812014314.GA1012@felix-thinkpad.cavium.com
State Accepted
Delegated to: David Miller
Headers show

Commit Message

Felix Manlunas Aug. 12, 2017, 1:43 a.m.
From: Rick Farrington <ricardo.farrington@cavium.com>

- remove logging dependency upon global func octeon_console_debug_enabled()
- abstract debug console logging using console structure (via function ptr)
  to allow for more flexible logging

Signed-off-by: Rick Farrington <ricardo.farrington@cavium.com>
Signed-off-by: Raghu Vatsavayi <raghu.vatsavayi@cavium.com>
Signed-off-by: Felix Manlunas <felix.manlunas@cavium.com>
---
 drivers/net/ethernet/cavium/liquidio/lio_main.c    | 44 +++++++++++++++++-
 .../net/ethernet/cavium/liquidio/octeon_console.c  | 54 ++++++++++++++--------
 .../net/ethernet/cavium/liquidio/octeon_device.h   | 17 +++++--
 3 files changed, 90 insertions(+), 25 deletions(-)

Comments

Leon Romanovsky Aug. 13, 2017, 5:55 a.m. | #1
On Fri, Aug 11, 2017 at 06:43:14PM -0700, Felix Manlunas wrote:
> From: Rick Farrington <ricardo.farrington@cavium.com>
>
> - remove logging dependency upon global func octeon_console_debug_enabled()
> - abstract debug console logging using console structure (via function ptr)
>   to allow for more flexible logging
>
> Signed-off-by: Rick Farrington <ricardo.farrington@cavium.com>
> Signed-off-by: Raghu Vatsavayi <raghu.vatsavayi@cavium.com>
> Signed-off-by: Felix Manlunas <felix.manlunas@cavium.com>
> ---
>  drivers/net/ethernet/cavium/liquidio/lio_main.c    | 44 +++++++++++++++++-
>  .../net/ethernet/cavium/liquidio/octeon_console.c  | 54 ++++++++++++++--------
>  .../net/ethernet/cavium/liquidio/octeon_device.h   | 17 +++++--
>  3 files changed, 90 insertions(+), 25 deletions(-)
>

I'm probably missing something important, but why do you need your custom console
implementation if kernel is full of such built-in options?

Thanks
Ricardo Farrington Aug. 14, 2017, 4:28 p.m. | #2
Hi Leon - the code to which this patch applies handles a data stream from our card's firmware to the host (over PCI).
There are device-specific registers which are accessed to transfer log data from our firmware to the host.
I don't think there is kernel code that we could use to perform this; this is not general purpose host driver logging.

Rick

-----Original Message-----
From: Leon Romanovsky [mailto:leon@kernel.org] 
Sent: Saturday, August 12, 2017 10:56 PM
To: Manlunas, Felix <Felix.Manlunas@cavium.com>
Cc: davem@davemloft.net; netdev@vger.kernel.org; Vatsavayi, Raghu <Raghu.Vatsavayi@cavium.com>; Chickles, Derek <Derek.Chickles@cavium.com>; Burla, Satananda <Satananda.Burla@cavium.com>; Ricardo Farrington <Ricardo.Farrington@cavium.com>
Subject: Re: [PATCH net-next] liquidio: update debug console logging mechanism

On Fri, Aug 11, 2017 at 06:43:14PM -0700, Felix Manlunas wrote:
> From: Rick Farrington <ricardo.farrington@cavium.com>
>
> - remove logging dependency upon global func 
> octeon_console_debug_enabled()
> - abstract debug console logging using console structure (via function ptr)
>   to allow for more flexible logging
>
> Signed-off-by: Rick Farrington <ricardo.farrington@cavium.com>
> Signed-off-by: Raghu Vatsavayi <raghu.vatsavayi@cavium.com>
> Signed-off-by: Felix Manlunas <felix.manlunas@cavium.com>
> ---
>  drivers/net/ethernet/cavium/liquidio/lio_main.c    | 44 +++++++++++++++++-
>  .../net/ethernet/cavium/liquidio/octeon_console.c  | 54 ++++++++++++++--------
>  .../net/ethernet/cavium/liquidio/octeon_device.h   | 17 +++++--
>  3 files changed, 90 insertions(+), 25 deletions(-)
>

I'm probably missing something important, but why do you need your custom console implementation if kernel is full of such built-in options?

Thanks
David Miller Aug. 14, 2017, 5:57 p.m. | #3
From: Felix Manlunas <felix.manlunas@cavium.com>
Date: Fri, 11 Aug 2017 18:43:14 -0700

> From: Rick Farrington <ricardo.farrington@cavium.com>
> 
> - remove logging dependency upon global func octeon_console_debug_enabled()
> - abstract debug console logging using console structure (via function ptr)
>   to allow for more flexible logging
> 
> Signed-off-by: Rick Farrington <ricardo.farrington@cavium.com>
> Signed-off-by: Raghu Vatsavayi <raghu.vatsavayi@cavium.com>
> Signed-off-by: Felix Manlunas <felix.manlunas@cavium.com>

Applied, thanks.
Leon Romanovsky Aug. 15, 2017, 7:22 a.m. | #4
On Mon, Aug 14, 2017 at 04:28:50PM +0000, Ricardo Farrington wrote:
> Hi Leon - the code to which this patch applies handles a data stream from our card's firmware to the host (over PCI).
> There are device-specific registers which are accessed to transfer log data from our firmware to the host.
> I don't think there is kernel code that we could use to perform this; this is not general purpose host driver logging.

I see, it is not really clear form the name "console".

Thanks

Patch

diff --git a/drivers/net/ethernet/cavium/liquidio/lio_main.c b/drivers/net/ethernet/cavium/liquidio/lio_main.c
index cbd6287..a5b752e 100644
--- a/drivers/net/ethernet/cavium/liquidio/lio_main.c
+++ b/drivers/net/ethernet/cavium/liquidio/lio_main.c
@@ -73,7 +73,7 @@ 
  * @param console console to check
  * @returns  1 = enabled. 0 otherwise
  */
-int octeon_console_debug_enabled(u32 console)
+static int octeon_console_debug_enabled(u32 console)
 {
 	return (console_bitmask >> (console)) & 0x1;
 }
@@ -187,6 +187,9 @@  struct octeon_device_priv {
 static int liquidio_enable_sriov(struct pci_dev *dev, int num_vfs);
 #endif
 
+static int octeon_dbg_console_print(struct octeon_device *oct, u32 console_num,
+				    char *prefix, char *suffix);
+
 static int octeon_device_init(struct octeon_device *);
 static int liquidio_stop(struct net_device *netdev);
 static void liquidio_remove(struct pci_dev *pdev);
@@ -4555,6 +4558,7 @@  static int octeon_device_init(struct octeon_device *octeon_dev)
 	int j, ret;
 	int fw_loaded = 0;
 	char bootcmd[] = "\n";
+	char *dbg_enb = NULL;
 	struct octeon_device_priv *oct_priv =
 		(struct octeon_device_priv *)octeon_dev->priv;
 	atomic_set(&octeon_dev->status, OCT_DEV_BEGIN_STATE);
@@ -4761,10 +4765,19 @@  static int octeon_device_init(struct octeon_device *octeon_dev)
 			dev_err(&octeon_dev->pci_dev->dev, "Could not access board consoles\n");
 			return 1;
 		}
-		ret = octeon_add_console(octeon_dev, 0);
+		/* If console debug enabled, specify empty string to use default
+		 * enablement ELSE specify NULL string for 'disabled'.
+		 */
+		dbg_enb = octeon_console_debug_enabled(0) ? "" : NULL;
+		ret = octeon_add_console(octeon_dev, 0, dbg_enb);
 		if (ret) {
 			dev_err(&octeon_dev->pci_dev->dev, "Could not access board console\n");
 			return 1;
+		} else if (octeon_console_debug_enabled(0)) {
+			/* If console was added AND we're logging console output
+			 * then set our console print function.
+			 */
+			octeon_dev->console[0].print = octeon_dbg_console_print;
 		}
 
 		atomic_set(&octeon_dev->status, OCT_DEV_CONSOLE_INIT_DONE);
@@ -4800,6 +4813,33 @@  static int octeon_device_init(struct octeon_device *octeon_dev)
 }
 
 /**
+ * \brief Debug console print function
+ * @param octeon_dev  octeon device
+ * @param console_num console number
+ * @param prefix      first portion of line to display
+ * @param suffix      second portion of line to display
+ *
+ * The OCTEON debug console outputs entire lines (excluding '\n').
+ * Normally, the line will be passed in the 'prefix' parameter.
+ * However, due to buffering, it is possible for a line to be split into two
+ * parts, in which case they will be passed as the 'prefix' parameter and
+ * 'suffix' parameter.
+ */
+static int octeon_dbg_console_print(struct octeon_device *oct, u32 console_num,
+				    char *prefix, char *suffix)
+{
+	if (prefix && suffix)
+		dev_info(&oct->pci_dev->dev, "%u: %s%s\n", console_num, prefix,
+			 suffix);
+	else if (prefix)
+		dev_info(&oct->pci_dev->dev, "%u: %s\n", console_num, prefix);
+	else if (suffix)
+		dev_info(&oct->pci_dev->dev, "%u: %s\n", console_num, suffix);
+
+	return 0;
+}
+
+/**
  * \brief Exits the module
  */
 static void __exit liquidio_exit(void)
diff --git a/drivers/net/ethernet/cavium/liquidio/octeon_console.c b/drivers/net/ethernet/cavium/liquidio/octeon_console.c
index dd0efc9..19e5212 100644
--- a/drivers/net/ethernet/cavium/liquidio/octeon_console.c
+++ b/drivers/net/ethernet/cavium/liquidio/octeon_console.c
@@ -437,20 +437,31 @@  static void output_console_line(struct octeon_device *oct,
 {
 	char *line;
 	s32 i;
+	size_t len;
 
 	line = console_buffer;
 	for (i = 0; i < bytes_read; i++) {
 		/* Output a line at a time, prefixed */
 		if (console_buffer[i] == '\n') {
 			console_buffer[i] = '\0';
-			if (console->leftover[0]) {
-				dev_info(&oct->pci_dev->dev, "%lu: %s%s\n",
-					 console_num, console->leftover,
-					 line);
+			/* We need to output 'line', prefaced by 'leftover'.
+			 * However, it is possible we're being called to
+			 * output 'leftover' by itself (in the case of nothing
+			 * having been read from the console).
+			 *
+			 * To avoid duplication, check for this condition.
+			 */
+			if (console->leftover[0] &&
+			    (line != console->leftover)) {
+				if (console->print)
+					(*console->print)(oct, (u32)console_num,
+							  console->leftover,
+							  line);
 				console->leftover[0] = '\0';
 			} else {
-				dev_info(&oct->pci_dev->dev, "%lu: %s\n",
-					 console_num, line);
+				if (console->print)
+					(*console->print)(oct, (u32)console_num,
+							  line, NULL);
 			}
 			line = &console_buffer[i + 1];
 		}
@@ -459,13 +470,16 @@  static void output_console_line(struct octeon_device *oct,
 	/* Save off any leftovers */
 	if (line != &console_buffer[bytes_read]) {
 		console_buffer[bytes_read] = '\0';
-		strcpy(console->leftover, line);
+		len = strlen(console->leftover);
+		strncpy(&console->leftover[len], line,
+			sizeof(console->leftover) - len);
 	}
 }
 
 static void check_console(struct work_struct *work)
 {
 	s32 bytes_read, tries, total_read;
+	size_t len;
 	struct octeon_console *console;
 	struct cavium_wk *wk = (struct cavium_wk *)work;
 	struct octeon_device *oct = (struct octeon_device *)wk->ctxptr;
@@ -487,7 +501,7 @@  static void check_console(struct work_struct *work)
 			total_read += bytes_read;
 			if (console->waiting)
 				octeon_console_handle_result(oct, console_num);
-			if (octeon_console_debug_enabled(console_num)) {
+			if (console->print) {
 				output_console_line(oct, console, console_num,
 						    console_buffer, bytes_read);
 			}
@@ -502,10 +516,13 @@  static void check_console(struct work_struct *work)
 	/* If nothing is read after polling the console,
 	 * output any leftovers if any
 	 */
-	if (octeon_console_debug_enabled(console_num) &&
-	    (total_read == 0) && (console->leftover[0])) {
-		dev_info(&oct->pci_dev->dev, "%u: %s\n",
-			 console_num, console->leftover);
+	if (console->print && (total_read == 0) &&
+	    (console->leftover[0])) {
+		/* append '\n' as terminator for 'output_console_line' */
+		len = strlen(console->leftover);
+		console->leftover[len] = '\n';
+		output_console_line(oct, console, console_num,
+				    console->leftover, (s32)(len + 1));
 		console->leftover[0] = '\0';
 	}
 
@@ -557,7 +574,8 @@  int octeon_init_consoles(struct octeon_device *oct)
 	return ret;
 }
 
-int octeon_add_console(struct octeon_device *oct, u32 console_num)
+int octeon_add_console(struct octeon_device *oct, u32 console_num,
+		       char *dbg_enb)
 {
 	int ret = 0;
 	u32 delay;
@@ -599,11 +617,11 @@  int octeon_add_console(struct octeon_device *oct, u32 console_num)
 		delay = OCTEON_CONSOLE_POLL_INTERVAL_MS;
 		schedule_delayed_work(work, msecs_to_jiffies(delay));
 
-		if (octeon_console_debug_enabled(console_num)) {
-			ret = octeon_console_send_cmd(oct,
-						      "setenv pci_console_active 1",
-						      2000);
-		}
+		/* an empty string means use default debug console enablement */
+		if (dbg_enb && !dbg_enb[0])
+			dbg_enb = "setenv pci_console_active 1";
+		if (dbg_enb)
+			ret = octeon_console_send_cmd(oct, dbg_enb, 2000);
 
 		console->active = 1;
 	}
diff --git a/drivers/net/ethernet/cavium/liquidio/octeon_device.h b/drivers/net/ethernet/cavium/liquidio/octeon_device.h
index 31efdef..bf51d54 100644
--- a/drivers/net/ethernet/cavium/liquidio/octeon_device.h
+++ b/drivers/net/ethernet/cavium/liquidio/octeon_device.h
@@ -194,6 +194,8 @@  struct octeon_reg_list {
 };
 
 #define OCTEON_CONSOLE_MAX_READ_BYTES 512
+typedef int (*octeon_console_print_fn)(struct octeon_device *oct,
+				       u32 num, char *pre, char *suf);
 struct octeon_console {
 	u32 active;
 	u32 waiting;
@@ -201,6 +203,7 @@  struct octeon_console {
 	u32 buffer_size;
 	u64 input_base_addr;
 	u64 output_base_addr;
+	octeon_console_print_fn print;
 	char leftover[OCTEON_CONSOLE_MAX_READ_BYTES];
 };
 
@@ -739,16 +742,20 @@  int octeon_wait_for_bootloader(struct octeon_device *oct,
  */
 int octeon_init_consoles(struct octeon_device *oct);
 
-int octeon_console_debug_enabled(u32 console);
-
 /**
  * Adds access to a console to the device.
  *
- * @param oct which octeon to add to
- * @param console_num which console
+ * @param oct:          which octeon to add to
+ * @param console_num:  which console
+ * @param dbg_enb:      ptr to debug enablement string, one of:
+ *                    * NULL for no debug output (i.e. disabled)
+ *                    * empty string enables debug output (via default method)
+ *                    * specific string to enable debug console output
+ *
  * @return Zero on success, negative on failure.
  */
-int octeon_add_console(struct octeon_device *oct, u32 console_num);
+int octeon_add_console(struct octeon_device *oct, u32 console_num,
+		       char *dbg_enb);
 
 /** write or read from a console */
 int octeon_console_write(struct octeon_device *oct, u32 console_num,