[PATCH 0/3] printf: Add struct range print specifier

[PATCH 0/3] printf: Add struct range print specifier

[Ira Weiny]

Support for the Compute Express Link (CXL) Dynamic Capacity Devices
(DCD) have grown a number of uses to print struct range.[1]  Support for
a printf specifier '%pra' was being worked within a large series and has
garnered a number of comments and discussion.

To better accelerate both features introduce a separate series to settle
the struct range print enhancement divorced from the CXL DCD feature.

Struct range is used to store a number range similar to struct resource.
Printing struct range becomes cumbersome having to specify 2 specifiers
and the members of the struct.  Furthermore, print output benefits from
using a standardized format.

Add to the pointer specifier support for struct range.  Share code with
struct resource for more standardization.  Add tests for struct resource
to help prevent regressions.

%pra was settled on as the most reasonable format in previous
discussions.[2]

Link: https://lore.kernel.org/all/20241007-dcd-type2-upstream-v4-2-c261ee6eeded@intel.com/ [1]
Link: https://lore.kernel.org/all/66cea3bf3332f_f937b29424@iweiny-mobl.notmuch/ [2]

Signed-off-by: Ira Weiny <ira.weiny@intel.com>
---
Ira Weiny (3):
      test printf: Add very basic struct resource tests
      printf: Add print format (%pra) for struct range
      cxl/cdat: Use %pra for dpa range outputs

 Documentation/core-api/printk-formats.rst | 13 +++++++
 drivers/cxl/core/cdat.c                   |  8 ++--
 include/linux/range.h                     |  6 +++
 lib/test_printf.c                         | 61 +++++++++++++++++++++++++++++++
 lib/vsprintf.c                            | 57 ++++++++++++++++++++++++++---
 5 files changed, 135 insertions(+), 10 deletions(-)
---
base-commit: 9852d85ec9d492ebef56dc5f229416c925758edc
change-id: 20240930-cxl-pra-53956ac5fc1e

Best regards,
-- 
Ira Weiny <ira.weiny@intel.com>

[PATCH 1/3] test printf: Add very basic struct resource tests

[Ira Weiny]

The printf tests for struct resource were stubbed out.  struct range
printing will leverage the struct resource implementation.

To prevent regression add some basic sanity tests for struct resource.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Fan Ni <fan.ni@samsung.com>
Tested-by: Fan Ni <fan.ni@samsung.com>
Acked-by: Petr Mladek <pmladek@suse.com>
Link: https://patch.msgid.link/20241007-dcd-type2-upstream-v4-1-c261ee6eeded@intel.com
Signed-off-by: Ira Weiny <ira.weiny@intel.com>
---
 lib/test_printf.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/lib/test_printf.c b/lib/test_printf.c
index 8448b6d02bd9..5afdf5efc627 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -386,6 +386,50 @@ kernel_ptr(void)
 static void __init
 struct_resource(void)
 {
+	struct resource test_resource = {
+		.start = 0xc0ffee00,
+		.end = 0xc0ffee00,
+		.flags = IORESOURCE_MEM,
+	};
+
+	test("[mem 0xc0ffee00 flags 0x200]",
+	     "%pr", &test_resource);
+
+	test_resource = (struct resource) {
+		.start = 0xc0ffee,
+		.end = 0xba5eba11,
+		.flags = IORESOURCE_MEM,
+	};
+	test("[mem 0x00c0ffee-0xba5eba11 flags 0x200]",
+	     "%pr", &test_resource);
+
+	test_resource = (struct resource) {
+		.start = 0xba5eba11,
+		.end = 0xc0ffee,
+		.flags = IORESOURCE_MEM,
+	};
+	test("[mem 0xba5eba11-0x00c0ffee flags 0x200]",
+	     "%pr", &test_resource);
+
+	test_resource = (struct resource) {
+		.start = 0xba5eba11,
+		.end = 0xba5eca11,
+		.flags = IORESOURCE_MEM,
+	};
+
+	test("[mem 0xba5eba11-0xba5eca11 flags 0x200]",
+	     "%pr", &test_resource);
+
+	test_resource = (struct resource) {
+		.start = 0xba11,
+		.end = 0xca10,
+		.flags = IORESOURCE_IO |
+			 IORESOURCE_DISABLED |
+			 IORESOURCE_UNSET,
+	};
+
+	test("[io  size 0x1000 disabled]",
+	     "%pR", &test_resource);
 }
 
 static void __init

