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] [day] [month] [year] [list]

Message-ID: <ZxmtiOlYNklu4sKr@aschofie-mobl2.lan>
Date: Wed, 23 Oct 2024 19:14:32 -0700
From: Alison Schofield <alison.schofield@...el.com>
To: 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

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

Thanks for the info. LGTM!

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

> 
> Ira