Message ID | 168357883158.2756219.14426990857899261700.stgit@djiang5-mobl3 |
---|---|
State | Superseded |
Headers | show |
Series | cxl: Add support for QTG ID retrieval for CXL subsystem | expand |
On Mon, 08 May 2023 13:47:11 -0700 Dave Jiang <dave.jiang@intel.com> wrote: > Provide a callback to parse the Switched Scoped Latency and Bandwidth > Information Structure (SSLBIS) in the CDAT structures. The SSLBIS > contains the bandwidth and latency information that's tied to the > CXL switch that the data table has been read from. The extracted > values are stored to the cxl_dport correlated by the port_id > depending on the SSLBIS entry. > > Coherent Device Attribute Table 1.03 2.1 Switched Scoped Latency > and Bandwidth Information Structure (DSLBIS) > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> I'm disagreeing with myself again. Some comments inline based on a fresh read. > Signed-off-by: Dave Jiang <dave.jiang@intel.com> > > --- > v5: > - Store data to cxl_dport directly instead. (Dan) > - Use acpi_table_parse_cdat(). > v3: > - Add spec section in commit header (Alison) > - Move CDAT parse to cxl_switch_port_probe() > - Use 'struct node_hmem_attrs' > --- > drivers/cxl/core/cdat.c | 88 +++++++++++++++++++++++++++++++++++++++++++++++ > drivers/cxl/cxl.h | 8 ++++ > drivers/cxl/port.c | 12 ++++++ > include/acpi/actbl1.h | 3 ++ > 4 files changed, 110 insertions(+), 1 deletion(-) > > diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c > index 6e14d04c0453..37b135c900d5 100644 > --- a/drivers/cxl/core/cdat.c > +++ b/drivers/cxl/core/cdat.c > @@ -124,3 +124,91 @@ int cxl_cdat_endpoint_process(struct cxl_port *port, struct list_head *list) > return rc; > } > EXPORT_SYMBOL_NS_GPL(cxl_cdat_endpoint_process, CXL); > + > +static int cdat_sslbis_handler(union acpi_subtable_headers *header, void *arg, > + const unsigned long end) > +{ > + struct acpi_cdat_sslbis *sslbis = (struct acpi_cdat_sslbis *)header; > + struct acpi_cdat_sslbe *entry; > + struct cxl_port *port = arg; > + struct device *dev = &port->dev; > + int remain, entries, i; > + u16 len; > + > + len = le16_to_cpu((__force __le16)sslbis->header.length); > + remain = len - sizeof(*sslbis); > + if (!remain || remain % sizeof(*entry) || > + (unsigned long)header + len > end) { > + dev_warn(dev, "Malformed SSLBIS table length: (%u)\n", len); > + return -EINVAL; > + } > + > + /* Unrecognized data type, we can skip */ > + if (sslbis->data_type > ACPI_HMAT_WRITE_BANDWIDTH) > + return 0; > + > + entries = remain / sizeof(*entry); > + entry = (struct acpi_cdat_sslbe *)((unsigned long)header + sizeof(*sslbis)); That's a little nasty to follow. Perhaps. entry = (struct acpi_cdat_sslbe *)(sslbis + 1); Bonus points if we can just use a variable length entry on the end of struct acpi_cdat_sslbis. Might break some other locations but would make this one nicer. > + > + for (i = 0; i < entries; i++) { > + u16 x = le16_to_cpu(entry->portx_id); > + u16 y = le16_to_cpu(entry->porty_id); > + struct cxl_dport *dport; > + unsigned long index; > + u16 dsp_id; > + u64 val; > + > + switch (x) { > + case ACPI_CDAT_SSLBIS_US_PORT: > + dsp_id = y; > + break; > + case ACPI_CDAT_SSLBIS_ANY_PORT: > + switch (y) { > + case ACPI_CDAT_SSLBIS_US_PORT: > + dsp_id = x; > + break; > + case ACPI_CDAT_SSLBIS_ANY_PORT: > + dsp_id = ACPI_CDAT_SSLBIS_ANY_PORT; > + break; > + default: > + dsp_id = y; > + break; > + } > + break; > + default: > + dsp_id = x; > + break; > + } > + > + if (check_mul_overflow(le64_to_cpu(sslbis->entry_base_unit), > + le16_to_cpu(entry->latency_or_bandwidth), > + &val)) > + dev_warn(dev, "SSLBIS value overflowed!\n"); > + > + xa_for_each(&port->dports, index, dport) { > + if (dsp_id == ACPI_CDAT_SSLBIS_ANY_PORT || > + dsp_id == dport->port_id) > + cxl_access_coordinate_set(&dport->coord, > + sslbis->data_type, > + val); > + } > + > + entry++; > + } > + > + return 0; > +} > + > +int cxl_cdat_switch_process(struct cxl_port *port) > +{ > + int rc; > + > + rc = acpi_table_parse_cdat(ACPI_CDAT_TYPE_SSLBIS, > + cdat_sslbis_handler, > + port, port->cdat.table); > + if (rc == 0) > + rc = -ENOENT; I'm bored of this pattern now so hide it... Howabout acpi_table_parse_cdat_fail_emtpy() that handles the rc rewrite internally. (with a better name). Gets rid of all this boilerplate. > + > + return rc; > +} > +EXPORT_SYMBOL_NS_GPL(cxl_cdat_switch_process, CXL); > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h > index ca3d0d74f2e5..3e8020e0a132 100644 > --- a/drivers/cxl/cxl.h > +++ b/drivers/cxl/cxl.h > @@ -600,6 +600,7 @@ cxl_find_dport_by_dev(struct cxl_port *port, const struct device *dport_dev) > * @rcrb: base address for the Root Complex Register Block > * @rch: Indicate whether this dport was enumerated in RCH or VH mode > * @port: reference to cxl_port that contains this downstream port > + * @coord: access coordinates (performance) for switch from CDAT > */ > struct cxl_dport { > struct device *dport; > @@ -608,6 +609,7 @@ struct cxl_dport { > resource_size_t rcrb; > bool rch; > struct cxl_port *port; > + struct access_coordinate coord; > }; > > /** > @@ -803,12 +805,18 @@ struct dsmas_entry { > > #ifdef CONFIG_ACPI > int cxl_cdat_endpoint_process(struct cxl_port *port, struct list_head *list); > +int cxl_cdat_switch_process(struct cxl_port *port); > #else > static inline int cxl_cdat_endpoint_process(struct cxl_port *port, > struct list_head *list) > { > return -EOPNOTSUPP; > } > + > +static inline int cxl_cdat_switch_process(struct cxl_port *port) > +{ > + return -EOPNOTSUPP; > +} > #endif > > /* > diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c > index da023feaa6e2..c5a24b75bf03 100644 > --- a/drivers/cxl/port.c > +++ b/drivers/cxl/port.c > @@ -86,7 +86,17 @@ static int cxl_switch_port_probe(struct cxl_port *port) > if (IS_ERR(cxlhdm)) > return PTR_ERR(cxlhdm); > > - return devm_cxl_enumerate_decoders(cxlhdm, NULL); > + rc = devm_cxl_enumerate_decoders(cxlhdm, NULL); > + if (rc < 0) > + return rc; > + > + if (port->cdat.table) { > + rc = cxl_cdat_switch_process(port); > + if (rc < 0) > + dev_warn(&port->dev, "Failed to parse SSLBIS: %d\n", rc); > + } > + > + return 0; > } > > static int cxl_endpoint_port_probe(struct cxl_port *port) > diff --git a/include/acpi/actbl1.h b/include/acpi/actbl1.h > index 8ea7e5d64bc1..82def138a7e4 100644 > --- a/include/acpi/actbl1.h > +++ b/include/acpi/actbl1.h > @@ -419,6 +419,9 @@ struct acpi_cdat_sslbis { > u64 entry_base_unit; > }; > > +#define ACPI_CDAT_SSLBIS_US_PORT 0x0100 > +#define ACPI_CDAT_SSLBIS_ANY_PORT 0xffff > + > /* Sub-subtable for above, sslbe_entries field */ > > struct acpi_cdat_sslbe { > >
On 5/12/23 7:41 AM, Jonathan Cameron wrote: > On Mon, 08 May 2023 13:47:11 -0700 > Dave Jiang <dave.jiang@intel.com> wrote: > >> Provide a callback to parse the Switched Scoped Latency and Bandwidth >> Information Structure (SSLBIS) in the CDAT structures. The SSLBIS >> contains the bandwidth and latency information that's tied to the >> CXL switch that the data table has been read from. The extracted >> values are stored to the cxl_dport correlated by the port_id >> depending on the SSLBIS entry. >> >> Coherent Device Attribute Table 1.03 2.1 Switched Scoped Latency >> and Bandwidth Information Structure (DSLBIS) >> >> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > I'm disagreeing with myself again. Some comments inline based > on a fresh read. > >> Signed-off-by: Dave Jiang <dave.jiang@intel.com> >> >> --- >> v5: >> - Store data to cxl_dport directly instead. (Dan) >> - Use acpi_table_parse_cdat(). >> v3: >> - Add spec section in commit header (Alison) >> - Move CDAT parse to cxl_switch_port_probe() >> - Use 'struct node_hmem_attrs' >> --- >> drivers/cxl/core/cdat.c | 88 +++++++++++++++++++++++++++++++++++++++++++++++ >> drivers/cxl/cxl.h | 8 ++++ >> drivers/cxl/port.c | 12 ++++++ >> include/acpi/actbl1.h | 3 ++ >> 4 files changed, 110 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c >> index 6e14d04c0453..37b135c900d5 100644 >> --- a/drivers/cxl/core/cdat.c >> +++ b/drivers/cxl/core/cdat.c >> @@ -124,3 +124,91 @@ int cxl_cdat_endpoint_process(struct cxl_port *port, struct list_head *list) >> return rc; >> } >> EXPORT_SYMBOL_NS_GPL(cxl_cdat_endpoint_process, CXL); >> + >> +static int cdat_sslbis_handler(union acpi_subtable_headers *header, void *arg, >> + const unsigned long end) >> +{ >> + struct acpi_cdat_sslbis *sslbis = (struct acpi_cdat_sslbis *)header; >> + struct acpi_cdat_sslbe *entry; >> + struct cxl_port *port = arg; >> + struct device *dev = &port->dev; >> + int remain, entries, i; >> + u16 len; >> + >> + len = le16_to_cpu((__force __le16)sslbis->header.length); >> + remain = len - sizeof(*sslbis); >> + if (!remain || remain % sizeof(*entry) || >> + (unsigned long)header + len > end) { >> + dev_warn(dev, "Malformed SSLBIS table length: (%u)\n", len); >> + return -EINVAL; >> + } >> + >> + /* Unrecognized data type, we can skip */ >> + if (sslbis->data_type > ACPI_HMAT_WRITE_BANDWIDTH) >> + return 0; >> + >> + entries = remain / sizeof(*entry); >> + entry = (struct acpi_cdat_sslbe *)((unsigned long)header + sizeof(*sslbis)); > > That's a little nasty to follow. Perhaps. > > entry = (struct acpi_cdat_sslbe *)(sslbis + 1); Ok. I'll also update previous 2 patches with similar usages. > > Bonus points if we can just use a variable length entry on the end of > struct acpi_cdat_sslbis. Might break some other locations but would make > this one nicer. I had that when I was using local data structures for CDAT tables. But changing the ACPI version results in touching a lot of ACPICA code unfortunately. > >> + >> + for (i = 0; i < entries; i++) { >> + u16 x = le16_to_cpu(entry->portx_id); >> + u16 y = le16_to_cpu(entry->porty_id); >> + struct cxl_dport *dport; >> + unsigned long index; >> + u16 dsp_id; >> + u64 val; >> + >> + switch (x) { >> + case ACPI_CDAT_SSLBIS_US_PORT: >> + dsp_id = y; >> + break; >> + case ACPI_CDAT_SSLBIS_ANY_PORT: >> + switch (y) { >> + case ACPI_CDAT_SSLBIS_US_PORT: >> + dsp_id = x; >> + break; >> + case ACPI_CDAT_SSLBIS_ANY_PORT: >> + dsp_id = ACPI_CDAT_SSLBIS_ANY_PORT; >> + break; >> + default: >> + dsp_id = y; >> + break; >> + } >> + break; >> + default: >> + dsp_id = x; >> + break; >> + } >> + >> + if (check_mul_overflow(le64_to_cpu(sslbis->entry_base_unit), >> + le16_to_cpu(entry->latency_or_bandwidth), >> + &val)) >> + dev_warn(dev, "SSLBIS value overflowed!\n"); >> + >> + xa_for_each(&port->dports, index, dport) { >> + if (dsp_id == ACPI_CDAT_SSLBIS_ANY_PORT || >> + dsp_id == dport->port_id) >> + cxl_access_coordinate_set(&dport->coord, >> + sslbis->data_type, >> + val); >> + } >> + >> + entry++; >> + } >> + >> + return 0; >> +} >> + >> +int cxl_cdat_switch_process(struct cxl_port *port) >> +{ >> + int rc; >> + >> + rc = acpi_table_parse_cdat(ACPI_CDAT_TYPE_SSLBIS, >> + cdat_sslbis_handler, >> + port, port->cdat.table); >> + if (rc == 0) >> + rc = -ENOENT; > > I'm bored of this pattern now so hide it... > > Howabout > > acpi_table_parse_cdat_fail_emtpy() that handles the rc > rewrite internally. (with a better name). > > Gets rid of all this boilerplate. ok > > > >> + >> + return rc; >> +} >> +EXPORT_SYMBOL_NS_GPL(cxl_cdat_switch_process, CXL); >> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h >> index ca3d0d74f2e5..3e8020e0a132 100644 >> --- a/drivers/cxl/cxl.h >> +++ b/drivers/cxl/cxl.h >> @@ -600,6 +600,7 @@ cxl_find_dport_by_dev(struct cxl_port *port, const struct device *dport_dev) >> * @rcrb: base address for the Root Complex Register Block >> * @rch: Indicate whether this dport was enumerated in RCH or VH mode >> * @port: reference to cxl_port that contains this downstream port >> + * @coord: access coordinates (performance) for switch from CDAT >> */ >> struct cxl_dport { >> struct device *dport; >> @@ -608,6 +609,7 @@ struct cxl_dport { >> resource_size_t rcrb; >> bool rch; >> struct cxl_port *port; >> + struct access_coordinate coord; >> }; >> >> /** >> @@ -803,12 +805,18 @@ struct dsmas_entry { >> >> #ifdef CONFIG_ACPI >> int cxl_cdat_endpoint_process(struct cxl_port *port, struct list_head *list); >> +int cxl_cdat_switch_process(struct cxl_port *port); >> #else >> static inline int cxl_cdat_endpoint_process(struct cxl_port *port, >> struct list_head *list) >> { >> return -EOPNOTSUPP; >> } >> + >> +static inline int cxl_cdat_switch_process(struct cxl_port *port) >> +{ >> + return -EOPNOTSUPP; >> +} >> #endif >> >> /* >> diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c >> index da023feaa6e2..c5a24b75bf03 100644 >> --- a/drivers/cxl/port.c >> +++ b/drivers/cxl/port.c >> @@ -86,7 +86,17 @@ static int cxl_switch_port_probe(struct cxl_port *port) >> if (IS_ERR(cxlhdm)) >> return PTR_ERR(cxlhdm); >> >> - return devm_cxl_enumerate_decoders(cxlhdm, NULL); >> + rc = devm_cxl_enumerate_decoders(cxlhdm, NULL); >> + if (rc < 0) >> + return rc; >> + >> + if (port->cdat.table) { >> + rc = cxl_cdat_switch_process(port); >> + if (rc < 0) >> + dev_warn(&port->dev, "Failed to parse SSLBIS: %d\n", rc); >> + } >> + >> + return 0; >> } >> >> static int cxl_endpoint_port_probe(struct cxl_port *port) >> diff --git a/include/acpi/actbl1.h b/include/acpi/actbl1.h >> index 8ea7e5d64bc1..82def138a7e4 100644 >> --- a/include/acpi/actbl1.h >> +++ b/include/acpi/actbl1.h >> @@ -419,6 +419,9 @@ struct acpi_cdat_sslbis { >> u64 entry_base_unit; >> }; >> >> +#define ACPI_CDAT_SSLBIS_US_PORT 0x0100 >> +#define ACPI_CDAT_SSLBIS_ANY_PORT 0xffff >> + >> /* Sub-subtable for above, sslbe_entries field */ >> >> struct acpi_cdat_sslbe { >> >> >
diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c index 6e14d04c0453..37b135c900d5 100644 --- a/drivers/cxl/core/cdat.c +++ b/drivers/cxl/core/cdat.c @@ -124,3 +124,91 @@ int cxl_cdat_endpoint_process(struct cxl_port *port, struct list_head *list) return rc; } EXPORT_SYMBOL_NS_GPL(cxl_cdat_endpoint_process, CXL); + +static int cdat_sslbis_handler(union acpi_subtable_headers *header, void *arg, + const unsigned long end) +{ + struct acpi_cdat_sslbis *sslbis = (struct acpi_cdat_sslbis *)header; + struct acpi_cdat_sslbe *entry; + struct cxl_port *port = arg; + struct device *dev = &port->dev; + int remain, entries, i; + u16 len; + + len = le16_to_cpu((__force __le16)sslbis->header.length); + remain = len - sizeof(*sslbis); + if (!remain || remain % sizeof(*entry) || + (unsigned long)header + len > end) { + dev_warn(dev, "Malformed SSLBIS table length: (%u)\n", len); + return -EINVAL; + } + + /* Unrecognized data type, we can skip */ + if (sslbis->data_type > ACPI_HMAT_WRITE_BANDWIDTH) + return 0; + + entries = remain / sizeof(*entry); + entry = (struct acpi_cdat_sslbe *)((unsigned long)header + sizeof(*sslbis)); + + for (i = 0; i < entries; i++) { + u16 x = le16_to_cpu(entry->portx_id); + u16 y = le16_to_cpu(entry->porty_id); + struct cxl_dport *dport; + unsigned long index; + u16 dsp_id; + u64 val; + + switch (x) { + case ACPI_CDAT_SSLBIS_US_PORT: + dsp_id = y; + break; + case ACPI_CDAT_SSLBIS_ANY_PORT: + switch (y) { + case ACPI_CDAT_SSLBIS_US_PORT: + dsp_id = x; + break; + case ACPI_CDAT_SSLBIS_ANY_PORT: + dsp_id = ACPI_CDAT_SSLBIS_ANY_PORT; + break; + default: + dsp_id = y; + break; + } + break; + default: + dsp_id = x; + break; + } + + if (check_mul_overflow(le64_to_cpu(sslbis->entry_base_unit), + le16_to_cpu(entry->latency_or_bandwidth), + &val)) + dev_warn(dev, "SSLBIS value overflowed!\n"); + + xa_for_each(&port->dports, index, dport) { + if (dsp_id == ACPI_CDAT_SSLBIS_ANY_PORT || + dsp_id == dport->port_id) + cxl_access_coordinate_set(&dport->coord, + sslbis->data_type, + val); + } + + entry++; + } + + return 0; +} + +int cxl_cdat_switch_process(struct cxl_port *port) +{ + int rc; + + rc = acpi_table_parse_cdat(ACPI_CDAT_TYPE_SSLBIS, + cdat_sslbis_handler, + port, port->cdat.table); + if (rc == 0) + rc = -ENOENT; + + return rc; +} +EXPORT_SYMBOL_NS_GPL(cxl_cdat_switch_process, CXL); diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h index ca3d0d74f2e5..3e8020e0a132 100644 --- a/drivers/cxl/cxl.h +++ b/drivers/cxl/cxl.h @@ -600,6 +600,7 @@ cxl_find_dport_by_dev(struct cxl_port *port, const struct device *dport_dev) * @rcrb: base address for the Root Complex Register Block * @rch: Indicate whether this dport was enumerated in RCH or VH mode * @port: reference to cxl_port that contains this downstream port + * @coord: access coordinates (performance) for switch from CDAT */ struct cxl_dport { struct device *dport; @@ -608,6 +609,7 @@ struct cxl_dport { resource_size_t rcrb; bool rch; struct cxl_port *port; + struct access_coordinate coord; }; /** @@ -803,12 +805,18 @@ struct dsmas_entry { #ifdef CONFIG_ACPI int cxl_cdat_endpoint_process(struct cxl_port *port, struct list_head *list); +int cxl_cdat_switch_process(struct cxl_port *port); #else static inline int cxl_cdat_endpoint_process(struct cxl_port *port, struct list_head *list) { return -EOPNOTSUPP; } + +static inline int cxl_cdat_switch_process(struct cxl_port *port) +{ + return -EOPNOTSUPP; +} #endif /* diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c index da023feaa6e2..c5a24b75bf03 100644 --- a/drivers/cxl/port.c +++ b/drivers/cxl/port.c @@ -86,7 +86,17 @@ static int cxl_switch_port_probe(struct cxl_port *port) if (IS_ERR(cxlhdm)) return PTR_ERR(cxlhdm); - return devm_cxl_enumerate_decoders(cxlhdm, NULL); + rc = devm_cxl_enumerate_decoders(cxlhdm, NULL); + if (rc < 0) + return rc; + + if (port->cdat.table) { + rc = cxl_cdat_switch_process(port); + if (rc < 0) + dev_warn(&port->dev, "Failed to parse SSLBIS: %d\n", rc); + } + + return 0; } static int cxl_endpoint_port_probe(struct cxl_port *port) diff --git a/include/acpi/actbl1.h b/include/acpi/actbl1.h index 8ea7e5d64bc1..82def138a7e4 100644 --- a/include/acpi/actbl1.h +++ b/include/acpi/actbl1.h @@ -419,6 +419,9 @@ struct acpi_cdat_sslbis { u64 entry_base_unit; }; +#define ACPI_CDAT_SSLBIS_US_PORT 0x0100 +#define ACPI_CDAT_SSLBIS_ANY_PORT 0xffff + /* Sub-subtable for above, sslbe_entries field */ struct acpi_cdat_sslbe {