Re: [PATCH 3/4] cxl/core: Add region info to cxl_general_media and cxl_dram events

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Wed, 27 Mar 2024 21:36:32 -0700
alison.schofield@intel.com wrote:

> From: Alison Schofield <alison.schofield@intel.com>
> 
> User space may need to know which region, if any, maps the DPAs
> (device physical addresses) reported in a cxl_general_media or
> cxl_dram event. Since the mapping can change, the kernel provides
> this information at the time the event occurs. This informs user
> space that at event <timestamp> this <region> mapped this <DPA>
> to this <HPA>.
> 
> Add the same region info that is included in the cxl_poison trace
> event: the DPA->HPA translation, region name, and region uuid.
> Introduce and use new helpers that lookup that region info using
> the struct cxl_memdev and a DPA.
> 
> The new fields are inserted in the trace event and no existing
> fields are modified. If the DPA is not mapped, user will see:
> hpa=ULLONG_MAX, region="", and uuid=0
> 
> This work must be protected by dpa_rwsem & region_rwsem since
> it is looking up region mappings.
> 
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
One query one the stub.. Otherwise all lgtm
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/cxl/core/core.h   |  6 +++++
>  drivers/cxl/core/mbox.c   | 17 ++++++++++----
>  drivers/cxl/core/region.c |  8 +++++++
>  drivers/cxl/core/trace.h  | 47 ++++++++++++++++++++++++++++++++++-----
>  4 files changed, 69 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
> index 24454a1cb250..848ef6904beb 100644
> --- a/drivers/cxl/core/core.h
> +++ b/drivers/cxl/core/core.h
> @@ -30,8 +30,14 @@ 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 dpa);
> +const char *cxl_trace_to_region_name(const struct cxl_memdev *cxlmd, u64 dpa);
>  
>  #else
> +static inline
> +const char *cxl_trace_to_region_name(const struct cxl_memdev *cxlmd, u64 dpa)
> +{
> +	return NULL;

Is this ever going to be a problem for the reason you have commented below?

Maybe 
return ""
?

...

> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 45eb9c560fd6..a5b1eaee1e58 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -2723,6 +2723,14 @@ struct cxl_region *cxl_dpa_to_region(const struct cxl_memdev *cxlmd, u64 dpa)
>  	return ctx.cxlr;
>  }
>  
> +const char *cxl_trace_to_region_name(const struct cxl_memdev *cxlmd, u64 dpa)
> +{
> +	struct cxl_region *cxlr = cxl_dpa_to_region(cxlmd, dpa);
> +
> +	/* trace __string() assignment requires "", not NULL */
> +	return cxlr ? dev_name(&cxlr->dev) : "";
> +}
> +