[PATCH 00/18] log: Add commands for manipulating filters

[PATCH 00/18] log: Add commands for manipulating filters

[Sean Anderson]

This series adds several commands for adding, listing, and removing log filters.
It also adds getopt, since the filter-add command needs to have several
optional arguments to be complete, and positional specification of those
arguments would have been difficult.


Sean Anderson (18):
  log: Fix missing negation of ENOMEM
  log: Fix incorrect documentation of log_filter.cat_list
  log: Add new category names to log_cat_name
  log: Use CONFIG_IS_ENABLED() for LOG_TEST
  log: Expose log_device_find_by_name
  log: Add function to create a filter with flags
  log: Add filter flag to deny on match
  test: Add tests for LOGFF_DENY
  log: Add filter flag to match greater than a log level
  test: Add test for LOGFF_MIN
  cmd: log: Use sub-commands for log
  cmd: log: Split off log level parsing
  lib: Add getopt
  test: Add a test for getopt
  cmd: log: Add commands to manipulate filters
  test: py: Add a test for log filter-*
  doc: Add log kerneldocs to documentation
  doc: Update logging documentation

 MAINTAINERS               |   1 +
 cmd/Kconfig               |   1 +
 cmd/log.c                 | 277 +++++++++++++++++++++++++++++++++-----
 common/log.c              |  37 +++--
 doc/api/getopt.rst        |   8 ++
 doc/api/index.rst         |   1 +
 doc/develop/logging.rst   |  47 +++++--
 include/getopt.h          | 105 +++++++++++++++
 include/log.h             | 166 ++++++++++++++++-------
 lib/Kconfig               |   5 +
 lib/Makefile              |   1 +
 lib/getopt.c              | 125 +++++++++++++++++
 test/lib/Makefile         |   1 +
 test/lib/getopt.c         | 123 +++++++++++++++++
 test/log/log_test.c       |  94 ++++++++++++-
 test/py/tests/test_log.py |  65 ++++++++-
 16 files changed, 951 insertions(+), 106 deletions(-)
 create mode 100644 doc/api/getopt.rst
 create mode 100644 include/getopt.h
 create mode 100644 lib/getopt.c
 create mode 100644 test/lib/getopt.c

-- 
2.28.0

[PATCH 01/18] log: Fix missing negation of ENOMEM

[Sean Anderson]

Errors returned should be negative.

Fixes: 45fac9fc18 ("log: Correct missing free() on error in log_add_filter()")

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---

 common/log.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/common/log.c b/common/log.c
