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

[Ira Weiny <ira.weiny@intel.com>]

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