Re: [PATCH v2 1/4] cxl/core: Rename cxl_trace_hpa() to cxl_translate()

[Alison Schofield <alison.schofield@intel.com>]

On Fri, Jun 07, 2024 at 03:40:25PM +0100, Jonathan Cameron wrote:
> On Wed,  8 May 2024 11:47:50 -0700
> alison.schofield@intel.com wrote:
> 
> > From: Alison Schofield <alison.schofield@intel.com>
> > 
> > Although cxl_trace_hpa() is used to populate TRACE EVENTs with HPA
> > addresses, the work it performs is a translation (dpa->hpa), not a
> > trace. Rename it. Since this is the only translate work in CXL,
> > drop the _hpa suffix in the rename.
> > 
> > Suggested-by: Dan Williams <dan.j.williams@intel.com>
> > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> 
> Maybe makes some sense to keep the direction in the name?
> 
> I can see for things like PPR we may need to go HPA to DPA as
> it'll be based on error counts that may be reported in HPA.
> 
> So Friday bikeshedding time. 
> cxl_dpa_to_hpa()?

Thanks for the review Jonathan!

Let's sync on the possible dpa->hpa->dpa use cases and then I'll
weigh in on your bikeshedding.

Agree we need HPA->DPA support for userspace. User needs ability
to find which device maps an HPA.

wrt PPR, do you think that is an in kernel need?
If the PPR maintenance was done in response to a CXL event, the DPA
is already in the event trace along with the HPA so I wouldn't expect
that a lookup is needed, at least not within the kernel.

Your explicit cxl_dpa_to_hpa() name is good.

--Alison



> 
> 
> > ---
> >  drivers/cxl/core/core.h   | 4 ++--
> >  drivers/cxl/core/mbox.c   | 2 +-
> >  drivers/cxl/core/region.c | 2 +-
> >  drivers/cxl/core/trace.h  | 2 +-
> >  4 files changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
> > index 625394486459..8ceebd3b51d2 100644
> > --- a/drivers/cxl/core/core.h
> > +++ b/drivers/cxl/core/core.h
> > @@ -28,12 +28,12 @@ int cxl_region_init(void);
> >  void cxl_region_exit(void);
> >  int cxl_get_poison_by_endpoint(struct cxl_port *port);
> >  struct cxl_region *cxl_dpa_to_region(const struct cxl_memdev *cxlmd, u64 dpa);
> > -u64 cxl_trace_hpa(struct cxl_region *cxlr, const struct cxl_memdev *cxlmd,
> > +u64 cxl_translate(struct cxl_region *cxlr, const struct cxl_memdev *cxlmd,
> >  		  u64 dpa);
> >  
> >  #else
> >  static inline u64
> > -cxl_trace_hpa(struct cxl_region *cxlr, const struct cxl_memdev *cxlmd, u64 dpa)
> > +cxl_translate(struct cxl_region *cxlr, const struct cxl_memdev *cxlmd, u64 dpa)
> >  {
> >  	return ULLONG_MAX;
> >  }
> > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> > index 2626f3fff201..edc54a1ca298 100644
> > --- a/drivers/cxl/core/mbox.c
> > +++ b/drivers/cxl/core/mbox.c
> > @@ -878,7 +878,7 @@ void cxl_event_trace_record(const struct cxl_memdev *cxlmd,
> >  		dpa = le64_to_cpu(evt->common.phys_addr) & CXL_DPA_MASK;
> >  		cxlr = cxl_dpa_to_region(cxlmd, dpa);
> >  		if (cxlr)
> > -			hpa = cxl_trace_hpa(cxlr, cxlmd, dpa);
> > +			hpa = cxl_translate(cxlr, cxlmd, dpa);
> >  
> >  		if (event_type == CXL_CPER_EVENT_GEN_MEDIA)
> >  			trace_cxl_general_media(cxlmd, type, cxlr, hpa,
> > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> > index 00a9f0eef8dd..245edf748906 100644
> > --- a/drivers/cxl/core/region.c
> > +++ b/drivers/cxl/core/region.c
> > @@ -2797,7 +2797,7 @@ static u64 cxl_dpa_to_hpa(u64 dpa,  struct cxl_region *cxlr,
> >  	return hpa;
> >  }
> >  
> > -u64 cxl_trace_hpa(struct cxl_region *cxlr, const struct cxl_memdev *cxlmd,
> > +u64 cxl_translate(struct cxl_region *cxlr, const struct cxl_memdev *cxlmd,
> >  		  u64 dpa)
> >  {
> >  	struct cxl_region_params *p = &cxlr->params;
> > diff --git a/drivers/cxl/core/trace.h b/drivers/cxl/core/trace.h
> > index 07a0394b1d99..6925a6a31f01 100644
> > --- a/drivers/cxl/core/trace.h
> > +++ b/drivers/cxl/core/trace.h
> > @@ -704,7 +704,7 @@ TRACE_EVENT(cxl_poison,
> >  		if (cxlr) {
> >  			__assign_str(region, dev_name(&cxlr->dev));
> >  			memcpy(__entry->uuid, &cxlr->params.uuid, 16);
> > -			__entry->hpa = cxl_trace_hpa(cxlr, cxlmd,
> > +			__entry->hpa = cxl_translate(cxlr, cxlmd,
> >  						     __entry->dpa);
> >  		} else {
> >  			__assign_str(region, "");
>