diff mbox series

[RFC,03/15] Revert "cxl/core: Convert decoder range to resource"

Message ID 20220413183720.2444089-4-ben.widawsky@intel.com (mailing list archive)
State New
Headers show
Series Region driver | expand

Commit Message

Ben Widawsky April 13, 2022, 6:37 p.m. UTC
This reverts commit 608135db1b790170d22848815c4671407af74e37. All
decoders do have a host physical address space and the revert allows us
to keep that uniformity. Decoder disambiguation will allow for decoder
type-specific members which is needed, but will be handled separately.

Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>

---
The explanation for why it is impossible to make CFMWS ranges be
iomem_resources is explain in a later patch.
---
 drivers/cxl/acpi.c      | 17 ++++++++++-------
 drivers/cxl/core/hdm.c  |  2 +-
 drivers/cxl/core/port.c | 28 ++++++----------------------
 drivers/cxl/cxl.h       |  8 ++------
 4 files changed, 19 insertions(+), 36 deletions(-)

Comments

Dan Williams April 13, 2022, 9:43 p.m. UTC | #1
On Wed, Apr 13, 2022 at 11:38 AM Ben Widawsky <ben.widawsky@intel.com> wrote:
>
> This reverts commit 608135db1b790170d22848815c4671407af74e37. All

Did checkpatch not complain about this being in "commit
<12-character-commit-id> <commit summary format>"? However, I'd rather
just drop the revert language and say:

Change root decoders to reuse the existing ->range field to track the
decoder's programmed HPA range. The infrastructure to track the
allocations out of the root decoder range is still a work-in-progress,
but in the meantime it simplifies the code to always represent the
current decoder range setting in the ->range field regardless of
decoder type.

> decoders do have a host physical address space and the revert allows us
> to keep that uniformity. Decoder disambiguation will allow for decoder
> type-specific members which is needed, but will be handled separately.
>
> Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
>
> ---
> The explanation for why it is impossible to make CFMWS ranges be
> iomem_resources is explain in a later patch.

This change stands alone / is independent of any iomem_resource concerns, right?

