[PATCH v3 1/3] lib/vsprintf: add two device node flags for %pOF

[PATCH v3 1/3] lib/vsprintf: add two device node flags for %pOF

[lijiazi]

Add two device node flags into printk-formats.rst.

Suggested-by: Randy Dunlap <rdunlap@infradead.org>
Signed-off-by: lijiazi <lijiazi@xiaomi.com>
---
Changes in v3:
 - use tab instead of space.
 - split v2 to 3 patches.
---
 Documentation/core-api/printk-formats.rst | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
index 8ebe46b1..9271f20 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.
 
-- 
2.7.4

[PATCH v3 2/3] lib/vsprintf: introduce OF_DEVICE_NODE_FLAG_MAX

[lijiazi]

introduce OF_DEVICE_NODE_FLAG_MAX for %pOF related printk.

Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: lijiazi <lijiazi@xiaomi.com>
---
Changes in v3:
 - fix incorrect multi-line comment style.
 - split v2 to 3 patches.
---
 include/linux/of.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/include/linux/of.h b/include/linux/of.h
index c669c0a..c5bbfa6 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -147,6 +147,13 @@ 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 OF_DEVICE_NODE_FLAG_MAX for %pOF related printk.
+ * if there is any change on these flags, please synchronize the change
+ * to device_node_string function in lib/vsprintf.c.
+ */
+#define OF_DEVICE_NODE_FLAG_MAX 6 /* Maximum available flags */
+
 #define OF_BAD_ADDR	((u64)-1)
 
 #ifdef CONFIG_OF
-- 
2.7.4

[PATCH v3 3/3] lib/vsprintf: add two device node flags

[lijiazi]

Add two device node flags, and use OF_DEVICE_NODE_FLAG_MAX instead
of sizeof("xxxx").

Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: lijiazi <lijiazi@xiaomi.com>
---
Changes in v3:
 - check the flag in the same way as before.
 - split v2 to 3 patches.
---
 lib/vsprintf.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 7c488a1..b73e584 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1937,7 +1937,7 @@ 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];
 	const char *p;
 	int ret;
 	char *buf_start = buf;
@@ -2001,7 +2001,9 @@ char *device_node_string(char *buf, char *end, struct device_node *dn,
 			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;
+			tbuf[4] = of_node_check_flag(dn, OF_OVERLAY) ? 'O' : '-';
+			tbuf[5] = of_node_check_flag(dn, OF_OVERLAY_FREE_CSET) ? 'F' : '-';
+			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 v3 1/3] lib/vsprintf: add two device node flags for %pOF

[Sergey Senozhatsky]

On (20/01/20 19:38), lijiazi wrote:
>  Documentation/core-api/printk-formats.rst | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
> index 8ebe46b1..9271f20 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

I think these 3 can be just one patch. Patching Documentation and
vsprintf in separate steps might be a bit inconvenient.

	-ss

Re: [PATCH v3 2/3] lib/vsprintf: introduce OF_DEVICE_NODE_FLAG_MAX

[Sergey Senozhatsky]

On (20/01/20 19:38), lijiazi wrote:
[..]
> Changes in v3:
>  - fix incorrect multi-line comment style.
>  - split v2 to 3 patches.
> ---
>  include/linux/of.h | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/include/linux/of.h b/include/linux/of.h
> index c669c0a..c5bbfa6 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -147,6 +147,13 @@ 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 OF_DEVICE_NODE_FLAG_MAX for %pOF related printk.
> + * if there is any change on these flags, please synchronize the change
> + * to device_node_string function in lib/vsprintf.c.

So maybe the flags can become enum then?

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

	-ss

Re: [PATCH v3 1/3] lib/vsprintf: add two device node flags for %pOF

[Andy Shevchenko]

On Mon, Jan 20, 2020 at 10:36:52PM +0900, Sergey Senozhatsky wrote:
> On (20/01/20 19:38), lijiazi wrote:
> >  Documentation/core-api/printk-formats.rst | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
> > index 8ebe46b1..9271f20 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
> 
> I think these 3 can be just one patch. Patching Documentation and
> vsprintf in separate steps might be a bit inconvenient.

The new flags addition may be a subject to discuss. That's why I suggested
to split it out. Though the above misses the functionality for these flags.

Adding a flag counter to the OF header is orthogonal to printf() changes,
it may be there independently. So, it should be a separate patch.

