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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <67193e446b625_d944929428@iweiny-mobl.notmuch>
Date: Wed, 23 Oct 2024 13:19:48 -0500
From: Ira Weiny <ira.weiny@...el.com>
To: Alison Schofield <alison.schofield@...el.com>, Ira Weiny
	<ira.weiny@...el.com>
CC: Andrew Morton <akpm@...ux-foundation.org>, Petr Mladek <pmladek@...e.com>,
	Steven Rostedt <rostedt@...dmis.org>, Andy Shevchenko
	<andriy.shevchenko@...ux.intel.com>, Rasmus Villemoes
	<linux@...musvillemoes.dk>, Sergey Senozhatsky <senozhatsky@...omium.org>,
	Jonathan Corbet <corbet@....net>, Davidlohr Bueso <dave@...olabs.net>,
	Jonathan Cameron <jonathan.cameron@...wei.com>, Dave Jiang
	<dave.jiang@...el.com>, Vishal Verma <vishal.l.verma@...el.com>, Dan Williams
	<dan.j.williams@...el.com>, Fan Ni <fan.ni@...sung.com>, Bagas Sanjaya
	<bagasdotme@...il.com>, <linux-kernel@...r.kernel.org>,
	<linux-doc@...r.kernel.org>, <linux-cxl@...r.kernel.org>
Subject: Re: [PATCH 3/3] cxl/cdat: Use %pra for dpa range outputs

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@...el.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