> ---
>  drivers/cxl/acpi.c      | 17 ++++++++++-------
>  drivers/cxl/core/hdm.c  |  2 +-
>  drivers/cxl/core/port.c | 28 ++++++----------------------
>  drivers/cxl/cxl.h       |  8 ++------
>  4 files changed, 19 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index d15a6aec0331..9b69955b90cb 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -108,8 +108,10 @@ 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->platform_res = (struct resource)DEFINE_RES_MEM(cfmws->base_hpa,
> -                                                            cfmws->window_size);
> +       cxld->range = (struct range){
> +               .start = cfmws->base_hpa,
> +               .end = cfmws->base_hpa + cfmws->window_size - 1,
> +       };
>         cxld->interleave_ways = CFMWS_INTERLEAVE_WAYS(cfmws);
>         cxld->interleave_granularity = CFMWS_INTERLEAVE_GRANULARITY(cfmws);
>
> @@ -119,13 +121,14 @@ static int cxl_parse_cfmws(union acpi_subtable_headers *header, void *arg,
>         else
>                 rc = cxl_decoder_autoremove(dev, cxld);
>         if (rc) {
> -               dev_err(dev, "Failed to add decoder for %pr\n",
> -                       &cxld->platform_res);
> +               dev_err(dev, "Failed to add decoder for %#llx-%#llx\n",
> +                       cfmws->base_hpa,
> +                       cfmws->base_hpa + cfmws->window_size - 1);
>                 return 0;
>         }
> -       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);
> +       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);
>
>         return 0;
>  }
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index c3c021b54079..3055e246aab9 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -172,7 +172,7 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>                 return -ENXIO;
>         }
>
> -       cxld->decoder_range = (struct range) {
> +       cxld->range = (struct range) {
>                 .start = base,
>                 .end = base + size - 1,
>         };
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index 74c8e47bf915..86f451ecb7ed 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -73,14 +73,8 @@ static ssize_t start_show(struct device *dev, struct device_attribute *attr,
>                           char *buf)
>  {
>         struct cxl_decoder *cxld = to_cxl_decoder(dev);
> -       u64 start;
>
> -       if (is_root_decoder(dev))
> -               start = cxld->platform_res.start;
> -       else
> -               start = cxld->decoder_range.start;
> -
> -       return sysfs_emit(buf, "%#llx\n", start);
> +       return sysfs_emit(buf, "%#llx\n", cxld->range.start);
>  }
>  static DEVICE_ATTR_ADMIN_RO(start);
>
> @@ -88,14 +82,8 @@ static ssize_t size_show(struct device *dev, struct device_attribute *attr,
>                         char *buf)
>  {
>         struct cxl_decoder *cxld = to_cxl_decoder(dev);
> -       u64 size;
>
> -       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);
> +       return sysfs_emit(buf, "%#llx\n", range_len(&cxld->range));
>  }
>  static DEVICE_ATTR_RO(size);
>
> @@ -1228,7 +1216,10 @@ static struct cxl_decoder *cxl_decoder_alloc(struct cxl_port *port,
>         cxld->interleave_ways = 1;
>         cxld->interleave_granularity = PAGE_SIZE;
>         cxld->target_type = CXL_DECODER_EXPANDER;
> -       cxld->platform_res = (struct resource)DEFINE_RES_MEM(0, 0);
> +       cxld->range = (struct range) {
> +               .start = 0,
> +               .end = -1,
> +       };
>
>         return cxld;
>  err:
> @@ -1342,13 +1333,6 @@ int cxl_decoder_add_locked(struct cxl_decoder *cxld, int *target_map)
>         if (rc)
>                 return rc;
>
> -       /*
> -        * Platform decoder resources should show up with a reasonable name. All
> -        * other resources are just sub ranges within the main decoder resource.
> -        */
> -       if (is_root_decoder(dev))
> -               cxld->platform_res.name = dev_name(dev);
> -
>         return device_add(dev);
>  }
>  EXPORT_SYMBOL_NS_GPL(cxl_decoder_add_locked, CXL);
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 5102491e8d13..6517d5cdf5ee 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -197,8 +197,7 @@ enum cxl_decoder_type {
>   * struct cxl_decoder - CXL address range decode configuration
>   * @dev: this decoder's device
>   * @id: kernel device name id
> - * @platform_res: address space resources considered by root decoder
> - * @decoder_range: address space resources considered by midlevel decoder
> + * @range: address range considered by this decoder
>   * @interleave_ways: number of cxl_dports in this decode
>   * @interleave_granularity: data stride per dport
>   * @target_type: accelerator vs expander (type2 vs type3) selector
> @@ -210,10 +209,7 @@ enum cxl_decoder_type {
>  struct cxl_decoder {
>         struct device dev;
>         int id;
> -       union {
> -               struct resource platform_res;
> -               struct range decoder_range;
> -       };
> +       struct range range;
>         int interleave_ways;
>         int interleave_granularity;
>         enum cxl_decoder_type target_type;
> --
> 2.35.1
>
Ben Widawsky May 12, 2022, 4:09 p.m. UTC | #2
On 22-04-13 14:43:48, Dan Williams wrote:
> On Wed, Apr 13, 2022 at 11:38 AM Ben Widawsky <ben.widawsky@intel.com> wrote:
> >
> > This reverts commit 608135db1b790170d22848815c4671407af74e37. All
> 
> Did checkpatch not complain about this being in "commit
> <12-character-commit-id> <commit summary format>"? However, I'd rather
> just drop the revert language and say:
> 
> Change root decoders to reuse the existing ->range field to track the
> decoder's programmed HPA range. The infrastructure to track the
> allocations out of the root decoder range is still a work-in-progress,
> but in the meantime it simplifies the code to always represent the
> current decoder range setting in the ->range field regardless of
> decoder type.
> 
> > decoders do have a host physical address space and the revert allows us
> > to keep that uniformity. Decoder disambiguation will allow for decoder
> > type-specific members which is needed, but will be handled separately.
> >
> > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> >
> > ---
> > The explanation for why it is impossible to make CFMWS ranges be
> > iomem_resources is explain in a later patch.
> 
> This change stands alone / is independent of any iomem_resource concerns, right?
> 

I think I need to revisit this per the discussion at LSFMM. Ideally a CFMWS
resource would just be insert_resource()'d, but that won't work. I'm going to
attempt what we discussed and this patch will likely go away.

> > ---
> >  drivers/cxl/acpi.c      | 17 ++++++++++-------
> >  drivers/cxl/core/hdm.c  |  2 +-
> >  drivers/cxl/core/port.c | 28 ++++++----------------------
> >  drivers/cxl/cxl.h       |  8 ++------
> >  4 files changed, 19 insertions(+), 36 deletions(-)
> >
> > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> > index d15a6aec0331..9b69955b90cb 100644
> > --- a/drivers/cxl/acpi.c
> > +++ b/drivers/cxl/acpi.c
> > @@ -108,8 +108,10 @@ 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->platform_res = (struct resource)DEFINE_RES_MEM(cfmws->base_hpa,
> > -                                                            cfmws->window_size);
> > +       cxld->range = (struct range){
> > +               .start = cfmws->base_hpa,
> > +               .end = cfmws->base_hpa + cfmws->window_size - 1,
> > +       };
> >         cxld->interleave_ways = CFMWS_INTERLEAVE_WAYS(cfmws);
> >         cxld->interleave_granularity = CFMWS_INTERLEAVE_GRANULARITY(cfmws);
> >
> > @@ -119,13 +121,14 @@ static int cxl_parse_cfmws(union acpi_subtable_headers *header, void *arg,
> >         else
> >                 rc = cxl_decoder_autoremove(dev, cxld);
> >         if (rc) {
> > -               dev_err(dev, "Failed to add decoder for %pr\n",
> > -                       &cxld->platform_res);
> > +               dev_err(dev, "Failed to add decoder for %#llx-%#llx\n",
> > +                       cfmws->base_hpa,
> > +                       cfmws->base_hpa + cfmws->window_size - 1);
> >                 return 0;
> >         }
> > -       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);
> > +       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);
> >
> >         return 0;
> >  }
> > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> > index c3c021b54079..3055e246aab9 100644
> > --- a/drivers/cxl/core/hdm.c
> > +++ b/drivers/cxl/core/hdm.c
> > @@ -172,7 +172,7 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
> >                 return -ENXIO;
> >         }
> >
> > -       cxld->decoder_range = (struct range) {
> > +       cxld->range = (struct range) {
> >                 .start = base,
> >                 .end = base + size - 1,
> >         };
> > diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> > index 74c8e47bf915..86f451ecb7ed 100644
> > --- a/drivers/cxl/core/port.c
> > +++ b/drivers/cxl/core/port.c
> > @@ -73,14 +73,8 @@ static ssize_t start_show(struct device *dev, struct device_attribute *attr,
> >                           char *buf)
> >  {
> >         struct cxl_decoder *cxld = to_cxl_decoder(dev);
> > -       u64 start;
> >
> > -       if (is_root_decoder(dev))
> > -               start = cxld->platform_res.start;
> > -       else
> > -               start = cxld->decoder_range.start;
> > -
> > -       return sysfs_emit(buf, "%#llx\n", start);
> > +       return sysfs_emit(buf, "%#llx\n", cxld->range.start);
> >  }
> >  static DEVICE_ATTR_ADMIN_RO(start);
> >
> > @@ -88,14 +82,8 @@ static ssize_t size_show(struct device *dev, struct device_attribute *attr,
> >                         char *buf)
> >  {
> >         struct cxl_decoder *cxld = to_cxl_decoder(dev);
> > -       u64 size;
> >
> > -       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);
> > +       return sysfs_emit(buf, "%#llx\n", range_len(&cxld->range));
> >  }
> >  static DEVICE_ATTR_RO(size);
> >
> > @@ -1228,7 +1216,10 @@ static struct cxl_decoder *cxl_decoder_alloc(struct cxl_port *port,
> >         cxld->interleave_ways = 1;
> >         cxld->interleave_granularity = PAGE_SIZE;
> >         cxld->target_type = CXL_DECODER_EXPANDER;
> > -       cxld->platform_res = (struct resource)DEFINE_RES_MEM(0, 0);
> > +       cxld->range = (struct range) {
> > +               .start = 0,
> > +               .end = -1,
> > +       };
> >
> >         return cxld;
> >  err:
> > @@ -1342,13 +1333,6 @@ int cxl_decoder_add_locked(struct cxl_decoder *cxld, int *target_map)
> >         if (rc)
> >                 return rc;
> >
> > -       /*
> > -        * Platform decoder resources should show up with a reasonable name. All
> > -        * other resources are just sub ranges within the main decoder resource.
> > -        */
> > -       if (is_root_decoder(dev))
> > -               cxld->platform_res.name = dev_name(dev);
> > -
> >         return device_add(dev);
> >  }
> >  EXPORT_SYMBOL_NS_GPL(cxl_decoder_add_locked, CXL);
> > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> > index 5102491e8d13..6517d5cdf5ee 100644
> > --- a/drivers/cxl/cxl.h
> > +++ b/drivers/cxl/cxl.h
> > @@ -197,8 +197,7 @@ enum cxl_decoder_type {
> >   * struct cxl_decoder - CXL address range decode configuration
> >   * @dev: this decoder's device
> >   * @id: kernel device name id
> > - * @platform_res: address space resources considered by root decoder
> > - * @decoder_range: address space resources considered by midlevel decoder
> > + * @range: address range considered by this decoder
> >   * @interleave_ways: number of cxl_dports in this decode
> >   * @interleave_granularity: data stride per dport
> >   * @target_type: accelerator vs expander (type2 vs type3) selector
> > @@ -210,10 +209,7 @@ enum cxl_decoder_type {
> >  struct cxl_decoder {
> >         struct device dev;
> >         int id;
> > -       union {
> > -               struct resource platform_res;
> > -               struct range decoder_range;
> > -       };
> > +       struct range range;
> >         int interleave_ways;
> >         int interleave_granularity;
> >         enum cxl_decoder_type target_type;
> > --
> > 2.35.1
> >
diff mbox series

Patch

diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
index d15a6aec0331..9b69955b90cb 100644
--- a/drivers/cxl/acpi.c
+++ b/drivers/cxl/acpi.c
@@ -108,8 +108,10 @@  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->platform_res = (struct resource)DEFINE_RES_MEM(cfmws->base_hpa,
-							     cfmws->window_size);
+	cxld->range = (struct range){
+		.start = cfmws->base_hpa,
+		.end = cfmws->base_hpa + cfmws->window_size - 1,
+	};
 	cxld->interleave_ways = CFMWS_INTERLEAVE_WAYS(cfmws);
 	cxld->interleave_granularity = CFMWS_INTERLEAVE_GRANULARITY(cfmws);
 
@@ -119,13 +121,14 @@  static int cxl_parse_cfmws(union acpi_subtable_headers *header, void *arg,
 	else
 		rc = cxl_decoder_autoremove(dev, cxld);
 	if (rc) {
-		dev_err(dev, "Failed to add decoder for %pr\n",
-			&cxld->platform_res);
+		dev_err(dev, "Failed to add decoder for %#llx-%#llx\n",
+			cfmws->base_hpa,
+			cfmws->base_hpa + cfmws->window_size - 1);
 		return 0;
 	}
-	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);
+	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);
 
 	return 0;
 }
diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
index c3c021b54079..3055e246aab9 100644
--- a/drivers/cxl/core/hdm.c
+++ b/drivers/cxl/core/hdm.c
@@ -172,7 +172,7 @@  static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
 		return -ENXIO;
 	}
 