The comment added there is doubtful, though.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 2/3] lib/vsprintf: introduce OF_DEVICE_NODE_FLAG_MAX

[Andy Shevchenko]

On Mon, Jan 20, 2020 at 07:38:28PM +0800, lijiazi wrote:
> introduce OF_DEVICE_NODE_FLAG_MAX for %pOF related printk.

introduce -> Introduce

> @@ -147,6 +147,13 @@ 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 OF_DEVICE_NODE_FLAG_MAX for %pOF related printk.
> + * if there is any change on these flags, please synchronize the change
> + * to device_node_string function in lib/vsprintf.c.

This is doubtful comment. If we don't want to print some flags in printf()?
And in general this is orthogonal to the printf() changes.

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

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 3/3] lib/vsprintf: add two device node flags

[Andy Shevchenko]

On Mon, Jan 20, 2020 at 07:38:29PM +0800, lijiazi wrote:
> Add two device node flags, and use OF_DEVICE_NODE_FLAG_MAX instead
> of sizeof("xxxx").

...

>  			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;

This is fine to leave untouched. See below.

> +			tbuf[4] = of_node_check_flag(dn, OF_OVERLAY) ? 'O' : '-';
> +			tbuf[5] = of_node_check_flag(dn, OF_OVERLAY_FREE_CSET) ? 'F' : '-';

These two should be part of patch 1, which in turn should be last in the series.

> +			tbuf[OF_DEVICE_NODE_FLAG_MAX] = 0;

This one also, but in a form of explicit number, if you afraid of problems
here, we may add something like

	BUILD_BUG_ON(OF_DEVICE_NODE_FLAG_MAX < ...);

where ... depends on amount of flags we print here.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 1/3] lib/vsprintf: add two device node flags for %pOF

[Andy Shevchenko]

On Mon, Jan 20, 2020 at 07:38:27PM +0800, lijiazi wrote:
> Add two device node flags into printk-formats.rst.

This should be rather third patch in the series.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 1/3] lib/vsprintf: add two device node flags for %pOF

[Sergey Senozhatsky]

On (20/01/20 16:14), Andy Shevchenko wrote:
> On Mon, Jan 20, 2020 at 10:36:52PM +0900, Sergey Senozhatsky wrote:
> > On (20/01/20 19:38), lijiazi wrote:
> > >  Documentation/core-api/printk-formats.rst | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
> > > index 8ebe46b1..9271f20 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
> > 
> > I think these 3 can be just one patch. Patching Documentation and
> > vsprintf in separate steps might be a bit inconvenient.
> 
> The new flags addition may be a subject to discuss. That's why I suggested
> to split it out. Though the above misses the functionality for these flags.
> 
> Adding a flag counter to the OF header is orthogonal to printf() changes,
> it may be there independently. So, it should be a separate patch.

Oh, yes! I meant 2 patches, not 3 patches - Documentation and vsprintf().
OF header patch is completely independent. Agreed.

	-ss

Re: [PATCH v3 3/3] lib/vsprintf: add two device node flags

[Petr Mladek]

On Mon 2020-01-20 16:19:51, Andy Shevchenko wrote:
> On Mon, Jan 20, 2020 at 07:38:29PM +0800, lijiazi wrote:
> > Add two device node flags, and use OF_DEVICE_NODE_FLAG_MAX instead
> > of sizeof("xxxx").
> 
> ...
> 
> >  			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;
> 
> This is fine to leave untouched. See below.
> 
> > +			tbuf[4] = of_node_check_flag(dn, OF_OVERLAY) ? 'O' : '-';
> > +			tbuf[5] = of_node_check_flag(dn, OF_OVERLAY_FREE_CSET) ? 'F' : '-';
> 
> These two should be part of patch 1, which in turn should be last in the series.
> 
> > +			tbuf[OF_DEVICE_NODE_FLAG_MAX] = 0;
> 
> This one also, but in a form of explicit number, if you afraid of problems
> here, we may add something like

I agree that explicit number would be better here:

			tbuf[6] = 0;

And I like the build check. I would even check for the exact number.
It would help to keep the code updated when the number is modified.

	BUILD_BUG_ON(OF_DEVICE_NODE_FLAG_MAX != 6);

Finally, I would put all three changes into the same patch. And
remove the comment above OF_DEVICE_NODE_FLAG_MAX definition
completely.

Best Regards,
Petr