Linux CXL
 help / color / mirror / Atom feed
From: Petr Mladek <pmladek@suse.com>
To: Ira Weiny <ira.weiny@intel.com>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	Steven Rostedt <rostedt@goodmis.org>,
	Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
	Rasmus Villemoes <linux@rasmusvillemoes.dk>,
	Sergey Senozhatsky <senozhatsky@chromium.org>,
	Jonathan Corbet <corbet@lwn.net>,
	Davidlohr Bueso <dave@stgolabs.net>,
	Jonathan Cameron <jonathan.cameron@huawei.com>,
	Dave Jiang <dave.jiang@intel.com>,
	Alison Schofield <alison.schofield@intel.com>,
	Vishal Verma <vishal.l.verma@intel.com>,
	Dan Williams <dan.j.williams@intel.com>,
	Fan Ni <fan.ni@samsung.com>, Bagas Sanjaya <bagasdotme@gmail.com>,
	linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org,
	linux-cxl@vger.kernel.org
Subject: Re: [PATCH v2 3/4] printf: Add print format (%pra) for struct range
Date: Thu, 7 Nov 2024 15:24:58 +0100	[thread overview]
Message-ID: <ZyzNuh9BJxQxihYv@pathway.suse.cz> (raw)
In-Reply-To: <20241025-cxl-pra-v2-3-123a825daba2@intel.com>

On Fri 2024-10-25 19:46:55, 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.
> 
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -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_or_range(const char *fmt, char *buf, char *end, void *ptr,
> +			struct printf_spec spec)
> +{
> +	if (*fmt == 'r' && fmt[1] == 'a')

This function is called only when (*fmt == 'r'). We do not need to
check it here.

Otherwise, this function should trigger an error when (*fmt != 'r').

> +		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]

The range is printed without the space ' ' around the dash '-'.
I mean that this should be:

 * - '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


Otherwise, the patch looks good.

I am sorry for the late reply. I had vacation... The problems are
rather cosmetic and could be fixed by a followup patch later.

Best Regards,
Petr

  parent reply	other threads:[~2024-11-07 14:25 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-10-26  0:46 [PATCH v2 0/4] printf: Add struct range print specifier Ira Weiny
2024-10-26  0:46 ` [PATCH v2 1/4] test printf: Add very basic struct resource tests Ira Weiny
2024-10-28  9:07   ` Andy Shevchenko
2024-10-28 21:29     ` Ira Weiny
2024-10-26  0:46 ` [PATCH v2 2/4] Documentation/printf: struct resource add start == end special case Ira Weiny
2024-10-28  9:07   ` Andy Shevchenko
2024-10-29 15:19   ` Jonathan Cameron
2024-11-07 12:34   ` Petr Mladek
2024-10-26  0:46 ` [PATCH v2 3/4] printf: Add print format (%pra) for struct range Ira Weiny
2024-10-28  9:11   ` Andy Shevchenko
2024-11-07 14:24   ` Petr Mladek [this message]
2024-10-26  0:46 ` [PATCH v2 4/4] cxl/cdat: Use %pra for dpa range outputs Ira Weiny
2024-10-30 10:51 ` [PATCH v2 0/4] printf: Add struct range print specifier metux
2024-10-31 21:34   ` Ira Weiny
2024-11-07 14:43   ` Petr Mladek

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ZyzNuh9BJxQxihYv@pathway.suse.cz \
    --to=pmladek@suse.com \
    --cc=akpm@linux-foundation.org \
    --cc=alison.schofield@intel.com \
    --cc=andriy.shevchenko@linux.intel.com \
    --cc=bagasdotme@gmail.com \
    --cc=corbet@lwn.net \
    --cc=dan.j.williams@intel.com \
    --cc=dave.jiang@intel.com \
    --cc=dave@stgolabs.net \
    --cc=fan.ni@samsung.com \
    --cc=ira.weiny@intel.com \
    --cc=jonathan.cameron@huawei.com \
    --cc=linux-cxl@vger.kernel.org \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux@rasmusvillemoes.dk \
    --cc=rostedt@goodmis.org \
    --cc=senozhatsky@chromium.org \
    --cc=vishal.l.verma@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox