[v4,02/28] printk: Add print format (%pra) for struct range

Commit Message

Ira Weiny Oct. 7, 2024, 11:16 p.m. UTC

The use of struct range in the CXL subsystem is growing.  In particular,
the addition of Dynamic Capacity devices uses struct range in a number
of places which are reported in debug and error messages.

To wit requiring the printing of the start/end fields in each print
became cumbersome.  Dan Williams mentions in [1] that it might be time
to have a print specifier for struct range similar to struct resource

A few alternatives were considered including '%par', '%r', and '%pn'.
%pra follows that struct range is similar to struct resource (%p[rR])
but need to be different.  Based on discussions with Petr and Andy
'%pra' was chosen.[2]

Andy also suggested to keep the range prints similar to struct resource
though combined code.  Add hex_range() to handle printing for both
pointer types.

To: Petr Mladek <[e-mail]>
To: Steven Rostedt <[e-mail]>
To: Andy Shevchenko <[e-mail]>
To: Rasmus Villemoes <[e-mail]>
To: Sergey Senozhatsky <[e-mail]>
To: Jonathan Corbet <[e-mail]> (maintainer:DOCUMENTATION)
Cc: [e-mail]
Cc: [e-mail] (open list)
Link: https://lore.kernel.org/all/[e-mail]/ [1]
Link: https://lore.kernel.org/all/[e-mail]/ [2]
Suggested-by: "Dan Williams" <[e-mail]>
Signed-off-by: Ira Weiny <[e-mail]>

---
Changes:
[Andy: create new hex_range() and use it in both range/resource]
[Petr/Andy: Use %pra]
[Andy: Add test case start > end]
[Petr: Update documentation]
[Petr: use 'range -']
[Petr: fixup printf_spec specifiers]
[Petr: add lib/test_printf test]
---
 Documentation/core-api/printk-formats.rst | 13 ++++++++
 lib/test_printf.c                         | 26 +++++++++++++++
 lib/vsprintf.c                            | 55 +++++++++++++++++++++++++++----
 3 files changed, 88 insertions(+), 6 deletions(-)

Comments

Andy Shevchenko Oct. 8, 2024, 4:56 p.m. UTC | #1

On Mon, Oct 07, 2024 at 06:16:08PM -0500, Ira Weiny wrote:
> The use of struct range in the CXL subsystem is growing.  In particular,
> the addition of Dynamic Capacity devices uses struct range in a number
> of places which are reported in debug and error messages.
> 
> To wit requiring the printing of the start/end fields in each print
> became cumbersome.  Dan Williams mentions in [1] that it might be time
> to have a print specifier for struct range similar to struct resource
> 
> A few alternatives were considered including '%par', '%r', and '%pn'.
> %pra follows that struct range is similar to struct resource (%p[rR])
> but need to be different.  Based on discussions with Petr and Andy
> '%pra' was chosen.[2]
> 
> Andy also suggested to keep the range prints similar to struct resource
> though combined code.  Add hex_range() to handle printing for both
> pointer types.

...

> +static void __init
> +struct_range(void)
> +{
> +	struct range test_range = {
> +		.start = 0xc0ffee00ba5eba11,
> +		.end = 0xc0ffee00ba5eba11,
> +	};

A side note, can we add something like

#define DEFINE_RANGE(start, end)	\
	(struct range) {		\
		.start = (start),	\
		.end = (end),		\
	}

in range.h and use here and in the similar cases?

> +	test("[range 0xc0ffee00ba5eba11]", "%pra", &test_range);
> +
> +	test_range = (struct range) {
> +		.start = 0xc0ffee,
> +		.end = 0xba5eba11,
> +	};
> +	test("[range 0x0000000000c0ffee-0x00000000ba5eba11]",
> +	     "%pra", &test_range);
> +
> +	test_range = (struct range) {
> +		.start = 0xba5eba11,
> +		.end = 0xc0ffee,
> +	};
> +	test("[range 0x00000000ba5eba11-0x0000000000c0ffee]",
> +	     "%pra", &test_range);
> +}

...


> +char *hex_range(char *buf, char *end, u64 start_val, u64 end_val,
> +		struct printf_spec spec)
> +{
> +	buf = number(buf, end, start_val, spec);
> +	if (start_val != end_val) {
> +		if (buf < end)
> +			*buf++ = '-';
> +		buf = number(buf, end, end_val, spec);
> +	}
> +	return buf;
> +}

Perhaps

	buf = number(buf, end, start_val, spec);
	if (start_val == end_val)
		return buf;

	if (buf < end)
		*buf++ = '-';
	return number(buf, end, end_val, spec);

(yes, I have seen the original code)?