-- 
2.47.0

[PATCH 2/3] printf: Add print format (%pra) for struct range

[Ira Weiny]

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 needs 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.

Finally introduce DEFINE_RANGE() as a parallel to DEFINE_RES_*() and use
it in the tests.

Cc: Jonathan Corbet <corbet@lwn.net> (maintainer:DOCUMENTATION)
Cc: linux-doc@vger.kernel.org
Cc: linux-kernel@vger.kernel.org (open list)
Link: https://lore.kernel.org/all/663922b475e50_d54d72945b@dwillia2-xfh.jf.intel.com.notmuch/ [1]
Link: https://lore.kernel.org/all/66cea3bf3332f_f937b29424@iweiny-mobl.notmuch/ [2]
Suggested-by: "Dan Williams" <dan.j.williams@intel.com>
Signed-off-by: Ira Weiny <ira.weiny@intel.com>

---
Changes:
[iweiny: split from CXL patch set]
[Andy: Define DEFINE_RANGE()]
[Andy: rework hex_range() logic]
[Andy: Don't use fallthrough]
[Andy: clean up help comment]
[Fan: clean up printk comment]
[Rasmus: fix buf processing in hex_range()]
[Rasmus: simplify buffer sizing]
[Rasmus: add '[' to string]
[Bagas: Clarify the start == end print behavior]
---
 Documentation/core-api/printk-formats.rst | 13 +++++++
 include/linux/range.h                     |  6 ++++
 lib/test_printf.c                         | 17 +++++++++
 lib/vsprintf.c                            | 57 +++++++++++++++++++++++++++----
 4 files changed, 87 insertions(+), 6 deletions(-)

diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
index 14e093da3ccd..e1ebf0376154 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 print the start value.
+
+Passed by reference.
+
 DMA address types dma_addr_t
 ----------------------------
 
diff --git a/include/linux/range.h b/include/linux/range.h
index 6ad0b73cb7ad..1358d4b1807a 100644
--- a/include/linux/range.h
+++ b/include/linux/range.h
@@ -31,4 +31,10 @@ int clean_sort_range(struct range *range, int az);
 
 void sort_range(struct range *range, int nr_range);
 
+#define DEFINE_RANGE(_start, _end)		\
+(struct range) {				\
+		.start = (_start),		\
+		.end = (_end),			\
+	}
+
 #endif
diff --git a/lib/test_printf.c b/lib/test_printf.c
index 5afdf5efc627..59dbe4f9a4cb 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -432,6 +432,22 @@ struct_resource(void)
 	     "%pR", &test_resource);
 }
 
+static void __init
+struct_range(void)
+{
+	struct range test_range = DEFINE_RANGE(0xc0ffee00ba5eba11,
+					       0xc0ffee00ba5eba11);
+	test("[range 0xc0ffee00ba5eba11]", "%pra", &test_range);
+
+	test_range = DEFINE_RANGE(0xc0ffee, 0xba5eba11);
+	test("[range 0x0000000000c0ffee-0x00000000ba5eba11]",
+	     "%pra", &test_range);
+
+	test_range = DEFINE_RANGE(0xba5eba11, 0xc0ffee);
+	test("[range 0x00000000ba5eba11-0x0000000000c0ffee]",
+	     "%pra", &test_range);
+}
+
 static void __init
 addr(void)
 {
@@ -807,6 +823,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..f6a62f273fad 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1039,6 +1039,20 @@ 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)
+		return buf;
+
+	if (buf < end)
+		*buf = '-';
+	++buf;
+	return number(buf, end, end_val, spec);
+}
+
 static noinline_for_stack
 char *resource_string(char *buf, char *end, struct resource *res,
 		      struct printf_spec spec, const char *fmt)
@@ -1115,11 +1129,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 +1150,31 @@ 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)
+{
+	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 = 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)
@@ -2229,6 +2264,15 @@ char *fwnode_string(char *buf, char *end, struct fwnode_handle *fwnode,
 	return widen_string(buf, buf - buf_start, end, spec);
 }
 
