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 :)
> + 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 {
>
>