> +static noinline_for_stack
> +char *range_string(char *buf, char *end, const struct range *range,
> +		   struct printf_spec spec, const char *fmt)
> +{
> +#define RANGE_DECODED_BUF_SIZE		((2 * sizeof(struct range)) + 4)
> +#define RANGE_PRINT_BUF_SIZE		sizeof("[range -]")
> +	char sym[RANGE_DECODED_BUF_SIZE + RANGE_PRINT_BUF_SIZE];
> +	char *p = sym, *pend = sym + sizeof(sym);
> +
> +	struct printf_spec range_spec = {
> +		.field_width = 2 + 2 * sizeof(range->start), /* 0x + 2 * 8 */
> +		.flags = SPECIAL | SMALL | ZEROPAD,
> +		.base = 16,
> +		.precision = -1,
> +	};
> +
> +	if (check_pointer(&buf, end, range, spec))
> +		return buf;
> +
> +	*p++ = '[';
> +	p = string_nocheck(p, pend, "range ", default_str_spec);
> +	p = hex_range(p, pend, range->start, range->end, range_spec);
> +	*p++ = ']';
> +	*p = '\0';
> +
> +	return string_nocheck(buf, end, sym, spec);
> +}

...

> + * - 'ra' struct ranges [range 0x00 - 0xff]

Is it possible to get only bytes out of this? I thought we have always
64-bit values here, no?

...

>  	case 'B':
>  		return symbol_string(buf, end, ptr, spec, fmt);
> -	case 'R':
>  	case 'r':
> +		switch (fmt[1]) {
> +		case 'a':
> +			return range_string(buf, end, ptr, spec, fmt);
> +		}
> +		fallthrough;
> +	case 'R':
>  		return resource_string(buf, end, ptr, spec, fmt);

Do we have default-less switches in the code (in this file)?

Actually I would suggest to move this to a wrapper like time_and_date().

Jonathan Cameron Oct. 9, 2024, 12:27 p.m. UTC | #2

On Tue, 8 Oct 2024 19:56:20 +0300
Andy Shevchenko <[e-mail]> wrote:

> On Mon, Oct 07, 2024 at 06:16:08PM -0500, Ira Weiny wrote:
> > The use of struct range in the CXL subsystem is growing.  In particular,
> > the addition of Dynamic Capacity devices uses struct range in a number
> > of places which are reported in debug and error messages.
> > 
> > To wit requiring the printing of the start/end fields in each print
> > became cumbersome.  Dan Williams mentions in [1] that it might be time
> > to have a print specifier for struct range similar to struct resource
> > 
> > A few alternatives were considered including '%par', '%r', and '%pn'.
> > %pra follows that struct range is similar to struct resource (%p[rR])
> > but need to be different.  Based on discussions with Petr and Andy
> > '%pra' was chosen.[2]
> > 
> > Andy also suggested to keep the range prints similar to struct resource
> > though combined code.  Add hex_range() to handle printing for both
> > pointer types.  
> 
> ...
> 
> > +static void __init
> > +struct_range(void)
> > +{
> > +	struct range test_range = {
> > +		.start = 0xc0ffee00ba5eba11,
> > +		.end = 0xc0ffee00ba5eba11,
> > +	};  
> 
> A side note, can we add something like
> 
> #define DEFINE_RANGE(start, end)	\
> 	(struct range) {		\
> 		.start = (start),	\
> 		.end = (end),		\
> 	}
> 
> in range.h and use here and in the similar cases?

DEFINE_XXXX at least sometimes is used in cases that create the
variable as well.  E.g. DEFINE_MUTEX()

INIT_RANGE() maybe?

Rasmus Villemoes Oct. 9, 2024, 1:30 p.m. UTC | #3

Ira Weiny <[e-mail]> writes:

> ---
>  Documentation/core-api/printk-formats.rst | 13 ++++++++
>  lib/test_printf.c                         | 26 +++++++++++++++
>  lib/vsprintf.c                            | 55 +++++++++++++++++++++++++++----
>  3 files changed, 88 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
> index 14e093da3ccd..03b102fc60bb 100644
> --- a/Documentation/core-api/printk-formats.rst
> +++ b/Documentation/core-api/printk-formats.rst
> @@ -231,6 +231,19 @@ width of the CPU data path.
>  
>  Passed by reference.
>  
> +Struct Range
> +------------

Probably neither of those words should be capitalized.

> +
> +::
> +
> +	%pra    [range 0x0000000060000000-0x000000006fffffff]
> +	%pra    [range 0x0000000060000000]
> +
> +For printing struct range.  struct range holds an arbitrary range of u64
> +values.  If start is equal to end only 1 value is printed.
> +
> +Passed by reference.
> +
>  DMA address types dma_addr_t
>  ----------------------------
>  
> diff --git a/lib/test_printf.c b/lib/test_printf.c
> index 5afdf5efc627..e3e75b6d10a0 100644
> --- a/lib/test_printf.c
> +++ b/lib/test_printf.c
> @@ -432,6 +432,31 @@ struct_resource(void)
>  	     "%pR", &test_resource);
>  }
>  
> +static void __init
> +struct_range(void)
> +{
> +	struct range test_range = {
> +		.start = 0xc0ffee00ba5eba11,
> +		.end = 0xc0ffee00ba5eba11,
> +	};
> +
> +	test("[range 0xc0ffee00ba5eba11]", "%pra", &test_range);
> +
> +	test_range = (struct range) {
> +		.start = 0xc0ffee,
> +		.end = 0xba5eba11,
> +	};
> +	test("[range 0x0000000000c0ffee-0x00000000ba5eba11]",
> +	     "%pra", &test_range);
> +
> +	test_range = (struct range) {
> +		.start = 0xba5eba11,
> +		.end = 0xc0ffee,
> +	};
> +	test("[range 0x00000000ba5eba11-0x0000000000c0ffee]",
> +	     "%pra", &test_range);
> +}
> +