+static noinline_for_stack
+char *resource_and_range(const char *fmt, char *buf, char *end, void *ptr,
+			 struct printf_spec spec)
+{
+	if (*fmt == 'r' && fmt[1] == 'a')
+		return range_string(buf, end, ptr, spec, fmt);
+	return resource_string(buf, end, ptr, spec, fmt);
+}
+
 int __init no_hash_pointers_enable(char *str)
 {
 	if (no_hash_pointers)
@@ -2277,6 +2321,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' For struct ranges, e.g., [range 0x0000000000000000 - 0x00000000000000ff]
  * - '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
@@ -2401,7 +2446,7 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 		return symbol_string(buf, end, ptr, spec, fmt);
 	case 'R':
 	case 'r':
-		return resource_string(buf, end, ptr, spec, fmt);
+		return resource_and_range(fmt, buf, end, ptr, spec);
 	case 'h':
 		return hex_string(buf, end, ptr, spec, fmt);
 	case 'b':

-- 
2.47.0

[PATCH 3/3] cxl/cdat: Use %pra for dpa range outputs

[Ira Weiny]

Now that there is a printf specifier for struct range use it to enhance
the debug output of CDAT data.

Signed-off-by: Ira Weiny <ira.weiny@intel.com>
---
 drivers/cxl/core/cdat.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
index ef1621d40f05..438869df241a 100644
--- a/drivers/cxl/core/cdat.c
+++ b/drivers/cxl/core/cdat.c
@@ -247,8 +247,8 @@ static void update_perf_entry(struct device *dev, struct dsmas_entry *dent,
 	dpa_perf->dpa_range = dent->dpa_range;
 	dpa_perf->qos_class = dent->qos_class;
 	dev_dbg(dev,
-		"DSMAS: dpa: %#llx qos: %d read_bw: %d write_bw %d read_lat: %d write_lat: %d\n",
-		dent->dpa_range.start, dpa_perf->qos_class,
+		"DSMAS: dpa: %pra qos: %d read_bw: %d write_bw %d read_lat: %d write_lat: %d\n",
+		&dent->dpa_range, dpa_perf->qos_class,
 		dent->coord[ACCESS_COORDINATE_CPU].read_bandwidth,
 		dent->coord[ACCESS_COORDINATE_CPU].write_bandwidth,
 		dent->coord[ACCESS_COORDINATE_CPU].read_latency,
@@ -279,8 +279,8 @@ static void cxl_memdev_set_qos_class(struct cxl_dev_state *cxlds,
 			 range_contains(&pmem_range, &dent->dpa_range))
 			update_perf_entry(dev, dent, &mds->pmem_perf);
 		else
-			dev_dbg(dev, "no partition for dsmas dpa: %#llx\n",
-				dent->dpa_range.start);
+			dev_dbg(dev, "no partition for dsmas dpa: %pra\n",
+				&dent->dpa_range);
 	}
 }
 

-- 
2.47.0

Re: [PATCH 2/3] printf: Add print format (%pra) for struct range

[Dan Williams]

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 needs 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.
> 
> Finally introduce DEFINE_RANGE() as a parallel to DEFINE_RES_*() and use
> it in the tests.
> 
> Cc: Jonathan Corbet <corbet@lwn.net> (maintainer:DOCUMENTATION)
> Cc: linux-doc@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org (open list)
> Link: https://lore.kernel.org/all/663922b475e50_d54d72945b@dwillia2-xfh.jf.intel.com.notmuch/ [1]
> Link: https://lore.kernel.org/all/66cea3bf3332f_f937b29424@iweiny-mobl.notmuch/ [2]
> Suggested-by: "Dan Williams" <dan.j.williams@intel.com>
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> 
> ---
> Changes:
> [iweiny: split from CXL patch set]
> [Andy: Define DEFINE_RANGE()]
> [Andy: rework hex_range() logic]
> [Andy: Don't use fallthrough]
> [Andy: clean up help comment]
> [Fan: clean up printk comment]
> [Rasmus: fix buf processing in hex_range()]
> [Rasmus: simplify buffer sizing]
> [Rasmus: add '[' to string]
> [Bagas: Clarify the start == end print behavior]
> ---
>  Documentation/core-api/printk-formats.rst | 13 +++++++
>  include/linux/range.h                     |  6 ++++
>  lib/test_printf.c                         | 17 +++++++++
>  lib/vsprintf.c                            | 57 +++++++++++++++++++++++++++----
>  4 files changed, 87 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
> index 14e093da3ccd..e1ebf0376154 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 print the start value.

I was going to say "why this special case that does not exist for the
%pr case?", but then checked the code and found it *does* do this for %pr.
So if you're going to document this special case for %pra might as well
update the documentation for %pr too.

Alternatively, drop the new %pra documentation for this corner case as
accommodating the U64_MAX size range case is arguably a mistake in the
caller.

Either way, just make it consistent.

Re: [PATCH 2/3] printf: Add print format (%pra) for struct range

[Ira Weiny]

Dan Williams wrote:
> Ira Weiny wrote:

[snip]

> > diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
> > index 14e093da3ccd..e1ebf0376154 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 print the start value.
> 
> I was going to say "why this special case that does not exist for the
> %pr case?", but then checked the code and found it *does* do this for %pr.
> So if you're going to document this special case for %pra might as well
> update the documentation for %pr too.
> 
> Alternatively, drop the new %pra documentation for this corner case as
> accommodating the U64_MAX size range case is arguably a mistake in the
> caller.
> 
> Either way, just make it consistent.

I've dropped the special case in the documentation.
Ira

Re: [PATCH 2/3] printf: Add print format (%pra) for struct range

[Andy Shevchenko]

On Fri, Oct 18, 2024 at 01:05:22PM -0700, Dan Williams wrote:
> Ira Weiny wrote:

...

> > +	%pra    [range 0x0000000060000000]
> > +
> > +For printing struct range.  struct range holds an arbitrary range of u64
> > +values.  If start is equal to end only print the start value.
> 
> I was going to say "why this special case that does not exist for the
> %pr case?", but then checked the code and found it *does* do this for %pr.
> So if you're going to document this special case for %pra might as well
> update the documentation for %pr too.
> 
> Alternatively, drop the new %pra documentation for this corner case as
> accommodating the U64_MAX size range case is arguably a mistake in the
> caller.

You probably meant "...(U64_MAX + 1) size..." as we end up with the same value.
But yeah, I also noticed the same.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/3] printf: Add print format (%pra) for struct range

[Andy Shevchenko]

On Fri, Oct 18, 2024 at 02:46:25PM -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 needs 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.
> 
> Finally introduce DEFINE_RANGE() as a parallel to DEFINE_RES_*() and use
> it in the tests.

...

>  	case 'R':
>  	case 'r':
> -		return resource_string(buf, end, ptr, spec, fmt);
> +		return resource_and_range(fmt, buf, end, ptr, spec);

Since you are going to have a new version, I think this should be _or_ instead
of _and_.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/3] printf: Add print format (%pra) for struct range

