[PATCH v2 1/3] lib/vsprintf: Replace hidden BUILD_BUG_ON() with static_assert()

[PATCH v2 1/3] lib/vsprintf: Replace hidden BUILD_BUG_ON() with static_assert()

[Andy Shevchenko]

First of all, there is no compile time check for the SMALL
to be ' ' (0x20, i.e. space). Second, for ZEROPAD the check
is hidden in the code.

For better maintenance replace BUILD_BUG_ON() with static_assert()
for ZEROPAD and move it closer to the definition. While at it,
introduce check for SMALL.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
v2: replaced to have plain defined constant on left side, added tag (Steven)
 lib/vsprintf.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 31a674dd2674..f90f09682977 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -381,6 +381,9 @@ int num_to_str(char *buf, int size, unsigned long long num, unsigned int width)
 #define SMALL	32		/* use lowercase in hex (must be 32 == 0x20) */
 #define SPECIAL	64		/* prefix hex with "0x", octal with "0" */
 
+static_assert(ZEROPAD == ('0' - ' '));
+static_assert(SMALL == ' ');
+
 enum format_type {
 	FORMAT_TYPE_NONE, /* Just a string part */
 	FORMAT_TYPE_WIDTH,
@@ -507,7 +510,7 @@ char *number(char *buf, char *end, unsigned long long num,
 	/* zero or space padding */
 	if (!(spec.flags & LEFT)) {
 		char c = ' ' + (spec.flags & ZEROPAD);
-		BUILD_BUG_ON(' ' + ZEROPAD != '0');
+
 		while (--field_width >= 0) {
 			if (buf < end)
 				*buf = c;
-- 
2.27.0

[PATCH v2 2/3] lib/vsprintf: Replace custom spec to print decimals with generic one

[Andy Shevchenko]

When printing phandle via %pOFp the custom spec is used. First of all,
it has a SMALL flag which makes no sense for decimal numbers. Second,
we have already default spec for decimal numbers. Use the latter in
the %pOFp case as well.

Cc: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
Cc: Rob Herring <robh@kernel.org>
Cc: Joe Perches <joe@perches.com>
Cc: Grant Likely <grant.likely@arm.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
v2: added people, updated commit message to reflect %pOFp, added tag (Steven)
 lib/vsprintf.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index f90f09682977..182a3e2e1629 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1979,12 +1979,6 @@ char *device_node_string(char *buf, char *end, struct device_node *dn,
 	char *buf_start = buf;
 	struct property *prop;
 	bool has_mult, pass;
-	static const struct printf_spec num_spec = {
-		.flags = SMALL,
-		.field_width = -1,
-		.precision = -1,
-		.base = 10,
-	};
 
 	struct printf_spec str_spec = spec;
 	str_spec.field_width = -1;
@@ -2024,7 +2018,7 @@ char *device_node_string(char *buf, char *end, struct device_node *dn,
 			str_spec.precision = precision;
 			break;
 		case 'p':	/* phandle */
-			buf = number(buf, end, (unsigned int)dn->phandle, num_spec);
+			buf = number(buf, end, (unsigned int)dn->phandle, default_dec_spec);
 			break;
 		case 'P':	/* path-spec */
 			p = fwnode_get_name(of_fwnode_handle(dn));
-- 
2.27.0

Re: [PATCH v2 2/3] lib/vsprintf: Replace custom spec to print decimals with generic one

[Andy Shevchenko]

On Fri, Jul 31, 2020 at 09:08:23PM +0300, Andy Shevchenko wrote:
> When printing phandle via %pOFp the custom spec is used. First of all,
> it has a SMALL flag which makes no sense for decimal numbers. Second,
> we have already default spec for decimal numbers. Use the latter in
> the %pOFp case as well.

Forgot to mention that the patch (and actually the series) has been tested
against test_printf and OF unittest modules.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/3] lib/vsprintf: Replace custom spec to print decimals with generic one

[Joe Perches]

On Fri, 2020-07-31 at 21:08 +0300, Andy Shevchenko wrote:
> When printing phandle via %pOFp the custom spec is used. First of all,
> it has a SMALL flag which makes no sense for decimal numbers. Second,
> we have already default spec for decimal numbers. Use the latter in
> the %pOFp case as well.
[]
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
[]
> @@ -1979,12 +1979,6 @@ char *device_node_string(char *buf, char *end, struct device_node *dn,
>  	char *buf_start = buf;
>  	struct property *prop;
>  	bool has_mult, pass;
> -	static const struct printf_spec num_spec = {
> -		.flags = SMALL,
> -		.field_width = -1,
> -		.precision = -1,
> -		.base = 10,
> -	};
>  
>  	struct printf_spec str_spec = spec;
>  	str_spec.field_width = -1;
> @@ -2024,7 +2018,7 @@ char *device_node_string(char *buf, char *end, struct device_node *dn,
>  			str_spec.precision = precision;
>  			break;
>  		case 'p':	/* phandle */
> -			buf = number(buf, end, (unsigned int)dn->phandle, num_spec);
> +			buf = number(buf, end, (unsigned int)dn->phandle, default_dec_spec);

Could changing .precision = -1 to 0 change any output?

Re: [PATCH v2 2/3] lib/vsprintf: Replace custom spec to print decimals with generic one

[Steven Rostedt]

On Fri, 31 Jul 2020 11:20:21 -0700
Joe Perches <joe@perches.com> wrote:

> On Fri, 2020-07-31 at 21:08 +0300, Andy Shevchenko wrote:
> > When printing phandle via %pOFp the custom spec is used. First of all,
> > it has a SMALL flag which makes no sense for decimal numbers. Second,
> > we have already default spec for decimal numbers. Use the latter in
> > the %pOFp case as well.  
> []
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c  
> []
> > @@ -1979,12 +1979,6 @@ char *device_node_string(char *buf, char *end, struct device_node *dn,
> >  	char *buf_start = buf;
> >  	struct property *prop;
> >  	bool has_mult, pass;
> > -	static const struct printf_spec num_spec = {
> > -		.flags = SMALL,
> > -		.field_width = -1,
> > -		.precision = -1,
> > -		.base = 10,
> > -	};
> >  
> >  	struct printf_spec str_spec = spec;
> >  	str_spec.field_width = -1;
> > @@ -2024,7 +2018,7 @@ char *device_node_string(char *buf, char *end, struct device_node *dn,
> >  			str_spec.precision = precision;
> >  			break;
> >  		case 'p':	/* phandle */
> > -			buf = number(buf, end, (unsigned int)dn->phandle, num_spec);
> > +			buf = number(buf, end, (unsigned int)dn->phandle, default_dec_spec);  
> 
> Could changing .precision = -1 to 0 change any output?
> 

static const struct printf_spec default_dec_spec = {
	.base = 10,
	.precision = -1,
};


It's the field_width that changes from -1 to 0, which shouldn't cause
any issue.

-- Steve

Re: [PATCH v2 2/3] lib/vsprintf: Replace custom spec to print decimals with generic one

[Sergey Senozhatsky]

On (20/07/31 16:48), Steven Rostedt wrote:
[..]
> > > -	static const struct printf_spec num_spec = {
> > > -		.flags = SMALL,
> > > -		.field_width = -1,
> > > -		.precision = -1,
> > > -		.base = 10,
> > > -	};
> > >  
> > >  	struct printf_spec str_spec = spec;
> > >  	str_spec.field_width = -1;
> > > @@ -2024,7 +2018,7 @@ char *device_node_string(char *buf, char *end, struct device_node *dn,
> > >  			str_spec.precision = precision;
> > >  			break;
> > >  		case 'p':	/* phandle */
> > > -			buf = number(buf, end, (unsigned int)dn->phandle, num_spec);
> > > +			buf = number(buf, end, (unsigned int)dn->phandle, default_dec_spec);  
> > 
> > Could changing .precision = -1 to 0 change any output?
> > 
> 
> static const struct printf_spec default_dec_spec = {
> 	.base = 10,
> 	.precision = -1,
> };
> 
> 
> It's the field_width that changes from -1 to 0, which shouldn't cause
> any issue.

That's a good question, both .field_width and .precision are now zeroes,
and not -1. But it seems that this doesn't change anything in the end.

Reviewed-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>

	-ss

Re: [PATCH v2 2/3] lib/vsprintf: Replace custom spec to print decimals with generic one

[Sergey Senozhatsky]

On (20/07/31 21:08), Andy Shevchenko wrote:
> When printing phandle via %pOFp the custom spec is used. First of all,
> it has a SMALL flag which makes no sense for decimal numbers. Second,
> we have already default spec for decimal numbers. Use the latter in
> the %pOFp case as well.
> 
> Cc: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> Cc: Rob Herring <robh@kernel.org>
> Cc: Joe Perches <joe@perches.com>
> Cc: Grant Likely <grant.likely@arm.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

Reviewed-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>

	-ss

[PATCH v2 3/3] lib/vsprintf: Force type of flags value for gfp_t

[Andy Shevchenko]

Sparse is not happy about restricted type being assigned:
  lib/vsprintf.c:1940:23: warning: incorrect type in assignment (different base types)
  lib/vsprintf.c:1940:23:    expected unsigned long [assigned] flags
  lib/vsprintf.c:1940:23:    got restricted gfp_t [usertype]

Force type of flags value to make sparse happy.

Cc: Steven Rostedt (VMware) <rostedt@goodmis.org>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v2: used explicit type to be forced to (Steven)
 lib/vsprintf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 182a3e2e1629..c155769559ab 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1937,7 +1937,7 @@ char *flags_string(char *buf, char *end, void *flags_ptr,
 		names = vmaflag_names;
 		break;
 	case 'g':
-		flags = *(gfp_t *)flags_ptr;
+		flags = (__force unsigned long)(*(gfp_t *)flags_ptr);
 		names = gfpflag_names;
 		break;
 	default:
-- 
2.27.0

Re: [PATCH v2 3/3] lib/vsprintf: Force type of flags value for gfp_t

[Steven Rostedt]

On Fri, 31 Jul 2020 21:08:24 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> Sparse is not happy about restricted type being assigned:
>   lib/vsprintf.c:1940:23: warning: incorrect type in assignment (different base types)
>   lib/vsprintf.c:1940:23:    expected unsigned long [assigned] flags
>   lib/vsprintf.c:1940:23:    got restricted gfp_t [usertype]
> 
> Force type of flags value to make sparse happy.
> 
> Cc: Steven Rostedt (VMware) <rostedt@goodmis.org>

Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

Petr or Sergey,

Want to take these through the printk tree?

-- Steve

> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> v2: used explicit type to be forced to (Steven)
>  lib/vsprintf.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 182a3e2e1629..c155769559ab 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -1937,7 +1937,7 @@ char *flags_string(char *buf, char *end, void *flags_ptr,
>  		names = vmaflag_names;
>  		break;
>  	case 'g':
> -		flags = *(gfp_t *)flags_ptr;
> +		flags = (__force unsigned long)(*(gfp_t *)flags_ptr);
>  		names = gfpflag_names;
>  		break;
>  	default:

Re: [PATCH v2 3/3] lib/vsprintf: Force type of flags value for gfp_t

[Sergey Senozhatsky]

On (20/07/31 16:49), Steven Rostedt wrote:
> On Fri, 31 Jul 2020 21:08:24 +0300
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> 
> > Sparse is not happy about restricted type being assigned:
> >   lib/vsprintf.c:1940:23: warning: incorrect type in assignment (different base types)
> >   lib/vsprintf.c:1940:23:    expected unsigned long [assigned] flags
> >   lib/vsprintf.c:1940:23:    got restricted gfp_t [usertype]
> > 
> > Force type of flags value to make sparse happy.
> > 
> > Cc: Steven Rostedt (VMware) <rostedt@goodmis.org>
> 
> Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

Thanks.

> Petr or Sergey,
>
> Want to take these through the printk tree?

Can take through the printk-tree.

	-ss

Re: [PATCH v2 1/3] lib/vsprintf: Replace hidden BUILD_BUG_ON() with static_assert()

[Sergey Senozhatsky]

On (20/07/31 21:08), Andy Shevchenko wrote:
> First of all, there is no compile time check for the SMALL
> to be ' ' (0x20, i.e. space). Second, for ZEROPAD the check
> is hidden in the code.
> 
> For better maintenance replace BUILD_BUG_ON() with static_assert()
> for ZEROPAD and move it closer to the definition. While at it,
> introduce check for SMALL.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

Reviewed-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>

	-ss

Re: [PATCH v2 1/3] lib/vsprintf: Replace hidden BUILD_BUG_ON() with static_assert()

[Sergey Senozhatsky]

On (20/07/31 21:08), Andy Shevchenko wrote:
> First of all, there is no compile time check for the SMALL
> to be ' ' (0x20, i.e. space). Second, for ZEROPAD the check
> is hidden in the code.
> 
> For better maintenance replace BUILD_BUG_ON() with static_assert()
> for ZEROPAD and move it closer to the definition. While at it,
> introduce check for SMALL.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

The series was applied to for-5.9


I ran more tests (+ cross compile tests: ppc/ppc64/arm/arm64/x86/x86_64).

	-ss

Re: [PATCH v2 1/3] lib/vsprintf: Replace hidden BUILD_BUG_ON() with static_assert()

[Petr Mladek]

On Sat 2020-08-01 13:46:20, Sergey Senozhatsky wrote:
> On (20/07/31 21:08), Andy Shevchenko wrote:
> > First of all, there is no compile time check for the SMALL
> > to be ' ' (0x20, i.e. space). Second, for ZEROPAD the check
> > is hidden in the code.
> > 
> > For better maintenance replace BUILD_BUG_ON() with static_assert()
> > for ZEROPAD and move it closer to the definition. While at it,
> > introduce check for SMALL.
> > 
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

Just for record, for the entire series:

Reviewed-by: Petr Mladek <pmladek@suse.com>

> The series was applied to for-5.9

There is no need to rebase the git repository. I just want to inform you
that the changes look fine to me.

Thanks a lot for the fixes and their handling.

Best Regards,
Petr