Thanks for including tests!

Rather than the struct assignments, I think it's easier to read if you
just do

  struct range r;

  r.start = 0xc0ffee00ba5eba11;
  r.end   = r.start;
  ...

  r.start = 0xc0ffee;
  r.end   = 0xba5eba11;
  ...

which saves two lines per test and for the first one makes it more
obvious that the start and end values are identical.

>  static void __init
>  addr(void)
>  {
> @@ -807,6 +832,7 @@ test_pointer(void)
>  	symbol_ptr();
>  	kernel_ptr();
>  	struct_resource();
> +	struct_range();
>  	addr();
>  	escaped_str();
>  	hex_string();
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 09f022ba1c05..f8f5ed8f4d39 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -1039,6 +1039,19 @@ static const struct printf_spec default_dec04_spec = {
>  	.flags = ZEROPAD,
>  };
>  
> +static noinline_for_stack
> +char *hex_range(char *buf, char *end, u64 start_val, u64 end_val,
> +		struct printf_spec spec)
> +{
> +	buf = number(buf, end, start_val, spec);
> +	if (start_val != end_val) {
> +		if (buf < end)
> +			*buf++ = '-';

No. Either all your callers pass a (probably stack-allocated) buffer
which is guaranteed to be big enough, in which case you don't need the
"if (buf < end)", or if some callers may "print" directly to the buffer
passed to vsnprintf(), the buf++ must still be done unconditionally in
order that vsnprintf(NULL, 0, ...) [used by fx kasprintf] can accurately
determine how large the output string would be.

So, either

  *buf++ = '-'

or

  if (buf < end)
    *buf = '-';
  buf++;

Please don't mix the two. 



> +		buf = number(buf, end, end_val, spec);
> +	}
> +	return buf;
> +}
> +
>  static noinline_for_stack
>  char *resource_string(char *buf, char *end, struct resource *res,
>  		      struct printf_spec spec, const char *fmt)
> @@ -1115,11 +1128,7 @@ char *resource_string(char *buf, char *end, struct resource *res,
>  		p = string_nocheck(p, pend, "size ", str_spec);
>  		p = number(p, pend, resource_size(res), *specp);
>  	} else {
> -		p = number(p, pend, res->start, *specp);
> -		if (res->start != res->end) {
> -			*p++ = '-';
> -			p = number(p, pend, res->end, *specp);
> -		}
> +		p = hex_range(p, pend, res->start, res->end, *specp);
>  	}
>  	if (decode) {
>  		if (res->flags & IORESOURCE_MEM_64)
> @@ -1140,6 +1149,34 @@ char *resource_string(char *buf, char *end, struct resource *res,
>  	return string_nocheck(buf, end, sym, spec);
>  }
>  
> +static noinline_for_stack
> +char *range_string(char *buf, char *end, const struct range *range,
> +		   struct printf_spec spec, const char *fmt)
> +{
> +#define RANGE_DECODED_BUF_SIZE		((2 * sizeof(struct range)) + 4)
> +#define RANGE_PRINT_BUF_SIZE		sizeof("[range -]")
> +	char sym[RANGE_DECODED_BUF_SIZE + RANGE_PRINT_BUF_SIZE];

I don't think these names or the split in two constants helps
convincing that's the right amount. I have to think quite a bit to see
that 2*sizeof is because struct range has two u64 and we're printing in
hex so four-bits-per-char and probably the +4 are for two time "0x".

Why not just size the buffer directly using an "example" string?

  char sym[sizeof("[range 0x0123456789abcdef-0x0123456789abcdef]")]

> +	char *p = sym, *pend = sym + sizeof(sym);
> +
> +	struct printf_spec range_spec = {
> +		.field_width = 2 + 2 * sizeof(range->start), /* 0x + 2 * 8 */
> +		.flags = SPECIAL | SMALL | ZEROPAD,
> +		.base = 16,
> +		.precision = -1,
> +	};
> +
> +	if (check_pointer(&buf, end, range, spec))
> +		return buf;
> +
> +	*p++ = '[';
> +	p = string_nocheck(p, pend, "range ", default_str_spec);

We really should have mempcpy or stpcpy. I don't see the point of using
string_nocheck here, or not including the [ in the string copy (however
it's done). But yeah, without stpcpy() that's a bit awkward. 

Rasmus

Andy Shevchenko Oct. 9, 2024, 2:41 p.m. UTC | #4

On Wed, Oct 09, 2024 at 03:30:14PM +0200, Rasmus Villemoes wrote:

...

> Rather than the struct assignments, I think it's easier to read if you
> just do
> 
>   struct range r;
> 
>   r.start = 0xc0ffee00ba5eba11;
>   r.end   = r.start;
>   ...
> 
>   r.start = 0xc0ffee;
>   r.end   = 0xba5eba11;
>   ...
> 
> which saves two lines per test and for the first one makes it more
> obvious that the start and end values are identical.

With DEFINE_RANGE() it will save even more lines!

..

> > +		if (buf < end)
> > +			*buf++ = '-';
> 
> No. Either all your callers pass a (probably stack-allocated) buffer
> which is guaranteed to be big enough, in which case you don't need the
> "if (buf < end)", or if some callers may "print" directly to the buffer
> passed to vsnprintf(), the buf++ must still be done unconditionally in
> order that vsnprintf(NULL, 0, ...) [used by fx kasprintf] can accurately
> determine how large the output string would be.

Ah, good catch, I would add...

> So, either
> 
>   *buf++ = '-'
> 
> or
> 
>   if (buf < end)
>     *buf = '-';
>   buf++;

...that we use rather ++buf in such cases, but it doesn't really matter.

> Please don't mix the two.

Andy Shevchenko Oct. 9, 2024, 2:42 p.m. UTC | #5

On Wed, Oct 09, 2024 at 01:27:37PM +0100, Jonathan Cameron wrote:
> On Tue, 8 Oct 2024 19:56:20 +0300
> Andy Shevchenko <[e-mail]> wrote:
> > On Mon, Oct 07, 2024 at 06:16:08PM -0500, Ira Weiny wrote:

...

> > > +static void __init
> > > +struct_range(void)
> > > +{
> > > +	struct range test_range = {
> > > +		.start = 0xc0ffee00ba5eba11,
> > > +		.end = 0xc0ffee00ba5eba11,
> > > +	};  
> > 
> > A side note, can we add something like
> > 
> > #define DEFINE_RANGE(start, end)	\
> > 	(struct range) {		\
> > 		.start = (start),	\
> > 		.end = (end),		\
> > 	}
> > 
> > in range.h and use here and in the similar cases?
> 
> DEFINE_XXXX at least sometimes is used in cases that create the
> variable as well.  E.g. DEFINE_MUTEX()

I understand your point, but since there are many similarities to struct
resource, I would stick with naming convention in ioport.h.

> INIT_RANGE() maybe?

Fan Ni Oct. 9, 2024, 5:33 p.m. UTC | #6

On Mon, Oct 07, 2024 at 06:16:08PM -0500, Ira Weiny wrote:
> The use of struct range in the CXL subsystem is growing.  In particular,
> the addition of Dynamic Capacity devices uses struct range in a number
> of places which are reported in debug and error messages.
> 
> To wit requiring the printing of the start/end fields in each print
> became cumbersome.  Dan Williams mentions in [1] that it might be time
> to have a print specifier for struct range similar to struct resource
> 
> A few alternatives were considered including '%par', '%r', and '%pn'.
> %pra follows that struct range is similar to struct resource (%p[rR])
> but need to be different.  Based on discussions with Petr and Andy
> '%pra' was chosen.[2]
> 
> Andy also suggested to keep the range prints similar to struct resource
> though combined code.  Add hex_range() to handle printing for both
> pointer types.
> 
> To: Petr Mladek <[e-mail]>
> To: Steven Rostedt <[e-mail]>
> To: Andy Shevchenko <[e-mail]>
> To: Rasmus Villemoes <[e-mail]>
> To: Sergey Senozhatsky <[e-mail]>
> To: Jonathan Corbet <[e-mail]> (maintainer:DOCUMENTATION)
> Cc: [e-mail]
> Cc: [e-mail] (open list)
> Link: https://lore.kernel.org/all/[e-mail]/ [1]
> Link: https://lore.kernel.org/all/[e-mail]/ [2]
> Suggested-by: "Dan Williams" <[e-mail]>
> Signed-off-by: Ira Weiny <[e-mail]>
> 
> ---
> Changes:
> [Andy: create new hex_range() and use it in both range/resource]
> [Petr/Andy: Use %pra]
> [Andy: Add test case start > end]
> [Petr: Update documentation]
> [Petr: use 'range -']
> [Petr: fixup printf_spec specifiers]
> [Petr: add lib/test_printf test]
> ---
>  Documentation/core-api/printk-formats.rst | 13 ++++++++
>  lib/test_printf.c                         | 26 +++++++++++++++
>  lib/vsprintf.c                            | 55 +++++++++++++++++++++++++++----
>  3 files changed, 88 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
> index 14e093da3ccd..03b102fc60bb 100644
> --- a/Documentation/core-api/printk-formats.rst
> +++ b/Documentation/core-api/printk-formats.rst
> @@ -231,6 +231,19 @@ width of the CPU data path.
>  
>  Passed by reference.
>  
> +Struct Range
> +------------
> +
> +::
> +
> +	%pra    [range 0x0000000060000000-0x000000006fffffff]
> +	%pra    [range 0x0000000060000000]
> +
> +For printing struct range.  struct range holds an arbitrary range of u64
> +values.  If start is equal to end only 1 value is printed.
> +
> +Passed by reference.
> +
>  DMA address types dma_addr_t
>  ----------------------------
>  
> diff --git a/lib/test_printf.c b/lib/test_printf.c
> index 5afdf5efc627..e3e75b6d10a0 100644
> --- a/lib/test_printf.c
> +++ b/lib/test_printf.c
> @@ -432,6 +432,31 @@ struct_resource(void)
>  	     "%pR", &test_resource);
>  }
>  
> +static void __init
> +struct_range(void)
> +{
> +	struct range test_range = {
> +		.start = 0xc0ffee00ba5eba11,
> +		.end = 0xc0ffee00ba5eba11,
> +	};
> +
> +	test("[range 0xc0ffee00ba5eba11]", "%pra", &test_range);
> +
> +	test_range = (struct range) {
> +		.start = 0xc0ffee,
> +		.end = 0xba5eba11,
> +	};
> +	test("[range 0x0000000000c0ffee-0x00000000ba5eba11]",
> +	     "%pra", &test_range);
> +
> +	test_range = (struct range) {
> +		.start = 0xba5eba11,
> +		.end = 0xc0ffee,
> +	};
> +	test("[range 0x00000000ba5eba11-0x0000000000c0ffee]",
> +	     "%pra", &test_range);
> +}
> +
 ...
>  static noinline_for_stack
>  char *hex_string(char *buf, char *end, u8 *addr, struct printf_spec spec,
>  		 const char *fmt)
> @@ -2277,6 +2314,7 @@ char *rust_fmt_argument(char *buf, char *end, void *ptr);
>   * - 'Bb' as above with module build ID (for use in backtraces)
>   * - 'R' For decoded struct resource, e.g., [mem 0x0-0x1f 64bit pref]
>   * - 'r' For raw struct resource, e.g., [mem 0x0-0x1f flags 0x201]
> + * - 'ra' struct ranges [range 0x00 - 0xff]

Maybe follow the existing examples here, like
'ra" For struct ranges, e.g., ...

fan

>   * - 'b[l]' For a bitmap, the number of bits is determined by the field
>   *       width which must be explicitly specified either as part of the
>   *       format string '%32b[l]' or through '%*b[l]', [l] selects
> @@ -2399,8 +2437,13 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
>  		fallthrough;
>  	case 'B':
>  		return symbol_string(buf, end, ptr, spec, fmt);
> -	case 'R':
>  	case 'r':
> +		switch (fmt[1]) {
> +		case 'a':
> +			return range_string(buf, end, ptr, spec, fmt);
> +		}
> +		fallthrough;
> +	case 'R':
>  		return resource_string(buf, end, ptr, spec, fmt);
>  	case 'h':
>  		return hex_string(buf, end, ptr, spec, fmt);
> 
> -- 
> 2.46.0
>

Bagas Sanjaya Oct. 11, 2024, 2:09 a.m. UTC | #7

On Mon, Oct 07, 2024 at 06:16:08PM -0500, Ira Weiny wrote:
> +Struct Range
> +------------
> +
> +::
> +
> +	%pra    [range 0x0000000060000000-0x000000006fffffff]
> +	%pra    [range 0x0000000060000000]
> +
> +For printing struct range.  struct range holds an arbitrary range of u64
> +values.  If start is equal to end only 1 value is printed.

Do you mean printing only start value in start=equal case?

Confused...

Ira Weiny Oct. 11, 2024, 4:54 p.m. UTC | #8

Rasmus Villemoes wrote:
> Ira Weiny <[e-mail]> writes:
> 
> > ---
> >  Documentation/core-api/printk-formats.rst | 13 ++++++++
> >  lib/test_printf.c                         | 26 +++++++++++++++
> >  lib/vsprintf.c                            | 55 +++++++++++++++++++++++++++----
> >  3 files changed, 88 insertions(+), 6 deletions(-)
> >
> > diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
> > index 14e093da3ccd..03b102fc60bb 100644
> > --- a/Documentation/core-api/printk-formats.rst
> > +++ b/Documentation/core-api/printk-formats.rst
> > @@ -231,6 +231,19 @@ width of the CPU data path.
> >  
> >  Passed by reference.
> >  
> > +Struct Range
> > +------------
> 
> Probably neither of those words should be capitalized.

I was following the format of the header of struct resource

	Struct Resources
	----------------

I can change it but I was trying to be consistent here.

[snip]

> > +static void __init
> > +struct_range(void)
> > +{
> > +	struct range test_range = {
> > +		.start = 0xc0ffee00ba5eba11,
> > +		.end = 0xc0ffee00ba5eba11,
> > +	};
> > +
> > +	test("[range 0xc0ffee00ba5eba11]", "%pra", &test_range);
> > +
> > +	test_range = (struct range) {
> > +		.start = 0xc0ffee,
> > +		.end = 0xba5eba11,
> > +	};
> > +	test("[range 0x0000000000c0ffee-0x00000000ba5eba11]",
> > +	     "%pra", &test_range);
> > +
> > +	test_range = (struct range) {
> > +		.start = 0xba5eba11,
> > +		.end = 0xc0ffee,
> > +	};
> > +	test("[range 0x00000000ba5eba11-0x0000000000c0ffee]",
> > +	     "%pra", &test_range);
> > +}
> > +
> 
> Thanks for including tests!
> 
> Rather than the struct assignments, I think it's easier to read if you
> just do

I'm using Andy's suggestion of DEFINE_RANGE()

> 
>   struct range r;
> 
>   r.start = 0xc0ffee00ba5eba11;
>   r.end   = r.start;
>   ...
> 
>   r.start = 0xc0ffee;
>   r.end   = 0xba5eba11;
>   ...
> 
> which saves two lines per test and for the first one makes it more
> obvious that the start and end values are identical.
> 
> >  static void __init
> >  addr(void)
> >  {
> > @@ -807,6 +832,7 @@ test_pointer(void)
> >  	symbol_ptr();
> >  	kernel_ptr();
> >  	struct_resource();
> > +	struct_range();
> >  	addr();
> >  	escaped_str();
> >  	hex_string();
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > index 09f022ba1c05..f8f5ed8f4d39 100644
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -1039,6 +1039,19 @@ static const struct printf_spec default_dec04_spec = {
> >  	.flags = ZEROPAD,
> >  };
> >  
> > +static noinline_for_stack
> > +char *hex_range(char *buf, char *end, u64 start_val, u64 end_val,
> > +		struct printf_spec spec)
> > +{
> > +	buf = number(buf, end, start_val, spec);
> > +	if (start_val != end_val) {
> > +		if (buf < end)
> > +			*buf++ = '-';
> 
> No. Either all your callers pass a (probably stack-allocated) buffer
> which is guaranteed to be big enough, in which case you don't need the
> "if (buf < end)", or if some callers may "print" directly to the buffer
> passed to vsnprintf(), the buf++ must still be done unconditionally in
> order that vsnprintf(NULL, 0, ...) [used by fx kasprintf] can accurately
> determine how large the output string would be.
> 
> So, either
> 
>   *buf++ = '-'
> 
> or
> 
>   if (buf < end)
>     *buf = '-';
>   buf++;
> 
> Please don't mix the two. 

Ah ok yea fixed building on Andy's comment.

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index a7b5e4618f6a..7aa47f7d9d5b 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1048,7 +1048,8 @@ char *hex_range(char *buf, char *end, u64 start_val, u64 end_val,
                return buf;

        if (buf < end)
-               *buf++ = '-';
+               *buf = '-';
+       ++buf;
        return number(buf, end, end_val, spec);
 }

> 
> 
> 
> > +		buf = number(buf, end, end_val, spec);
> > +	}
> > +	return buf;
> > +}
> > +
> >  static noinline_for_stack
> >  char *resource_string(char *buf, char *end, struct resource *res,
> >  		      struct printf_spec spec, const char *fmt)
> > @@ -1115,11 +1128,7 @@ char *resource_string(char *buf, char *end, struct resource *res,
> >  		p = string_nocheck(p, pend, "size ", str_spec);
> >  		p = number(p, pend, resource_size(res), *specp);
> >  	} else {
> > -		p = number(p, pend, res->start, *specp);
> > -		if (res->start != res->end) {
> > -			*p++ = '-';
> > -			p = number(p, pend, res->end, *specp);
> > -		}
> > +		p = hex_range(p, pend, res->start, res->end, *specp);
> >  	}
> >  	if (decode) {
> >  		if (res->flags & IORESOURCE_MEM_64)
> > @@ -1140,6 +1149,34 @@ char *resource_string(char *buf, char *end, struct resource *res,
> >  	return string_nocheck(buf, end, sym, spec);
> >  }
> >  
> > +static noinline_for_stack
> > +char *range_string(char *buf, char *end, const struct range *range,
> > +		   struct printf_spec spec, const char *fmt)
> > +{
> > +#define RANGE_DECODED_BUF_SIZE		((2 * sizeof(struct range)) + 4)
> > +#define RANGE_PRINT_BUF_SIZE		sizeof("[range -]")
> > +	char sym[RANGE_DECODED_BUF_SIZE + RANGE_PRINT_BUF_SIZE];
> 
> I don't think these names or the split in two constants helps
> convincing that's the right amount. I have to think quite a bit to see
> that 2*sizeof is because struct range has two u64 and we're printing in
> hex so four-bits-per-char and probably the +4 are for two time "0x".

Yea.

> 
> Why not just size the buffer directly using an "example" string?
> 
>   char sym[sizeof("[range 0x0123456789abcdef-0x0123456789abcdef]")]

Ok that is simpler.

> 
> > +	char *p = sym, *pend = sym + sizeof(sym);
> > +
> > +	struct printf_spec range_spec = {
> > +		.field_width = 2 + 2 * sizeof(range->start), /* 0x + 2 * 8 */
> > +		.flags = SPECIAL | SMALL | ZEROPAD,
> > +		.base = 16,
> > +		.precision = -1,
> > +	};
> > +
> > +	if (check_pointer(&buf, end, range, spec))
> > +		return buf;
> > +
> > +	*p++ = '[';
> > +	p = string_nocheck(p, pend, "range ", default_str_spec);
> 
> We really should have mempcpy or stpcpy. I don't see the point of using
> string_nocheck here, or not including the [ in the string copy (however
> it's done). But yeah, without stpcpy() that's a bit awkward. 

Added '[' to the string.  The prevalent use of string_nocheck() seems
reasonable to me but it is pretty heavyweight for this case.

Ira

Ira Weiny Oct. 14, 2024, 12:08 a.m. UTC | #9

Andy Shevchenko wrote:
> On Wed, Oct 09, 2024 at 03:30:14PM +0200, Rasmus Villemoes wrote:
> 
> ...
> 
> > Rather than the struct assignments, I think it's easier to read if you
> > just do
> > 
> >   struct range r;
> > 
> >   r.start = 0xc0ffee00ba5eba11;
> >   r.end   = r.start;
> >   ...
> > 
> >   r.start = 0xc0ffee;
> >   r.end   = 0xba5eba11;
> >   ...
> > 
> > which saves two lines per test and for the first one makes it more
> > obvious that the start and end values are identical.
> 
> With DEFINE_RANGE() it will save even more lines!

Yea I've added DEFINE_RANGE().  Thanks.

> 
> ..
> 
> > > +		if (buf < end)
> > > +			*buf++ = '-';
> > 
> > No. Either all your callers pass a (probably stack-allocated) buffer
> > which is guaranteed to be big enough, in which case you don't need the
> > "if (buf < end)", or if some callers may "print" directly to the buffer
> > passed to vsnprintf(), the buf++ must still be done unconditionally in
> > order that vsnprintf(NULL, 0, ...) [used by fx kasprintf] can accurately
> > determine how large the output string would be.
> 
> Ah, good catch, I would add...
> 
> > So, either
> > 
> >   *buf++ = '-'
> > 
> > or
> > 
> >   if (buf < end)
> >     *buf = '-';
> >   buf++;
> 
> ...that we use rather ++buf in such cases, but it doesn't really matter.

Done.
Ira

Ira Weiny Oct. 17, 2024, 8:57 p.m. UTC | #10

Bagas Sanjaya wrote:
> On Mon, Oct 07, 2024 at 06:16:08PM -0500, Ira Weiny wrote:
> > +Struct Range
> > +------------
> > +
> > +::
> > +
> > +	%pra    [range 0x0000000060000000-0x000000006fffffff]
> > +	%pra    [range 0x0000000060000000]
> > +
> > +For printing struct range.  struct range holds an arbitrary range of u64
> > +values.  If start is equal to end only 1 value is printed.
> 
> Do you mean printing only start value in start=equal case?

Yes I'll change the verbiage.

Ira

diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
index 03b102fc60bb..e1ebf0376154 100644
--- a/Documentation/core-api/printk-formats.rst
+++ b/Documentation/core-api/printk-formats.rst
@@ -240,7 +240,7 @@ Struct Range
        %pra    [range 0x0000000060000000]

 For printing struct range.  struct range holds an arbitrary range of u64
-values.  If start is equal to end only 1 value is printed.
+values.  If start is equal to end only print the start value.

 Passed by reference.

Bagas Sanjaya Oct. 25, 2024, 12:42 p.m. UTC | #11

On Thu, Oct 17, 2024 at 03:57:50PM -0500, Ira Weiny wrote:
> Bagas Sanjaya wrote:
> > On Mon, Oct 07, 2024 at 06:16:08PM -0500, Ira Weiny wrote:
> > > +Struct Range
> > > +------------
> > > +
> > > +::
> > > +
> > > +	%pra    [range 0x0000000060000000-0x000000006fffffff]
> > > +	%pra    [range 0x0000000060000000]
> > > +
> > > +For printing struct range.  struct range holds an arbitrary range of u64
> > > +values.  If start is equal to end only 1 value is printed.
> > 
> > Do you mean printing only start value in start=equal case?
> 
> Yes I'll change the verbiage.
> 
> Ira
> 
> diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
> index 03b102fc60bb..e1ebf0376154 100644
> --- a/Documentation/core-api/printk-formats.rst
> +++ b/Documentation/core-api/printk-formats.rst
> @@ -240,7 +240,7 @@ Struct Range
>         %pra    [range 0x0000000060000000]
> 
>  For printing struct range.  struct range holds an arbitrary range of u64
> -values.  If start is equal to end only 1 value is printed.
> +values.  If start is equal to end only print the start value.
> 
>  Passed by reference.

That's nice, thanks!

Patch

diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
index 14e093da3ccd..03b102fc60bb 100644
--- a/Documentation/core-api/printk-formats.rst
+++ b/Documentation/core-api/printk-formats.rst
@@ -231,6 +231,19 @@  width of the CPU data path.
 
 Passed by reference.
 
+Struct Range
+------------
+
+::
+
+	%pra    [range 0x0000000060000000-0x000000006fffffff]
+	%pra    [range 0x0000000060000000]
+
+For printing struct range.  struct range holds an arbitrary range of u64
+values.  If start is equal to end only 1 value is printed.
+
+Passed by reference.
+
 DMA address types dma_addr_t
 ----------------------------
 
diff --git a/lib/test_printf.c b/lib/test_printf.c
index 5afdf5efc627..e3e75b6d10a0 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -432,6 +432,31 @@  struct_resource(void)
 	     "%pR", &test_resource);
 }
 
+static void __init
+struct_range(void)
+{
+	struct range test_range = {
+		.start = 0xc0ffee00ba5eba11,
+		.end = 0xc0ffee00ba5eba11,
+	};
+
+	test("[range 0xc0ffee00ba5eba11]", "%pra", &test_range);
+
+	test_range = (struct range) {
+		.start = 0xc0ffee,
+		.end = 0xba5eba11,
+	};
+	test("[range 0x0000000000c0ffee-0x00000000ba5eba11]",
+	     "%pra", &test_range);
+
+	test_range = (struct range) {
+		.start = 0xba5eba11,
+		.end = 0xc0ffee,
+	};
+	test("[range 0x00000000ba5eba11-0x0000000000c0ffee]",
+	     "%pra", &test_range);
+}
+
 static void __init
 addr(void)
 {
@@ -807,6 +832,7 @@  test_pointer(void)
 	symbol_ptr();
 	kernel_ptr();
 	struct_resource();
+	struct_range();
 	addr();
 	escaped_str();
 	hex_string();
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 09f022ba1c05..f8f5ed8f4d39 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1039,6 +1039,19 @@  static const struct printf_spec default_dec04_spec = {
 	.flags = ZEROPAD,
 };
 
+static noinline_for_stack
+char *hex_range(char *buf, char *end, u64 start_val, u64 end_val,
+		struct printf_spec spec)
+{
+	buf = number(buf, end, start_val, spec);
+	if (start_val != end_val) {
+		if (buf < end)
+			*buf++ = '-';
+		buf = number(buf, end, end_val, spec);
+	}
+	return buf;
+}
+
 static noinline_for_stack
 char *resource_string(char *buf, char *end, struct resource *res,
 		      struct printf_spec spec, const char *fmt)
@@ -1115,11 +1128,7 @@  char *resource_string(char *buf, char *end, struct resource *res,
 		p = string_nocheck(p, pend, "size ", str_spec);
 		p = number(p, pend, resource_size(res), *specp);
 	} else {
-		p = number(p, pend, res->start, *specp);
-		if (res->start != res->end) {
-			*p++ = '-';
-			p = number(p, pend, res->end, *specp);
-		}
+		p = hex_range(p, pend, res->start, res->end, *specp);
 	}
 	if (decode) {
 		if (res->flags & IORESOURCE_MEM_64)
@@ -1140,6 +1149,34 @@  char *resource_string(char *buf, char *end, struct resource *res,
 	return string_nocheck(buf, end, sym, spec);
 }
 
+static noinline_for_stack
+char *range_string(char *buf, char *end, const struct range *range,
+		   struct printf_spec spec, const char *fmt)
+{
+#define RANGE_DECODED_BUF_SIZE		((2 * sizeof(struct range)) + 4)
+#define RANGE_PRINT_BUF_SIZE		sizeof("[range -]")
+	char sym[RANGE_DECODED_BUF_SIZE + RANGE_PRINT_BUF_SIZE];
+	char *p = sym, *pend = sym + sizeof(sym);
+
+	struct printf_spec range_spec = {
+		.field_width = 2 + 2 * sizeof(range->start), /* 0x + 2 * 8 */
+		.flags = SPECIAL | SMALL | ZEROPAD,
+		.base = 16,
+		.precision = -1,
+	};
+
+	if (check_pointer(&buf, end, range, spec))
+		return buf;
+
+	*p++ = '[';
+	p = string_nocheck(p, pend, "range ", default_str_spec);
+	p = hex_range(p, pend, range->start, range->end, range_spec);
+	*p++ = ']';
+	*p = '\0';
+
+	return string_nocheck(buf, end, sym, spec);
+}
+
 static noinline_for_stack
 char *hex_string(char *buf, char *end, u8 *addr, struct printf_spec spec,
 		 const char *fmt)
@@ -2277,6 +2314,7 @@  char *rust_fmt_argument(char *buf, char *end, void *ptr);
  * - 'Bb' as above with module build ID (for use in backtraces)
  * - 'R' For decoded struct resource, e.g., [mem 0x0-0x1f 64bit pref]
  * - 'r' For raw struct resource, e.g., [mem 0x0-0x1f flags 0x201]
+ * - 'ra' struct ranges [range 0x00 - 0xff]
  * - 'b[l]' For a bitmap, the number of bits is determined by the field
  *       width which must be explicitly specified either as part of the
  *       format string '%32b[l]' or through '%*b[l]', [l] selects
@@ -2399,8 +2437,13 @@  char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 		fallthrough;
 	case 'B':
 		return symbol_string(buf, end, ptr, spec, fmt);
-	case 'R':
 	case 'r':
+		switch (fmt[1]) {
+		case 'a':
+			return range_string(buf, end, ptr, spec, fmt);
+		}
+		fallthrough;
+	case 'R':
 		return resource_string(buf, end, ptr, spec, fmt);
 	case 'h':
 		return hex_string(buf, end, ptr, spec, fmt);