| Message ID | 165603875016.551046.17236943065932132355.stgit@dwillia2-xfh |
|---|---|
| State | New |
| Headers | show |
| Series | CXL PMEM Region Provisioning | expand |
On Thu, 23 Jun 2022 19:45:50 -0700 Dan Williams <dan.j.williams@intel.com> wrote: > Interleave granularity and ways have CXL specification defined encodings. > Promote the conversion helpers to a common header, and use them to > replace other open-coded instances. > > Force caller to consider the error case of the conversion as well. What was the reasoning behind not just returning the value (rather than the extra *val parameter)? Negative values would be errors still. Plenty of room to do that in an int. I don't really mind, just feels a tiny bit uglier than it could be. Also, there is one little unrelated type change in here. > > Co-developed-by: Ben Widawsky <bwidawsk@kernel.org> > Signed-off-by: Ben Widawsky <bwidawsk@kernel.org> > Signed-off-by: Dan Williams <dan.j.williams@intel.com> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > drivers/cxl/acpi.c | 34 +++++++++++++++++++--------------- > drivers/cxl/core/hdm.c | 35 +++++++++-------------------------- > drivers/cxl/cxl.h | 26 ++++++++++++++++++++++++++ > 3 files changed, 54 insertions(+), 41 deletions(-) > > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c > index 951695cdb455..544cb10ce33e 100644 > --- a/drivers/cxl/acpi.c > +++ b/drivers/cxl/acpi.c > @@ -9,10 +9,6 @@ > #include "cxlpci.h" > #include "cxl.h" > > -/* Encode defined in CXL 2.0 8.2.5.12.7 HDM Decoder Control Register */ > -#define CFMWS_INTERLEAVE_WAYS(x) (1 << (x)->interleave_ways) > -#define CFMWS_INTERLEAVE_GRANULARITY(x) ((x)->granularity + 8) > - > static unsigned long cfmws_to_decoder_flags(int restrictions) > { > unsigned long flags = CXL_DECODER_F_ENABLE; > @@ -34,7 +30,8 @@ static unsigned long cfmws_to_decoder_flags(int restrictions) > static int cxl_acpi_cfmws_verify(struct device *dev, > struct acpi_cedt_cfmws *cfmws) > { > - int expected_len; > + unsigned int expected_len, ways; Type change for expected_len seems fine but isn't mentioned in the patch description. > + int rc; > > if (cfmws->interleave_arithmetic != ACPI_CEDT_CFMWS_ARITHMETIC_MODULO) { > dev_err(dev, "CFMWS Unsupported Interleave Arithmetic\n"); > @@ -51,14 +48,14 @@ static int cxl_acpi_cfmws_verify(struct device *dev, > return -EINVAL; > } > > - if (CFMWS_INTERLEAVE_WAYS(cfmws) > CXL_DECODER_MAX_INTERLEAVE) { > - dev_err(dev, "CFMWS Interleave Ways (%d) too large\n", > - CFMWS_INTERLEAVE_WAYS(cfmws)); > + rc = cxl_to_ways(cfmws->interleave_ways, &ways); > + if (rc) { > + dev_err(dev, "CFMWS Interleave Ways (%d) invalid\n", > + cfmws->interleave_ways); > return -EINVAL; > } > > - expected_len = struct_size((cfmws), interleave_targets, > - CFMWS_INTERLEAVE_WAYS(cfmws)); > + expected_len = struct_size(cfmws, interleave_targets, ways); > > if (cfmws->header.length < expected_len) { > dev_err(dev, "CFMWS length %d less than expected %d\n", > @@ -87,7 +84,8 @@ static int cxl_parse_cfmws(union acpi_subtable_headers *header, void *arg, > struct device *dev = ctx->dev; > struct acpi_cedt_cfmws *cfmws; > struct cxl_decoder *cxld; > - int rc, i; > + unsigned int ways, i, ig; > + int rc; > > cfmws = (struct acpi_cedt_cfmws *) header; > > @@ -99,10 +97,16 @@ static int cxl_parse_cfmws(union acpi_subtable_headers *header, void *arg, > return 0; > } > > - for (i = 0; i < CFMWS_INTERLEAVE_WAYS(cfmws); i++) > + rc = cxl_to_ways(cfmws->interleave_ways, &ways); > + if (rc) > + return rc; > + rc = cxl_to_granularity(cfmws->granularity, &ig); > + if (rc) > + return rc; > + for (i = 0; i < ways; i++) > target_map[i] = cfmws->interleave_targets[i]; > > - cxld = cxl_root_decoder_alloc(root_port, CFMWS_INTERLEAVE_WAYS(cfmws)); > + cxld = cxl_root_decoder_alloc(root_port, ways); > if (IS_ERR(cxld)) > return 0; > > @@ -112,8 +116,8 @@ static int cxl_parse_cfmws(union acpi_subtable_headers *header, void *arg, > .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); > + cxld->interleave_ways = ways; > + cxld->interleave_granularity = ig; > > rc = cxl_decoder_add(cxld, target_map); > if (rc) > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c > index 5c070c93b07f..46635105a1f1 100644 > --- a/drivers/cxl/core/hdm.c > +++ b/drivers/cxl/core/hdm.c > @@ -128,33 +128,12 @@ struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port) > } > EXPORT_SYMBOL_NS_GPL(devm_cxl_setup_hdm, CXL); > > -static int to_interleave_granularity(u32 ctrl) > -{ > - int val = FIELD_GET(CXL_HDM_DECODER0_CTRL_IG_MASK, ctrl); > - > - return 256 << val; > -} > - > -static int to_interleave_ways(u32 ctrl) > -{ > - int val = FIELD_GET(CXL_HDM_DECODER0_CTRL_IW_MASK, ctrl); > - > - switch (val) { > - case 0 ... 4: > - return 1 << val; > - case 8 ... 10: > - return 3 << (val - 8); > - default: > - return 0; > - } > -} > - > static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld, > int *target_map, void __iomem *hdm, int which) > { > u64 size, base; > + int i, rc; > u32 ctrl; > - int i; > union { > u64 value; > unsigned char target_id[8]; > @@ -183,14 +162,18 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld, > if (ctrl & CXL_HDM_DECODER0_CTRL_LOCK) > cxld->flags |= CXL_DECODER_F_LOCK; > } > - cxld->interleave_ways = to_interleave_ways(ctrl); > - if (!cxld->interleave_ways) { > + rc = cxl_to_ways(FIELD_GET(CXL_HDM_DECODER0_CTRL_IW_MASK, ctrl), > + &cxld->interleave_ways); > + if (rc) { > dev_warn(&port->dev, > "decoder%d.%d: Invalid interleave ways (ctrl: %#x)\n", > port->id, cxld->id, ctrl); > - return -ENXIO; > + return rc; > } > - cxld->interleave_granularity = to_interleave_granularity(ctrl); > + rc = cxl_to_granularity(FIELD_GET(CXL_HDM_DECODER0_CTRL_IG_MASK, ctrl), > + &cxld->interleave_granularity); > + if (rc) > + return rc; > > if (FIELD_GET(CXL_HDM_DECODER0_CTRL_TYPE, ctrl)) > cxld->target_type = CXL_DECODER_EXPANDER; > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h > index 6e08fe8cc0fe..fd02f9e2a829 100644 > --- a/drivers/cxl/cxl.h > +++ b/drivers/cxl/cxl.h > @@ -64,6 +64,32 @@ static inline int cxl_hdm_decoder_count(u32 cap_hdr) > return val ? val * 2 : 1; > } > > +/* Encode defined in CXL 2.0 8.2.5.12.7 HDM Decoder Control Register */ > +static inline int cxl_to_granularity(u16 ig, unsigned int *val) > +{ > + if (ig > 6) > + return -EINVAL; > + *val = 256 << ig; > + return 0; > +} > + > +/* Encode defined in CXL ECN "3, 6, 12 and 16-way memory Interleaving" */ > +static inline int cxl_to_ways(u8 eniw, unsigned int *val) > +{ > + switch (eniw) { > + case 0 ... 4: > + *val = 1 << eniw; > + break; > + case 8 ... 10: > + *val = 3 << (eniw - 8); > + break; > + default: > + return -EINVAL; > + } > + > + return 0; > +} > + > /* CXL 2.0 8.2.8.1 Device Capabilities Array Register */ > #define CXLDEV_CAP_ARRAY_OFFSET 0x0 > #define CXLDEV_CAP_ARRAY_CAP_ID 0 >
Jonathan Cameron wrote: > On Thu, 23 Jun 2022 19:45:50 -0700 > Dan Williams <dan.j.williams@intel.com> wrote: > > > Interleave granularity and ways have CXL specification defined encodings. > > Promote the conversion helpers to a common header, and use them to > > replace other open-coded instances. > > > > Force caller to consider the error case of the conversion as well. > > What was the reasoning behind not just returning the value (rather > than the extra *val parameter)? Negative values would be errors > still. Plenty of room to do that in an int. > > I don't really mind, just feels a tiny bit uglier than it could be. The rationale was to make it symmetric with reverse translation to encoded values where those encode helpers are used directly for sysfs input validation like the kstrto*() helpers. Added a note to that effect. > > Also, there is one little unrelated type change in here. > > > > > Co-developed-by: Ben Widawsky <bwidawsk@kernel.org> > > Signed-off-by: Ben Widawsky <bwidawsk@kernel.org> > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > --- > > drivers/cxl/acpi.c | 34 +++++++++++++++++++--------------- > > drivers/cxl/core/hdm.c | 35 +++++++++-------------------------- > > drivers/cxl/cxl.h | 26 ++++++++++++++++++++++++++ > > 3 files changed, 54 insertions(+), 41 deletions(-) > > > > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c > > index 951695cdb455..544cb10ce33e 100644 > > --- a/drivers/cxl/acpi.c > > +++ b/drivers/cxl/acpi.c > > @@ -9,10 +9,6 @@ > > #include "cxlpci.h" > > #include "cxl.h" > > > > -/* Encode defined in CXL 2.0 8.2.5.12.7 HDM Decoder Control Register */ > > -#define CFMWS_INTERLEAVE_WAYS(x) (1 << (x)->interleave_ways) > > -#define CFMWS_INTERLEAVE_GRANULARITY(x) ((x)->granularity + 8) > > - > > static unsigned long cfmws_to_decoder_flags(int restrictions) > > { > > unsigned long flags = CXL_DECODER_F_ENABLE; > > @@ -34,7 +30,8 @@ static unsigned long cfmws_to_decoder_flags(int restrictions) > > static int cxl_acpi_cfmws_verify(struct device *dev, > > struct acpi_cedt_cfmws *cfmws) > > { > > - int expected_len; > > + unsigned int expected_len, ways; > > Type change for expected_len seems fine but isn't mentioned in the patch description. Yeah, that seems a thoughtless change to me since only @ways needs to be an unsigned int. I fixed it up.
diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c index 951695cdb455..544cb10ce33e 100644 --- a/drivers/cxl/acpi.c +++ b/drivers/cxl/acpi.c @@ -9,10 +9,6 @@ #include "cxlpci.h" #include "cxl.h" -/* Encode defined in CXL 2.0 8.2.5.12.7 HDM Decoder Control Register */ -#define CFMWS_INTERLEAVE_WAYS(x) (1 << (x)->interleave_ways) -#define CFMWS_INTERLEAVE_GRANULARITY(x) ((x)->granularity + 8) - static unsigned long cfmws_to_decoder_flags(int restrictions) { unsigned long flags = CXL_DECODER_F_ENABLE; @@ -34,7 +30,8 @@ static unsigned long cfmws_to_decoder_flags(int restrictions) static int cxl_acpi_cfmws_verify(struct device *dev, struct acpi_cedt_cfmws *cfmws) { - int expected_len; + unsigned int expected_len, ways; + int rc; if (cfmws->interleave_arithmetic != ACPI_CEDT_CFMWS_ARITHMETIC_MODULO) { dev_err(dev, "CFMWS Unsupported Interleave Arithmetic\n"); @@ -51,14 +48,14 @@ static int cxl_acpi_cfmws_verify(struct device *dev, return -EINVAL; } - if (CFMWS_INTERLEAVE_WAYS(cfmws) > CXL_DECODER_MAX_INTERLEAVE) { - dev_err(dev, "CFMWS Interleave Ways (%d) too large\n", - CFMWS_INTERLEAVE_WAYS(cfmws)); + rc = cxl_to_ways(cfmws->interleave_ways, &ways); + if (rc) { + dev_err(dev, "CFMWS Interleave Ways (%d) invalid\n", + cfmws->interleave_ways); return -EINVAL; } - expected_len = struct_size((cfmws), interleave_targets, - CFMWS_INTERLEAVE_WAYS(cfmws)); + expected_len = struct_size(cfmws, interleave_targets, ways); if (cfmws->header.length < expected_len) { dev_err(dev, "CFMWS length %d less than expected %d\n", @@ -87,7 +84,8 @@ static int cxl_parse_cfmws(union acpi_subtable_headers *header, void *arg, struct device *dev = ctx->dev; struct acpi_cedt_cfmws *cfmws; struct cxl_decoder *cxld; - int rc, i; + unsigned int ways, i, ig; + int rc; cfmws = (struct acpi_cedt_cfmws *) header; @@ -99,10 +97,16 @@ static int cxl_parse_cfmws(union acpi_subtable_headers *header, void *arg, return 0; } - for (i = 0; i < CFMWS_INTERLEAVE_WAYS(cfmws); i++) + rc = cxl_to_ways(cfmws->interleave_ways, &ways); + if (rc) + return rc; + rc = cxl_to_granularity(cfmws->granularity, &ig); + if (rc) + return rc; + for (i = 0; i < ways; i++) target_map[i] = cfmws->interleave_targets[i]; - cxld = cxl_root_decoder_alloc(root_port, CFMWS_INTERLEAVE_WAYS(cfmws)); + cxld = cxl_root_decoder_alloc(root_port, ways); if (IS_ERR(cxld)) return 0; @@ -112,8 +116,8 @@ static int cxl_parse_cfmws(union acpi_subtable_headers *header, void *arg, .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); + cxld->interleave_ways = ways; + cxld->interleave_granularity = ig; rc = cxl_decoder_add(cxld, target_map); if (rc) diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c index 5c070c93b07f..46635105a1f1 100644 --- a/drivers/cxl/core/hdm.c +++ b/drivers/cxl/core/hdm.c @@ -128,33 +128,12 @@ struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port) } EXPORT_SYMBOL_NS_GPL(devm_cxl_setup_hdm, CXL); -static int to_interleave_granularity(u32 ctrl) -{ - int val = FIELD_GET(CXL_HDM_DECODER0_CTRL_IG_MASK, ctrl); - - return 256 << val; -} - -static int to_interleave_ways(u32 ctrl) -{ - int val = FIELD_GET(CXL_HDM_DECODER0_CTRL_IW_MASK, ctrl); - - switch (val) { - case 0 ... 4: - return 1 << val; - case 8 ... 10: - return 3 << (val - 8); - default: - return 0; - } -} - static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld, int *target_map, void __iomem *hdm, int which) { u64 size, base; + int i, rc; u32 ctrl; - int i; union { u64 value; unsigned char target_id[8]; @@ -183,14 +162,18 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld, if (ctrl & CXL_HDM_DECODER0_CTRL_LOCK) cxld->flags |= CXL_DECODER_F_LOCK; } - cxld->interleave_ways = to_interleave_ways(ctrl); - if (!cxld->interleave_ways) { + rc = cxl_to_ways(FIELD_GET(CXL_HDM_DECODER0_CTRL_IW_MASK, ctrl), + &cxld->interleave_ways); + if (rc) { dev_warn(&port->dev, "decoder%d.%d: Invalid interleave ways (ctrl: %#x)\n", port->id, cxld->id, ctrl); - return -ENXIO; + return rc; } - cxld->interleave_granularity = to_interleave_granularity(ctrl); + rc = cxl_to_granularity(FIELD_GET(CXL_HDM_DECODER0_CTRL_IG_MASK, ctrl), + &cxld->interleave_granularity); + if (rc) + return rc; if (FIELD_GET(CXL_HDM_DECODER0_CTRL_TYPE, ctrl)) cxld->target_type = CXL_DECODER_EXPANDER; diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h index 6e08fe8cc0fe..fd02f9e2a829 100644 --- a/drivers/cxl/cxl.h +++ b/drivers/cxl/cxl.h @@ -64,6 +64,32 @@ static inline int cxl_hdm_decoder_count(u32 cap_hdr) return val ? val * 2 : 1; } +/* Encode defined in CXL 2.0 8.2.5.12.7 HDM Decoder Control Register */ +static inline int cxl_to_granularity(u16 ig, unsigned int *val) +{ + if (ig > 6) + return -EINVAL; + *val = 256 << ig; + return 0; +} + +/* Encode defined in CXL ECN "3, 6, 12 and 16-way memory Interleaving" */ +static inline int cxl_to_ways(u8 eniw, unsigned int *val) +{ + switch (eniw) { + case 0 ... 4: + *val = 1 << eniw; + break; + case 8 ... 10: + *val = 3 << (eniw - 8); + break; + default: + return -EINVAL; + } + + return 0; +} + /* CXL 2.0 8.2.8.1 Device Capabilities Array Register */ #define CXLDEV_CAP_ARRAY_OFFSET 0x0 #define CXLDEV_CAP_ARRAY_CAP_ID 0