Re: [PATCH 10/23] cxl/core: Convert decoder range to resource

[Dan Williams <dan.j.williams@intel.com>]

On Fri, Nov 19, 2021 at 4:03 PM Ben Widawsky <ben.widawsky@intel.com> wrote:
>
> CXL decoders manage address ranges in a hierarchical fashion whereby a
> leaf is a unique subregion of its parent decoder (midlevel or root). It
> therefore makes sense to use the resource API for handling this.
>
> Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
>
> ---
> Changes since RFCv2
> - Switch to DEFINE_RES_MEM from NAMED variant (Dan)
> - Differentiate CFMWS resources and other decoder resources (Ben)
> - Make decoder resources be range, nor resource (Dan)
> - Set decoder name in cxl_decoder_add() (Dan)
> ---
>  drivers/cxl/acpi.c     | 16 ++++++----------
>  drivers/cxl/core/bus.c | 19 +++++++++++++++++--
>  drivers/cxl/cxl.h      |  8 ++++++--
>  3 files changed, 29 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index 7cfa8b568013..3415184a2e61 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -108,10 +108,8 @@ static int cxl_parse_cfmws(union acpi_subtable_headers *header, void *arg,
>
>         cxld->flags = cfmws_to_decoder_flags(cfmws->restrictions);
>         cxld->target_type = CXL_DECODER_EXPANDER;
> -       cxld->range = (struct range){
> -               .start = cfmws->base_hpa,
> -               .end = cfmws->base_hpa + cfmws->window_size - 1,
> -       };
> +       cxld->platform_res = (struct resource)DEFINE_RES_MEM(cfmws->base_hpa,
> +                                                            cfmws->window_size);
>         cxld->interleave_ways = CFMWS_INTERLEAVE_WAYS(cfmws);
>         cxld->interleave_granularity = CFMWS_INTERLEAVE_GRANULARITY(cfmws);
>
> @@ -127,8 +125,9 @@ static int cxl_parse_cfmws(union acpi_subtable_headers *header, void *arg,
>                 return 0;
>         }
>         dev_dbg(dev, "add: %s node: %d range %#llx-%#llx\n",
> -               dev_name(&cxld->dev), phys_to_target_node(cxld->range.start),
> -               cfmws->base_hpa, cfmws->base_hpa + cfmws->window_size - 1);
> +               dev_name(&cxld->dev),
> +               phys_to_target_node(cxld->platform_res.start), cfmws->base_hpa,
> +               cfmws->base_hpa + cfmws->window_size - 1);

Since you converted to resource you can us %pr:

        dev_dbg(dev, "add: %s node: %d range %pr\n", dev_name(&cxld->dev),
                phys_to_target_node(cxld->platform_res.start),
                &cxld->platform_res);


>
>         return 0;
>  }
> @@ -267,10 +266,7 @@ static int add_host_bridge_uport(struct device *match, void *arg)
>         cxld->interleave_ways = 1;
>         cxld->interleave_granularity = PAGE_SIZE;
>         cxld->target_type = CXL_DECODER_EXPANDER;
> -       cxld->range = (struct range) {
> -               .start = 0,
> -               .end = -1,
> -       };
> +       cxld->platform_res = (struct resource)DEFINE_RES_MEM(0, 0);
>
>         device_lock(&port->dev);
>         dport = list_first_entry(&port->dports, typeof(*dport), list);
> diff --git a/drivers/cxl/core/bus.c b/drivers/cxl/core/bus.c
> index 17a4fff029f8..8e80e85350b1 100644
> --- a/drivers/cxl/core/bus.c
> +++ b/drivers/cxl/core/bus.c
> @@ -46,8 +46,14 @@ static ssize_t start_show(struct device *dev, struct device_attribute *attr,
>                           char *buf)
>  {
>         struct cxl_decoder *cxld = to_cxl_decoder(dev);
> +       u64 start = 0;

No need to init to zero.

>
> -       return sysfs_emit(buf, "%#llx\n", cxld->range.start);
> +       if (is_root_decoder(dev))
> +               start = cxld->platform_res.start;
> +       else
> +               start = cxld->decoder_range.start;
> +
> +       return sysfs_emit(buf, "%#llx\n", start);
>  }
>  static DEVICE_ATTR_RO(start);
>
> @@ -55,8 +61,14 @@ static ssize_t size_show(struct device *dev, struct device_attribute *attr,
>                         char *buf)
>  {
>         struct cxl_decoder *cxld = to_cxl_decoder(dev);
> +       u64 size = 0;

Same "no init" comment.

>
> -       return sysfs_emit(buf, "%#llx\n", range_len(&cxld->range));
> +       if (is_root_decoder(dev))
> +               size = resource_size(&cxld->platform_res);
> +       else
> +               size = range_len(&cxld->decoder_range);
> +
> +       return sysfs_emit(buf, "%#llx\n", size);
>  }
>  static DEVICE_ATTR_RO(size);
>
> @@ -548,6 +560,9 @@ int cxl_decoder_add(struct cxl_decoder *cxld, int *target_map)
>         if (rc)
>                 return rc;
>
> +       if (is_root_decoder(dev))
> +               cxld->platform_res.name = dev_name(dev);

Maybe a comment about why the resource wants the name of the decoder?
Just to help explain the motivation to separate this initialization
step from the rest of the resource init.


Other than that you can add:

Reviewed-by: Dan Williams <dan.j.williams@intel.com>