Re: [PATCH v3 02/25] printk: Add print format (%par) for struct range

[Ira Weiny <ira.weiny@intel.com>]

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)