| Message ID | 20241018-cxl-pra-v1-3-7f49ba58208b@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | printf: Add struct range print specifier | expand |
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 >
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
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
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); } }
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(-)