index 9a5f100da3..3f6f4bdc2a 100644
--- a/common/log.c
+++ b/common/log.c
@@ -268,7 +268,7 @@ int log_add_filter(const char *drv_name, enum log_category_t cat_list[],
 	if (file_list) {
 		filt->file_list = strdup(file_list);
 		if (!filt->file_list) {
-			ret = ENOMEM;
+			ret = -ENOMEM;
 			goto err;
 		}
 	}
-- 
2.28.0

[PATCH 01/18] log: Fix missing negation of ENOMEM

[Heinrich Schuchardt]

On 10/6/20 9:15 PM, Sean Anderson wrote:
> Errors returned should be negative.
>
> Fixes: 45fac9fc18 ("log: Correct missing free() on error in log_add_filter()")
>
> Signed-off-by: Sean Anderson <seanga2@gmail.com>
> ---
>
>  common/log.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/common/log.c b/common/log.c
> index 9a5f100da3..3f6f4bdc2a 100644
> --- a/common/log.c
> +++ b/common/log.c
> @@ -268,7 +268,7 @@ int log_add_filter(const char *drv_name, enum log_category_t cat_list[],
>  	if (file_list) {
>  		filt->file_list = strdup(file_list);
>  		if (!filt->file_list) {
> -			ret = ENOMEM;
> +			ret = -ENOMEM;
>  			goto err;
>  		}
>  	}
>

According to the function description for log_add_filter() in
include/log.h errors should be returned as negative numbers.

Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

[PATCH 02/18] log: Fix incorrect documentation of log_filter.cat_list

[Sean Anderson]

This list is expected to be terminated by LOGC_END, not LOGC_NONE.

Fixes: e9c8d49d54 ("log: Add an implementation of logging")

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---

 include/log.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/log.h b/include/log.h
index 2859ce1f2e..670b9a665d 100644
--- a/include/log.h
+++ b/include/log.h
@@ -349,7 +349,7 @@ enum log_filter_flags {
  *	new filter, and must be provided when removing a previously added
  *	filter.
  * @flags: Flags for this filter (LOGFF_...)
- * @cat_list: List of categories to allow (terminated by LOGC_none). If empty
+ * @cat_list: List of categories to allow (terminated by LOGC_END). If empty
  *	then all categories are permitted. Up to LOGF_MAX_CATEGORIES entries
  *	can be provided
  * @max_level: Maximum log level to allow
-- 
2.28.0

[PATCH 02/18] log: Fix incorrect documentation of log_filter.cat_list

[Heinrich Schuchardt]

On 10/6/20 9:15 PM, Sean Anderson wrote:
> This list is expected to be terminated by LOGC_END, not LOGC_NONE.

%s/This list is expected to be/Logging category lists are/

>
> Fixes: e9c8d49d54 ("log: Add an implementation of logging")
>
> Signed-off-by: Sean Anderson <seanga2@gmail.com>
> ---
>
>  include/log.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/log.h b/include/log.h
> index 2859ce1f2e..670b9a665d 100644
> --- a/include/log.h
> +++ b/include/log.h
> @@ -349,7 +349,7 @@ enum log_filter_flags {
>   *	new filter, and must be provided when removing a previously added
>   *	filter.
>   * @flags: Flags for this filter (LOGFF_...)
> - * @cat_list: List of categories to allow (terminated by LOGC_none). If empty
> + * @cat_list: List of categories to allow (terminated by LOGC_END). If empty
>   *	then all categories are permitted. Up to LOGF_MAX_CATEGORIES entries
>   *	can be provided
>   * @max_level: Maximum log level to allow
>

This matches function log_has_cat().

Except for the commit message:

Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

[PATCH 03/18] log: Add new category names to log_cat_name

[Sean Anderson]

Without every category between LOGC_NONE and LOGC_COUNT present in
log_cat_name, log_get_cat_by_name will dereference NULL pointers if it
doesn't find a name early enough.

Fixes: c3aed5db59 ("sandbox: spi: Add more logging")
Fixes: a5c13b68e7 ("sandbox: log: Add a category for sandbox")
Fixes: 9f407d4ef0 ("Add core support for a bloblist to convey data from SPL")
Fixes: cce61fc428 ("dm: devres: Convert to use logging")
Fixes: 7ca2850cbc ("dm: core: Add basic ACPI support")

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---

 common/log.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/common/log.c b/common/log.c
index 3f6f4bdc2a..09e39b0eca 100644
--- a/common/log.c
+++ b/common/log.c
@@ -21,6 +21,11 @@ static const char *log_cat_name[LOGC_COUNT - LOGC_NONE] = {
 	"driver-model",
 	"device-tree",
 	"efi",
+	"alloc",
+	"sandbox",
+	"bloblist",
+	"devres",
+	"acpi",
 };
 
 static const char *log_level_name[LOGL_COUNT] = {
-- 
2.28.0

[PATCH 03/18] log: Add new category names to log_cat_name

[Heinrich Schuchardt]

On 10/6/20 9:15 PM, Sean Anderson wrote:
> Without every category between LOGC_NONE and LOGC_COUNT present in
> log_cat_name, log_get_cat_by_name will dereference NULL pointers if it
> doesn't find a name early enough.
>
> Fixes: c3aed5db59 ("sandbox: spi: Add more logging")
> Fixes: a5c13b68e7 ("sandbox: log: Add a category for sandbox")
> Fixes: 9f407d4ef0 ("Add core support for a bloblist to convey data from SPL")
> Fixes: cce61fc428 ("dm: devres: Convert to use logging")
> Fixes: 7ca2850cbc ("dm: core: Add basic ACPI support")
>
> Signed-off-by: Sean Anderson <seanga2@gmail.com>
> ---
>
>  common/log.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/common/log.c b/common/log.c
> index 3f6f4bdc2a..09e39b0eca 100644
> --- a/common/log.c
> +++ b/common/log.c
> @@ -21,6 +21,11 @@ static const char *log_cat_name[LOGC_COUNT - LOGC_NONE] = {

Please, make the array log_cat_name[] unsized and add a compile time
check like:

#if ARRAY_SIZE(log_cat_name) != LOGC_COUNT - LOGC_NONE
#error missing logging category name
#endif

Best regards

Heinrich

>  	"driver-model",
>  	"device-tree",
>  	"efi",
> +	"alloc",
> +	"sandbox",
> +	"bloblist",
> +	"devres",
> +	"acpi",
>  };
>
>  static const char *log_level_name[LOGL_COUNT] = {
>

[PATCH 04/18] log: Use CONFIG_IS_ENABLED() for LOG_TEST

[Sean Anderson]

Checkpatch complains about using #ifdef for CONFIG variables.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---

 cmd/log.c           | 4 ++--
 test/log/log_test.c | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/cmd/log.c b/cmd/log.c
index 6afe6ead25..16a6ef7539 100644
--- a/cmd/log.c
+++ b/cmd/log.c
@@ -105,7 +105,7 @@ static int do_log_rec(struct cmd_tbl *cmdtp, int flag, int argc,
 
 static struct cmd_tbl log_sub[] = {
 	U_BOOT_CMD_MKENT(level, CONFIG_SYS_MAXARGS, 1, do_log_level, "", ""),
-#ifdef CONFIG_LOG_TEST
+#if CONFIG_IS_ENABLED(LOG_TEST)
 	U_BOOT_CMD_MKENT(test, 2, 1, do_log_test, "", ""),
 #endif
 	U_BOOT_CMD_MKENT(format, CONFIG_SYS_MAXARGS, 1, do_log_format, "", ""),
@@ -133,7 +133,7 @@ static int do_log(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 #ifdef CONFIG_SYS_LONGHELP
 static char log_help_text[] =
 	"level - get/set log level\n"
-#ifdef CONFIG_LOG_TEST
+#if CONFIG_IS_ENABLED(LOG_TEST)
 	"log test - run log tests\n"
 #endif
 	"log format <fmt> - set log output format. <fmt> is a string where\n"
diff --git a/test/log/log_test.c b/test/log/log_test.c
index 4245372d65..787f680971 100644
--- a/test/log/log_test.c
+++ b/test/log/log_test.c
@@ -201,7 +201,7 @@ static int log_test(int testnum)
 	return 0;
 }
 
-#ifdef CONFIG_LOG_TEST
+#if CONFIG_IS_ENABLED(LOG_TEST)
 int do_log_test(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 {
 	int testnum = 0;
-- 
2.28.0

[PATCH 05/18] log: Expose log_device_find_by_name

[Sean Anderson]

This function is required by "cmd: log: Add commands to manipulate filters".

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---

 common/log.c  | 2 +-
 include/log.h | 8 ++++++++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/common/log.c b/common/log.c
index 09e39b0eca..bcb9e2634a 100644
--- a/common/log.c
+++ b/common/log.c
@@ -93,7 +93,7 @@ enum log_level_t log_get_level_by_name(const char *name)
 	return LOGL_NONE;
 }
 
-static struct log_device *log_device_find_by_name(const char *drv_name)
+struct log_device *log_device_find_by_name(const char *drv_name)
 {
 	struct log_device *ldev;
 
diff --git a/include/log.h b/include/log.h
index 670b9a665d..3496382bda 100644
--- a/include/log.h
+++ b/include/log.h
@@ -402,6 +402,14 @@ const char *log_get_level_name(enum log_level_t level);
  */
 enum log_level_t log_get_level_by_name(const char *name);
 
+/**
+ * log_device_find_by_name() - Look up a log device by its driver's name
+ *
+ * @drv_name: Name of the driver
+ * @return the log device, or NULL if not found
+ */
+struct log_device *log_device_find_by_name(const char *drv_name);
+
 /* Log format flags (bit numbers) for gd->log_fmt. See log_fmt_chars */
 enum log_fmt {
 	LOGF_CAT	= 0,
-- 
2.28.0

[PATCH 06/18] log: Add function to create a filter with flags

[Sean Anderson]

This function exposes a way to specify flags when creating a filter.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---

 common/log.c  |  6 ++++--
 include/log.h | 29 +++++++++++++++++++++++++++--
 2 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/common/log.c b/common/log.c
index bcb9e2634a..b42c4fad60 100644
--- a/common/log.c
+++ b/common/log.c
@@ -242,8 +242,9 @@ int _log(enum log_category_t cat, enum log_level_t level, const char *file,
 	return 0;
 }
 
-int log_add_filter(const char *drv_name, enum log_category_t cat_list[],
-		   enum log_level_t max_level, const char *file_list)
+int log_add_filter_flags(const char *drv_name, enum log_category_t cat_list[],
+			 enum log_level_t max_level, const char *file_list,
+			 int flags)
 {
 	struct log_filter *filt;
 	struct log_device *ldev;
@@ -257,6 +258,7 @@ int log_add_filter(const char *drv_name, enum log_category_t cat_list[],
 	if (!filt)
 		return -ENOMEM;
 
+	filt->flags = flags;
 	if (cat_list) {
 		filt->flags |= LOGFF_HAS_CAT;
 		for (i = 0; ; i++) {
diff --git a/include/log.h b/include/log.h
index 3496382bda..9116466fcf 100644
--- a/include/log.h
+++ b/include/log.h
@@ -426,6 +426,25 @@ enum log_fmt {
 /* Handle the 'log test' command */
 int do_log_test(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]);
 
+/**
+ * log_add_filter_flags() - Add a new filter to a log device, specifying flags
+ *
+ * @drv_name: Driver name to add the filter to (since each driver only has a
+ *	single device)
+ * @flags: Flags for this filter (LOGFF_...)
+ * @cat_list: List of categories to allow (terminated by LOGC_none). If empty
+ *	then all categories are permitted. Up to LOGF_MAX_CATEGORIES entries
+ *	can be provided
+ * @max_level: Maximum log level to allow
+ * @file_list: List of files to allow, separated by comma. If NULL then all
+ *	files are permitted
+ * @return the sequence number of the new filter (>=0) if the filter was added,
+ *	or a -ve value on error
+ */
+int log_add_filter_flags(const char *drv_name, enum log_category_t cat_list[],
+			 enum log_level_t max_level, const char *file_list,
+			 int flags);
+
 /**
  * log_add_filter() - Add a new filter to a log device
  *
@@ -440,8 +459,14 @@ int do_log_test(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]);
  * @return the sequence number of the new filter (>=0) if the filter was added,
  *	or a -ve value on error
  */
-int log_add_filter(const char *drv_name, enum log_category_t cat_list[],
-		   enum log_level_t max_level, const char *file_list);
+static inline int log_add_filter(const char *drv_name,
+				 enum log_category_t cat_list[],
+				 enum log_level_t max_level,
+				 const char *file_list)
+{
+	return log_add_filter_flags(drv_name, cat_list, max_level, file_list,
+				    0);
+}
 
 /**
  * log_remove_filter() - Remove a filter from a log device
-- 
2.28.0

[PATCH 07/18] log: Add filter flag to deny on match

[Sean Anderson]

Without this flag, log filters can only explicitly accept messages.
Allowing denial makes it easier to filter certain subsystems. Unlike
allow-ing filters, deny-ing filters are added to the beginning of the
filter list. This should do the Right Thing most of the time, but it's
less-universal than allowing filters to be inserted anywhere. If this
becomes a problem, then perhaps log_filter_add* should take a filter number
to insert before/after.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---

 common/log.c  | 12 ++++++++++--
 include/log.h | 11 ++++++++++-
 2 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/common/log.c b/common/log.c
index b42c4fad60..0b9071ba1c 100644
--- a/common/log.c
+++ b/common/log.c
@@ -178,7 +178,11 @@ static bool log_passes_filters(struct log_device *ldev, struct log_rec *rec)
 		if (filt->file_list &&
 		    !log_has_file(filt->file_list, rec->file))
 			continue;
-		return true;
+
+		if (filt->flags & LOGFF_DENY)
+			return false;
+		else
+			return true;
 	}
 
 	return false;
@@ -280,7 +284,11 @@ int log_add_filter_flags(const char *drv_name, enum log_category_t cat_list[],
 		}
 	}
 	filt->filter_num = ldev->next_filter_num++;
-	list_add_tail(&filt->sibling_node, &ldev->filter_head);
+	/* Add deny filters to the beginning of the list */
+	if (flags & LOGFF_DENY)
+		list_add(&filt->sibling_node, &ldev->filter_head);
+	else
+		list_add_tail(&filt->sibling_node, &ldev->filter_head);
 
 	return filt->filter_num;
 
diff --git a/include/log.h b/include/log.h
index 9116466fcf..00647cafbd 100644
--- a/include/log.h
+++ b/include/log.h
@@ -338,13 +338,22 @@ enum {
 	LOGF_MAX_CATEGORIES = 5,	/* maximum categories per filter */
 };
 
+/**
+ * enum log_filter_flags - Flags which modify a filter
+ */
 enum log_filter_flags {
-	LOGFF_HAS_CAT		= 1 << 0,	/* Filter has a category list */
+	/** @LOGFF_HAS_CAT: Filter has a category list */
+	LOGFF_HAS_CAT	= 1 << 0,
+	/** @LOGFF_DENY: Filter denies matching messages */
+	LOGFF_DENY	= 1 << 1,
 };
 
 /**
  * struct log_filter - criterial to filter out log messages
  *
+ * If a message matches all criteria, then it is allowed. If LOGFF_DENY is set,
+ * then it is denied instead.
+ *
  * @filter_num: Sequence number of this filter.  This is returned when adding a
  *	new filter, and must be provided when removing a previously added
  *	filter.
-- 
2.28.0

[PATCH 08/18] test: Add tests for LOGFF_DENY

[Sean Anderson]

This adds some tests for log filters which deny if they match.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---

 test/log/log_test.c       | 69 +++++++++++++++++++++++++++++++++++++++
 test/py/tests/test_log.py | 21 ++++++++++--
 2 files changed, 88 insertions(+), 2 deletions(-)

diff --git a/test/log/log_test.c b/test/log/log_test.c
index 787f680971..6a71d3db7d 100644
--- a/test/log/log_test.c
+++ b/test/log/log_test.c
@@ -196,6 +196,75 @@ static int log_test(int testnum)
 		log_io("level %d\n", LOGL_DEBUG_IO);
 		break;
 	}
+	case 11: {
+		/* Check denying based on category */
+		int filt1, filt2;
+		enum log_category_t cat_list[] = {
+			log_uc_cat(UCLASS_SPI), LOGC_END
+		};
+
+		ret = log_add_filter("console", cat_list, LOGL_MAX, NULL);
+		if (ret < 0)
+			return ret;
+		filt1 = ret;
+		ret = log_add_filter_flags("console", cat_list, LOGL_MAX, NULL,
+					   LOGFF_DENY);
+		if (ret < 0)
+			return ret;
+		filt2 = ret;
+		log_run(UCLASS_SPI, "file");
+		ret = log_remove_filter("console", filt1);
+		if (ret < 0)
+			return ret;
+		ret = log_remove_filter("console", filt2);
+		if (ret < 0)
+			return ret;
+		break;
+	}
+	case 12: {
+		/* Check denying based on file */
+		int filt1, filt2;
+
+		ret = log_add_filter("console", NULL, LOGL_MAX, "file");
+		if (ret < 0)
+			return ret;
+		filt1 = ret;
+		ret = log_add_filter_flags("console", NULL, LOGL_MAX, "file",
+					   LOGFF_DENY);
+		if (ret < 0)
+			return ret;
+		filt2 = ret;
+		log_run(UCLASS_SPI, "file");
+		ret = log_remove_filter("console", filt1);
+		if (ret < 0)
+			return ret;
+		ret = log_remove_filter("console", filt2);
+		if (ret < 0)
+			return ret;
+		break;
+	}
+	case 13: {
+		/* Check denying based on level */
+		int filt1, filt2;
+
+		ret = log_add_filter("console", NULL, LOGL_INFO, NULL);
+		if (ret < 0)
+			return ret;
+		filt1 = ret;
+		ret = log_add_filter_flags("console", NULL, LOGL_WARNING, NULL,
+					   LOGFF_DENY);
+		if (ret < 0)
+			return ret;
+		filt2 = ret;
+		log_run(UCLASS_SPI, "file");
+		ret = log_remove_filter("console", filt1);
+		if (ret < 0)
+			return ret;
+		ret = log_remove_filter("console", filt2);
+		if (ret < 0)
+			return ret;
+		break;
+	}
 	}
 
 	return 0;
diff --git a/test/py/tests/test_log.py b/test/py/tests/test_log.py
index ddc28f19ee..fabf3001cb 100644
--- a/test/py/tests/test_log.py
+++ b/test/py/tests/test_log.py
@@ -15,7 +15,8 @@ LOGL_FIRST, LOGL_WARNING, LOGL_INFO = (0, 4, 6)
 @pytest.mark.buildconfigspec('cmd_log')
 def test_log(u_boot_console):
     """Test that U-Boot logging works correctly."""
-    def check_log_entries(lines, mask, max_level=LOGL_INFO):
+    def check_log_entries(lines, mask, max_level=LOGL_INFO,
+		          min_level=LOGL_FIRST):
         """Check that the expected log records appear in the output
 
         Args:
@@ -24,8 +25,9 @@ def test_log(u_boot_console):
                 bit 0: standard log line
                 bit 1: _log line
             max_level: maximum log level to expect in the output
+            min_level: minimum log level to expect in the output
         """
-        for i in range(max_level):
+        for i in range(min_level, max_level + 1):
             if mask & 1:
                 assert 'log_run() log %d' % i == next(lines)
             if mask & 3:
@@ -92,6 +94,18 @@ def test_log(u_boot_console):
         for i in range(7):
             assert 'log_test() level %d' % i == next(lines)
 
+    def test11():
+        lines = run_test(11)
+        assert next(lines, None) == None
+
+    def test12():
+        lines = run_test(12)
+        assert next(lines, None) == None
+
+    def test13():
+        lines = run_test(13)
+        check_log_entries(lines, 1, LOGL_INFO, LOGL_WARNING + 1)
+
     # TODO(sjg@chromium.org): Consider structuring this as separate tests
     cons = u_boot_console
     test0()
@@ -105,6 +119,9 @@ def test_log(u_boot_console):
     test8()
     test9()
     test10()
+    test11()
+    test12()
+    test13()
 
 @pytest.mark.buildconfigspec('cmd_log')
 def test_log_format(u_boot_console):
-- 
2.28.0

[PATCH 09/18] log: Add filter flag to match greater than a log level

[Sean Anderson]

This is the compliment of the existing behavior to match only messages with
a log level less than a threshold. This is primarily useful in conjunction
with LOGFF_DENY.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---

 common/log.c  | 12 +++++++++---
 include/log.h | 10 ++++++----
 2 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/common/log.c b/common/log.c
index 0b9071ba1c..a9b039456c 100644
--- a/common/log.c
+++ b/common/log.c
@@ -170,11 +170,17 @@ static bool log_passes_filters(struct log_device *ldev, struct log_rec *rec)
 	}
 
 	list_for_each_entry(filt, &ldev->filter_head, sibling_node) {
-		if (rec->level > filt->max_level)
+		if (filt->flags & LOGFF_LEVEL_MIN) {
+			if (rec->level < filt->level)
+				continue;
+		} else if (rec->level > filt->level) {
 			continue;
+		}
+
 		if ((filt->flags & LOGFF_HAS_CAT) &&
 		    !log_has_cat(filt->cat_list, rec->cat))
 			continue;
+
 		if (filt->file_list &&
 		    !log_has_file(filt->file_list, rec->file))
 			continue;
@@ -247,7 +253,7 @@ int _log(enum log_category_t cat, enum log_level_t level, const char *file,
 }
 
 int log_add_filter_flags(const char *drv_name, enum log_category_t cat_list[],
-			 enum log_level_t max_level, const char *file_list,
+			 enum log_level_t level, const char *file_list,
 			 int flags)
 {
 	struct log_filter *filt;
@@ -275,7 +281,7 @@ int log_add_filter_flags(const char *drv_name, enum log_category_t cat_list[],
 				break;
 		}
 	}
-	filt->max_level = max_level;
+	filt->level = level;
 	if (file_list) {
 		filt->file_list = strdup(file_list);
 		if (!filt->file_list) {
diff --git a/include/log.h b/include/log.h
index 00647cafbd..4e5e3159f6 100644
--- a/include/log.h
+++ b/include/log.h
@@ -346,6 +346,8 @@ enum log_filter_flags {
 	LOGFF_HAS_CAT	= 1 << 0,
 	/** @LOGFF_DENY: Filter denies matching messages */
 	LOGFF_DENY	= 1 << 1,
+	/** @LOGFF_LEVEL_MIN: Filter's level is a minimum, not a maximum */
+	LOGFF_LEVEL_MIN = 1 << 2,
 };
 
 /**
@@ -361,7 +363,7 @@ enum log_filter_flags {
  * @cat_list: List of categories to allow (terminated by LOGC_END). If empty
  *	then all categories are permitted. Up to LOGF_MAX_CATEGORIES entries
  *	can be provided
- * @max_level: Maximum log level to allow
+ * @level: Maximum (or minimum, if LOGFF_MIN_LEVEL) log level to allow
  * @file_list: List of files to allow, separated by comma. If NULL then all
  *	files are permitted
  * @sibling_node: Next filter in the list of filters for this log device
@@ -370,7 +372,7 @@ struct log_filter {
 	int filter_num;
 	int flags;
 	enum log_category_t cat_list[LOGF_MAX_CATEGORIES];
-	enum log_level_t max_level;
+	enum log_level_t level;
 	const char *file_list;
 	struct list_head sibling_node;
 };
@@ -444,14 +446,14 @@ int do_log_test(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]);
  * @cat_list: List of categories to allow (terminated by LOGC_none). If empty
  *	then all categories are permitted. Up to LOGF_MAX_CATEGORIES entries
  *	can be provided
- * @max_level: Maximum log level to allow
+ * @level: Maximum (or minimum, if LOGFF_LEVEL_MIN) log level to allow
  * @file_list: List of files to allow, separated by comma. If NULL then all
  *	files are permitted
  * @return the sequence number of the new filter (>=0) if the filter was added,
  *	or a -ve value on error
  */
 int log_add_filter_flags(const char *drv_name, enum log_category_t cat_list[],
-			 enum log_level_t max_level, const char *file_list,
+			 enum log_level_t level, const char *file_list,
 			 int flags);
 
 /**
-- 
2.28.0

[PATCH 10/18] test: Add test for LOGFF_MIN

[Sean Anderson]

This tests log filters matching on a minimum level.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---

 test/log/log_test.c       | 23 +++++++++++++++++++++++
 test/py/tests/test_log.py |  5 +++++
 2 files changed, 28 insertions(+)

diff --git a/test/log/log_test.c b/test/log/log_test.c
index 6a71d3db7d..da433e332b 100644
--- a/test/log/log_test.c
+++ b/test/log/log_test.c
@@ -265,6 +265,29 @@ static int log_test(int testnum)
 			return ret;
 		break;
 	}
+	case 14: {
+		/* Check matching based on minimum level */
+		int filt1, filt2;
+
+		ret = log_add_filter_flags("console", NULL, LOGL_WARNING, NULL,
+					   LOGFF_LEVEL_MIN);
+		if (ret < 0)
+			return ret;
+		filt1 = ret;
+		ret = log_add_filter_flags("console", NULL, LOGL_INFO, NULL,
+					   LOGFF_DENY | LOGFF_LEVEL_MIN);
+		if (ret < 0)
+			return ret;
+		filt2 = ret;
+		log_run(UCLASS_SPI, "file");
+		ret = log_remove_filter("console", filt1);
+		if (ret < 0)
+			return ret;
+		ret = log_remove_filter("console", filt2);
+		if (ret < 0)
+			return ret;
+		break;
+	}
 	}
 
 	return 0;
diff --git a/test/py/tests/test_log.py b/test/py/tests/test_log.py
index fabf3001cb..f191c84c02 100644
--- a/test/py/tests/test_log.py
+++ b/test/py/tests/test_log.py
@@ -106,6 +106,10 @@ def test_log(u_boot_console):
         lines = run_test(13)
         check_log_entries(lines, 1, LOGL_INFO, LOGL_WARNING + 1)
 
+    def test14():
+        lines = run_test(14)
+        check_log_entries(lines, 1, LOGL_INFO - 1, LOGL_WARNING)
+
     # TODO(sjg@chromium.org): Consider structuring this as separate tests
     cons = u_boot_console
     test0()
@@ -122,6 +126,7 @@ def test_log(u_boot_console):
     test11()
     test12()
     test13()
+    test14()
 
 @pytest.mark.buildconfigspec('cmd_log')
 def test_log_format(u_boot_console):
-- 
2.28.0

[PATCH 11/18] cmd: log: Use sub-commands for log

[Sean Anderson]

This reduces duplicate code, and makes adding new sub-commands easier.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---

 cmd/log.c | 37 +++++++------------------------------
 1 file changed, 7 insertions(+), 30 deletions(-)

diff --git a/cmd/log.c b/cmd/log.c
index 16a6ef7539..e55ace9e14 100644
--- a/cmd/log.c
+++ b/cmd/log.c
@@ -103,33 +103,6 @@ static int do_log_rec(struct cmd_tbl *cmdtp, int flag, int argc,
 	return 0;
 }
 
-static struct cmd_tbl log_sub[] = {
-	U_BOOT_CMD_MKENT(level, CONFIG_SYS_MAXARGS, 1, do_log_level, "", ""),
-#if CONFIG_IS_ENABLED(LOG_TEST)
-	U_BOOT_CMD_MKENT(test, 2, 1, do_log_test, "", ""),
-#endif
-	U_BOOT_CMD_MKENT(format, CONFIG_SYS_MAXARGS, 1, do_log_format, "", ""),
-	U_BOOT_CMD_MKENT(rec, CONFIG_SYS_MAXARGS, 1, do_log_rec, "", ""),
-};
-
-static int do_log(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
-{
-	struct cmd_tbl *cp;
-
-	if (argc < 2)
-		return CMD_RET_USAGE;
-
-	/* drop initial "log" arg */
-	argc--;
-	argv++;
-
-	cp = find_cmd_tbl(argv[0], log_sub, ARRAY_SIZE(log_sub));
-	if (cp)
-		return cp->cmd(cmdtp, flag, argc, argv);
-
-	return CMD_RET_USAGE;
-}
-
 #ifdef CONFIG_SYS_LONGHELP
 static char log_help_text[] =
 	"level - get/set log level\n"
@@ -145,7 +118,11 @@ static char log_help_text[] =
 	;
 #endif
 
-U_BOOT_CMD(
-	log, CONFIG_SYS_MAXARGS, 1, do_log,
-	"log system", log_help_text
+U_BOOT_CMD_WITH_SUBCMDS(log, "log system", log_help_text,
+	U_BOOT_SUBCMD_MKENT(level, 2, 1, do_log_level),
+#if CONFIG_IS_ENABLED(LOG_TEST)
+	U_BOOT_SUBCMD_MKENT(test, 2, 1, do_log_test),
+#endif
+	U_BOOT_SUBCMD_MKENT(format, 2, 1, do_log_format),
+	U_BOOT_SUBCMD_MKENT(rec, 7, 1, do_log_rec),
 );
-- 
2.28.0

[PATCH 12/18] cmd: log: Split off log level parsing

[Sean Anderson]

Move parsing of log level into its own function so it can be re-used. This
also adds support for using log level names instead of just the integer
equivalent.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---

 cmd/log.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/cmd/log.c b/cmd/log.c
index e55ace9e14..71bfdbbc0f 100644
--- a/cmd/log.c
+++ b/cmd/log.c
@@ -11,23 +11,36 @@
 
 static char log_fmt_chars[LOGF_COUNT] = "clFLfm";
 
+static enum log_level_t parse_log_level(char *const arg)
+{
+	ulong level;
+
+	if (!strict_strtoul(arg, 10, &level)) {
+		if (level > _LOG_MAX_LEVEL) {
+			printf("Only log levels <= %d are supported\n",
+			       _LOG_MAX_LEVEL);
+			return LOGL_NONE;
+		}
+		return level;
+	}
+
+	return log_get_level_by_name(arg);
+}
+
 static int do_log_level(struct cmd_tbl *cmdtp, int flag, int argc,
 			char *const argv[])
 {
 	if (argc > 1) {
-		long log_level = simple_strtol(argv[1], NULL, 10);
+		enum log_level_t log_level = parse_log_level(argv[1]);
 
-		if (log_level < 0 || log_level > _LOG_MAX_LEVEL) {
-			printf("Only log levels <= %d are supported\n",
-			       _LOG_MAX_LEVEL);
+		if (log_level == LOGL_NONE)
 			return CMD_RET_FAILURE;
-		}
 		gd->default_log_level = log_level;
 	} else {
 		printf("Default log level: %d\n", gd->default_log_level);
 	}
 
-	return 0;
+	return CMD_RET_SUCCESS;
 }
 
 static int do_log_format(struct cmd_tbl *cmdtp, int flag, int argc,
-- 
2.28.0

[PATCH 13/18] lib: Add getopt

[Sean Anderson]

Some commands can get very unweildy if they have too many positional
arguments. Adding options makes them easier to read, remember, and
understand.

This implementation of getopt has been taken from barebox, which has had
option support for quite a while. I have made a few modifications to their
version, such as the removal of opterr in favor of a separate getopt_silent
function. In addition, I have moved all global variables into struct
getopt_context.

The getopt from barebox also re-orders the arguments passed to it so that
non-options are placed last. This allows users to specify options anywhere.
For example, `ls -l foo/ -R` would be re-ordered to `ls -l -R foo/` as
getopt parsed the options. However, this feature conflicts with the const
argv in cmd_tbl->cmd. This was originally added in 54841ab50c ("Make sure
that argv[] argument pointers are not modified."). The reason stated in
that commit is that hush requires argv to stay unmodified. Has this
situation changed? Barebox also uses hush, and does not have this problem.
Perhaps we could use their fix?

I have assigned maintenance of getopt to Simon Glass, as it is currently
only used by the log command. I would also be fine maintaining it.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---

 MAINTAINERS        |   1 +
 doc/api/getopt.rst |   8 +++
 doc/api/index.rst  |   1 +
 include/getopt.h   | 105 +++++++++++++++++++++++++++++++++++++
 lib/Kconfig        |   5 ++
 lib/Makefile       |   1 +
 lib/getopt.c       | 125 +++++++++++++++++++++++++++++++++++++++++++++
 7 files changed, 246 insertions(+)
 create mode 100644 doc/api/getopt.rst
 create mode 100644 include/getopt.h
 create mode 100644 lib/getopt.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 85babd1908..45ef4321d7 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -747,6 +747,7 @@ T:	git https://gitlab.denx.de/u-boot/u-boot.git
 F:	common/log*
 F:	cmd/log.c
 F:	doc/develop/logging.rst
+F:	lib/getopt.c
 F:	test/log/
 F:	test/py/tests/test_log.py
 
diff --git a/doc/api/getopt.rst b/doc/api/getopt.rst
new file mode 100644
index 0000000000..773f79aeb6
--- /dev/null
+++ b/doc/api/getopt.rst
@@ -0,0 +1,8 @@
+.. SPDX-License-Identifier: GPL-2.0+
+.. Copyright (C) 2020 Sean Anderson <seanga2@gmail.com>
+
+Option Parsing
+==============
+
+.. kernel-doc:: include/getopt.h
+   :internal:
diff --git a/doc/api/index.rst b/doc/api/index.rst
index b7eb5725f2..6c849c50b6 100644
--- a/doc/api/index.rst
+++ b/doc/api/index.rst
@@ -8,6 +8,7 @@ U-Boot API documentation
 
    dfu
    efi
+   getopt
    linker_lists
    rng
    serial
diff --git a/include/getopt.h b/include/getopt.h
new file mode 100644
index 0000000000..186a3c231e
--- /dev/null
+++ b/include/getopt.h
@@ -0,0 +1,105 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * getopt.h - a simple getopt(3) implementation.
+ *
+ * Copyright (C) 2020 Sean Anderson <seanga2@gmail.com>
+ * Copyright (c) 2007 Sascha Hauer <s.hauer@pengutronix.de>, Pengutronix
+ */
+
+#ifndef __GETOPT_H
+#define __GETOPT_H
+
+/**
+ * struct getopt_state - Saved state across getopt() calls
+ */
+struct getopt_state {
+	/** @optind: Index of the next unparsed argument of argv */
+	int optind;
+	/* private: */
+	/** @optindex: Index within the current argument */
+	int optindex;
+	union {
+		/* public: */
+		/** @optopt: Option being parsed when an error occurs */
+		int optopt;
+		/** @optarg: The argument to an option, NULL if there is none */
+		char *optarg;
+	/* private: */
+	};
+};
+
+/**
+ * getopt_init_state() - Initialize a &struct getopt_state
+ * @gs: The state to initialize
+ *
+ * This must be called before using @gs with getopt().
+ */
+void getopt_init_state(struct getopt_state *gs);
+
+int __getopt(struct getopt_state *gs, int argc, char *const argv[],
+	     const char *optstring, bool silent);
+
+/**
+ * getopt() - Parse short command-line options
+ * @gs: Internal state and out-of-band return arguments. This must be
+ *      initialized with getopt_init_context() beforehand.
+ * @argc: Number of arguments
+ * @argv: Argument list
+ * @optstring: Option specification, as described below
+ *
+ * getopt() parses short options. Short options are single characters. They may
+ * be followed by a required argument or an optional argument. Arguments to
+ * options may occur in the same argument as an option (like ``-loptarg``), or
+ * in the following argument (like ``-l optarg``). An argument containing
+ * options begins with a ``-``. If an option expects no arguments, then it may
+ * be immediately followed by another option (like ``ls -alR``).
+ *
+ * @optstring is a list of accepted options. If an option is followed by ``:``
+ * in @optstring, then it expects a mandatory argument. If an option is followed
+ * by ``::`` in @optstring, it expects an optional argument. @gs.optarg points
+ * to the argument, if one is parsed.
+ *
+ * getopt() stops parsing options when it encounters the first non-option
+ * argument, when it encounters the argument ``--``, or when it runs out of
+ * arguments. For example, in ``ls -l foo -R``, option parsing will stop when
+ * getopt() encounters ``foo``, if ``l`` does not expect an argument. However,
+ * the whole list of arguments would be parsed if ``l`` expects an argument.
+ *
+ * For further information, refer to the getopt(3) man page.
+ *
+ * Return:
+ * * An option character if an option is found. @gs.optarg is set to the
+ *   argument if there is one, otherwise it is set to ``NULL``.
+ * * ``-1`` if there are no more options, if a non-option argument is
+ *   encountered, or if an ``--`` argument is encountered.
+ * * ``'?'`` if we encounter an option not in @optstring. @gs.optopt is set to
+ *   the unknown option.
+ * * ``':'`` if an argument is required, but no argument follows the
+ *   option. @gs.optopt is set to the option missing its argument.
+ *
+ * @gs.optind is always set to the index of the next unparsed argument in @argv.
+ */
+static inline int getopt(struct getopt_state *gs, int argc,
+			 char *const argv[], const char *optstring)
+{
+	return __getopt(gs, argc, argv, optstring, false);
+}
+
+/**
+ * getopt_silent() - Parse short command-line options
+ * @gs: Internal state and out-of-band return arguments. This must be
+ *      initialized with getopt_init_context() beforehand.
+ * @argc: Number of arguments
+ * @argv: Argument list
+ * @optstring: Option specification
+ *
+ * Arguments and return values are the same as getopt(), except no error
+ * messages are printed.
+ */
+static inline int getopt_silent(struct getopt_state *gs, int argc,
+				char *const argv[], const char *optstring)
+{
+	return __getopt(gs, argc, argv, optstring, true);
+}
+
+#endif /* __GETOPT_H */
diff --git a/lib/Kconfig b/lib/Kconfig
index 8efb154f73..a3346eee04 100644
--- a/lib/Kconfig
+++ b/lib/Kconfig
@@ -542,6 +542,11 @@ config HEXDUMP
 	help
 	  This enables functions for printing dumps of binary data.
 
+config GETOPT
+	bool "Enable getopt"
+	help
+	  This enables functions for parsing command-line options.
+
 config OF_LIBFDT
 	bool "Enable the FDT library"
 	default y if OF_CONTROL
diff --git a/lib/Makefile b/lib/Makefile
index 0cd7bea282..7c7fb9aae7 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -106,6 +106,7 @@ obj-y += string.o
 obj-y += tables_csum.o
 obj-y += time.o
 obj-y += hexdump.o
+obj-$(CONFIG_GETOPT) += getopt.o
 obj-$(CONFIG_TRACE) += trace.o
 obj-$(CONFIG_LIB_UUID) += uuid.o
 obj-$(CONFIG_LIB_RAND) += rand.o
diff --git a/lib/getopt.c b/lib/getopt.c
new file mode 100644
index 0000000000..7a4bb9c150
--- /dev/null
+++ b/lib/getopt.c
@@ -0,0 +1,125 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * getopt.c - a simple getopt(3) implementation. See getopt.h for explanation.
+ *
+ * Copyright (C) 2020 Sean Anderson <seanga2@gmail.com>
+ * Copyright (c) 2007 Sascha Hauer <s.hauer@pengutronix.de>, Pengutronix
+ */
+
+#define LOG_CATEGORY LOGC_CORE
+
+#include <common.h>
+#include <getopt.h>
+#include <log.h>
+
+void getopt_init_state(struct getopt_state *gs)
+{
+	gs->optind = 1;
+	gs->optindex = 1;
+}
+
+int __getopt(struct getopt_state *gs, int argc, char *const argv[],
+	     const char *optstring, bool silent)
+{
+	char curopt;   /* current option character */
+	const char *curoptp; /* pointer to the current option in optstring */
+
+	while (1) {
+		log_debug("optindex: %d optind: %d\n", gs->optindex,
+			  gs->optind);
+
+		/* `--` indicates the end of options */
+		if (gs->optindex == 1 && argv[gs->optind] &&
+		    !strcmp(argv[gs->optind], "--")) {
+			gs->optind++;
+			return -1;
+		}
+
+		/* Out of arguments */
+		if (gs->optind >= argc)
+			return -1;
+
+		/* Can't parse non-options */
+		if (*argv[gs->optind] != '-')
+			return -1;
+
+		/* We have found an option */
+		curopt = argv[gs->optind][gs->optindex];
+		if (curopt)
+			break;
+		/*
+		 * no more options in current argv[] element; try the next one
+		 */
+		gs->optind++;
+		gs->optindex = 1;
+	}
+
+	/* look up current option in optstring */
+	curoptp = strchr(optstring, curopt);
+
+	if (!curoptp) {
+		if (!silent)
+			printf("%s: invalid option -- %c\n", argv[0], curopt);
+		gs->optopt = curopt;
+		gs->optindex++;
+		return '?';
+	}
+
+	if (*(curoptp + 1) != ':') {
+		/* option with no argument. Just return it */
+		gs->optarg = NULL;
+		gs->optindex++;
+		return curopt;
+	}
+
+	if (*(curoptp + 1) && *(curoptp + 2) == ':') {
+		/* optional argument */
+		if (argv[gs->optind][gs->optindex + 1]) {
+			/* optional argument with directly following optarg */
+			gs->optarg = argv[gs->optind++] + gs->optindex + 1;
+			gs->optindex = 1;
+			return curopt;
+		}
+		if (gs->optind + 1 == argc) {
+			/* We are at the last argv[] element */
+			gs->optarg = NULL;
+			gs->optind++;
+			return curopt;
+		}
+		if (*argv[gs->optind + 1] != '-') {
+			/*
+			 * optional argument with optarg in next argv[] element
+			 */
+			gs->optind++;
+			gs->optarg = argv[gs->optind++];
+			gs->optindex = 1;
+			return curopt;
+		}
+
+		/* no optional argument found */
+		gs->optarg = NULL;
+		gs->optindex = 1;
+		gs->optind++;
+		return curopt;
+	}
+
+	if (argv[gs->optind][gs->optindex + 1]) {
+		/* required argument with directly following optarg */
+		gs->optarg = argv[gs->optind++] + gs->optindex + 1;
+		gs->optindex = 1;
+		return curopt;
+	}
+
+	gs->optind++;
+	gs->optindex = 1;
+
+	if (gs->optind >= argc || argv[gs->optind][0] == '-') {
+		if (!silent)
+			printf("option requires an argument -- %c\n", curopt);
+		gs->optopt = curopt;
+		return ':';
+	}
+
+	gs->optarg = argv[gs->optind++];
+	return curopt;
+}
-- 
2.28.0

[PATCH 14/18] test: Add a test for getopt

[Sean Anderson]

A few of these tests were inspired by those in glibc. The syntax for
invoking test_getopt is a bit funky, but it's necessary so that the CPP can
parse the arguments correctly.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---

 test/lib/Makefile |   1 +
 test/lib/getopt.c | 123 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 124 insertions(+)
 create mode 100644 test/lib/getopt.c

diff --git a/test/lib/Makefile b/test/lib/Makefile
index 22236f8587..3140c2dad7 100644
--- a/test/lib/Makefile
+++ b/test/lib/Makefile
@@ -13,3 +13,4 @@ obj-$(CONFIG_ERRNO_STR) += test_errno_str.o
 obj-$(CONFIG_UT_LIB_ASN1) += asn1.o
 obj-$(CONFIG_UT_LIB_RSA) += rsa.o
 obj-$(CONFIG_AES) += test_aes.o
+obj-$(CONFIG_GETOPT) += getopt.o
diff --git a/test/lib/getopt.c b/test/lib/getopt.c
new file mode 100644
index 0000000000..b46fbf6255
--- /dev/null
+++ b/test/lib/getopt.c
@@ -0,0 +1,123 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2020 Sean Anderson <seanga2@gmail.com>
+ *
+ * Portions of these tests were inspired by glibc's posix/bug-getopt1.c and
+ * posix/tst-getopt-cancel.c
+ */
+
+#include <common.h>
+#include <getopt.h>
+#include <test/lib.h>
+#include <test/test.h>
+#include <test/ut.h>
+
+static int do_test_getopt(struct unit_test_state *uts, int line,
+			  struct getopt_state *gs, const char *optstring,
+			  int args, char *argv[], int expected_count,
+			  int expected[])
+{
+	int opt;
+
+	getopt_init_state(gs);
+	for (int i = 0; i < expected_count; i++) {
+		opt = getopt_silent(gs, args, argv, optstring);
+		if (expected[i] != opt) {
+			/*
+			 * Fudge the line number so we can tell which test
+			 * failed
+			 */
+			ut_failf(uts, __FILE__, line, __func__,
+				 "expected[i] == getopt()",
+				 "Expected '%c' (%d) with i=%d, got '%c' (%d)",
+				 expected[i], expected[i], i, opt, opt);
+			return CMD_RET_FAILURE;
+		}
+	}
+
+	opt = getopt_silent(gs, args, argv, optstring);
+	if (opt != -1) {
+		ut_failf(uts, __FILE__, line, __func__,
+			 "getopt() != -1",
+			 "Expected -1, got '%c' (%d)", opt, opt);
+		return CMD_RET_FAILURE;
+	}
+
+	return 0;
+}
+
+#define test_getopt(optstring, argv, expected) do { \
+	int ret = do_test_getopt(uts, __LINE__, &gs, optstring, \
+				 ARRAY_SIZE(argv) - 1, argv, \
+				 ARRAY_SIZE(expected), expected); \
+	if (ret) \
+		return ret; \
+} while (0)
+
+static int lib_test_getopt(struct unit_test_state *uts)
+{
+	struct getopt_state gs;
+
+	/* Happy path */
+	test_getopt("ab:c",
+		    ((char *[]){ "program", "-cb", "x", "-a", "foo", 0 }),
+		    ((int []){ 'c', 'b', 'a' }));
+	ut_asserteq(4, gs.optind);
+
+	/* Make sure we pick up the optional argument */
+	test_getopt("a::b:c",
+		    ((char *[]){ "program", "-cbx", "-a", "foo", 0 }),
+		    ((int []){ 'c', 'b', 'a' }));
+	ut_asserteq(4, gs.optind);
+
+	/* Test required arguments */
+	test_getopt("a:b", ((char *[]){ "program", "-a", 0 }),
+		    ((int []){ ':' }));
+	ut_asserteq('a', gs.optopt);
+	test_getopt("a:b", ((char *[]){ "program", "-b", "-a", 0 }),
+		    ((int []){ 'b', ':' }));
+	ut_asserteq('a', gs.optopt);
+
+	/* Test invalid arguments */
+	test_getopt("ab:c", ((char *[]){ "program", "-d", 0 }),
+		    ((int []){ '?' }));
+	ut_asserteq('d', gs.optopt);
+
+	/* Test optarg */
+	test_getopt("a::b:c",
+		    ((char *[]){ "program", "-a", 0 }),
+		    ((int []){ 'a' }));
+	ut_asserteq(2, gs.optind);
+	ut_assertnull(gs.optarg);
+
+	test_getopt("a::b:c",
+		    ((char *[]){ "program", "-afoo", 0 }),
+		    ((int []){ 'a' }));
+	ut_asserteq(2, gs.optind);
+	ut_assertnonnull(gs.optarg);
+	ut_asserteq_str("foo", gs.optarg);
+
+	test_getopt("a::b:c",
+		    ((char *[]){ "program", "-a", "foo", 0 }),
+		    ((int []){ 'a' }));
+	ut_asserteq(3, gs.optind);
+	ut_assertnonnull(gs.optarg);
+	ut_asserteq_str("foo", gs.optarg);
+
+	test_getopt("a::b:c",
+		    ((char *[]){ "program", "-bfoo", 0 }),
+		    ((int []){ 'b' }));
+	ut_asserteq(2, gs.optind);
+	ut_assertnonnull(gs.optarg);
+	ut_asserteq_str("foo", gs.optarg);
+
+	test_getopt("a::b:c",
+		    ((char *[]){ "program", "-b", "foo", 0 }),
+		    ((int []){ 'b' }));
+	ut_asserteq(3, gs.optind);
+	ut_assertnonnull(gs.optarg);
+	ut_asserteq_str("foo", gs.optarg);
+
+	return 0;
+}
+LIB_TEST(lib_test_getopt, 0);
-- 
2.28.0

[PATCH 15/18] cmd: log: Add commands to manipulate filters

[Sean Anderson]

This adds several commands to add, list, and remove log filters. Due to the
complexity of adding a filter, `log filter-list` uses options instead of
positional arguments.

These commands have been added as subcommands to log by using a dash to
join the subcommand and subsubcommand. This is stylistic, and they could be
converted to proper subsubcommands if it is wished.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---

 cmd/Kconfig |   1 +
 cmd/log.c   | 213 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 214 insertions(+)

diff --git a/cmd/Kconfig b/cmd/Kconfig
index 999b6cf239..c5cb112171 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -2255,6 +2255,7 @@ config CMD_KGDB
 config CMD_LOG
 	bool "log - Generation, control and access to logging"
 	select LOG
+	select GETOPT
 	help
 	  This provides access to logging features. It allows the output of
 	  log data to be controlled to a limited extent (setting up the default
diff --git a/cmd/log.c b/cmd/log.c
index 71bfdbbc0f..c9eaff7b94 100644
--- a/cmd/log.c
+++ b/cmd/log.c
@@ -7,6 +7,7 @@
 #include <common.h>
 #include <command.h>
 #include <dm.h>
+#include <getopt.h>
 #include <log.h>
 
 static char log_fmt_chars[LOGF_COUNT] = "clFLfm";
@@ -43,6 +44,195 @@ static int do_log_level(struct cmd_tbl *cmdtp, int flag, int argc,
 	return CMD_RET_SUCCESS;
 }
 
+static int do_log_filter_list(struct cmd_tbl *cmdtp, int flag, int argc,
+			      char *const argv[])
+{
+	int opt;
+	const char *drv_name = "console";
+	struct getopt_state gs;
+	struct log_filter *filt;
+	struct log_device *ldev;
+
+	getopt_init_state(&gs);
+	while ((opt = getopt(&gs, argc, argv, "d:")) > 0) {
+		switch (opt) {
+		case 'd':
+			drv_name = gs.optarg;
+			break;
+		default:
+			return CMD_RET_USAGE;
+		}
+	}
+
+	if (gs.optind != argc)
+		return CMD_RET_USAGE;
+
+	ldev = log_device_find_by_name(drv_name);
+	if (!ldev) {
+		printf("Could not find log device for \"%s\"\n", drv_name);
+		return CMD_RET_FAILURE;
+	}
+
+	printf("num policy level           categories files\n");
+	list_for_each_entry(filt, &ldev->filter_head, sibling_node) {
+		printf("%3d %6.6s %s%-7.7s ", filt->filter_num,
+		       filt->flags & LOGFF_DENY ? "deny" : "allow",
+		       filt->flags & LOGFF_LEVEL_MIN ? ">=" : "<=",
+		       log_get_level_name(filt->level));
+
+		if (filt->flags & LOGFF_HAS_CAT) {
+			int i;
+
+			if (filt->cat_list[0] != LOGC_END)
+				printf("%16.16s %s\n",
+				       log_get_cat_name(filt->cat_list[0]),
+				       filt->file_list ? filt->file_list : "");
+
+			for (i = 1; i < LOGF_MAX_CATEGORIES &&
+				    filt->cat_list[i] != LOGC_END; i++)
+				printf("%20c %16.16s\n", ' ',
+				       log_get_cat_name(filt->cat_list[i]));
+		} else {
+			printf("%16c %s\n", ' ',
+			       filt->file_list ? filt->file_list : "");
+		}
+	}
+
+	return CMD_RET_SUCCESS;
+}
+
+static int do_log_filter_add(struct cmd_tbl *cmdtp, int flag, int argc,
+			     char *const argv[])
+{
+	bool level_set = false;
+	bool print_num = false;
+	bool type_set = false;
+	char *file_list = NULL;
+	const char *drv_name = "console";
+	int opt, err;
+	int cat_count = 0;
+	int flags = 0;
+	enum log_category_t cat_list[LOGF_MAX_CATEGORIES + 1];
+	enum log_level_t level = LOGL_MAX;
+	struct getopt_state gs;
+
+	getopt_init_state(&gs);
+	while ((opt = getopt(&gs, argc, argv, "Ac:d:Df:l:L:p")) > 0) {
+		switch (opt) {
+		case 'A':
+#define do_type() do { \
+			if (type_set) { \
+				printf("Allow or deny set twice\n"); \
+				return CMD_RET_USAGE; \
+			} \
+			type_set = true; \
+} while (0)
+			do_type();
+			break;
+		case 'c': {
+			enum log_category_t cat;
+
+			if (cat_count >= LOGF_MAX_CATEGORIES) {
+				printf("Too many categories\n");
+				return CMD_RET_FAILURE;
+			}
+
+			cat = log_get_cat_by_name(gs.optarg);
+			if (cat == LOGC_NONE) {
+				printf("Unknown category \"%s\"\n", gs.optarg);
+				return CMD_RET_FAILURE;
+			}
+
+			cat_list[cat_count++] = cat;
+			break;
+		}
+		case 'd':
+			drv_name = gs.optarg;
+			break;
+		case 'D':
+			do_type();
+			flags |= LOGFF_DENY;
+			break;
+		case 'f':
+			file_list = gs.optarg;
+			break;
+		case 'l':
+#define do_level() do { \
+			if (level_set) { \
+				printf("Log level set twice\n"); \
+				return CMD_RET_USAGE; \
+			} \
+			level = parse_log_level(gs.optarg); \
+			if (level == LOGL_NONE) \
+				return CMD_RET_FAILURE; \
+			level_set = true; \
+} while (0)
+			do_level();
+			break;
+		case 'L':
+			do_level();
+			flags |= LOGFF_LEVEL_MIN;
+			break;
+		case 'p':
+			print_num = true;
+			break;
+		default:
+			return CMD_RET_USAGE;
+		}
+	}
+
+	if (gs.optind != argc)
+		return CMD_RET_USAGE;
+
+	cat_list[cat_count] = LOGC_END;
+	err = log_add_filter_flags(drv_name, cat_count ? cat_list : NULL, level,
+				   file_list, flags);
+	if (err < 0) {
+		printf("Could not add filter (err = %d)\n", err);
+		return CMD_RET_FAILURE;
+	} else if (print_num) {
+		printf("%d\n", err);
+	}
+
+	return CMD_RET_SUCCESS;
+}
+
+static int do_log_filter_remove(struct cmd_tbl *cmdtp, int flag, int argc,
+				char *const argv[])
+{
+	int opt, err;
+	ulong filter_num;
+	const char *drv_name = "console";
+	struct getopt_state gs;
+
+	getopt_init_state(&gs);
+	while ((opt = getopt(&gs, argc, argv, "d:")) > 0) {
+		switch (opt) {
+		case 'd':
+			drv_name = gs.optarg;
+			break;
+		default:
+			return CMD_RET_USAGE;
+		}
+	}
+
+	if (gs.optind + 1 != argc)
+		return CMD_RET_USAGE;
+
+	if (strict_strtoul(argv[gs.optind], 10, &filter_num)) {
+		printf("Invalid filter number \"%s\"\n", argv[gs.optind]);
+		return CMD_RET_FAILURE;
+	}
+
+	err = log_remove_filter(drv_name, filter_num);
+	if (err) {
+		printf("Could not remove filter (err = %d)\n", err);
+		return CMD_RET_FAILURE;
+	}
+
+	return CMD_RET_SUCCESS;
+}
+
 static int do_log_format(struct cmd_tbl *cmdtp, int flag, int argc,
 			 char *const argv[])
 {
@@ -122,6 +312,25 @@ static char log_help_text[] =
 #if CONFIG_IS_ENABLED(LOG_TEST)
 	"log test - run log tests\n"
 #endif
+	"log filter-list [OPTIONS] - list all filters for a log driver\n"
+	"\t-d <driver> - Specify the log driver to add the filter to; defaults\n"
+	"\t              to console\n"
+	"log filter-add [OPTIONS] - add a new filter to a driver\n"
+	"\t-A - Allow messages matching this filter; mutually exclusive with -D\n"
+	"\t     This is the default.\n"
+	"\t-c <category> - Category to match; may be specified multiple times\n"
+	"\t-d <driver> - Specify the log driver to add the filter to; defaults\n"
+	"\t              to console\n"
+	"\t-D - Deny messages matching this filter; mutually exclusive with -A\n"
+	"\t-f <files_list> - A comma-separated list of files to match\n"
+	"\t-l <level> - Match log levels below this level; mutually exclusive\n"
+	"\t             with -L\n"
+	"\t-L <level> - Match log levels above this level; mutually exclusive\n"
+	"\t             with -l\n"
+	"\t-p - Print the filter number on success\n"
+	"log filter-remove [OPTIONS] <num> - Remove filter number <num>\n"
+	"\t-d <driver> - Specify the log driver to add the filter to; defaults\n"
+	"\t              to console\n"
 	"log format <fmt> - set log output format. <fmt> is a string where\n"
 	"\teach letter indicates something that should be displayed:\n"
 	"\tc=category, l=level, F=file, L=line number, f=function, m=msg\n"
@@ -136,6 +345,10 @@ U_BOOT_CMD_WITH_SUBCMDS(log, "log system", log_help_text,
 #if CONFIG_IS_ENABLED(LOG_TEST)
 	U_BOOT_SUBCMD_MKENT(test, 2, 1, do_log_test),
 #endif
+	U_BOOT_SUBCMD_MKENT(filter-list, 3, 1, do_log_filter_list),
+	U_BOOT_SUBCMD_MKENT(filter-add, CONFIG_SYS_MAXARGS, 1,
+			    do_log_filter_add),
+	U_BOOT_SUBCMD_MKENT(filter-remove, 4, 1, do_log_filter_remove),
 	U_BOOT_SUBCMD_MKENT(format, 2, 1, do_log_format),
 	U_BOOT_SUBCMD_MKENT(rec, 7, 1, do_log_rec),
 );
-- 
2.28.0

[PATCH 15/18] cmd: log: Add commands to manipulate filters

[Heinrich Schuchardt]

On 10/6/20 9:16 PM, Sean Anderson wrote:
> This adds several commands to add, list, and remove log filters. Due to the
> complexity of adding a filter, `log filter-list` uses options instead of
> positional arguments.
>
> These commands have been added as subcommands to log by using a dash to
> join the subcommand and subsubcommand. This is stylistic, and they could be
> converted to proper subsubcommands if it is wished.
>
> Signed-off-by: Sean Anderson <seanga2@gmail.com>
> ---
>
>  cmd/Kconfig |   1 +
>  cmd/log.c   | 213 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 214 insertions(+)
>
> diff --git a/cmd/Kconfig b/cmd/Kconfig
> index 999b6cf239..c5cb112171 100644
> --- a/cmd/Kconfig
> +++ b/cmd/Kconfig
> @@ -2255,6 +2255,7 @@ config CMD_KGDB
>  config CMD_LOG
>  	bool "log - Generation, control and access to logging"
>  	select LOG
> +	select GETOPT
>  	help
>  	  This provides access to logging features. It allows the output of
>  	  log data to be controlled to a limited extent (setting up the default
> diff --git a/cmd/log.c b/cmd/log.c
> index 71bfdbbc0f..c9eaff7b94 100644
> --- a/cmd/log.c
> +++ b/cmd/log.c
> @@ -7,6 +7,7 @@
>  #include <common.h>
>  #include <command.h>
>  #include <dm.h>
> +#include <getopt.h>
>  #include <log.h>
>
>  static char log_fmt_chars[LOGF_COUNT] = "clFLfm";
> @@ -43,6 +44,195 @@ static int do_log_level(struct cmd_tbl *cmdtp, int flag, int argc,
>  	return CMD_RET_SUCCESS;
>  }
>
> +static int do_log_filter_list(struct cmd_tbl *cmdtp, int flag, int argc,
> +			      char *const argv[])
> +{
> +	int opt;
> +	const char *drv_name = "console";
> +	struct getopt_state gs;
> +	struct log_filter *filt;
> +	struct log_device *ldev;
> +
> +	getopt_init_state(&gs);
> +	while ((opt = getopt(&gs, argc, argv, "d:")) > 0) {
> +		switch (opt) {
> +		case 'd':
> +			drv_name = gs.optarg;
> +			break;
> +		default:
> +			return CMD_RET_USAGE;
> +		}
> +	}
> +
> +	if (gs.optind != argc)
> +		return CMD_RET_USAGE;
> +
> +	ldev = log_device_find_by_name(drv_name);
> +	if (!ldev) {
> +		printf("Could not find log device for \"%s\"\n", drv_name);
> +		return CMD_RET_FAILURE;
> +	}
> +
> +	printf("num policy level           categories files\n");
> +	list_for_each_entry(filt, &ldev->filter_head, sibling_node) {
> +		printf("%3d %6.6s %s%-7.7s ", filt->filter_num,
> +		       filt->flags & LOGFF_DENY ? "deny" : "allow",
> +		       filt->flags & LOGFF_LEVEL_MIN ? ">=" : "<=",
> +		       log_get_level_name(filt->level));
> +
> +		if (filt->flags & LOGFF_HAS_CAT) {
> +			int i;
> +
> +			if (filt->cat_list[0] != LOGC_END)
> +				printf("%16.16s %s\n",
> +				       log_get_cat_name(filt->cat_list[0]),
> +				       filt->file_list ? filt->file_list : "");
> +
> +			for (i = 1; i < LOGF_MAX_CATEGORIES &&
> +				    filt->cat_list[i] != LOGC_END; i++)
> +				printf("%20c %16.16s\n", ' ',
> +				       log_get_cat_name(filt->cat_list[i]));
> +		} else {
> +			printf("%16c %s\n", ' ',
> +			       filt->file_list ? filt->file_list : "");
> +		}
> +	}
> +
> +	return CMD_RET_SUCCESS;
> +}
> +
> +static int do_log_filter_add(struct cmd_tbl *cmdtp, int flag, int argc,
> +			     char *const argv[])
> +{
> +	bool level_set = false;
> +	bool print_num = false;
> +	bool type_set = false;
> +	char *file_list = NULL;
> +	const char *drv_name = "console";
> +	int opt, err;
> +	int cat_count = 0;
> +	int flags = 0;
> +	enum log_category_t cat_list[LOGF_MAX_CATEGORIES + 1];
> +	enum log_level_t level = LOGL_MAX;
> +	struct getopt_state gs;
> +
> +	getopt_init_state(&gs);
> +	while ((opt = getopt(&gs, argc, argv, "Ac:d:Df:l:L:p")) > 0) {
> +		switch (opt) {
> +		case 'A':
> +#define do_type() do { \
> +			if (type_set) { \
> +				printf("Allow or deny set twice\n"); \
> +				return CMD_RET_USAGE; \
> +			} \
> +			type_set = true; \
> +} while (0)
> +			do_type();
> +			break;
> +		case 'c': {
> +			enum log_category_t cat;
> +
> +			if (cat_count >= LOGF_MAX_CATEGORIES) {
> +				printf("Too many categories\n");
> +				return CMD_RET_FAILURE;
> +			}
> +
> +			cat = log_get_cat_by_name(gs.optarg);
> +			if (cat == LOGC_NONE) {
> +				printf("Unknown category \"%s\"\n", gs.optarg);
> +				return CMD_RET_FAILURE;
> +			}
> +
> +			cat_list[cat_count++] = cat;
> +			break;
> +		}
> +		case 'd':
> +			drv_name = gs.optarg;
> +			break;
> +		case 'D':
> +			do_type();
> +			flags |= LOGFF_DENY;
> +			break;
> +		case 'f':
> +			file_list = gs.optarg;
> +			break;
> +		case 'l':
> +#define do_level() do { \
> +			if (level_set) { \
> +				printf("Log level set twice\n"); \
> +				return CMD_RET_USAGE; \
> +			} \
> +			level = parse_log_level(gs.optarg); \
> +			if (level == LOGL_NONE) \
> +				return CMD_RET_FAILURE; \
> +			level_set = true; \
> +} while (0)
> +			do_level();
> +			break;
> +		case 'L':
> +			do_level();
> +			flags |= LOGFF_LEVEL_MIN;
> +			break;
> +		case 'p':
> +			print_num = true;
> +			break;
> +		default:
> +			return CMD_RET_USAGE;
> +		}
> +	}
> +
> +	if (gs.optind != argc)
> +		return CMD_RET_USAGE;
> +
> +	cat_list[cat_count] = LOGC_END;
> +	err = log_add_filter_flags(drv_name, cat_count ? cat_list : NULL, level,
> +				   file_list, flags);
> +	if (err < 0) {
> +		printf("Could not add filter (err = %d)\n", err);
> +		return CMD_RET_FAILURE;
> +	} else if (print_num) {
> +		printf("%d\n", err);
> +	}
> +
> +	return CMD_RET_SUCCESS;
> +}
> +
> +static int do_log_filter_remove(struct cmd_tbl *cmdtp, int flag, int argc,
> +				char *const argv[])
> +{
> +	int opt, err;
> +	ulong filter_num;
> +	const char *drv_name = "console";
> +	struct getopt_state gs;
> +
> +	getopt_init_state(&gs);
> +	while ((opt = getopt(&gs, argc, argv, "d:")) > 0) {
> +		switch (opt) {
> +		case 'd':
> +			drv_name = gs.optarg;
> +			break;
> +		default:
> +			return CMD_RET_USAGE;
> +		}
> +	}
> +
> +	if (gs.optind + 1 != argc)
> +		return CMD_RET_USAGE;
> +
> +	if (strict_strtoul(argv[gs.optind], 10, &filter_num)) {
> +		printf("Invalid filter number \"%s\"\n", argv[gs.optind]);
> +		return CMD_RET_FAILURE;
> +	}
> +
> +	err = log_remove_filter(drv_name, filter_num);
> +	if (err) {
> +		printf("Could not remove filter (err = %d)\n", err);
> +		return CMD_RET_FAILURE;
> +	}
> +
> +	return CMD_RET_SUCCESS;
> +}
> +
>  static int do_log_format(struct cmd_tbl *cmdtp, int flag, int argc,
>  			 char *const argv[])
>  {
> @@ -122,6 +312,25 @@ static char log_help_text[] =
>  #if CONFIG_IS_ENABLED(LOG_TEST)
>  	"log test - run log tests\n"

This is probably what a typical user is least interested in. Please,
move it to the end of the description.

>  #endif
> +	"log filter-list [OPTIONS] - list all filters for a log driver\n"
> +	"\t-d <driver> - Specify the log driver to add the filter to; defaults\n"> +	"\t              to console\n"

The text seems not to relate to filter-list.

Why don't we filter all log drivers by default? When would a user be
interested to filter by driver?

How should I know which drivers and categories exist on my system?

> +	"log filter-add [OPTIONS] - add a new filter to a driver\n"
> +	"\t-A - Allow messages matching this filter; mutually exclusive with -D\n"
> +	"\t     This is the default.\n"
> +	"\t-c <category> - Category to match; may be specified multiple times\n"
> +	"\t-d <driver> - Specify the log driver to add the filter to; defaults\n"
> +	"\t              to console\n"
> +	"\t-D - Deny messages matching this filter; mutually exclusive with -A\n"
> +	"\t-f <files_list> - A comma-separated list of files to match\n"
> +	"\t-l <level> - Match log levels below this level; mutually exclusive\n"
> +	"\t             with -L\n"

Is this below or below and equal?

> +	"\t-L <level> - Match log levels above this level; mutually exclusive\n"
> +	"\t             with -l\n"

Is this above or above and equal?

> +	"\t-p - Print the filter number on success\n"

Why do we need this parameter? Can't we simply always print the filter
numbers or never?

> +	"log filter-remove [OPTIONS] <num> - Remove filter number <num>\n"
> +	"\t-d <driver> - Specify the log driver to add the filter to; defaults\n"
> +	"\t              to console\n"

This entry seems not to be related to removal.

How do I simply delete all filters?
How do I remove all filters for a category?

Best regards

Heinrich

>  	"log format <fmt> - set log output format. <fmt> is a string where\n"
>  	"\teach letter indicates something that should be displayed:\n"
>  	"\tc=category, l=level, F=file, L=line number, f=function, m=msg\n"
> @@ -136,6 +345,10 @@ U_BOOT_CMD_WITH_SUBCMDS(log, "log system", log_help_text,
>  #if CONFIG_IS_ENABLED(LOG_TEST)
>  	U_BOOT_SUBCMD_MKENT(test, 2, 1, do_log_test),
>  #endif
> +	U_BOOT_SUBCMD_MKENT(filter-list, 3, 1, do_log_filter_list),
> +	U_BOOT_SUBCMD_MKENT(filter-add, CONFIG_SYS_MAXARGS, 1,
> +			    do_log_filter_add),
> +	U_BOOT_SUBCMD_MKENT(filter-remove, 4, 1, do_log_filter_remove),
>  	U_BOOT_SUBCMD_MKENT(format, 2, 1, do_log_format),
>  	U_BOOT_SUBCMD_MKENT(rec, 7, 1, do_log_rec),
>  );
>

[PATCH 15/18] cmd: log: Add commands to manipulate filters

[Sean Anderson]

On 10/6/20 5:14 PM, Heinrich Schuchardt wrote:
> On 10/6/20 9:16 PM, Sean Anderson wrote:
>> This adds several commands to add, list, and remove log filters. Due to the
>> complexity of adding a filter, `log filter-list` uses options instead of
>> positional arguments.
>>
>> These commands have been added as subcommands to log by using a dash to
>> join the subcommand and subsubcommand. This is stylistic, and they could be
>> converted to proper subsubcommands if it is wished.
>>
>> Signed-off-by: Sean Anderson <seanga2@gmail.com>
>> ---
>>
>>  cmd/Kconfig |   1 +
>>  cmd/log.c   | 213 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 214 insertions(+)
>>
>> diff --git a/cmd/Kconfig b/cmd/Kconfig
>> index 999b6cf239..c5cb112171 100644
>> --- a/cmd/Kconfig
>> +++ b/cmd/Kconfig
>> @@ -2255,6 +2255,7 @@ config CMD_KGDB
>>  config CMD_LOG
>>  	bool "log - Generation, control and access to logging"
>>  	select LOG
>> +	select GETOPT
>>  	help
>>  	  This provides access to logging features. It allows the output of
>>  	  log data to be controlled to a limited extent (setting up the default
>> diff --git a/cmd/log.c b/cmd/log.c
>> index 71bfdbbc0f..c9eaff7b94 100644
>> --- a/cmd/log.c
>> +++ b/cmd/log.c
>> @@ -7,6 +7,7 @@
>>  #include <common.h>
>>  #include <command.h>
>>  #include <dm.h>
>> +#include <getopt.h>
>>  #include <log.h>
>>
>>  static char log_fmt_chars[LOGF_COUNT] = "clFLfm";
>> @@ -43,6 +44,195 @@ static int do_log_level(struct cmd_tbl *cmdtp, int flag, int argc,
>>  	return CMD_RET_SUCCESS;
>>  }
>>
>> +static int do_log_filter_list(struct cmd_tbl *cmdtp, int flag, int argc,
>> +			      char *const argv[])
>> +{
>> +	int opt;
>> +	const char *drv_name = "console";
>> +	struct getopt_state gs;
>> +	struct log_filter *filt;
>> +	struct log_device *ldev;
>> +
>> +	getopt_init_state(&gs);
>> +	while ((opt = getopt(&gs, argc, argv, "d:")) > 0) {
>> +		switch (opt) {
>> +		case 'd':
>> +			drv_name = gs.optarg;
>> +			break;
>> +		default:
>> +			return CMD_RET_USAGE;
>> +		}
>> +	}
>> +
>> +	if (gs.optind != argc)
>> +		return CMD_RET_USAGE;
>> +
>> +	ldev = log_device_find_by_name(drv_name);
>> +	if (!ldev) {
>> +		printf("Could not find log device for \"%s\"\n", drv_name);
>> +		return CMD_RET_FAILURE;
>> +	}
>> +
>> +	printf("num policy level           categories files\n");
>> +	list_for_each_entry(filt, &ldev->filter_head, sibling_node) {
>> +		printf("%3d %6.6s %s%-7.7s ", filt->filter_num,
>> +		       filt->flags & LOGFF_DENY ? "deny" : "allow",
>> +		       filt->flags & LOGFF_LEVEL_MIN ? ">=" : "<=",
>> +		       log_get_level_name(filt->level));
>> +
>> +		if (filt->flags & LOGFF_HAS_CAT) {
>> +			int i;
>> +
>> +			if (filt->cat_list[0] != LOGC_END)
>> +				printf("%16.16s %s\n",
>> +				       log_get_cat_name(filt->cat_list[0]),
>> +				       filt->file_list ? filt->file_list : "");
>> +
>> +			for (i = 1; i < LOGF_MAX_CATEGORIES &&
>> +				    filt->cat_list[i] != LOGC_END; i++)
>> +				printf("%20c %16.16s\n", ' ',
>> +				       log_get_cat_name(filt->cat_list[i]));
>> +		} else {
>> +			printf("%16c %s\n", ' ',
>> +			       filt->file_list ? filt->file_list : "");
>> +		}
>> +	}
>> +
>> +	return CMD_RET_SUCCESS;
>> +}
>> +
>> +static int do_log_filter_add(struct cmd_tbl *cmdtp, int flag, int argc,
>> +			     char *const argv[])
>> +{
>> +	bool level_set = false;
>> +	bool print_num = false;
>> +	bool type_set = false;
>> +	char *file_list = NULL;
>> +	const char *drv_name = "console";
>> +	int opt, err;
>> +	int cat_count = 0;
>> +	int flags = 0;
>> +	enum log_category_t cat_list[LOGF_MAX_CATEGORIES + 1];
>> +	enum log_level_t level = LOGL_MAX;
>> +	struct getopt_state gs;
>> +
>> +	getopt_init_state(&gs);
>> +	while ((opt = getopt(&gs, argc, argv, "Ac:d:Df:l:L:p")) > 0) {
>> +		switch (opt) {
>> +		case 'A':
>> +#define do_type() do { \
>> +			if (type_set) { \
>> +				printf("Allow or deny set twice\n"); \
>> +				return CMD_RET_USAGE; \
>> +			} \
>> +			type_set = true; \
>> +} while (0)
>> +			do_type();
>> +			break;
>> +		case 'c': {
>> +			enum log_category_t cat;
>> +
>> +			if (cat_count >= LOGF_MAX_CATEGORIES) {
>> +				printf("Too many categories\n");
>> +				return CMD_RET_FAILURE;
>> +			}
>> +
>> +			cat = log_get_cat_by_name(gs.optarg);
>> +			if (cat == LOGC_NONE) {
>> +				printf("Unknown category \"%s\"\n", gs.optarg);
>> +				return CMD_RET_FAILURE;
>> +			}
>> +
>> +			cat_list[cat_count++] = cat;
>> +			break;
>> +		}
>> +		case 'd':
>> +			drv_name = gs.optarg;
>> +			break;
>> +		case 'D':
>> +			do_type();
>> +			flags |= LOGFF_DENY;
>> +			break;
>> +		case 'f':
>> +			file_list = gs.optarg;
>> +			break;
>> +		case 'l':
>> +#define do_level() do { \
>> +			if (level_set) { \
>> +				printf("Log level set twice\n"); \
>> +				return CMD_RET_USAGE; \
>> +			} \
>> +			level = parse_log_level(gs.optarg); \
>> +			if (level == LOGL_NONE) \
>> +				return CMD_RET_FAILURE; \
>> +			level_set = true; \
>> +} while (0)
>> +			do_level();
>> +			break;
>> +		case 'L':
>> +			do_level();
>> +			flags |= LOGFF_LEVEL_MIN;
>> +			break;
>> +		case 'p':
>> +			print_num = true;
>> +			break;
>> +		default:
>> +			return CMD_RET_USAGE;
>> +		}
>> +	}
>> +
>> +	if (gs.optind != argc)
>> +		return CMD_RET_USAGE;
>> +
>> +	cat_list[cat_count] = LOGC_END;
>> +	err = log_add_filter_flags(drv_name, cat_count ? cat_list : NULL, level,
>> +				   file_list, flags);
>> +	if (err < 0) {
>> +		printf("Could not add filter (err = %d)\n", err);
>> +		return CMD_RET_FAILURE;
>> +	} else if (print_num) {
>> +		printf("%d\n", err);
>> +	}
>> +
>> +	return CMD_RET_SUCCESS;
>> +}
>> +
>> +static int do_log_filter_remove(struct cmd_tbl *cmdtp, int flag, int argc,
>> +				char *const argv[])
>> +{
>> +	int opt, err;
>> +	ulong filter_num;
>> +	const char *drv_name = "console";
>> +	struct getopt_state gs;
>> +
>> +	getopt_init_state(&gs);
>> +	while ((opt = getopt(&gs, argc, argv, "d:")) > 0) {
>> +		switch (opt) {
>> +		case 'd':
>> +			drv_name = gs.optarg;
>> +			break;
>> +		default:
>> +			return CMD_RET_USAGE;
>> +		}
>> +	}
>> +
>> +	if (gs.optind + 1 != argc)
>> +		return CMD_RET_USAGE;
>> +
>> +	if (strict_strtoul(argv[gs.optind], 10, &filter_num)) {
>> +		printf("Invalid filter number \"%s\"\n", argv[gs.optind]);
>> +		return CMD_RET_FAILURE;
>> +	}
>> +
>> +	err = log_remove_filter(drv_name, filter_num);
>> +	if (err) {
>> +		printf("Could not remove filter (err = %d)\n", err);
>> +		return CMD_RET_FAILURE;
>> +	}
>> +
>> +	return CMD_RET_SUCCESS;
>> +}
>> +
>>  static int do_log_format(struct cmd_tbl *cmdtp, int flag, int argc,
>>  			 char *const argv[])
>>  {
>> @@ -122,6 +312,25 @@ static char log_help_text[] =
>>  #if CONFIG_IS_ENABLED(LOG_TEST)
>>  	"log test - run log tests\n"
> 
> This is probably what a typical user is least interested in. Please,
> move it to the end of the description.

Ok.

> 
>>  #endif
>> +	"log filter-list [OPTIONS] - list all filters for a log driver\n"
>> +	"\t-d <driver> - Specify the log driver to add the filter to; defaults\n"> +	"\t              to console\n"
> 
> The text seems not to relate to filter-list.

Oh, whoops, the text should be something like

>> +	"\t-d <driver> - Specify the log driver to list filters from; defaults to console\n"

I originally wrote that line for the log filter-add command.

> Why don't we filter all log drivers by default? When would a user be
> interested to filter by driver?

Good question. This part of the API seems to have no real users at the
moment. One use could be in restricting the log level over ethernet. For
example, you could have the default log level on the system be INFO, but
deny messages above WARNING from the syslog driver. It might be easier
to do this via a driver-specific log level instead. However, I think we
should leave per-driver filters in until it becomes clear that they are
not necessary.

> 
> How should I know which drivers and categories exist on my system?
> 
>> +	"log filter-add [OPTIONS] - add a new filter to a driver\n"
>> +	"\t-A - Allow messages matching this filter; mutually exclusive with -D\n"
>> +	"\t     This is the default.\n"
>> +	"\t-c <category> - Category to match; may be specified multiple times\n"
>> +	"\t-d <driver> - Specify the log driver to add the filter to; defaults\n"
>> +	"\t              to console\n"
>> +	"\t-D - Deny messages matching this filter; mutually exclusive with -A\n"
>> +	"\t-f <files_list> - A comma-separated list of files to match\n"
>> +	"\t-l <level> - Match log levels below this level; mutually exclusive\n"
>> +	"\t             with -L\n"
> 
> Is this below or below and equal?

Below and equal. Perhaps something like

>> +	"\t-l <level> - Match log levels less than or equal to <level>; mutually exclusive\n"

would be better.

> 
>> +	"\t-L <level> - Match log levels above this level; mutually exclusive\n"
>> +	"\t             with -l\n"
> 
> Is this above or above and equal?

This is above and equal.

There is a bit of asymmetry here, as both of the comparison operators
include the listed level. I'm not sure what the most ergonomic choice
here is.

> 
>> +	"\t-p - Print the filter number on success\n"
> 
> Why do we need this parameter? Can't we simply always print the filter
> numbers or never?

Commands should be silent if they succeed, especially simple ones like
this. For a human, it is always possible to find the filter number of a
given filter by examining the output of `log filter-list`. However, it
is useful for machines (e.g. for testing) if there is an option to get
the filter number when it is produced.

>> +	"log filter-remove [OPTIONS] <num> - Remove filter number <num>\n"
>> +	"\t-d <driver> - Specify the log driver to add the filter to; defaults\n"
>> +	"\t              to console\n"
> 
> This entry seems not to be related to removal.

Yes, same mistake as above.

> 
> How do I simply delete all filters?

You must delete them one-by-one. This could be added.

> How do I remove all filters for a category?

You must delete them one-by-one. This could also be added.

However, I would like to keep the number of features to a minimum until
it becomes clear how these filters will be used. I think a remove-all
version would probably be useful, but I'm less convinced about removing
filters for a category. Should there also be a command to remove all
filters for a given file?

--Sean

[PATCH 15/18] cmd: log: Add commands to manipulate filters

[Simon Glass]

Hi Sean,

On Tue, 6 Oct 2020 at 13:16, Sean Anderson <seanga2@gmail.com> wrote:
>
> This adds several commands to add, list, and remove log filters. Due to the
> complexity of adding a filter, `log filter-list` uses options instead of
> positional arguments.
>
> These commands have been added as subcommands to log by using a dash to
> join the subcommand and subsubcommand. This is stylistic, and they could be
> converted to proper subsubcommands if it is wished.
>
> Signed-off-by: Sean Anderson <seanga2@gmail.com>
> ---
>
>  cmd/Kconfig |   1 +
>  cmd/log.c   | 213 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 214 insertions(+)

I see you have a lot of comments already,  It's great to see this
feature. But please do add some sort of test for the command itself.

You don't need to test that the filters work; that sort of thing
belongs in the log tests. But the command test should check that
various commands are interpreted correctly, result in some action (you
can check the log state directly if you provide a helper function to
export data structures) and output things correctly (for commands that
produce output).

Regards,
Simon

[PATCH 15/18] cmd: log: Add commands to manipulate filters

[Sean Anderson]

On 10/6/20 6:02 PM, Simon Glass wrote:
> Hi Sean,
> 
> On Tue, 6 Oct 2020 at 13:16, Sean Anderson <seanga2@gmail.com> wrote:
>>
>> This adds several commands to add, list, and remove log filters. Due to the
>> complexity of adding a filter, `log filter-list` uses options instead of
>> positional arguments.
>>
>> These commands have been added as subcommands to log by using a dash to
>> join the subcommand and subsubcommand. This is stylistic, and they could be
>> converted to proper subsubcommands if it is wished.
>>
>> Signed-off-by: Sean Anderson <seanga2@gmail.com>
>> ---
>>
>>  cmd/Kconfig |   1 +
>>  cmd/log.c   | 213 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 214 insertions(+)
> 
> I see you have a lot of comments already,  It's great to see this
> feature. But please do add some sort of test for the command itself.
> 
> You don't need to test that the filters work; that sort of thing
> belongs in the log tests. But the command test should check that
> various commands are interpreted correctly, result in some action (you
> can check the log state directly if you provide a helper function to
> export data structures) and output things correctly (for commands that
> produce output).

Is the test in the next patch inadequate?

--Sean

[PATCH 16/18] test: py: Add a test for log filter-*

[Sean Anderson]

This exercises a few success and failure modes of the log filter-*
commands.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---

 test/py/tests/test_log.py | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/test/py/tests/test_log.py b/test/py/tests/test_log.py
index f191c84c02..87c0c21575 100644
--- a/test/py/tests/test_log.py
+++ b/test/py/tests/test_log.py
@@ -154,3 +154,42 @@ def test_log_format(u_boot_console):
         run_with_format('FLfm', 'file.c:123-func() msg')
         run_with_format('lm', 'NOTICE. msg')
         run_with_format('m', 'msg')
+
+ at pytest.mark.buildconfigspec('cmd_log')
+def test_log_filter(u_boot_console):
+    c = u_boot_console
+
+    # Test invalid options
+    assert 'rc:1' in c.run_command('log filter-add -AD; echo rc:$?')
+    assert 'rc:1' in c.run_command('log filter-add -l1 -L1; echo rc:$?')
+    assert 'rc:1' in c.run_command('log filter-add -l1 -L1; echo rc:$?')
+    assert 'rc:1' in c.run_command('log filter-add -lfoo; echo rc:$?')
+    assert 'rc:1' in c.run_command('log filter-add -cfoo; echo rc:$?')
+    assert 'rc:1' in c.run_command('log filter-add -ccore -ccore -ccore -ccore -ccore -ccore; echo rc:$?')
+
+    # Test filters
+    assert c.run_command('log format m') == ''
+    filt1 = c.run_command('log filter-add -p')
+    assert c.run_command('log rec mmc warning file 123 func msg') == 'msg'
+    filt2 = c.run_command('log filter-add -p -DL warning -cmmc -cspi -ffile')
+    assert c.run_command('log rec mmc warning file 123 func msg') == ''
+    assert c.run_command('log rec spi warning file 123 func msg') == ''
+    assert c.run_command('log rec arch warning file 123 func msg') == 'msg'
+    assert c.run_command('log rec mmc warning file2 123 func msg') == 'msg'
+    assert c.run_command('log rec mmc err file 123 func msg') == 'msg'
+
+    # "Test" filter-list; ignore the header
+    resp = c.run_command('log filter-list')[:-1]
+    assert filt1 in resp
+    assert filt2 in resp
+    assert 'allow' in resp
+    assert 'deny' in resp
+    assert '>=WARNING' in resp
+    assert '<=IO' in resp
+    assert 'mmc' in resp
+    assert 'spi' in resp
+    assert 'file' in resp
+
+    # Ensure removal works
+    assert c.run_command('log filter-remove ' + filt2) == ''
+    assert c.run_command('log rec mmc warning file 123 func msg') == 'msg'
-- 
2.28.0

[PATCH 16/18] test: py: Add a test for log filter-*

[Simon Glass]

Hi Sean,

On Tue, 6 Oct 2020 at 13:16, Sean Anderson <seanga2@gmail.com> wrote:
>
> This exercises a few success and failure modes of the log filter-*
> commands.
>
> Signed-off-by: Sean Anderson <seanga2@gmail.com>
> ---
>
>  test/py/tests/test_log.py | 39 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
>

Can this test be written in C?

Regards,
Simon

[PATCH 16/18] test: py: Add a test for log filter-*

[Sean Anderson]

On 10/6/20 6:07 PM, Simon Glass wrote:
> Hi Sean,
> 
> On Tue, 6 Oct 2020 at 13:16, Sean Anderson <seanga2@gmail.com> wrote:
>>
>> This exercises a few success and failure modes of the log filter-*
>> commands.
>>
>> Signed-off-by: Sean Anderson <seanga2@gmail.com>
>> ---
>>
>>  test/py/tests/test_log.py | 39 +++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 39 insertions(+)
>>
> 
> Can this test be written in C?

Probably.

--Sean

[PATCH 17/18] doc: Add log kerneldocs to documentation

[Sean Anderson]

The functions in log.h are already mostly documented, so add them to the
generated documentation. This commit adds no new documentation, so several
enum members are left undocumented.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---

 doc/develop/logging.rst |   5 ++
 include/log.h           | 118 +++++++++++++++++++++++++---------------
 2 files changed, 79 insertions(+), 44 deletions(-)

diff --git a/doc/develop/logging.rst b/doc/develop/logging.rst
index 7ce8482ab6..6a22062073 100644
--- a/doc/develop/logging.rst
+++ b/doc/develop/logging.rst
@@ -288,3 +288,8 @@ information
 Add a command to add new log records and delete existing records.
 
 Provide additional log() functions - e.g. logc() to specify the category
+
+Logging API
+-----------
+.. kernel-doc:: include/log.h
+   :internal:
diff --git a/include/log.h b/include/log.h
index 4e5e3159f6..bbc8d94acb 100644
--- a/include/log.h
+++ b/include/log.h
@@ -17,18 +17,30 @@
 
 struct cmd_tbl;
 
-/** Log levels supported, ranging from most to least important */
+/**
+ * enum log_level_t - Log levels supported, ranging from most to least important
+ */
 enum log_level_t {
-	LOGL_EMERG = 0,		/* U-Boot is unstable */
-	LOGL_ALERT,		/* Action must be taken immediately */
-	LOGL_CRIT,		/* Critical conditions */
-	LOGL_ERR,		/* Error that prevents something from working */
-	LOGL_WARNING,		/* Warning may prevent optimial operation */
-	LOGL_NOTICE,		/* Normal but significant condition, printf() */
-	LOGL_INFO,		/* General information message */
-	LOGL_DEBUG,		/* Basic debug-level message */
-	LOGL_DEBUG_CONTENT,	/* Debug message showing full message content */
-	LOGL_DEBUG_IO,		/* Debug message showing hardware I/O access */
+	/** @LOGL_EMERG: U-Boot is unstable */
+	LOGL_EMERG = 0,
+	/** @LOGL_ALERT: Action must be taken immediately */
+	LOGL_ALERT,
+	/** @LOGL_CRIT: Critical conditions */
+	LOGL_CRIT,
+	/** @LOGL_ERR: Error that prevents something from working */
+	LOGL_ERR,
+	/** @LOGL_WARNING: Warning may prevent optimial operation */
+	LOGL_WARNING,
+	/** @LOGL_NOTICE: Normal but significant condition, printf() */
+	LOGL_NOTICE,
+	/** @LOGL_INFO: General information message */
+	LOGL_INFO,
+	/** @LOGL_DEBUG: Basic debug-level message */
+	LOGL_DEBUG,
+	/** @LOGL_DEBUG_CONTENT: Debug message showing full message content */
+	LOGL_DEBUG_CONTENT,
+	/** @LOGL_DEBUG_IO: Debug message showing hardware I/O access */
+	LOGL_DEBUG_IO,
 
 	LOGL_COUNT,
 	LOGL_NONE,
@@ -38,28 +50,42 @@ enum log_level_t {
 };
 
 /**
- * Log categories supported. Most of these correspond to uclasses (i.e.
- * enum uclass_id) but there are also some more generic categories
+ * enum log_category_t - Log categories supported.
+ *
+ * Most of these correspond to uclasses (i.e. &enum uclass_id) but there are
+ * also some more generic categories
  */
 enum log_category_t {
 	LOGC_FIRST = 0,	/* First part mirrors UCLASS_... */
 
 	LOGC_NONE = UCLASS_COUNT,	/* First number is after all uclasses */
-	LOGC_ARCH,	/* Related to arch-specific code */
-	LOGC_BOARD,	/* Related to board-specific code */
-	LOGC_CORE,	/* Related to core features (non-driver-model) */
-	LOGC_DM,	/* Core driver-model */
-	LOGC_DT,	/* Device-tree */
-	LOGC_EFI,	/* EFI implementation */
-	LOGC_ALLOC,	/* Memory allocation */
-	LOGC_SANDBOX,	/* Related to the sandbox board */
-	LOGC_BLOBLIST,	/* Bloblist */
-	LOGC_DEVRES,	/* Device resources (devres_... functions) */
-	/* Advanced Configuration and Power Interface (ACPI) */
+	/** @LOGC_ARCH: Related to arch-specific code */
+	LOGC_ARCH,
+	/** @LOGC_BOARD: Related to board-specific code */
+	LOGC_BOARD,
+	/** @LOGC_CORE: Related to core features (non-driver-model) */
+	LOGC_CORE,
+	/** @LOGC_DM: Core driver-model */
+	LOGC_DM,
+	/** @LOGC_DT: Device-tree */
+	LOGC_DT,
+	/** @LOGC_EFI: EFI implementation */
+	LOGC_EFI,
+	/** @LOGC_ALLOC: Memory allocation */
+	LOGC_ALLOC,
+	/** @LOGC_SANDBOX: Related to the sandbox board */
+	LOGC_SANDBOX,
+	/** @LOGC_BLOBLIST: Bloblist */
+	LOGC_BLOBLIST,
+	/** @LOGC_DEVRES: Device resources (``devres_...`` functions) */
+	LOGC_DEVRES,
+	/** @LOGC_ACPI: Advanced Configuration and Power Interface (ACPI) */
 	LOGC_ACPI,
 
-	LOGC_COUNT,	/* Number of log categories */
-	LOGC_END,	/* Sentinel value for a list of log categories */
+	/** @LOGC_COUNT: Number of log categories */
+	LOGC_COUNT,
+	/** @LOGC_END: Sentinel value for lists of log categories */
+	LOGC_END,
 };
 
 /* Helper to cast a uclass ID to a log category */
@@ -78,7 +104,7 @@ static inline int log_uc_cat(enum uclass_id id)
  * @func: Function where log record was generated
  * @fmt: printf() format string for log record
  * @...: Optional parameters, according to the format string @fmt
- * @return 0 if log record was emitted, -ve on error
+ * Return: 0 if log record was emitted, -ve on error
  */
 int _log(enum log_category_t cat, enum log_level_t level, const char *file,
 	 int line, const char *func, const char *fmt, ...)
@@ -231,7 +257,7 @@ void __assert_fail(const char *assertion, const char *file, unsigned int line,
  * full pathname as it may be huge. Only use this when the user should be
  * warning, similar to BUG_ON() in linux.
  *
- * @return true if assertion succeeded (condition is true), else false
+ * Return: true if assertion succeeded (condition is true), else false
  */
 #define assert_noisy(x) \
 	({ bool _val = (x); \
@@ -304,8 +330,9 @@ struct log_device;
  */
 struct log_driver {
 	const char *name;
+
 	/**
-	 * emit() - emit a log record
+	 * @emit: emit a log record
 	 *
 	 * Called by the log system to pass a log record to a particular driver
 	 * for processing. The filter is checked before calling this function.
@@ -359,7 +386,7 @@ enum log_filter_flags {
  * @filter_num: Sequence number of this filter.  This is returned when adding a
  *	new filter, and must be provided when removing a previously added
  *	filter.
- * @flags: Flags for this filter (LOGFF_...)
+ * @flags: Flags for this filter (``LOGFF_...``)
  * @cat_list: List of categories to allow (terminated by LOGC_END). If empty
  *	then all categories are permitted. Up to LOGF_MAX_CATEGORIES entries
  *	can be provided
@@ -384,8 +411,8 @@ struct log_filter {
  * log_get_cat_name() - Get the name of a category
  *
  * @cat: Category to look up
- * @return category name (which may be a uclass driver name) if found, or
- *	 "<invalid>" if invalid, or "<missing>" if not found
+ * Return: category name (which may be a uclass driver name) if found, or
+ *	   "<invalid>" if invalid, or "<missing>" if not found
  */
 const char *log_get_cat_name(enum log_category_t cat);
 
@@ -393,7 +420,7 @@ const char *log_get_cat_name(enum log_category_t cat);
  * log_get_cat_by_name() - Look up a category by name
  *
  * @name: Name to look up
- * @return category ID, or LOGC_NONE if not found
+ * Return: category ID, or LOGC_NONE if not found
  */
 enum log_category_t log_get_cat_by_name(const char *name);
 
@@ -401,7 +428,7 @@ enum log_category_t log_get_cat_by_name(const char *name);
  * log_get_level_name() - Get the name of a log level
  *
  * @level: Log level to look up
- * @return log level name (in ALL CAPS)
+ * Return: log level name (in ALL CAPS)
  */
 const char *log_get_level_name(enum log_level_t level);
 
@@ -409,7 +436,7 @@ const char *log_get_level_name(enum log_level_t level);
  * log_get_level_by_name() - Look up a log level by name
  *
  * @name: Name to look up
- * @return log level ID, or LOGL_NONE if not found
+ * Return: log level ID, or LOGL_NONE if not found
  */
 enum log_level_t log_get_level_by_name(const char *name);
 
@@ -417,7 +444,7 @@ enum log_level_t log_get_level_by_name(const char *name);
  * log_device_find_by_name() - Look up a log device by its driver's name
  *
  * @drv_name: Name of the driver
- * @return the log device, or NULL if not found
+ * Return: the log device, or NULL if not found
  */
 struct log_device *log_device_find_by_name(const char *drv_name);
 
@@ -442,15 +469,16 @@ int do_log_test(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]);
  *
  * @drv_name: Driver name to add the filter to (since each driver only has a
  *	single device)
- * @flags: Flags for this filter (LOGFF_...)
+ * @flags: Flags for this filter (``LOGFF_...``)
  * @cat_list: List of categories to allow (terminated by LOGC_none). If empty
  *	then all categories are permitted. Up to LOGF_MAX_CATEGORIES entries
  *	can be provided
  * @level: Maximum (or minimum, if LOGFF_LEVEL_MIN) log level to allow
  * @file_list: List of files to allow, separated by comma. If NULL then all
  *	files are permitted
- * @return the sequence number of the new filter (>=0) if the filter was added,
- *	or a -ve value on error
+ * Return:
+ *   the sequence number of the new filter (>=0) if the filter was added, or a
+ *   -ve value on error
  */
 int log_add_filter_flags(const char *drv_name, enum log_category_t cat_list[],
 			 enum log_level_t level, const char *file_list,
@@ -467,8 +495,9 @@ int log_add_filter_flags(const char *drv_name, enum log_category_t cat_list[],
  * @max_level: Maximum log level to allow
  * @file_list: List of files to allow, separated by comma. If NULL then all
  *	files are permitted
- * @return the sequence number of the new filter (>=0) if the filter was added,
- *	or a -ve value on error
+ * Return:
+ *   the sequence number of the new filter (>=0) if the filter was added, or a
+ *   -ve value on error
  */
 static inline int log_add_filter(const char *drv_name,
 				 enum log_category_t cat_list[],
@@ -485,8 +514,9 @@ static inline int log_add_filter(const char *drv_name,
  * @drv_name: Driver name to remove the filter from (since each driver only has
  *	a single device)
  * @filter_num: Filter number to remove (as returned by log_add_filter())
- * @return 0 if the filter was removed, -ENOENT if either the driver or the
- *	filter number was not found
+ * Return:
+ *   0 if the filter was removed, -ENOENT if either the driver or the filter
+ *   number was not found
  */
 int log_remove_filter(const char *drv_name, int filter_num);
 
@@ -494,7 +524,7 @@ int log_remove_filter(const char *drv_name, int filter_num);
 /**
  * log_init() - Set up the log system ready for use
  *
- * @return 0 if OK, -ENOMEM if out of memory
+ * Return: 0 if OK, -ENOMEM if out of memory
  */
 int log_init(void);
 #else
-- 
2.28.0

[PATCH 18/18] doc: Update logging documentation

[Sean Anderson]

This updates logging documentation with some examples of the new commands
added in the previous commits. It also removes some items from the to-do
list which have been implemented.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---

 doc/develop/logging.rst | 42 +++++++++++++++++++++++++++++------------
 1 file changed, 30 insertions(+), 12 deletions(-)

diff --git a/doc/develop/logging.rst b/doc/develop/logging.rst
index 6a22062073..8b6237f829 100644
--- a/doc/develop/logging.rst
+++ b/doc/develop/logging.rst
@@ -174,18 +174,45 @@ fields are present, but not the field order.
 Filters
 -------
 
-Filters are attached to log drivers to control what those drivers emit. Only
-records that pass through the filter make it to the driver.
+Filters are attached to log drivers to control what those drivers emit. FIlters
+can either allow or deny a log message when they match it. Only records which
+are allowed by a filter make it to the driver.
 
 Filters can be based on several criteria:
 
-* maximum log level
+* minimum or maximum log level
 * in a set of categories
 * in a set of files
 
 If no filters are attached to a driver then a default filter is used, which
 limits output to records with a level less than CONFIG_MAX_LOG_LEVEL.
 
+Adding Filters
+~~~~~~~~~~~~~~
+
+To add new filters at runtime, use the 'log filter-add' command. For example, to
+filter messages from the SPI subsystem, run::
+
+    log filter-add -D -c spi
+
+You will also need to allow everything else to emit messages (because the
+default filter no longer applies)::
+
+    log filter-add -A -l debug
+
+To view active filters, use the 'log filter-list' command. Some example output
+is::
+
+    => log filter-list
+    num policy level           categories files
+      0   deny <=IO                   spi
+      1  allow <=DEBUG
+
+Note that filters are processed in-order from top to bottom, not in the order of
+their filter number. Filters are added to the top of the list if they deny when
+they match, and to the bottom if they allow when they match. For more
+information, consult the usage of the 'log' command, by running 'help log'.
+
 
 Logging statements
 ------------------
@@ -259,8 +286,6 @@ Add a way to browse log records
 
 Add a way to record log records for browsing using an external tool
 
-Add commands to add and remove filters
-
 Add commands to add and remove log devices
 
 Allow sharing of printf format strings in log records to reduce storage size
@@ -268,10 +293,6 @@ for large numbers of log records
 
 Add a command-line option to sandbox to set the default logging level
 
-Convert core driver model code to use logging
-
-Convert uclasses to use logging with the correct category
-
 Consider making log() calls emit an automatic newline, perhaps with a logn()
 function to avoid that
 
@@ -282,9 +303,6 @@ number dropped due to them being generated before the log system was ready.
 
 Add a printf() format string pragma so that log statements are checked properly
 
-Enhance the log console driver to show level / category / file / line
-information
-
 Add a command to add new log records and delete existing records.
 
 Provide additional log() functions - e.g. logc() to specify the category
-- 
2.28.0

[PATCH 18/18] doc: Update logging documentation

[Heinrich Schuchardt]

On 10/6/20 9:16 PM, Sean Anderson wrote:
> This updates logging documentation with some examples of the new commands
> added in the previous commits. It also removes some items from the to-do
> list which have been implemented.
>
> Signed-off-by: Sean Anderson <seanga2@gmail.com>
> ---
>
>  doc/develop/logging.rst | 42 +++++++++++++++++++++++++++++------------
>  1 file changed, 30 insertions(+), 12 deletions(-)
>
> diff --git a/doc/develop/logging.rst b/doc/develop/logging.rst
> index 6a22062073..8b6237f829 100644
> --- a/doc/develop/logging.rst
> +++ b/doc/develop/logging.rst
> @@ -174,18 +174,45 @@ fields are present, but not the field order.
>  Filters
>  -------
>
> -Filters are attached to log drivers to control what those drivers emit. Only
> -records that pass through the filter make it to the driver.
> +Filters are attached to log drivers to control what those drivers emit. FIlters
> +can either allow or deny a log message when they match it. Only records which
> +are allowed by a filter make it to the driver.
>
>  Filters can be based on several criteria:
>
> -* maximum log level
> +* minimum or maximum log level
>  * in a set of categories
>  * in a set of files
>
>  If no filters are attached to a driver then a default filter is used, which
>  limits output to records with a level less than CONFIG_MAX_LOG_LEVEL.
>
> +Adding Filters
> +~~~~~~~~~~~~~~
> +
> +To add new filters at runtime, use the 'log filter-add' command. For example, to
> +filter messages from the SPI subsystem, run::

Does "filter" mean I see only messages from SPI or does this mean I see
no messages from SPI? If you mean suppress, please, use that word or
something similar.

> +
> +    log filter-add -D -c spi
> +
> +You will also need to allow everything else to emit messages (because the
> +default filter no longer applies)::
> +
> +    log filter-add -A -l debug

Thank you for starting the documenation.

Unfortunately after reading the lines above I have no clue what the
meaning of -D -A -c -l are.

Users will need a complete description of the log command with all
parameters.

Best regards

Heinrich

> +
> +To view active filters, use the 'log filter-list' command. Some example output
> +is::
> +
> +    => log filter-list
> +    num policy level           categories files
> +      0   deny <=IO                   spi
> +      1  allow <=DEBUG
> +
> +Note that filters are processed in-order from top to bottom, not in the order of
> +their filter number. Filters are added to the top of the list if they deny when
> +they match, and to the bottom if they allow when they match. For more
> +information, consult the usage of the 'log' command, by running 'help log'.
> +
>
>  Logging statements
>  ------------------
> @@ -259,8 +286,6 @@ Add a way to browse log records
>
>  Add a way to record log records for browsing using an external tool
>
> -Add commands to add and remove filters
> -
>  Add commands to add and remove log devices
>
>  Allow sharing of printf format strings in log records to reduce storage size
> @@ -268,10 +293,6 @@ for large numbers of log records
>
>  Add a command-line option to sandbox to set the default logging level
>
> -Convert core driver model code to use logging
> -
> -Convert uclasses to use logging with the correct category
> -
>  Consider making log() calls emit an automatic newline, perhaps with a logn()
>  function to avoid that
>
> @@ -282,9 +303,6 @@ number dropped due to them being generated before the log system was ready.
>
>  Add a printf() format string pragma so that log statements are checked properly
>
> -Enhance the log console driver to show level / category / file / line
> -information
> -
>  Add a command to add new log records and delete existing records.
>
>  Provide additional log() functions - e.g. logc() to specify the category
>

[PATCH 18/18] doc: Update logging documentation

[Sean Anderson]

On 10/6/20 4:34 PM, Heinrich Schuchardt wrote:
> On 10/6/20 9:16 PM, Sean Anderson wrote:
>> This updates logging documentation with some examples of the new commands
>> added in the previous commits. It also removes some items from the to-do
>> list which have been implemented.
>>
>> Signed-off-by: Sean Anderson <seanga2@gmail.com>
>> ---
>>
>>  doc/develop/logging.rst | 42 +++++++++++++++++++++++++++++------------
>>  1 file changed, 30 insertions(+), 12 deletions(-)
>>
>> diff --git a/doc/develop/logging.rst b/doc/develop/logging.rst
>> index 6a22062073..8b6237f829 100644
>> --- a/doc/develop/logging.rst
>> +++ b/doc/develop/logging.rst
>> @@ -174,18 +174,45 @@ fields are present, but not the field order.
>>  Filters
>>  -------
>>
>> -Filters are attached to log drivers to control what those drivers emit. Only
>> -records that pass through the filter make it to the driver.
>> +Filters are attached to log drivers to control what those drivers emit. FIlters
>> +can either allow or deny a log message when they match it. Only records which
>> +are allowed by a filter make it to the driver.
>>
>>  Filters can be based on several criteria:
>>
>> -* maximum log level
>> +* minimum or maximum log level
>>  * in a set of categories
>>  * in a set of files
>>
>>  If no filters are attached to a driver then a default filter is used, which
>>  limits output to records with a level less than CONFIG_MAX_LOG_LEVEL.
>>
>> +Adding Filters
>> +~~~~~~~~~~~~~~
>> +
>> +To add new filters at runtime, use the 'log filter-add' command. For example, to
>> +filter messages from the SPI subsystem, run::
> 
> Does "filter" mean I see only messages from SPI or does this mean I see
> no messages from SPI? If you mean suppress, please, use that word or
> something similar.

Ok.

> 
>> +
>> +    log filter-add -D -c spi
>> +
>> +You will also need to allow everything else to emit messages (because the
>> +default filter no longer applies)::
>> +
>> +    log filter-add -A -l debug
> 
> Thank you for starting the documenation.
> 
> Unfortunately after reading the lines above I have no clue what the
> meaning of -D -A -c -l are.

These options are currently documented in the help text for log. Should
they be added here? I am concerned that duplicating documentation will
result in drift over time.

> 
> Users will need a complete description of the log command with all
> parameters.
> 
> Best regards
> 
> Heinrich
> 
>> +
>> +To view active filters, use the 'log filter-list' command. Some example output
>> +is::
>> +
>> +    => log filter-list
>> +    num policy level           categories files
>> +      0   deny <=IO                   spi
>> +      1  allow <=DEBUG
>> +
>> +Note that filters are processed in-order from top to bottom, not in the order of
>> +their filter number. Filters are added to the top of the list if they deny when
>> +they match, and to the bottom if they allow when they match. For more
>> +information, consult the usage of the 'log' command, by running 'help log'.

Here is my suggestion of where to find a complete list of options.

--Sean

>>
>>  Logging statements
>>  ------------------
>> @@ -259,8 +286,6 @@ Add a way to browse log records
>>
>>  Add a way to record log records for browsing using an external tool
>>
>> -Add commands to add and remove filters
>> -
>>  Add commands to add and remove log devices
>>
>>  Allow sharing of printf format strings in log records to reduce storage size
>> @@ -268,10 +293,6 @@ for large numbers of log records
>>
>>  Add a command-line option to sandbox to set the default logging level
>>
>> -Convert core driver model code to use logging
>> -
>> -Convert uclasses to use logging with the correct category
>> -
>>  Consider making log() calls emit an automatic newline, perhaps with a logn()
>>  function to avoid that
>>
>> @@ -282,9 +303,6 @@ number dropped due to them being generated before the log system was ready.
>>
>>  Add a printf() format string pragma so that log statements are checked properly
>>
>> -Enhance the log console driver to show level / category / file / line
>> -information
>> -
>>  Add a command to add new log records and delete existing records.
>>
>>  Provide additional log() functions - e.g. logc() to specify the category
>>
> 

[PATCH 18/18] doc: Update logging documentation

[Heinrich Schuchardt]

On 10/6/20 10:38 PM, Sean Anderson wrote:
> On 10/6/20 4:34 PM, Heinrich Schuchardt wrote:
>> On 10/6/20 9:16 PM, Sean Anderson wrote:
>>> This updates logging documentation with some examples of the new commands
>>> added in the previous commits. It also removes some items from the to-do
>>> list which have been implemented.
>>>
>>> Signed-off-by: Sean Anderson <seanga2@gmail.com>
>>> ---
>>>
>>>  doc/develop/logging.rst | 42 +++++++++++++++++++++++++++++------------
>>>  1 file changed, 30 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/doc/develop/logging.rst b/doc/develop/logging.rst
>>> index 6a22062073..8b6237f829 100644
>>> --- a/doc/develop/logging.rst
>>> +++ b/doc/develop/logging.rst
>>> @@ -174,18 +174,45 @@ fields are present, but not the field order.
>>>  Filters
>>>  -------
>>>
>>> -Filters are attached to log drivers to control what those drivers emit. Only
>>> -records that pass through the filter make it to the driver.
>>> +Filters are attached to log drivers to control what those drivers emit. FIlters
>>> +can either allow or deny a log message when they match it. Only records which
>>> +are allowed by a filter make it to the driver.
>>>
>>>  Filters can be based on several criteria:
>>>
>>> -* maximum log level
>>> +* minimum or maximum log level
>>>  * in a set of categories
>>>  * in a set of files
>>>
>>>  If no filters are attached to a driver then a default filter is used, which
>>>  limits output to records with a level less than CONFIG_MAX_LOG_LEVEL.
>>>
>>> +Adding Filters
>>> +~~~~~~~~~~~~~~
>>> +
>>> +To add new filters at runtime, use the 'log filter-add' command. For example, to
>>> +filter messages from the SPI subsystem, run::
>>
>> Does "filter" mean I see only messages from SPI or does this mean I see
>> no messages from SPI? If you mean suppress, please, use that word or
>> something similar.
>
> Ok.
>
>>
>>> +
>>> +    log filter-add -D -c spi
>>> +
>>> +You will also need to allow everything else to emit messages (because the
>>> +default filter no longer applies)::
>>> +
>>> +    log filter-add -A -l debug
>>
>> Thank you for starting the documenation.
>>
>> Unfortunately after reading the lines above I have no clue what the
>> meaning of -D -A -c -l are.
>
> These options are currently documented in the help text for log. Should
> they be added here? I am concerned that duplicating documentation will
> result in drift over time.

If I look at https://u-boot.readthedocs.io/en/latest/, it is currently
far from what users needs.

The chapter "Develop U-Boot" seems to be the wrong place for describing
commands.

We need a chapter in our online documentation documenting all commands
in enough detail for a rookie to understand how to use them.

Something like like https://man7.org/linux/man-pages/man1/git-commit.1.html.

Best regards

Heinrich

>
>>
>> Users will need a complete description of the log command with all
>> parameters.
>>
>> Best regards
>>
>> Heinrich
>>
>>> +
>>> +To view active filters, use the 'log filter-list' command. Some example output
>>> +is::
>>> +
>>> +    => log filter-list
>>> +    num policy level           categories files
>>> +      0   deny <=IO                   spi
>>> +      1  allow <=DEBUG
>>> +
>>> +Note that filters are processed in-order from top to bottom, not in the order of
>>> +their filter number. Filters are added to the top of the list if they deny when
>>> +they match, and to the bottom if they allow when they match. For more
>>> +information, consult the usage of the 'log' command, by running 'help log'.
>
> Here is my suggestion of where to find a complete list of options.
>
> --Sean
>
>>>
>>>  Logging statements
>>>  ------------------
>>> @@ -259,8 +286,6 @@ Add a way to browse log records
>>>
>>>  Add a way to record log records for browsing using an external tool
>>>
>>> -Add commands to add and remove filters
>>> -
>>>  Add commands to add and remove log devices
>>>
>>>  Allow sharing of printf format strings in log records to reduce storage size
>>> @@ -268,10 +293,6 @@ for large numbers of log records
>>>
>>>  Add a command-line option to sandbox to set the default logging level
>>>
>>> -Convert core driver model code to use logging
>>> -
>>> -Convert uclasses to use logging with the correct category
>>> -
>>>  Consider making log() calls emit an automatic newline, perhaps with a logn()
>>>  function to avoid that
>>>
>>> @@ -282,9 +303,6 @@ number dropped due to them being generated before the log system was ready.
>>>
>>>  Add a printf() format string pragma so that log statements are checked properly
>>>
>>> -Enhance the log console driver to show level / category / file / line
>>> -information
>>> -
>>>  Add a command to add new log records and delete existing records.
>>>
>>>  Provide additional log() functions - e.g. logc() to specify the category
>>>
>>
>

[PATCH 18/18] doc: Update logging documentation

[Sean Anderson]

On 10/6/20 5:28 PM, Heinrich Schuchardt wrote:
> On 10/6/20 10:38 PM, Sean Anderson wrote:
>> On 10/6/20 4:34 PM, Heinrich Schuchardt wrote:
>>> On 10/6/20 9:16 PM, Sean Anderson wrote:
>>>> This updates logging documentation with some examples of the new commands
>>>> added in the previous commits. It also removes some items from the to-do
>>>> list which have been implemented.
>>>>
>>>> Signed-off-by: Sean Anderson <seanga2@gmail.com>
>>>> ---
>>>>
>>>>  doc/develop/logging.rst | 42 +++++++++++++++++++++++++++++------------
>>>>  1 file changed, 30 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/doc/develop/logging.rst b/doc/develop/logging.rst
>>>> index 6a22062073..8b6237f829 100644
>>>> --- a/doc/develop/logging.rst
>>>> +++ b/doc/develop/logging.rst
>>>> @@ -174,18 +174,45 @@ fields are present, but not the field order.
>>>>  Filters
>>>>  -------
>>>>
>>>> -Filters are attached to log drivers to control what those drivers emit. Only
>>>> -records that pass through the filter make it to the driver.
>>>> +Filters are attached to log drivers to control what those drivers emit. FIlters
>>>> +can either allow or deny a log message when they match it. Only records which
>>>> +are allowed by a filter make it to the driver.
>>>>
>>>>  Filters can be based on several criteria:
>>>>
>>>> -* maximum log level
>>>> +* minimum or maximum log level
>>>>  * in a set of categories
>>>>  * in a set of files
>>>>
>>>>  If no filters are attached to a driver then a default filter is used, which
>>>>  limits output to records with a level less than CONFIG_MAX_LOG_LEVEL.
>>>>
>>>> +Adding Filters
>>>> +~~~~~~~~~~~~~~
>>>> +
>>>> +To add new filters at runtime, use the 'log filter-add' command. For example, to
>>>> +filter messages from the SPI subsystem, run::
>>>
>>> Does "filter" mean I see only messages from SPI or does this mean I see
>>> no messages from SPI? If you mean suppress, please, use that word or
>>> something similar.
>>
>> Ok.
>>
>>>
>>>> +
>>>> +    log filter-add -D -c spi
>>>> +
>>>> +You will also need to allow everything else to emit messages (because the
>>>> +default filter no longer applies)::
>>>> +
>>>> +    log filter-add -A -l debug
>>>
>>> Thank you for starting the documenation.
>>>
>>> Unfortunately after reading the lines above I have no clue what the
>>> meaning of -D -A -c -l are.
>>
>> These options are currently documented in the help text for log. Should
>> they be added here? I am concerned that duplicating documentation will
>> result in drift over time.
> 
> If I look at https://u-boot.readthedocs.io/en/latest/, it is currently
> far from what users needs.
> 
> The chapter "Develop U-Boot" seems to be the wrong place for describing
> commands.

Well, I think that this really depends on who the log subsystem is
intended for. From what I can tell, it was originally intended as an aid
to developers. Some users are starting to make use of it (e.g. via
syslog), but most users probably have CONFIG_LOG disabled to reduce boot
times. The commands I have added are also developer-oriented. While
users certainly could use the new filter flags, they would probably want
to create their filters very early (e.g. as soon as the log subsystem
comes up). Going forward, this may not be the best place for log
documentation.

> 
> We need a chapter in our online documentation documenting all commands
> in enough detail for a rookie to understand how to use them.

Yes, I agree. If possible, they should be generated from U-Boot source
files, so that the documentation can live close to the commands (and so
existing help strings could be used). I'm not really sure what the best
way to go about that is. Perhaps kerneldoc could be (ab)used? In any
case, such a change is outside the scope of this patch.

--Sean