| Message ID | 20240816-dcd-type2-upstream-v3-2-7c9b96cba6d7@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | DCD: Add support for Dynamic Capacity Devices (DCD) | expand |
On Fri 2024-08-16 09:44:10, 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 '%pn' for 'print raNge' but > %par follows that struct range is most often used to store a range of > physical addresses. So use '%par' for 'print address range'. > > --- a/Documentation/core-api/printk-formats.rst > +++ b/Documentation/core-api/printk-formats.rst > @@ -231,6 +231,20 @@ width of the CPU data path. > > Passed by reference. > > +Struct Range > +------------ > + > +:: > + > + %par [range 0x60000000-0x6fffffff] or It seems that it is always 64-bit. It prints: struct range { u64 start; u64 end; }; > + [range 0x0000000060000000-0x000000006fffffff] > + > +For printing struct range. A variation of printing a physical address is to > +print the value of struct range which are often used to hold a physical address > +range. > + > +Passed by reference. > + > DMA address types dma_addr_t > ---------------------------- > > diff --git a/lib/vsprintf.c b/lib/vsprintf.c > index 2d71b1115916..c132178fac07 100644 > --- a/lib/vsprintf.c > +++ b/lib/vsprintf.c > @@ -1140,6 +1140,39 @@ 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_PRINTK_SIZE 16 > +#define RANGE_DECODED_BUF_SIZE ((2 * sizeof(struct range)) + 4) > +#define RANGE_PRINT_BUF_SIZE sizeof("[range - ]") I think that it should be "[range -]" > + char sym[RANGE_DECODED_BUF_SIZE + RANGE_PRINT_BUF_SIZE]; > + char *p = sym, *pend = sym + sizeof(sym); > + > + static const struct printf_spec str_spec = { > + .field_width = -1, > + .precision = 10, > + .flags = LEFT, > + }; Is this really needed? What about using "default_str_spec" instead? > + static const struct printf_spec range_spec = { > + .base = 16, > + .field_width = RANGE_PRINTK_SIZE, > + .precision = -1, > + .flags = SPECIAL | SMALL | ZEROPAD, > + }; > + > + *p++ = '['; > + p = string_nocheck(p, pend, "range ", str_spec); > + p = number(p, pend, range->start, range_spec); > + *p++ = '-'; > + p = number(p, pend, 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) Also add a selftest into lib/test_printf.c, please. Best Regards, Petr
Petr Mladek wrote: > On Fri 2024-08-16 09:44:10, 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 '%pn' for 'print raNge' but > > %par follows that struct range is most often used to store a range of > > physical addresses. So use '%par' for 'print address range'. > > > > --- a/Documentation/core-api/printk-formats.rst > > +++ b/Documentation/core-api/printk-formats.rst > > @@ -231,6 +231,20 @@ width of the CPU data path. > > > > Passed by reference. > > > > +Struct Range > > +------------ > > + > > +:: > > + > > + %par [range 0x60000000-0x6fffffff] or > > It seems that it is always 64-bit. It prints: > > struct range { > u64 start; > u64 end; > }; Indeed. Thanks I should not have just copied/pasted. > > > + [range 0x0000000060000000-0x000000006fffffff] > > + > > +For printing struct range. A variation of printing a physical address is to > > +print the value of struct range which are often used to hold a physical address > > +range. > > + > > +Passed by reference. > > + > > DMA address types dma_addr_t > > ---------------------------- > > > > diff --git a/lib/vsprintf.c b/lib/vsprintf.c > > index 2d71b1115916..c132178fac07 100644 > > --- a/lib/vsprintf.c > > +++ b/lib/vsprintf.c > > @@ -1140,6 +1140,39 @@ 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_PRINTK_SIZE 16 > > +#define RANGE_DECODED_BUF_SIZE ((2 * sizeof(struct range)) + 4) > > +#define RANGE_PRINT_BUF_SIZE sizeof("[range - ]") > > I think that it should be "[range -]" Sounds good. > > > + char sym[RANGE_DECODED_BUF_SIZE + RANGE_PRINT_BUF_SIZE]; > > + char *p = sym, *pend = sym + sizeof(sym); > > + > > + static const struct printf_spec str_spec = { > > + .field_width = -1, > > + .precision = 10, > > + .flags = LEFT, > > + }; > > Is this really needed? What about using "default_str_spec" instead? Because I got confused and was coping from resource_string(). Deleted now... > > > + static const struct printf_spec range_spec = { > > + .base = 16, > > + .field_width = RANGE_PRINTK_SIZE, However, my testing indicates this needs to be. .field_width = 18, /* 2 (0x) + 2 * 8 (bytes) */ ... to properly zero pad the value. Does that make sense? > > + .precision = -1, > > + .flags = SPECIAL | SMALL | ZEROPAD, > > + }; > > + > > + *p++ = '['; > > + p = string_nocheck(p, pend, "range ", str_spec); > > + p = number(p, pend, range->start, range_spec); > > + *p++ = '-'; > > + p = number(p, pend, 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) > > Also add a selftest into lib/test_printf.c, please. Yes of course... Makes testing easier too. Thanks, Ira > > Best Regards, > Petr
On Thu, Aug 22, 2024 at 12:53:32PM -0500, Ira Weiny wrote: > Petr Mladek wrote: > > On Fri 2024-08-16 09:44:10, Ira Weiny wrote: ... > > > + %par [range 0x60000000-0x6fffffff] or > > > > It seems that it is always 64-bit. It prints: > > > > struct range { > > u64 start; > > u64 end; > > }; > > Indeed. Thanks I should not have just copied/pasted. With that said, I'm not sure the %pa is a good placeholder for this ('a' stands to "address" AFAIU). Perhaps this should go somewhere under %pr/%pR? > > > + [range 0x0000000060000000-0x000000006fffffff] > > > + > > > +For printing struct range. A variation of printing a physical address is to > > > +print the value of struct range which are often used to hold a physical address > > > +range. > > > + > > > +Passed by reference. ... > > Is this really needed? What about using "default_str_spec" instead? > > Because I got confused and was coping from resource_string(). > > Deleted now... > > > > + .field_width = RANGE_PRINTK_SIZE, > > However, my testing indicates this needs to be. > > .field_width = 18, /* 2 (0x) + 2 * 8 (bytes) */ > > ... to properly zero pad the value. Does that make sense? Looking at this, moving under %pr/R should deduplicate the code, no? I.o.w. better to use existing code for them to print struct range, no?
On Thu 2024-08-22 12:53:32, Ira Weiny wrote: > Petr Mladek wrote: > > On Fri 2024-08-16 09:44:10, 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 '%pn' for 'print raNge' but > > > %par follows that struct range is most often used to store a range of > > > physical addresses. So use '%par' for 'print address range'. > > > > > > diff --git a/lib/vsprintf.c b/lib/vsprintf.c > > > index 2d71b1115916..c132178fac07 100644 > > > --- a/lib/vsprintf.c > > > +++ b/lib/vsprintf.c > > > @@ -1140,6 +1140,39 @@ 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_PRINTK_SIZE 16 > > > +#define RANGE_DECODED_BUF_SIZE ((2 * sizeof(struct range)) + 4) > > > +#define RANGE_PRINT_BUF_SIZE sizeof("[range - ]") [...] > > > + static const struct printf_spec range_spec = { > > > + .base = 16, > > > + .field_width = RANGE_PRINTK_SIZE, > > However, my testing indicates this needs to be. > > .field_width = 18, /* 2 (0x) + 2 * 8 (bytes) */ Makes sense. Great catch! > ... to properly zero pad the value. Does that make sense? > > > > + .precision = -1, > > > + .flags = SPECIAL | SMALL | ZEROPAD, > > > + }; > > > + > > > + *p++ = '['; > > > + p = string_nocheck(p, pend, "range ", str_spec); > > > + p = number(p, pend, range->start, range_spec); > > > + *p++ = '-'; > > > + p = number(p, pend, range->end, range_spec); > > > + *p++ = ']'; > > > + *p = '\0'; Best Regards, Petr
On Thu 2024-08-22 21:10:25, Andy Shevchenko wrote: > On Thu, Aug 22, 2024 at 12:53:32PM -0500, Ira Weiny wrote: > > Petr Mladek wrote: > > > On Fri 2024-08-16 09:44:10, Ira Weiny wrote: > > ... > > > > > + %par [range 0x60000000-0x6fffffff] or > > > > > > It seems that it is always 64-bit. It prints: > > > > > > struct range { > > > u64 start; > > > u64 end; > > > }; > > > > Indeed. Thanks I should not have just copied/pasted. > > With that said, I'm not sure the %pa is a good placeholder for this ('a' stands > to "address" AFAIU). Perhaps this should go somewhere under %pr/%pR? The r/R in %pr/%pR actually stands for "resource". But "%ra" really looks like a better choice than "%par". Both "resource" and "range" starts with 'r'. Also the struct resource is printed as a range of values. > > > > + [range 0x0000000060000000-0x000000006fffffff] > > > > + > > > > +For printing struct range. A variation of printing a physical address is to > > > > +print the value of struct range which are often used to hold a physical address > > > > +range. > > > > + > > > > +Passed by reference. Best Regards, Petr
On Mon, Aug 26, 2024 at 03:17:26PM +0200, Petr Mladek wrote: > On Thu 2024-08-22 12:53:32, Ira Weiny wrote: > > Petr Mladek wrote: > > > On Fri 2024-08-16 09:44:10, Ira Weiny wrote: [...] > > > > + static const struct printf_spec range_spec = { > > > > + .base = 16, > > > > + .field_width = RANGE_PRINTK_SIZE, > > > > However, my testing indicates this needs to be. > > > > .field_width = 18, /* 2 (0x) + 2 * 8 (bytes) */ > > Makes sense. Great catch! Which effectively means usage of special_hex_number(). But again, consider to unite this with %pR/r implementation(s). > > ... to properly zero pad the value. Does that make sense? > > > > > > + .precision = -1, > > > > + .flags = SPECIAL | SMALL | ZEROPAD, > > > > + };
On Mon, Aug 26, 2024 at 03:23:50PM +0200, Petr Mladek wrote: > On Thu 2024-08-22 21:10:25, Andy Shevchenko wrote: > > On Thu, Aug 22, 2024 at 12:53:32PM -0500, Ira Weiny wrote: > > > Petr Mladek wrote: > > > > On Fri 2024-08-16 09:44:10, Ira Weiny wrote: ... > > > > > + %par [range 0x60000000-0x6fffffff] or > > > > > > > > It seems that it is always 64-bit. It prints: > > > > > > > > struct range { > > > > u64 start; > > > > u64 end; > > > > }; > > > > > > Indeed. Thanks I should not have just copied/pasted. > > > > With that said, I'm not sure the %pa is a good placeholder for this ('a' stands > > to "address" AFAIU). Perhaps this should go somewhere under %pr/%pR? > > The r/R in %pr/%pR actually stands for "resource". > > But "%ra" really looks like a better choice than "%par". Both > "resource" and "range" starts with 'r'. Also the struct resource > is printed as a range of values. Fine with me as long as it: 1) doesn't collide with %pa namespace 2) tries to deduplicate existing code as much as possible. > > > > > + [range 0x0000000060000000-0x000000006fffffff] > > > > > + > > > > > +For printing struct range. A variation of printing a physical address is to > > > > > +print the value of struct range which are often used to hold a physical address > > > > > +range. > > > > > + > > > > > +Passed by reference.
Andy Shevchenko wrote: > On Mon, Aug 26, 2024 at 03:23:50PM +0200, Petr Mladek wrote: > > On Thu 2024-08-22 21:10:25, Andy Shevchenko wrote: > > > On Thu, Aug 22, 2024 at 12:53:32PM -0500, Ira Weiny wrote: > > > > Petr Mladek wrote: > > > > > On Fri 2024-08-16 09:44:10, Ira Weiny wrote: > > ... > > > > > > > + %par [range 0x60000000-0x6fffffff] or > > > > > > > > > > It seems that it is always 64-bit. It prints: > > > > > > > > > > struct range { > > > > > u64 start; > > > > > u64 end; > > > > > }; > > > > > > > > Indeed. Thanks I should not have just copied/pasted. > > > > > > With that said, I'm not sure the %pa is a good placeholder for this ('a' stands > > > to "address" AFAIU). Perhaps this should go somewhere under %pr/%pR? I'm speaking a bit for Dan here but also the logical way I thought of things. 1) %p does not dictate anything about the format of the data. Rather indicates that what is passed is a pointer. Because we are passing a pointer to a range struct %pXX makes sense. 2) %pa indicates what follows is 'address'. This was a bit of creative license because, as I said in the commit message most of the time struct range contains an address range. So for this narrow use case it also makes sense. 3) %par r for range. %p[rR] is taken. %pra confuses things IMO. > > > > The r/R in %pr/%pR actually stands for "resource". > > > > But "%ra" really looks like a better choice than "%par". Both > > "resource" and "range" starts with 'r'. Also the struct resource > > is printed as a range of values. %r could be used I think. But this breaks with the convention of passing a pointer and how to interpret it. The other idea I had, mentioned in the commit message was %pn. Meaning passed by pointer 'raNge'. I think that follows better than %r. That would be another break from C99. But we don't have to follow that. > > Fine with me as long as it: > 1) doesn't collide with %pa namespace > 2) tries to deduplicate existing code as much as possible. Andy, I'm not quite following how you expect to share the code between resource_string() and range_string()? There is very little duplicated code. In fact with Petr's suggestions and some more work range_string() is quite simple: +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); + + *p++ = '['; + p = string_nocheck(p, pend, "range ", default_str_spec); + p = special_hex_number(p, pend, range->start, sizeof(range->start)); + *p++ = '-'; + p = special_hex_number(p, pend, range->end, sizeof(range->end)); + *p++ = ']'; + *p = '\0'; + + return string_nocheck(buf, end, sym, spec); +} Also this is the bulk of the patch except for documentation and the new testing code. [new patch below] Am I missing your point somehow? I considered cramming a struct range into a struct resource to let resource_string() process the data. But that would involve creating a new IORESOURCE_* flag (not ideal) and also does not allow for the larger u64 data in struct range should this be a 32 bit physical address config. Most importantly that would not be much less code AFAICT. Ira [snip] <new patch> commit a5f0305d319eac7c6e480851378695f8bd42a3d0 Author: Ira Weiny <ira.weiny@intel.com> Date: Fri Jun 28 16:47:06 2024 -0500 printk: Add print format (%par) for struct range 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 '%pn' for 'print raNge' but %par follows that struct range is most often used to store a range of physical addresses. So use '%par' for 'print address range'. To: Petr Mladek <pmladek@suse.com> (maintainer:VSPRINTF) To: Steven Rostedt <rostedt@goodmis.org> (maintainer:VSPRINTF) To: Jonathan Corbet <corbet@lwn.net> (maintainer:DOCUMENTATION) Cc: linux-doc@vger.kernel.org (open list:DOCUMENTATION) Cc: linux-kernel@vger.kernel.org (open list) Link: https://lore.kernel.org/all/663922b475e50_d54d72945b@dwillia2-xfh.jf.intel.com.notmuch/ [1] Suggested-by: "Dan Williams" <dan.j.williams@intel.com> Signed-off-by: Ira Weiny <ira.weiny@intel.com> --- Changes: [iweiny: use special_hex_number()] [Petr: Update documentation] [Petr: use 'range -'] [Petr: fixup printf_spec specifiers] [Petr: add lib/test_printf test] diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst index 4451ef501936..1bdfcd40c81e 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 +------------ + +:: + + %par [range 0x0000000060000000-0x000000006fffffff] + +For printing struct range. A variation of printing a physical address is to +print the value of struct range which are often used to hold a physical address +range. + +Passed by reference. + DMA address types dma_addr_t ---------------------------- diff --git a/lib/test_printf.c b/lib/test_printf.c index 965cb6f28527..2f20b0c30024 100644 --- a/lib/test_printf.c +++ b/lib/test_printf.c @@ -388,6 +388,25 @@ struct_resource(void) { } +static void __init +struct_range(void) +{ + struct range test_range = { + .start = 0xc0ffee00ba5eba11, + .end = 0xc0ffee00ba5eba11, + }; + + test("[range 0xc0ffee00ba5eba11-0xc0ffee00ba5eba11]", + "%par", &test_range); + + test_range = (struct range) { + .start = 0xc0ffee, + .end = 0xba5eba11, + }; + test("[range 0x0000000000c0ffee-0x00000000ba5eba11]", + "%par", &test_range); +} + static void __init addr(void) { @@ -789,6 +808,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 2d71b1115916..a754eefef252 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -1140,6 +1140,26 @@ 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); + + *p++ = '['; + p = string_nocheck(p, pend, "range ", default_str_spec); + p = special_hex_number(p, pend, range->start, sizeof(range->start)); + *p++ = '-'; + p = special_hex_number(p, pend, range->end, sizeof(range->end)); + *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) @@ -1802,6 +1822,8 @@ char *address_val(char *buf, char *end, const void *addr, return buf; switch (fmt[1]) { + case 'r': + return range_string(buf, end, addr, spec, fmt); case 'd': num = *(const dma_addr_t *)addr; size = sizeof(dma_addr_t); @@ -2364,6 +2386,8 @@ char *rust_fmt_argument(char *buf, char *end, void *ptr); * to use print_hex_dump() for the larger input. * - 'a[pd]' For address types [p] phys_addr_t, [d] dma_addr_t and derivatives * (default assumed to be phys_addr_t, passed by reference) + * - 'ar' For decoded struct ranges (a variation of physical address which are + * most often stored in struct ranges. * - 'd[234]' For a dentry name (optionally 2-4 last components) * - 'D[234]' Same as 'd' but for a struct file * - 'g' For block_device name (gendisk + partition number)
On Mon 2024-08-26 16:17:52, Ira Weiny wrote: > Andy Shevchenko wrote: > > On Mon, Aug 26, 2024 at 03:23:50PM +0200, Petr Mladek wrote: > > > On Thu 2024-08-22 21:10:25, Andy Shevchenko wrote: > > > > On Thu, Aug 22, 2024 at 12:53:32PM -0500, Ira Weiny wrote: > > > > > Petr Mladek wrote: > > > > > > On Fri 2024-08-16 09:44:10, Ira Weiny wrote: > > > > ... > > > > > > > > > + %par [range 0x60000000-0x6fffffff] or > > > > > > > > > > > > It seems that it is always 64-bit. It prints: > > > > > > > > > > > > struct range { > > > > > > u64 start; > > > > > > u64 end; > > > > > > }; > > > > > > > > > > Indeed. Thanks I should not have just copied/pasted. > > > > > > > > With that said, I'm not sure the %pa is a good placeholder for this ('a' stands > > > > to "address" AFAIU). Perhaps this should go somewhere under %pr/%pR? > > I'm speaking a bit for Dan here but also the logical way I thought of > things. > > 1) %p does not dictate anything about the format of the data. Rather > indicates that what is passed is a pointer. Because we are passing a > pointer to a range struct %pXX makes sense. > 2) %pa indicates what follows is 'address'. This was a bit of creative > license because, as I said in the commit message most of the time > struct range contains an address range. So for this narrow use case it > also makes sense. > 3) %par r for range. Yes. I got it. Well, is struct range really used for addresses? It rather looks like a range of any 64-bit values. > %p[rR] is taken. %pra confuses things IMO. Another variants might be %pr64 or %prange. IMHO, there is no good solution. We are trying to find the least bad one. The meaning should be as obvious and as least confusing as possible. Honestly, I do not have a strong opinion. I kind of like %prange ;-) But I could live with all other variants, except for %pn mentioned below. > > > The r/R in %pr/%pR actually stands for "resource". > > > > > > But "%ra" really looks like a better choice than "%par". Both > > > "resource" and "range" starts with 'r'. Also the struct resource > > > is printed as a range of values. > > %r could be used I think. But this breaks with the convention of passing a > pointer and how to interpret it. How exactly does it break the convention, please? Do you passing a pointer to struct range instead of a pointer to struct resource? It should not be a big problem as long as the vsprintf() code is able to guess the right pointer type from the %pXX modifier. > The other idea I had, mentioned in the commit > message was %pn. Meaning passed by pointer 'raNge'. This looks like the worst variant to me. > > Fine with me as long as it: > > 1) doesn't collide with %pa namespace > > 2) tries to deduplicate existing code as much as possible. > > Andy, I'm not quite following how you expect to share the code between > resource_string() and range_string()? > > There is very little duplicated code. In fact with Petr's suggestions and some > more work range_string() is quite simple: > > +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); > + > + *p++ = '['; > + p = string_nocheck(p, pend, "range ", default_str_spec); > + p = special_hex_number(p, pend, range->start, sizeof(range->start)); > + *p++ = '-'; > + p = special_hex_number(p, pend, range->end, sizeof(range->end)); > + *p++ = ']'; > + *p = '\0'; > + > + return string_nocheck(buf, end, sym, spec); > +} I agree that there is not much duplicated code in the end. > Also this is the bulk of the patch except for documentation and the new > testing code. [new patch below] > > Am I missing your point somehow? I considered cramming a struct range into a > struct resource to let resource_string() process the data. But that would > involve creating a new IORESOURCE_* flag (not ideal) and also does not allow > for the larger u64 data in struct range should this be a 32 bit physical > address config. This would be nasty. I believe that this is not what Andy meant. Best Regards, Petr PS: I have vacation until the end of the week, so my next eventual reaction would be delayed.
On Mon, Aug 26, 2024 at 04:17:52PM -0500, Ira Weiny wrote: > Andy Shevchenko wrote: > > On Mon, Aug 26, 2024 at 03:23:50PM +0200, Petr Mladek wrote: > > > On Thu 2024-08-22 21:10:25, Andy Shevchenko wrote: > > > > On Thu, Aug 22, 2024 at 12:53:32PM -0500, Ira Weiny wrote: > > > > > Petr Mladek wrote: > > > > > > On Fri 2024-08-16 09:44:10, Ira Weiny wrote: ... > > > > > > > + %par [range 0x60000000-0x6fffffff] or > > > > > > > > > > > > It seems that it is always 64-bit. It prints: > > > > > > > > > > > > struct range { > > > > > > u64 start; > > > > > > u64 end; > > > > > > }; > > > > > > > > > > Indeed. Thanks I should not have just copied/pasted. > > > > > > > > With that said, I'm not sure the %pa is a good placeholder for this ('a' stands > > > > to "address" AFAIU). Perhaps this should go somewhere under %pr/%pR? > > I'm speaking a bit for Dan here but also the logical way I thought of > things. > > 1) %p does not dictate anything about the format of the data. Rather > indicates that what is passed is a pointer. Because we are passing a > pointer to a range struct %pXX makes sense. There is no objection to that. > 2) %pa indicates what follows is 'address'. This was a bit of creative > license because, as I said in the commit message most of the time > struct range contains an address range. So for this narrow use case it > also makes sense. As in the discussion it was pointed out that struct range is always 64-bit, limiting it to the "address" is a wrong assumption as we are talking generic printing routine here. We don't know what users will be in the future on 32-bit platforms, or what data (semantically) is being held by this structure. > 3) %par r for range. I understand, but again struct range != address. > %p[rR] is taken. > %pra confuses things IMO. It doesn't confuse me. :-) But I believe Petr also has a rationale behind this proposal as he described earlier. > > > The r/R in %pr/%pR actually stands for "resource". > > > > > > But "%ra" really looks like a better choice than "%par". Both > > > "resource" and "range" starts with 'r'. Also the struct resource > > > is printed as a range of values. > > %r could be used I think. But this breaks with the convention of passing a > pointer and how to interpret it. The other idea I had, mentioned in the commit > message was %pn. Meaning passed by pointer 'raNge'. No, we can't use %r or anything else that is documented for the standard printf() format specifiers, otherwise you will get a compiler warning and basically it means no go. > I think that follows better than %r. That would be another break from C99. > But we don't have to follow that. > > > Fine with me as long as it: > > 1) doesn't collide with %pa namespace > > 2) tries to deduplicate existing code as much as possible. > > Andy, I'm not quite following how you expect to share the code between > resource_string() and range_string()? > > There is very little duplicated code. In fact with Petr's suggestions and some > more work range_string() is quite simple: > > +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); Missing check for pointer, but it's not that I wanted to tell. > + *p++ = '['; > + p = string_nocheck(p, pend, "range ", default_str_spec); Hmm... %pr uses str_spec, what the difference can be here? > + p = special_hex_number(p, pend, range->start, sizeof(range->start)); > + *p++ = '-'; > + p = special_hex_number(p, pend, range->end, sizeof(range->end)); This is basically the copy of %pr implementation. p = number(p, pend, res->start, *specp); if (res->start != res->end) { *p++ = '-'; p = number(p, pend, res->end, *specp); } Would it be possible to unify? I think so, but it requires a bit of thinking. That's why testing is very important in this kind of generic code. > + *p++ = ']'; > + *p = '\0'; > + > + return string_nocheck(buf, end, sym, spec); > +} > > Also this is the bulk of the patch except for documentation and the new > testing code. [new patch below] > > Am I missing your point somehow? See above. > I considered cramming a struct range into a > struct resource to let resource_string() process the data. But that would > involve creating a new IORESOURCE_* flag (not ideal) and also does not allow > for the larger u64 data in struct range should this be a 32 bit physical > address config. No, that's not what I was expecting. > Most importantly that would not be much less code AFAICT. ... > + %par [range 0x0000000060000000-0x000000006fffffff] I still think this is not okay to use %pa namespace. ... > +static void __init > +struct_range(void) > +{ > + struct range test_range = { > + .start = 0xc0ffee00ba5eba11, > + .end = 0xc0ffee00ba5eba11, > + }; > + > + test("[range 0xc0ffee00ba5eba11-0xc0ffee00ba5eba11]", > + "%par", &test_range); > + > + test_range = (struct range) { > + .start = 0xc0ffee, > + .end = 0xba5eba11, > + }; > + test("[range 0x0000000000c0ffee-0x00000000ba5eba11]", > + "%par", &test_range); Case when start == end? Case when end < start? > +} ... > + *p++ = '['; > + p = string_nocheck(p, pend, "range ", default_str_spec); > + p = special_hex_number(p, pend, range->start, sizeof(range->start)); > + *p++ = '-'; > + p = special_hex_number(p, pend, range->end, sizeof(range->end)); > + *p++ = ']'; > + *p = '\0'; As per above comments.
On Tue, Aug 27, 2024 at 09:43:32AM +0200, Petr Mladek wrote: > On Mon 2024-08-26 16:17:52, Ira Weiny wrote: > > Andy Shevchenko wrote: ... > But I could live with all other variants, except for %pn mentioned below. I believe %r is also no go as we most likely get a complier warning. ... > > Am I missing your point somehow? I considered cramming a struct range into a > > struct resource to let resource_string() process the data. But that would > > involve creating a new IORESOURCE_* flag (not ideal) and also does not allow > > for the larger u64 data in struct range should this be a 32 bit physical > > address config. > > This would be nasty. I believe that this is not what Andy meant. You are right, this is not what I meant.
Petr Mladek wrote: > On Mon 2024-08-26 16:17:52, Ira Weiny wrote: > > Andy Shevchenko wrote: > > > On Mon, Aug 26, 2024 at 03:23:50PM +0200, Petr Mladek wrote: > > > > On Thu 2024-08-22 21:10:25, Andy Shevchenko wrote: > > > > > On Thu, Aug 22, 2024 at 12:53:32PM -0500, Ira Weiny wrote: > > > > > > Petr Mladek wrote: > > > > > > > On Fri 2024-08-16 09:44:10, Ira Weiny wrote: > > > > > > ... > > > > > > > > > > > + %par [range 0x60000000-0x6fffffff] or > > > > > > > > > > > > > > It seems that it is always 64-bit. It prints: > > > > > > > > > > > > > > struct range { > > > > > > > u64 start; > > > > > > > u64 end; > > > > > > > }; > > > > > > > > > > > > Indeed. Thanks I should not have just copied/pasted. > > > > > > > > > > With that said, I'm not sure the %pa is a good placeholder for this ('a' stands > > > > > to "address" AFAIU). Perhaps this should go somewhere under %pr/%pR? > > > > I'm speaking a bit for Dan here but also the logical way I thought of > > things. > > > > 1) %p does not dictate anything about the format of the data. Rather > > indicates that what is passed is a pointer. Because we are passing a > > pointer to a range struct %pXX makes sense. > > 2) %pa indicates what follows is 'address'. This was a bit of creative > > license because, as I said in the commit message most of the time > > struct range contains an address range. So for this narrow use case it > > also makes sense. > > 3) %par r for range. > > Yes. I got it. > > Well, is struct range really used for addresses? Commonly yes. But I agree with Andy that it is not always. > It rather looks like > a range of any 64-bit values. > > > %p[rR] is taken. %pra confuses things IMO. > > Another variants might be %pr64 or %prange. > > IMHO, there is no good solution. We are trying to find the least > bad one. The meaning should be as obvious and as least confusing > as possible. Yep. > > Honestly, I do not have a strong opinion. I kind of like %prange ;-) > But I could live with all other variants, except for %pn mentioned below. > > > > > The r/R in %pr/%pR actually stands for "resource". > > > > > > > > But "%ra" really looks like a better choice than "%par". Both > > > > "resource" and "range" starts with 'r'. Also the struct resource > > > > is printed as a range of values. > > > > %r could be used I think. But this breaks with the convention of passing a > > pointer and how to interpret it. > > How exactly does it break the convention, please? > > Do you passing a pointer to struct range instead of a pointer to > struct resource? Yes a pointer is passed as the parameter. This is what %p means AFAIU. Then the modifier is applied to know what we are pointing to. > > It should not be a big problem as long as the vsprintf() code is > able to guess the right pointer type from the %pXX modifier. > > > The other idea I had, mentioned in the commit > > message was %pn. Meaning passed by pointer 'raNge'. > > This looks like the worst variant to me. Fair enough. > > > > Fine with me as long as it: > > > 1) doesn't collide with %pa namespace > > > 2) tries to deduplicate existing code as much as possible. > > > > Andy, I'm not quite following how you expect to share the code between > > resource_string() and range_string()? > > > > There is very little duplicated code. In fact with Petr's suggestions and some > > more work range_string() is quite simple: > > > > +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); > > + > > + *p++ = '['; > > + p = string_nocheck(p, pend, "range ", default_str_spec); > > + p = special_hex_number(p, pend, range->start, sizeof(range->start)); > > + *p++ = '-'; > > + p = special_hex_number(p, pend, range->end, sizeof(range->end)); > > + *p++ = ']'; > > + *p = '\0'; > > + > > + return string_nocheck(buf, end, sym, spec); > > +} > > I agree that there is not much duplicated code in the end. > > > Also this is the bulk of the patch except for documentation and the new > > testing code. [new patch below] > > > > Am I missing your point somehow? I considered cramming a struct range into a > > struct resource to let resource_string() process the data. But that would > > involve creating a new IORESOURCE_* flag (not ideal) and also does not allow > > for the larger u64 data in struct range should this be a 32 bit physical > > address config. > > This would be nasty. I believe that this is not what Andy meant. Nope. > > Best Regards, > Petr > > PS: I have vacation until the end of the week, so my next eventual > reaction would be delayed. No hurry. I'm still mucking around with it, Ira
Andy Shevchenko wrote: > On Mon, Aug 26, 2024 at 04:17:52PM -0500, Ira Weiny wrote: > > Andy Shevchenko wrote: > > > On Mon, Aug 26, 2024 at 03:23:50PM +0200, Petr Mladek wrote: > > > > On Thu 2024-08-22 21:10:25, Andy Shevchenko wrote: > > > > > On Thu, Aug 22, 2024 at 12:53:32PM -0500, Ira Weiny wrote: > > > > > > Petr Mladek wrote: > > > > > > > On Fri 2024-08-16 09:44:10, Ira Weiny wrote: > [snip] > > > > > > > > > > With that said, I'm not sure the %pa is a good placeholder for this ('a' stands > > > > > to "address" AFAIU). Perhaps this should go somewhere under %pr/%pR? > > > > I'm speaking a bit for Dan here but also the logical way I thought of > > things. > > > > 1) %p does not dictate anything about the format of the data. Rather > > indicates that what is passed is a pointer. Because we are passing a > > pointer to a range struct %pXX makes sense. > > There is no objection to that. > > > 2) %pa indicates what follows is 'address'. This was a bit of creative > > license because, as I said in the commit message most of the time > > struct range contains an address range. So for this narrow use case it > > also makes sense. > > As in the discussion it was pointed out that struct range is always 64-bit, > limiting it to the "address" is a wrong assumption as we are talking generic > printing routine here. We don't know what users will be in the future on 32-bit > platforms, or what data (semantically) is being held by this structure. > > > 3) %par r for range. > > I understand, but again struct range != address. Agreed. > > > %p[rR] is taken. > > %pra confuses things IMO. > > It doesn't confuse me. :-) But I believe Petr also has a rationale behind this > proposal as he described earlier. %pra it is then. > > > > > The r/R in %pr/%pR actually stands for "resource". > > > > > > > > But "%ra" really looks like a better choice than "%par". Both > > > > "resource" and "range" starts with 'r'. Also the struct resource > > > > is printed as a range of values. > > > > %r could be used I think. But this breaks with the convention of passing a > > pointer and how to interpret it. The other idea I had, mentioned in the commit > > message was %pn. Meaning passed by pointer 'raNge'. > > No, we can't use %r or anything else that is documented for the standard > printf() format specifiers, otherwise you will get a compiler warning and > basically it means no go. I was not thrilled with %r anyway. > > > I think that follows better than %r. That would be another break from C99. > > But we don't have to follow that. > > > > > Fine with me as long as it: > > > 1) doesn't collide with %pa namespace > > > 2) tries to deduplicate existing code as much as possible. > > > > Andy, I'm not quite following how you expect to share the code between > > resource_string() and range_string()? > > > > There is very little duplicated code. In fact with Petr's suggestions and some > > more work range_string() is quite simple: > > > > +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); > > > Missing check for pointer, but it's not that I wanted to tell. No it was not missing. It was checked in address_val() already. However, with %pra I'll have to add it in. > > > + *p++ = '['; > > + p = string_nocheck(p, pend, "range ", default_str_spec); > > Hmm... %pr uses str_spec, what the difference can be here? str_spec is designed for variable length strings which are used based on the struct resource flags. Struct range does not vary so default_str_spec works. > > > + p = special_hex_number(p, pend, range->start, sizeof(range->start)); > > + *p++ = '-'; > > + p = special_hex_number(p, pend, range->end, sizeof(range->end)); > > This is basically the copy of %pr implementation. Only at a very basic level. struct resource has a variable spec while struct range does not. This causes complexity to make the code the same. > > p = number(p, pend, res->start, *specp); > if (res->start != res->end) { > *p++ = '-'; > p = number(p, pend, res->end, *specp); > } > > Would it be possible to unify? I think so, but it requires a bit of thinking. Not much thinking. But the issue is that they are not close enough to justify the extra complexity IMHO. Making the outputs match with a common function takes 13 lines of code[1] including the declaration of a print specification which, as this thread already showed, is non-trivial to understand. __Also__ this is currently crashing on me and I can't figure out why. $ git diff --stat lib/vsprintf.c | 32 ++++++++++++++++++++++++-------- 1 file changed, 24 insertions(+), 8 deletions(-) OTOH to force a unified output, only takes 2 lines of duplicated code.[2] This is a very minor expense of duplicate code which is much easier to follow. $ git diff --stat lib/vsprintf.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) > > That's why testing is very important in this kind of generic code. Yep. But the struct resource test was stubbed out. I've added some basic ones. But there are many more variations of struct resource prints. I'm not sure I've not broken them. > > > + *p++ = ']'; > > + *p = '\0'; > > + > > + return string_nocheck(buf, end, sym, spec); > > +} > > > > Also this is the bulk of the patch except for documentation and the new > > testing code. [new patch below] > > > > Am I missing your point somehow? > > See above. > > > I considered cramming a struct range into a > > struct resource to let resource_string() process the data. But that would > > involve creating a new IORESOURCE_* flag (not ideal) and also does not allow > > for the larger u64 data in struct range should this be a 32 bit physical > > address config. > > No, that's not what I was expecting. Good. > > > Most importantly that would not be much less code AFAICT. > > ... > > > + %par [range 0x0000000060000000-0x000000006fffffff] > > I still think this is not okay to use %pa namespace. Agreed. Lets go with %pra > > ... > > > +static void __init > > +struct_range(void) > > +{ > > + struct range test_range = { > > + .start = 0xc0ffee00ba5eba11, > > + .end = 0xc0ffee00ba5eba11, > > + }; > > + > > + test("[range 0xc0ffee00ba5eba11-0xc0ffee00ba5eba11]", > > + "%par", &test_range); > > + > > + test_range = (struct range) { > > + .start = 0xc0ffee, > > + .end = 0xba5eba11, > > + }; > > + test("[range 0x0000000000c0ffee-0x00000000ba5eba11]", > > + "%par", &test_range); > > Case when start == end? Yes, that is the 1st case. > Case when end < start? I had no intention of having the output dictated by the values. test("[range 0x0000000000c0ffee-0x0000000000c0ffee]", and test("[range 0x00000000ba5eba11-0x0000000000c0ffee]", ... are acceptable to me. > > > +} > > ... > > > + *p++ = '['; > > + p = string_nocheck(p, pend, "range ", default_str_spec); > > + p = special_hex_number(p, pend, range->start, sizeof(range->start)); > > + *p++ = '-'; > > + p = special_hex_number(p, pend, range->end, sizeof(range->end)); > > + *p++ = ']'; > > + *p = '\0'; > > As per above comments. Thanks for the review, Ira [1] sample diff diff --git a/lib/vsprintf.c b/lib/vsprintf.c index 6be1ca13790c..84757e75e047 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -1039,6 +1039,18 @@ 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) { + *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 +1127,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) @@ -1149,11 +1157,19 @@ char *range_string(char *buf, char *end, const struct range *range, char sym[RANGE_DECODED_BUF_SIZE + RANGE_PRINT_BUF_SIZE]; char *p = sym, *pend = sym + sizeof(sym); + struct printf_spec range_spec = { + spec.field_width = 2 + 2 * sizeof(range->start), /* 0x + 2 * u64 */ + spec.flags = SPECIAL | SMALL | ZEROPAD, + spec.base = 16, + spec.precision = -1, + }; + + if (check_pointer(&buf, end, range, spec)) + return buf; + *p++ = '['; p = string_nocheck(p, pend, "range ", default_str_spec); - p = special_hex_number(p, pend, range->start, sizeof(range->start)); - *p++ = '-'; - p = special_hex_number(p, pend, range->end, sizeof(range->end)); + p = hex_range(p, pend, range->start, range->end, range_spec); *p++ = ']'; *p = '\0'; [2] sample diff diff --git a/lib/vsprintf.c b/lib/vsprintf.c index a754eefef252..e6870eb703a4 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -1149,11 +1149,16 @@ char *range_string(char *buf, char *end, const struct range *range, char sym[RANGE_DECODED_BUF_SIZE + RANGE_PRINT_BUF_SIZE]; char *p = sym, *pend = sym + sizeof(sym); + if (check_pointer(&buf, end, range, spec)) + return buf; + *p++ = '['; p = string_nocheck(p, pend, "range ", default_str_spec); p = special_hex_number(p, pend, range->start, sizeof(range->start)); - *p++ = '-'; - p = special_hex_number(p, pend, range->end, sizeof(range->end)); + if (range->start != range->end) { + *p++ = '-'; + p = special_hex_number(p, pend, range->end, sizeof(range->end)); + } *p++ = ']'; *p = '\0';
On Tue, Aug 27, 2024 at 11:12:47PM -0500, Ira Weiny wrote: > Andy Shevchenko wrote: > > On Mon, Aug 26, 2024 at 04:17:52PM -0500, Ira Weiny wrote: > > > Andy Shevchenko wrote: > > > > On Mon, Aug 26, 2024 at 03:23:50PM +0200, Petr Mladek wrote: > > > > > On Thu 2024-08-22 21:10:25, Andy Shevchenko wrote: > > > > > > On Thu, Aug 22, 2024 at 12:53:32PM -0500, Ira Weiny wrote: > > > > > > > Petr Mladek wrote: > > > > > > > > On Fri 2024-08-16 09:44:10, Ira Weiny wrote: [snip] > > > +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); > > > > Missing check for pointer, but it's not that I wanted to tell. > > No it was not missing. It was checked in address_val() already. However, with > %pra I'll have to add it in. Ah, I haven't noticed the address_val() implementation details, thanks for elaborating! > > > + *p++ = '['; > > > + p = string_nocheck(p, pend, "range ", default_str_spec); > > > > Hmm... %pr uses str_spec, what the difference can be here? > > str_spec is designed for variable length strings which are used based on the > struct resource flags. Struct range does not vary so default_str_spec works. Okay, makes sense. > > > + p = special_hex_number(p, pend, range->start, sizeof(range->start)); > > > + *p++ = '-'; > > > + p = special_hex_number(p, pend, range->end, sizeof(range->end)); > > > > This is basically the copy of %pr implementation. > > Only at a very basic level. struct resource has a variable spec while struct > range does not. This causes complexity to make the code the same. Fair enough, that's why I said "as much as possible to deduplicate". If you think this is not worth it, let's do without an additional complications then. > > p = number(p, pend, res->start, *specp); > > if (res->start != res->end) { > > *p++ = '-'; > > p = number(p, pend, res->end, *specp); > > } > > > > Would it be possible to unify? I think so, but it requires a bit of thinking. > > Not much thinking. But the issue is that they are not close enough to justify > the extra complexity IMHO. Okay! > Making the outputs match with a common function takes 13 lines of code[1] > including the declaration of a print specification which, as this thread > already showed, is non-trivial to understand. > __Also__ this is currently crashing on me and I can't figure out why. > > $ git diff --stat > lib/vsprintf.c | 32 ++++++++++++++++++++++++-------- > 1 file changed, 24 insertions(+), 8 deletions(-) > > OTOH to force a unified output, only takes 2 lines of duplicated code.[2] This > is a very minor expense of duplicate code which is much easier to follow. > > $ git diff --stat > lib/vsprintf.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) Yep, got it. > > That's why testing is very important in this kind of generic code. > > Yep. But the struct resource test was stubbed out. I've added some basic > ones. But there are many more variations of struct resource prints. I'm not > sure I've not broken them. Yeah, so make it then separated branches for %pr and %pra. You will take the correct argument type in each of them. There are existing examples there. Probably an initial 'r'/'R' parsing should be moved to pointer(). > > > + *p++ = ']'; > > > + *p = '\0'; > > > + > > > + return string_nocheck(buf, end, sym, spec); > > > +} ... > > > + struct range test_range = { > > > + .start = 0xc0ffee00ba5eba11, > > > + .end = 0xc0ffee00ba5eba11, > > > + }; > > > + > > > + test("[range 0xc0ffee00ba5eba11-0xc0ffee00ba5eba11]", > > > + "%par", &test_range); > > > + > > > + test_range = (struct range) { > > > + .start = 0xc0ffee, > > > + .end = 0xba5eba11, > > > + }; > > > + test("[range 0x0000000000c0ffee-0x00000000ba5eba11]", > > > + "%par", &test_range); > > > > Case when start == end? > > Yes, that is the 1st case. Thumb up! > > Case when end < start? > > I had no intention of having the output dictated by the values. > > test("[range 0x0000000000c0ffee-0x0000000000c0ffee]", > and > test("[range 0x00000000ba5eba11-0x0000000000c0ffee]", > > ... are acceptable to me. But it seems the %pr in the first case doesn't do range, just a single value, which makes sense to me (and this thread proved it) to avoid needless pedantic checking of each value. It means that at a glance you may tell start == end. Not sure about end < start case, but the point is just let's make it mimicing %pr behaviour. ... > +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) { > + *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 +1127,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) > @@ -1149,11 +1157,19 @@ char *range_string(char *buf, char *end, const struct range *range, > char sym[RANGE_DECODED_BUF_SIZE + RANGE_PRINT_BUF_SIZE]; > char *p = sym, *pend = sym + sizeof(sym); > > + struct printf_spec range_spec = { > + spec.field_width = 2 + 2 * sizeof(range->start), /* 0x + 2 * u64 */ > + spec.flags = SPECIAL | SMALL | ZEROPAD, > + spec.base = 16, > + spec.precision = -1, > + }; But this can be deduplicated from special_hex_number(), no? Something like fill_special_hex_number_spec() { } special_hex_number() { fill_special_hex_number_spec(); } special_hex_range() { fill_special_hex_number_spec(); } Would it be better? > + if (check_pointer(&buf, end, range, spec)) > + return buf; > + > *p++ = '['; > p = string_nocheck(p, pend, "range ", default_str_spec); > - p = special_hex_number(p, pend, range->start, sizeof(range->start)); > - *p++ = '-'; > - p = special_hex_number(p, pend, range->end, sizeof(range->end)); > + p = hex_range(p, pend, range->start, range->end, range_spec); > *p++ = ']'; > *p = '\0'; so, can you check if with the above implemented we can actually enforce unified format for %pr and %pra? ... > [2] sample diff > p = special_hex_number(p, pend, range->start, sizeof(range->start)); > - *p++ = '-'; > - p = special_hex_number(p, pend, range->end, sizeof(range->end)); > + if (range->start != range->end) { > + *p++ = '-'; > + p = special_hex_number(p, pend, range->end, sizeof(range->end)); > + } There is a possibility to supply a callback, but it seems to me much overcomplicated approach. ... If we go the second way (the latter one here) can you add a comment in both %pr/%pra code excerpts to point to each other that the format is unified between them? It might help in the future to optimise the code if needed at all.
diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst index 4451ef501936..a02ef899b2a6 100644 --- a/Documentation/core-api/printk-formats.rst +++ b/Documentation/core-api/printk-formats.rst @@ -231,6 +231,20 @@ width of the CPU data path. Passed by reference. +Struct Range +------------ + +:: + + %par [range 0x60000000-0x6fffffff] or + [range 0x0000000060000000-0x000000006fffffff] + +For printing struct range. A variation of printing a physical address is to +print the value of struct range which are often used to hold a physical address +range. + +Passed by reference. + DMA address types dma_addr_t ---------------------------- diff --git a/lib/vsprintf.c b/lib/vsprintf.c index 2d71b1115916..c132178fac07 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -1140,6 +1140,39 @@ 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_PRINTK_SIZE 16 +#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); + + static const struct printf_spec str_spec = { + .field_width = -1, + .precision = 10, + .flags = LEFT, + }; + static const struct printf_spec range_spec = { + .base = 16, + .field_width = RANGE_PRINTK_SIZE, + .precision = -1, + .flags = SPECIAL | SMALL | ZEROPAD, + }; + + *p++ = '['; + p = string_nocheck(p, pend, "range ", str_spec); + p = number(p, pend, range->start, range_spec); + *p++ = '-'; + p = number(p, pend, 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) @@ -1802,6 +1835,8 @@ char *address_val(char *buf, char *end, const void *addr, return buf; switch (fmt[1]) { + case 'r': + return range_string(buf, end, addr, spec, fmt); case 'd': num = *(const dma_addr_t *)addr; size = sizeof(dma_addr_t); @@ -2364,6 +2399,8 @@ char *rust_fmt_argument(char *buf, char *end, void *ptr); * to use print_hex_dump() for the larger input. * - 'a[pd]' For address types [p] phys_addr_t, [d] dma_addr_t and derivatives * (default assumed to be phys_addr_t, passed by reference) + * - 'ar' For decoded struct ranges (a variation of physical address which are + * most often stored in struct ranges. * - 'd[234]' For a dentry name (optionally 2-4 last components) * - 'D[234]' Same as 'd' but for a struct file * - 'g' For block_device name (gendisk + partition number)
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 '%pn' for 'print raNge' but %par follows that struct range is most often used to store a range of physical addresses. So use '%par' for 'print address range'. To: Petr Mladek <pmladek@suse.com> (maintainer:VSPRINTF) To: Steven Rostedt <rostedt@goodmis.org> (maintainer:VSPRINTF) To: Jonathan Corbet <corbet@lwn.net> (maintainer:DOCUMENTATION) Cc: linux-doc@vger.kernel.org (open list:DOCUMENTATION) Cc: linux-kernel@vger.kernel.org (open list) Link: https://lore.kernel.org/all/663922b475e50_d54d72945b@dwillia2-xfh.jf.intel.com.notmuch/ [1] Suggested-by: "Dan Williams" <dan.j.williams@intel.com> Signed-off-by: Ira Weiny <ira.weiny@intel.com> --- Documentation/core-api/printk-formats.rst | 14 ++++++++++++ lib/vsprintf.c | 37 +++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+)