-	cxld->decoder_range = (struct range) {
+	cxld->range = (struct range) {
 		.start = base,
 		.end = base + size - 1,
 	};
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index 74c8e47bf915..86f451ecb7ed 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -73,14 +73,8 @@  static ssize_t start_show(struct device *dev, struct device_attribute *attr,
 			  char *buf)
 {
 	struct cxl_decoder *cxld = to_cxl_decoder(dev);
-	u64 start;
 
-	if (is_root_decoder(dev))
-		start = cxld->platform_res.start;
-	else
-		start = cxld->decoder_range.start;
-
-	return sysfs_emit(buf, "%#llx\n", start);
+	return sysfs_emit(buf, "%#llx\n", cxld->range.start);
 }
 static DEVICE_ATTR_ADMIN_RO(start);
 
@@ -88,14 +82,8 @@  static ssize_t size_show(struct device *dev, struct device_attribute *attr,
 			char *buf)
 {
 	struct cxl_decoder *cxld = to_cxl_decoder(dev);
-	u64 size;
 
-	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);
+	return sysfs_emit(buf, "%#llx\n", range_len(&cxld->range));
 }
 static DEVICE_ATTR_RO(size);
 
@@ -1228,7 +1216,10 @@  static struct cxl_decoder *cxl_decoder_alloc(struct cxl_port *port,
 	cxld->interleave_ways = 1;
 	cxld->interleave_granularity = PAGE_SIZE;
 	cxld->target_type = CXL_DECODER_EXPANDER;
-	cxld->platform_res = (struct resource)DEFINE_RES_MEM(0, 0);
+	cxld->range = (struct range) {
+		.start = 0,
+		.end = -1,
+	};
 
 	return cxld;
 err:
@@ -1342,13 +1333,6 @@  int cxl_decoder_add_locked(struct cxl_decoder *cxld, int *target_map)
 	if (rc)
 		return rc;
 
-	/*
-	 * Platform decoder resources should show up with a reasonable name. All
-	 * other resources are just sub ranges within the main decoder resource.
-	 */
-	if (is_root_decoder(dev))
-		cxld->platform_res.name = dev_name(dev);
-
 	return device_add(dev);
 }
 EXPORT_SYMBOL_NS_GPL(cxl_decoder_add_locked, CXL);
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 5102491e8d13..6517d5cdf5ee 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -197,8 +197,7 @@  enum cxl_decoder_type {
  * struct cxl_decoder - CXL address range decode configuration
  * @dev: this decoder's device
  * @id: kernel device name id
- * @platform_res: address space resources considered by root decoder
- * @decoder_range: address space resources considered by midlevel decoder
+ * @range: address range considered by this decoder
  * @interleave_ways: number of cxl_dports in this decode
  * @interleave_granularity: data stride per dport
  * @target_type: accelerator vs expander (type2 vs type3) selector
@@ -210,10 +209,7 @@  enum cxl_decoder_type {
 struct cxl_decoder {
 	struct device dev;
 	int id;
-	union {
-		struct resource platform_res;
-		struct range decoder_range;
-	};
+	struct range range;
 	int interleave_ways;
 	int interleave_granularity;
 	enum cxl_decoder_type target_type;