| Message ID | 167571662248.587790.4362747686454305108.stgit@djiang5-mobl3.local |
|---|---|
| State | Superseded |
| Headers | show |
| Series | cxl: Add support for QTG ID retrieval for CXL subsystem | expand |
On Mon, 06 Feb 2023 13:50:23 -0700 Dave Jiang <dave.jiang@intel.com> wrote: > Provide a callback to parse the Device Scoped Latency and Bandwidth > Information Structure (DSLBIS) in the CDAT structures. The DSLBIS > contains the bandwidth and latency information that's tied to a DSMAS > handle. The driver will retrieve the read and write latency and > bandwidth associated with the DSMAS which is tied to a DPA range. > > Signed-off-by: Dave Jiang <dave.jiang@intel.com> A few comments inline, Thanks, Jonathan > --- > drivers/cxl/core/cdat.c | 34 ++++++++++++++++++++++++++++++++++ > drivers/cxl/cxl.h | 2 ++ > drivers/cxl/port.c | 9 ++++++++- > include/acpi/actbl1.h | 5 +++++ > 4 files changed, 49 insertions(+), 1 deletion(-) > > diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c > index f9a64a0f1ee4..3c8f3956487e 100644 > --- a/drivers/cxl/core/cdat.c > +++ b/drivers/cxl/core/cdat.c > @@ -121,3 +121,37 @@ int cxl_dsmas_parse_entry(struct acpi_cdat_header *header, void *arg) > return 0; > } > EXPORT_SYMBOL_NS_GPL(cxl_dsmas_parse_entry, CXL); > + > +int cxl_dslbis_parse_entry(struct acpi_cdat_header *header, void *arg) > +{ > + struct cxl_port *port = (struct cxl_port *)arg; > + struct dsmas_entry *dent; > + struct acpi_cdat_dslbis *dslbis; Perhaps reorder to maintain the pretty upside-down Christmas trees (I don't care :) > + u64 val; > + > + if (header->type != ACPI_CDAT_TYPE_DSLBIS) > + return -EINVAL; Isn't this guaranteed by the caller? Seems overkill do it twice and I don't think these will ever be called outside of that wrapper that loops over the entries. I could be wrong though! > + > + dslbis = (struct acpi_cdat_dslbis *)((unsigned long)header + sizeof(*header)); header + 1 > + if ((dslbis->flags & ACPI_CEDT_DSLBIS_MEM_MASK) != This field 'must be ignored' if the DSMAS handle isn't a match (as it's an initiator only entry) Odd though it may seem I think we might see one of those on a type 3 device and we are probably going to have other users of this function anyway. I think you need to do the walk below to check we have a DSMAS match, before running this check. > + ACPI_CEDT_DSLBIS_MEM_MEMORY) > + return 0; > + > + if (dslbis->data_type > ACPI_HMAT_WRITE_BANDWIDTH) > + return -ENXIO; This would probably imply a new HMAT spec value, so probably just log it and ignore rather than error out. > + > + /* Value calculation with base_unit, see ACPI Spec 6.5 5.2.28.4 */ > + val = dslbis->entry[0] * dslbis->entry_base_unit; In theory this might overflow as u64 * u16. Doubt it will ever happen in reality, but maybe a check and debug print if it does? > + > + mutex_lock(&port->cdat.dsmas_lock); > + list_for_each_entry(dent, &port->cdat.dsmas_list, list) { > + if (dslbis->handle == dent->handle) { > + dent->qos[dslbis->data_type] = val; > + break; > + } > + } > + mutex_unlock(&port->cdat.dsmas_lock); > + > + return 0; > +} > +EXPORT_SYMBOL_NS_GPL(cxl_dslbis_parse_entry, CXL); > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h > index 1e5e69f08480..849b22236f1d 100644 > --- a/drivers/cxl/cxl.h > +++ b/drivers/cxl/cxl.h > @@ -705,6 +705,7 @@ struct dsmas_entry { > struct list_head list; > struct range dpa_range; > u16 handle; > + u64 qos[ACPI_HMAT_WRITE_BANDWIDTH + 1]; > }; > > typedef int (*cdat_tbl_entry_handler)(struct acpi_cdat_header *header, void *arg); > @@ -716,6 +717,7 @@ int cdat_table_parse_dslbis(void *table, cdat_tbl_entry_handler handler, > void *arg); > > int cxl_dsmas_parse_entry(struct acpi_cdat_header *header, void *arg); > +int cxl_dslbis_parse_entry(struct acpi_cdat_header *header, void *arg); > > /* > * Unit test builds overrides this to __weak, find the 'strong' version > diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c > index b1da73e99bab..8de311208b37 100644 > --- a/drivers/cxl/port.c > +++ b/drivers/cxl/port.c > @@ -65,8 +65,15 @@ static int cxl_port_probe(struct device *dev) > rc = cdat_table_parse_dsmas(port->cdat.table, > cxl_dsmas_parse_entry, > (void *)port); > - if (rc < 0) > + if (rc > 0) { > + rc = cdat_table_parse_dslbis(port->cdat.table, > + cxl_dslbis_parse_entry, > + (void *)port); > + if (rc <= 0) > + dev_dbg(dev, "Failed to parse DSLBIS: %d\n", rc); If we have entries and they won't parse, I think we should be screaming louder. dev_warn() would be my preference for this and the one in the previous patch. Sure we can carry on, but something on the device is not working as expected. > + } else { > dev_dbg(dev, "Failed to parse DSMAS: %d\n", rc); > + } > } > > rc = cxl_hdm_decode_init(cxlds, cxlhdm); > diff --git a/include/acpi/actbl1.h b/include/acpi/actbl1.h > index e8297cefde09..ff6092e45196 100644 > --- a/include/acpi/actbl1.h > +++ b/include/acpi/actbl1.h > @@ -369,6 +369,11 @@ struct acpi_cdat_dslbis { > u16 reserved2; > }; > > +/* Flags for subtable above */ > + > +#define ACPI_CEDT_DSLBIS_MEM_MASK GENMASK(3, 0) > +#define ACPI_CEDT_DSLBIS_MEM_MEMORY 0 > + > /* Subtable 2: Device Scoped Memory Side Cache Information Structure (DSMSCIS) */ > > struct acpi_cdat_dsmscis { > >
On 2/9/23 6:50 AM, Jonathan Cameron wrote: > On Mon, 06 Feb 2023 13:50:23 -0700 > Dave Jiang <dave.jiang@intel.com> wrote: > >> Provide a callback to parse the Device Scoped Latency and Bandwidth >> Information Structure (DSLBIS) in the CDAT structures. The DSLBIS >> contains the bandwidth and latency information that's tied to a DSMAS >> handle. The driver will retrieve the read and write latency and >> bandwidth associated with the DSMAS which is tied to a DPA range. >> >> Signed-off-by: Dave Jiang <dave.jiang@intel.com> > A few comments inline, > > Thanks, > > Jonathan > >> --- >> drivers/cxl/core/cdat.c | 34 ++++++++++++++++++++++++++++++++++ >> drivers/cxl/cxl.h | 2 ++ >> drivers/cxl/port.c | 9 ++++++++- >> include/acpi/actbl1.h | 5 +++++ >> 4 files changed, 49 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c >> index f9a64a0f1ee4..3c8f3956487e 100644 >> --- a/drivers/cxl/core/cdat.c >> +++ b/drivers/cxl/core/cdat.c >> @@ -121,3 +121,37 @@ int cxl_dsmas_parse_entry(struct acpi_cdat_header *header, void *arg) >> return 0; >> } >> EXPORT_SYMBOL_NS_GPL(cxl_dsmas_parse_entry, CXL); >> + >> +int cxl_dslbis_parse_entry(struct acpi_cdat_header *header, void *arg) >> +{ >> + struct cxl_port *port = (struct cxl_port *)arg; >> + struct dsmas_entry *dent; >> + struct acpi_cdat_dslbis *dslbis; > > Perhaps reorder to maintain the pretty upside-down Christmas trees > (I don't care :) will fix > >> + u64 val; >> + >> + if (header->type != ACPI_CDAT_TYPE_DSLBIS) >> + return -EINVAL; > > Isn't this guaranteed by the caller? Seems overkill do it twice > and I don't think these will ever be called outside of that wrapper that > loops over the entries. I could be wrong though! > ok will remove >> + >> + dslbis = (struct acpi_cdat_dslbis *)((unsigned long)header + sizeof(*header)); > header + 1 > >> + if ((dslbis->flags & ACPI_CEDT_DSLBIS_MEM_MASK) != > > This field 'must be ignored' if the DSMAS handle isn't a match > (as it's an initiator only entry) Odd though it may seem I think we > might see one of those on a type 3 device and we are probably going to > have other users of this function anyway. > > I think you need to do the walk below to check we have a DSMAS match, before > running this check. ok, will move down to where entry is matched > >> + ACPI_CEDT_DSLBIS_MEM_MEMORY) >> + return 0; >> + >> + if (dslbis->data_type > ACPI_HMAT_WRITE_BANDWIDTH) >> + return -ENXIO; > > This would probably imply a new HMAT spec value, so probably just > log it and ignore rather than error out. ok > >> + >> + /* Value calculation with base_unit, see ACPI Spec 6.5 5.2.28.4 */ >> + val = dslbis->entry[0] * dslbis->entry_base_unit; > > In theory this might overflow as u64 * u16. > Doubt it will ever happen in reality, but maybe a check and debug print if it does? ok will use check_mul_overflow() > >> + >> + mutex_lock(&port->cdat.dsmas_lock); >> + list_for_each_entry(dent, &port->cdat.dsmas_list, list) { >> + if (dslbis->handle == dent->handle) { >> + dent->qos[dslbis->data_type] = val; >> + break; >> + } >> + } >> + mutex_unlock(&port->cdat.dsmas_lock); >> + >> + return 0; >> +} >> +EXPORT_SYMBOL_NS_GPL(cxl_dslbis_parse_entry, CXL); >> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h >> index 1e5e69f08480..849b22236f1d 100644 >> --- a/drivers/cxl/cxl.h >> +++ b/drivers/cxl/cxl.h >> @@ -705,6 +705,7 @@ struct dsmas_entry { >> struct list_head list; >> struct range dpa_range; >> u16 handle; >> + u64 qos[ACPI_HMAT_WRITE_BANDWIDTH + 1]; >> }; >> >> typedef int (*cdat_tbl_entry_handler)(struct acpi_cdat_header *header, void *arg); >> @@ -716,6 +717,7 @@ int cdat_table_parse_dslbis(void *table, cdat_tbl_entry_handler handler, >> void *arg); >> >> int cxl_dsmas_parse_entry(struct acpi_cdat_header *header, void *arg); >> +int cxl_dslbis_parse_entry(struct acpi_cdat_header *header, void *arg); >> >> /* >> * Unit test builds overrides this to __weak, find the 'strong' version >> diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c >> index b1da73e99bab..8de311208b37 100644 >> --- a/drivers/cxl/port.c >> +++ b/drivers/cxl/port.c >> @@ -65,8 +65,15 @@ static int cxl_port_probe(struct device *dev) >> rc = cdat_table_parse_dsmas(port->cdat.table, >> cxl_dsmas_parse_entry, >> (void *)port); >> - if (rc < 0) >> + if (rc > 0) { >> + rc = cdat_table_parse_dslbis(port->cdat.table, >> + cxl_dslbis_parse_entry, >> + (void *)port); >> + if (rc <= 0) >> + dev_dbg(dev, "Failed to parse DSLBIS: %d\n", rc); > > If we have entries and they won't parse, I think we should be screaming louder. > dev_warn() would be my preference for this and the one in the previous patch. > Sure we can carry on, but something on the device is not working as expected. ok will fix this one and previous. > >> + } else { >> dev_dbg(dev, "Failed to parse DSMAS: %d\n", rc); >> + } >> } >> >> rc = cxl_hdm_decode_init(cxlds, cxlhdm); >> diff --git a/include/acpi/actbl1.h b/include/acpi/actbl1.h >> index e8297cefde09..ff6092e45196 100644 >> --- a/include/acpi/actbl1.h >> +++ b/include/acpi/actbl1.h >> @@ -369,6 +369,11 @@ struct acpi_cdat_dslbis { >> u16 reserved2; >> }; >> >> +/* Flags for subtable above */ >> + >> +#define ACPI_CEDT_DSLBIS_MEM_MASK GENMASK(3, 0) >> +#define ACPI_CEDT_DSLBIS_MEM_MEMORY 0 >> + >> /* Subtable 2: Device Scoped Memory Side Cache Information Structure (DSMSCIS) */ >> >> struct acpi_cdat_dsmscis { >> >> >
diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c index f9a64a0f1ee4..3c8f3956487e 100644 --- a/drivers/cxl/core/cdat.c +++ b/drivers/cxl/core/cdat.c @@ -121,3 +121,37 @@ int cxl_dsmas_parse_entry(struct acpi_cdat_header *header, void *arg) return 0; } EXPORT_SYMBOL_NS_GPL(cxl_dsmas_parse_entry, CXL); + +int cxl_dslbis_parse_entry(struct acpi_cdat_header *header, void *arg) +{ + struct cxl_port *port = (struct cxl_port *)arg; + struct dsmas_entry *dent; + struct acpi_cdat_dslbis *dslbis; + u64 val; + + if (header->type != ACPI_CDAT_TYPE_DSLBIS) + return -EINVAL; + + dslbis = (struct acpi_cdat_dslbis *)((unsigned long)header + sizeof(*header)); + if ((dslbis->flags & ACPI_CEDT_DSLBIS_MEM_MASK) != + ACPI_CEDT_DSLBIS_MEM_MEMORY) + return 0; + + if (dslbis->data_type > ACPI_HMAT_WRITE_BANDWIDTH) + return -ENXIO; + + /* Value calculation with base_unit, see ACPI Spec 6.5 5.2.28.4 */ + val = dslbis->entry[0] * dslbis->entry_base_unit; + + mutex_lock(&port->cdat.dsmas_lock); + list_for_each_entry(dent, &port->cdat.dsmas_list, list) { + if (dslbis->handle == dent->handle) { + dent->qos[dslbis->data_type] = val; + break; + } + } + mutex_unlock(&port->cdat.dsmas_lock); + + return 0; +} +EXPORT_SYMBOL_NS_GPL(cxl_dslbis_parse_entry, CXL); diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h index 1e5e69f08480..849b22236f1d 100644 --- a/drivers/cxl/cxl.h +++ b/drivers/cxl/cxl.h @@ -705,6 +705,7 @@ struct dsmas_entry { struct list_head list; struct range dpa_range; u16 handle; + u64 qos[ACPI_HMAT_WRITE_BANDWIDTH + 1]; }; typedef int (*cdat_tbl_entry_handler)(struct acpi_cdat_header *header, void *arg); @@ -716,6 +717,7 @@ int cdat_table_parse_dslbis(void *table, cdat_tbl_entry_handler handler, void *arg); int cxl_dsmas_parse_entry(struct acpi_cdat_header *header, void *arg); +int cxl_dslbis_parse_entry(struct acpi_cdat_header *header, void *arg); /* * Unit test builds overrides this to __weak, find the 'strong' version diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c index b1da73e99bab..8de311208b37 100644 --- a/drivers/cxl/port.c +++ b/drivers/cxl/port.c @@ -65,8 +65,15 @@ static int cxl_port_probe(struct device *dev) rc = cdat_table_parse_dsmas(port->cdat.table, cxl_dsmas_parse_entry, (void *)port); - if (rc < 0) + if (rc > 0) { + rc = cdat_table_parse_dslbis(port->cdat.table, + cxl_dslbis_parse_entry, + (void *)port); + if (rc <= 0) + dev_dbg(dev, "Failed to parse DSLBIS: %d\n", rc); + } else { dev_dbg(dev, "Failed to parse DSMAS: %d\n", rc); + } } rc = cxl_hdm_decode_init(cxlds, cxlhdm); diff --git a/include/acpi/actbl1.h b/include/acpi/actbl1.h index e8297cefde09..ff6092e45196 100644 --- a/include/acpi/actbl1.h +++ b/include/acpi/actbl1.h @@ -369,6 +369,11 @@ struct acpi_cdat_dslbis { u16 reserved2; }; +/* Flags for subtable above */ + +#define ACPI_CEDT_DSLBIS_MEM_MASK GENMASK(3, 0) +#define ACPI_CEDT_DSLBIS_MEM_MEMORY 0 + /* Subtable 2: Device Scoped Memory Side Cache Information Structure (DSMSCIS) */ struct acpi_cdat_dsmscis {
Provide a callback to parse the Device Scoped Latency and Bandwidth Information Structure (DSLBIS) in the CDAT structures. The DSLBIS contains the bandwidth and latency information that's tied to a DSMAS handle. The driver will retrieve the read and write latency and bandwidth associated with the DSMAS which is tied to a DPA range. Signed-off-by: Dave Jiang <dave.jiang@intel.com> --- drivers/cxl/core/cdat.c | 34 ++++++++++++++++++++++++++++++++++ drivers/cxl/cxl.h | 2 ++ drivers/cxl/port.c | 9 ++++++++- include/acpi/actbl1.h | 5 +++++ 4 files changed, 49 insertions(+), 1 deletion(-)