On 2/9/23 6:50 AM, Jonathan Cameron wrote:
On Mon, 06 Feb 2023 13:50:23 -0700
Dave Jiang <dave.jiang@xxxxxxxxx> 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@xxxxxxxxx>
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 {