[Petr Mladek]

On Sun 2024-10-20 21:49:50, Ira Weiny wrote:
> Dan Williams wrote:
> > Ira Weiny wrote:
> 
> [snip]
> 
> > > diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
> > > index 14e093da3ccd..e1ebf0376154 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 print the start value.
> > 
> > I was going to say "why this special case that does not exist for the
> > %pr case?", but then checked the code and found it *does* do this for %pr.
> > So if you're going to document this special case for %pra might as well
> > update the documentation for %pr too.
> > 
> > Alternatively, drop the new %pra documentation for this corner case as
> > accommodating the U64_MAX size range case is arguably a mistake in the
> > caller.
> > 
> > Either way, just make it consistent.
> 
> I've dropped the special case in the documentation.

I would actually prefer the opposite and update the %pr documentation.

The behavior might be surprising and people should beware of it,
for example when writing a parser for the output.

Best Regards,
Petr

Re: [PATCH 1/3] test printf: Add very basic struct resource tests

[Petr Mladek]

On Fri 2024-10-18 14:46:24, Ira Weiny wrote:
> The printf tests for struct resource were stubbed out.  struct range
> printing will leverage the struct resource implementation.
> 
> To prevent regression add some basic sanity tests for struct resource.
> 
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Reviewed-by: Fan Ni <fan.ni@samsung.com>
> Tested-by: Fan Ni <fan.ni@samsung.com>
> Acked-by: Petr Mladek <pmladek@suse.com>
> Link: https://patch.msgid.link/20241007-dcd-type2-upstream-v4-1-c261ee6eeded@intel.com
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>

