[PATCH v2] lib/vsprintf: introduce OF_DEVICE_NODE_FLAG_MAX for %pOF

[PATCH v2] lib/vsprintf: introduce OF_DEVICE_NODE_FLAG_MAX for %pOF

[lijiazi]

Introduce OF_DEVICE_NODE_FLAG_MAX for device node printk, if add
new device node flag, please add the corresponding flag abbreviation
to tbuf in device_node_string.

Signed-off-by: lijiazi <lijiazi@xiaomi.com>
---
 Documentation/core-api/printk-formats.rst |  2 ++
 include/linux/of.h                        |  6 ++++++
 lib/vsprintf.c                            | 14 +++++++-------
 3 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
index 8ebe46b1..c73ccad 100644
--- a/Documentation/core-api/printk-formats.rst
+++ b/Documentation/core-api/printk-formats.rst
@@ -441,6 +441,8 @@ Examples::
 							d - detached
 							P - Populated
 							B - Populated bus
+                                                        O - Overlay
+                                                        F - Overlay free cset
 
 Passed by reference.
 
diff --git a/include/linux/of.h b/include/linux/of.h
index c669c0a..0453601 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -147,6 +147,12 @@ extern raw_spinlock_t devtree_lock;
 #define OF_OVERLAY		5 /* allocated for an overlay */
 #define OF_OVERLAY_FREE_CSET	6 /* in overlay cset being freed */
 
+/* Add flag max for %pOF related printk, if add new flag, please
+ * increase following marco, and add abbreviations to tbuf in
+ * device_node_string function.
+ */
+#define OF_DEVICE_NODE_FLAG_MAX 6 /* maximum available flags */
+
 #define OF_BAD_ADDR	((u64)-1)
 
 #ifdef CONFIG_OF
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 7c488a1..3c219b4 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1937,9 +1937,9 @@ static noinline_for_stack
 char *device_node_string(char *buf, char *end, struct device_node *dn,
 			 struct printf_spec spec, const char *fmt)
 {
-	char tbuf[sizeof("xxxx") + 1];
+	char tbuf[OF_DEVICE_NODE_FLAG_MAX + 1] = { "DdPBOF" };
 	const char *p;
-	int ret;
+	int ret, i;
 	char *buf_start = buf;
 	struct property *prop;
 	bool has_mult, pass;
@@ -1997,11 +1997,11 @@ char *device_node_string(char *buf, char *end, struct device_node *dn,
 			buf = string(buf, end, p, str_spec);
 			break;
 		case 'F':	/* flags */
-			tbuf[0] = of_node_check_flag(dn, OF_DYNAMIC) ? 'D' : '-';
-			tbuf[1] = of_node_check_flag(dn, OF_DETACHED) ? 'd' : '-';
-			tbuf[2] = of_node_check_flag(dn, OF_POPULATED) ? 'P' : '-';
-			tbuf[3] = of_node_check_flag(dn, OF_POPULATED_BUS) ? 'B' : '-';
-			tbuf[4] = 0;
+			for (i = 0; i < OF_DEVICE_NODE_FLAG_MAX; i++) {
+				if (!of_node_check_flag(dn, OF_DYNAMIC + i))
+					tbuf[i] = '-';
+			}
+			tbuf[OF_DEVICE_NODE_FLAG_MAX] = 0;
 			buf = string_nocheck(buf, end, tbuf, str_spec);
 			break;
 		case 'c':	/* major compatible string */
-- 
2.7.4

Re: [PATCH v2] lib/vsprintf: introduce OF_DEVICE_NODE_FLAG_MAX for %pOF

[Andy Shevchenko]

On Fri, Jan 17, 2020 at 07:15:19PM +0800, lijiazi wrote:
> Introduce OF_DEVICE_NODE_FLAG_MAX for device node printk, if add
> new device node flag, please add the corresponding flag abbreviation
> to tbuf in device_node_string.

Thank you for an update!
My comments below.

...

> +                                                        O - Overlay
> +                                                        F - Overlay free cset

Perhaps this would be a separate patch.

> +/* Add flag max for %pOF related printk, if add new flag, please
> + * increase following marco, and add abbreviations to tbuf in
> + * device_node_string function.
> + */

I'm not sure it's correct style for multi-line comments.

> +#define OF_DEVICE_NODE_FLAG_MAX 6 /* maximum available flags */

Altogether I think this would be done as a separate patch (with corresponding
Suggested-by tag).

> -	char tbuf[sizeof("xxxx") + 1];
> +	char tbuf[OF_DEVICE_NODE_FLAG_MAX + 1] = { "DdPBOF" };

This trick is interesting. Had you checked what code is being produced
out of it (additional memcpy()? or constants assignment on the stack?) ?

> -			tbuf[0] = of_node_check_flag(dn, OF_DYNAMIC) ? 'D' : '-';
> -			tbuf[1] = of_node_check_flag(dn, OF_DETACHED) ? 'd' : '-';
> -			tbuf[2] = of_node_check_flag(dn, OF_POPULATED) ? 'P' : '-';
> -			tbuf[3] = of_node_check_flag(dn, OF_POPULATED_BUS) ? 'B' : '-';
> -			tbuf[4] = 0;

> +			for (i = 0; i < OF_DEVICE_NODE_FLAG_MAX; i++) {
> +				if (!of_node_check_flag(dn, OF_DYNAMIC + i))
> +					tbuf[i] = '-';
> +			}

For my personal opinion the former is clearer than the latter since we have no
exported iterator over the OF device node flags. What if we decide not to use
one in the middle?

> +			tbuf[OF_DEVICE_NODE_FLAG_MAX] = 0;

This line completely depends on how we assign the initial array (see above).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] lib/vsprintf: introduce OF_DEVICE_NODE_FLAG_MAX for %pOF

[Andy Shevchenko]

On Fri, Jan 17, 2020 at 07:15:19PM +0800, lijiazi wrote:

> Signed-off-by: lijiazi <lijiazi@xiaomi.com>

This is different from From field and there is nothing in the body about it.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] lib/vsprintf: introduce OF_DEVICE_NODE_FLAG_MAX for %pOF

