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@xxxxxxxxx>
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@xxxxxxxx> (maintainer:VSPRINTF)
To: Steven Rostedt <rostedt@xxxxxxxxxxx> (maintainer:VSPRINTF)
To: Jonathan Corbet <corbet@xxxxxxx> (maintainer:DOCUMENTATION)
Cc: linux-doc@xxxxxxxxxxxxxxx (open list:DOCUMENTATION)
Cc: linux-kernel@xxxxxxxxxxxxxxx (open list)
Link: https://lore.kernel.org/all/663922b475e50_d54d72945b@xxxxxxxxxxxxxxxxxxxxxxxxx.notmuch/ [1]
Suggested-by: "Dan Williams" <dan.j.williams@xxxxxxxxx>
Signed-off-by: Ira Weiny <ira.weiny@xxxxxxxxx>
---
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)