Tested-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

Re: [PATCH 2/3] printf: Add print format (%pra) for struct range

[Ira Weiny]

Petr Mladek wrote:
> On Sun 2024-10-20 21:49:50, Ira Weiny wrote:
> > Dan Williams wrote:
> > > Ira Weiny wrote:
> > 
> > [snip]
> > 
> > > > diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
> > > > index 14e093da3ccd..e1ebf0376154 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 print the start value.
> > > 
> > > I was going to say "why this special case that does not exist for the
> > > %pr case?", but then checked the code and found it *does* do this for %pr.
> > > So if you're going to document this special case for %pra might as well
> > > update the documentation for %pr too.
> > > 
> > > Alternatively, drop the new %pra documentation for this corner case as
> > > accommodating the U64_MAX size range case is arguably a mistake in the
> > > caller.
> > > 
> > > Either way, just make it consistent.
> > 
> > I've dropped the special case in the documentation.
> 
> I would actually prefer the opposite and update the %pr documentation.
> 
> The behavior might be surprising and people should beware of it,
> for example when writing a parser for the output.

Works for me.  I added a patch to enhance %pr and left in the %pra doc.

Ira

Re: [PATCH 3/3] cxl/cdat: Use %pra for dpa range outputs

[Alison Schofield]

On Fri, Oct 18, 2024 at 02:46:26PM -0500, Ira Weiny wrote:
> Now that there is a printf specifier for struct range use it to enhance
> the debug output of CDAT data.
> 
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> ---
>  drivers/cxl/core/cdat.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
> index ef1621d40f05..438869df241a 100644
> --- a/drivers/cxl/core/cdat.c
> +++ b/drivers/cxl/core/cdat.c
> @@ -247,8 +247,8 @@ static void update_perf_entry(struct device *dev, struct dsmas_entry *dent,
>  	dpa_perf->dpa_range = dent->dpa_range;
>  	dpa_perf->qos_class = dent->qos_class;
>  	dev_dbg(dev,
> -		"DSMAS: dpa: %#llx qos: %d read_bw: %d write_bw %d read_lat: %d write_lat: %d\n",
> -		dent->dpa_range.start, dpa_perf->qos_class,
> +		"DSMAS: dpa: %pra qos: %d read_bw: %d write_bw %d read_lat: %d write_lat: %d\n",
> +		&dent->dpa_range, dpa_perf->qos_class,
>  		dent->coord[ACCESS_COORDINATE_CPU].read_bandwidth,
>  		dent->coord[ACCESS_COORDINATE_CPU].write_bandwidth,
>  		dent->coord[ACCESS_COORDINATE_CPU].read_latency,
> @@ -279,8 +279,8 @@ static void cxl_memdev_set_qos_class(struct cxl_dev_state *cxlds,
>  			 range_contains(&pmem_range, &dent->dpa_range))
>  			update_perf_entry(dev, dent, &mds->pmem_perf);
>  		else
> -			dev_dbg(dev, "no partition for dsmas dpa: %#llx\n",
> -				dent->dpa_range.start);
> +			dev_dbg(dev, "no partition for dsmas dpa: %pra\n",
> +				&dent->dpa_range);
>  	}
>  }

This is a bit different than what I expected to find as the initial use case
because it wasn't printing a range. With this change we go from printing only
the .start to printing the range. Seems the wording of the dev_ message could
change too since 'dpa' has been replaced with a 'dpa range'.

There are a few places that print the range now and can be cleaned up w this
specifier. Those are the real 'uglies' like this:

diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
index 223c273c0cd1..85a121b7b2b5 100644
--- a/drivers/cxl/core/hdm.c
+++ b/drivers/cxl/core/hdm.c
@@ -941,8 +941,8 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
                return rc;
        }

