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

[PATCH v1 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>
---
 lib/vsprintf.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 31a674dd2674..8a8ac7ce0289 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 v1 2/3] lib/vsprintf: Replace custom spec to print decimals with generic one

[Andy Shevchenko]

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

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 lib/vsprintf.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 8a8ac7ce0289..90d818ef03c5 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 v1 2/3] lib/vsprintf: Replace custom spec to print decimals with generic one

[Steven Rostedt]

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

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

The change log threw me off, as this is specifically for %pOFp. This
only affects the phandle ('p') portion. Please update the change log to
reflect that.

Also, I would add in Cc those that added this code and looked it over:

Cc: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
Cc: Joe Perches <joe@perches.com>
Cc: Rob Herring <robh@kernel.org>

And mentioned in that code's change log:

Cc: Grant Likely <grant.likely@arm.com>


Otherwise,

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

-- Steve



> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  lib/vsprintf.c | 8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 8a8ac7ce0289..90d818ef03c5 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));

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

[Andy Shevchenko]

On Fri, Jul 31, 2020 at 6:17 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Fri, 31 Jul 2020 15:31:44 +0300
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
>
> > When printing phandle in %pOF the custom spec is used. First of all,
> > it has SMALL flag which makes no sense for decimal numbers. Second,
> > we have already default spec for decimal numbers. Use the latter in
> > %pOF case as well.
>
> The change log threw me off, as this is specifically for %pOFp. This
> only affects the phandle ('p') portion. Please update the change log to
> reflect that.

I think phandle implies this, but I update for sake of clarification.

> Also, I would add in Cc those that added this code and looked it over:
>
> Cc: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> Cc: Joe Perches <joe@perches.com>
> Cc: Rob Herring <robh@kernel.org>
>
> And mentioned in that code's change log:
>
> Cc: Grant Likely <grant.likely@arm.com>

Okay, will do.
Also I have to mention that this (actually entire series) has been
tested against test_printf and OF unittest modules.

> Otherwise,
>
> Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
>
> -- Steve
>
>
>
> >
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> >  lib/vsprintf.c | 8 +-------
> >  1 file changed, 1 insertion(+), 7 deletions(-)
> >
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > index 8a8ac7ce0289..90d818ef03c5 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));
>


-- 
With Best Regards,
Andy Shevchenko

[PATCH v1 3/3] lib/vsprintf: Force type of flags 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 to make sparse happy.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 lib/vsprintf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 90d818ef03c5..118e2727d058 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 typeof(flags))(*(gfp_t *)flags_ptr);
 		names = gfpflag_names;
 		break;
 	default:
-- 
2.27.0

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

[Steven Rostedt]

On Fri, 31 Jul 2020 15:31:45 +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 to make sparse happy.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  lib/vsprintf.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 90d818ef03c5..118e2727d058 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 typeof(flags))(*(gfp_t *)flags_ptr);

Do we really need to say "typeof(flags)" ? What about simply using
flags' type?

		flags = (__force unsigned long)(*(gfp_t *)flags_ptr);

?

I mean, it's not like flags is a global. It's defined a few lines above.

-- Steve


>  		names = gfpflag_names;
>  		break;
>  	default:

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

[Andy Shevchenko]

On Fri, Jul 31, 2020 at 6:38 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Fri, 31 Jul 2020 15:31:45 +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 to make sparse happy.
> >
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> >  lib/vsprintf.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > index 90d818ef03c5..118e2727d058 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 typeof(flags))(*(gfp_t *)flags_ptr);
>
> Do we really need to say "typeof(flags)" ? What about simply using
> flags' type?

Whatever you prefer. I actually came with the latter and switched to the former.
So, I'll switch back for v2.

>
>                 flags = (__force unsigned long)(*(gfp_t *)flags_ptr);
>
> ?
>
> I mean, it's not like flags is a global. It's defined a few lines above.
>
> -- Steve
>
>
> >               names = gfpflag_names;
> >               break;
> >       default:
>


-- 
With Best Regards,
Andy Shevchenko

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

[Steven Rostedt]

On Fri, 31 Jul 2020 18:49:46 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> > > +++ 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 typeof(flags))(*(gfp_t *)flags_ptr);  
> >
> > Do we really need to say "typeof(flags)" ? What about simply using
> > flags' type?  
> 
> Whatever you prefer. I actually came with the latter and switched to the former.
> So, I'll switch back for v2.

For this instance, I prefer spelling out the type.

Thanks!

-- Steve

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

[Steven Rostedt]

On Fri, 31 Jul 2020 15:31:43 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> 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>
> ---
>  lib/vsprintf.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 31a674dd2674..8a8ac7ce0289 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');

I would have it match the comment above:

static_assert(ZEROPAD == ('0' - ' '));

But other than that:

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

-- Steve

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

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

[Andy Shevchenko]

On Fri, Jul 31, 2020 at 5:40 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Fri, 31 Jul 2020 15:31:43 +0300
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> 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>
> > ---
> >  lib/vsprintf.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > index 31a674dd2674..8a8ac7ce0289 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');
>
> I would have it match the comment above:
>
> static_assert(ZEROPAD == ('0' - ' '));

Okay, makes sense.
Will do in v2.

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

Thanks!

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


-- 
With Best Regards,
Andy Shevchenko