[Randy Dunlap]

On 1/17/20 3:15 AM, lijiazi wrote:
> Introduce OF_DEVICE_NODE_FLAG_MAX for device node printk, if add
> new device node flag, please add the corresponding flag abbreviation
> to tbuf in device_node_string.
> 
> Signed-off-by: lijiazi <lijiazi@xiaomi.com>

Hi lijiazi,

A few small comments on this patch:

> ---
>  Documentation/core-api/printk-formats.rst |  2 ++
>  include/linux/of.h                        |  6 ++++++
>  lib/vsprintf.c                            | 14 +++++++-------
>  3 files changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
> index 8ebe46b1..c73ccad 100644
> --- a/Documentation/core-api/printk-formats.rst
> +++ b/Documentation/core-api/printk-formats.rst
> @@ -441,6 +441,8 @@ Examples::
>  							d - detached
>  							P - Populated
>  							B - Populated bus
> +                                                        O - Overlay
> +                                                        F - Overlay free cset

Use tabs for indentation, like the preceding lines.

>  
>  Passed by reference.
>  
> diff --git a/include/linux/of.h b/include/linux/of.h
> index c669c0a..0453601 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -147,6 +147,12 @@ extern raw_spinlock_t devtree_lock;
>  #define OF_OVERLAY		5 /* allocated for an overlay */
>  #define OF_OVERLAY_FREE_CSET	6 /* in overlay cset being freed */
>  
> +/* Add flag max for %pOF related printk, if add new flag, please
> + * increase following marco, and add abbreviations to tbuf in
> + * device_node_string function.
> + */

Incorrect multi-line comment style.

> +#define OF_DEVICE_NODE_FLAG_MAX 6 /* maximum available flags */
> +
>  #define OF_BAD_ADDR	((u64)-1)
>  
>  #ifdef CONFIG_OF


-- 
~Randy