-       dev_dbg(&port->dev, "decoder%d.%d: range: %#llx-%#llx iw: %d ig: %d\n",
-               port->id, cxld->id, cxld->hpa_range.start, cxld->hpa_range.end,
+       dev_dbg(&port->dev, "decoder%d.%d: range: %pra iw: %d ig: %d\n",
+               port->id, cxld->id, &cxld->hpa_range,
                cxld->interleave_ways, cxld->interleave_granularity);


I guess you could (ducks) pick them all up here, or we can leave it
for a future cleanup, or we can just say no cleanups and we'll use
%pra going forward only.

-- Alison

>  
> 
> -- 
> 2.47.0
> 

Re: [PATCH 3/3] cxl/cdat: Use %pra for dpa range outputs

[Ira Weiny]

Alison Schofield wrote:
> On Fri, Oct 18, 2024 at 02:46:26PM -0500, Ira Weiny wrote:
> > Now that there is a printf specifier for struct range use it to enhance
> > the debug output of CDAT data.
> > 
> > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> > ---
> >  drivers/cxl/core/cdat.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
> > index ef1621d40f05..438869df241a 100644
> > --- a/drivers/cxl/core/cdat.c
> > +++ b/drivers/cxl/core/cdat.c
> > @@ -247,8 +247,8 @@ static void update_perf_entry(struct device *dev, struct dsmas_entry *dent,
> >  	dpa_perf->dpa_range = dent->dpa_range;
> >  	dpa_perf->qos_class = dent->qos_class;
> >  	dev_dbg(dev,
> > -		"DSMAS: dpa: %#llx qos: %d read_bw: %d write_bw %d read_lat: %d write_lat: %d\n",
> > -		dent->dpa_range.start, dpa_perf->qos_class,
> > +		"DSMAS: dpa: %pra qos: %d read_bw: %d write_bw %d read_lat: %d write_lat: %d\n",
> > +		&dent->dpa_range, dpa_perf->qos_class,
> >  		dent->coord[ACCESS_COORDINATE_CPU].read_bandwidth,
> >  		dent->coord[ACCESS_COORDINATE_CPU].write_bandwidth,
> >  		dent->coord[ACCESS_COORDINATE_CPU].read_latency,
> > @@ -279,8 +279,8 @@ static void cxl_memdev_set_qos_class(struct cxl_dev_state *cxlds,
> >  			 range_contains(&pmem_range, &dent->dpa_range))
> >  			update_perf_entry(dev, dent, &mds->pmem_perf);
> >  		else
> > -			dev_dbg(dev, "no partition for dsmas dpa: %#llx\n",
> > -				dent->dpa_range.start);
> > +			dev_dbg(dev, "no partition for dsmas dpa: %pra\n",
> > +				&dent->dpa_range);
> >  	}
> >  }
> 
> This is a bit different than what I expected to find as the initial use case
> because it wasn't printing a range.

The reason this was chosen was I was adding to this code and found the
range to be advantageous while doing so.  But the patch was stand alone
in the original DCD series so could be included here.

> With this change we go from printing only
> the .start to printing the range.

Yes that is why I mentioned that %pra is used ...  "to enhance
the debug output of CDAT data."

>
> Seems the wording of the dev_ message could
> change too since 'dpa' has been replaced with a 'dpa range'.

Could be but it made sense to me to read:

"... dpa [range 0x...-0x...]"

Because %pra adds 'range'.

> 
> There are a few places that print the range now and can be cleaned up w this
> specifier. Those are the real 'uglies' like this:

True this is ugly and I would like to clean this up.  But the cdat code
was being modified and lead me to this particular call site.  But it was
also stand alone enough to be used here.

> 
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 223c273c0cd1..85a121b7b2b5 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -941,8 +941,8 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>                 return rc;
>         }
> 
> -       dev_dbg(&port->dev, "decoder%d.%d: range: %#llx-%#llx iw: %d ig: %d\n",
> -               port->id, cxld->id, cxld->hpa_range.start, cxld->hpa_range.end,
> +       dev_dbg(&port->dev, "decoder%d.%d: range: %pra iw: %d ig: %d\n",
> +               port->id, cxld->id, &cxld->hpa_range,
>                 cxld->interleave_ways, cxld->interleave_granularity);
> 
> 
> I guess you could (ducks) pick them all up here, or we can leave it
> for a future cleanup, or we can just say no cleanups and we'll use
> %pra going forward only.
 
I would say we get the specifier in then look at any clean up which works
for us going forward.  Like in this case where I was editing the code
anyway.

Ira

Re: [PATCH 3/3] cxl/cdat: Use %pra for dpa range outputs

[Alison Schofield]

On Wed, Oct 23, 2024 at 01:19:48PM -0500, Ira Weiny wrote:
> Alison Schofield wrote:
> > On Fri, Oct 18, 2024 at 02:46:26PM -0500, Ira Weiny wrote:
> > > Now that there is a printf specifier for struct range use it to enhance
> > > the debug output of CDAT data.
> > > 
> > > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> > > ---
> > >  drivers/cxl/core/cdat.c | 8 ++++----
> > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
> > > index ef1621d40f05..438869df241a 100644
> > > --- a/drivers/cxl/core/cdat.c
> > > +++ b/drivers/cxl/core/cdat.c
> > > @@ -247,8 +247,8 @@ static void update_perf_entry(struct device *dev, struct dsmas_entry *dent,
> > >  	dpa_perf->dpa_range = dent->dpa_range;
> > >  	dpa_perf->qos_class = dent->qos_class;
> > >  	dev_dbg(dev,
> > > -		"DSMAS: dpa: %#llx qos: %d read_bw: %d write_bw %d read_lat: %d write_lat: %d\n",
> > > -		dent->dpa_range.start, dpa_perf->qos_class,
> > > +		"DSMAS: dpa: %pra qos: %d read_bw: %d write_bw %d read_lat: %d write_lat: %d\n",
> > > +		&dent->dpa_range, dpa_perf->qos_class,
> > >  		dent->coord[ACCESS_COORDINATE_CPU].read_bandwidth,
> > >  		dent->coord[ACCESS_COORDINATE_CPU].write_bandwidth,
> > >  		dent->coord[ACCESS_COORDINATE_CPU].read_latency,
> > > @@ -279,8 +279,8 @@ static void cxl_memdev_set_qos_class(struct cxl_dev_state *cxlds,
> > >  			 range_contains(&pmem_range, &dent->dpa_range))
> > >  			update_perf_entry(dev, dent, &mds->pmem_perf);
> > >  		else
> > > -			dev_dbg(dev, "no partition for dsmas dpa: %#llx\n",
> > > -				dent->dpa_range.start);
> > > +			dev_dbg(dev, "no partition for dsmas dpa: %pra\n",
> > > +				&dent->dpa_range);
> > >  	}
> > >  }
> > 
> > This is a bit different than what I expected to find as the initial use case
> > because it wasn't printing a range.
> 
> The reason this was chosen was I was adding to this code and found the
> range to be advantageous while doing so.  But the patch was stand alone
> in the original DCD series so could be included here.
> 
> > With this change we go from printing only
> > the .start to printing the range.
> 
> Yes that is why I mentioned that %pra is used ...  "to enhance
> the debug output of CDAT data."
> 
> >
> > Seems the wording of the dev_ message could
> > change too since 'dpa' has been replaced with a 'dpa range'.
> 
> Could be but it made sense to me to read:
> 
> "... dpa [range 0x...-0x...]"
> 
> Because %pra adds 'range'.
> 
> > 
> > There are a few places that print the range now and can be cleaned up w this
> > specifier. Those are the real 'uglies' like this:
> 
> True this is ugly and I would like to clean this up.  But the cdat code
> was being modified and lead me to this particular call site.  But it was
> also stand alone enough to be used here.
> 
> > 
> > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> > index 223c273c0cd1..85a121b7b2b5 100644
> > --- a/drivers/cxl/core/hdm.c
> > +++ b/drivers/cxl/core/hdm.c
> > @@ -941,8 +941,8 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
> >                 return rc;
> >         }
> > 
> > -       dev_dbg(&port->dev, "decoder%d.%d: range: %#llx-%#llx iw: %d ig: %d\n",
> > -               port->id, cxld->id, cxld->hpa_range.start, cxld->hpa_range.end,
> > +       dev_dbg(&port->dev, "decoder%d.%d: range: %pra iw: %d ig: %d\n",
> > +               port->id, cxld->id, &cxld->hpa_range,
> >                 cxld->interleave_ways, cxld->interleave_granularity);
> > 
> > 
> > I guess you could (ducks) pick them all up here, or we can leave it
> > for a future cleanup, or we can just say no cleanups and we'll use
> > %pra going forward only.
>  
> I would say we get the specifier in then look at any clean up which works
> for us going forward.  Like in this case where I was editing the code
> anyway.

Thanks for the info. LGTM!

Reviewed-by: Alison Schofield <alison.schofield@intel.com>

> 
> Ira