Re: [PATCH v7 07/10] cxl: Change sslbis handler to only handle single dport

[<dan.j.williams@intel.com>]

Dave Jiang wrote:
> While cxl_switch_parse_cdat() is harmless to be run multiple times, it is
> not efficient in the current scheme where one dport is being updated at
> a time by the memdev probe path. Change the input parameter to the
> specific dport being updated to pick up the SSLBIS information for just
> that dport.
> 
> Reviewed-by: Gregory Price <gourry@gourry.net>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Reviewed-by: Li Ming <ming.li@zohomail.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  drivers/cxl/core/cdat.c | 23 ++++++++++-------------
>  drivers/cxl/core/port.c |  2 +-
>  drivers/cxl/cxl.h       |  2 +-
>  3 files changed, 12 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
> index 0ccef2f2a26a..58c21a2f22c1 100644
> --- a/drivers/cxl/core/cdat.c
> +++ b/drivers/cxl/core/cdat.c
> @@ -440,8 +440,8 @@ static int cdat_sslbis_handler(union acpi_subtable_headers *header, void *arg,
>  	} *tbl = (struct acpi_cdat_sslbis_table *)header;
>  	int size = sizeof(header->cdat) + sizeof(tbl->sslbis_header);
>  	struct acpi_cdat_sslbis *sslbis;
> -	struct cxl_port *port = arg;
> -	struct device *dev = &port->dev;
> +	struct cxl_dport *dport = arg;
> +	struct device *dev = &dport->port->dev;
>  	int remain, entries, i;
>  	u16 len;
>  
> @@ -467,8 +467,6 @@ static int cdat_sslbis_handler(union acpi_subtable_headers *header, void *arg,
>  		u16 y = le16_to_cpu((__force __le16)tbl->entries[i].porty_id);
>  		__le64 le_base;
>  		__le16 le_val;
> -		struct cxl_dport *dport;
> -		unsigned long index;
>  		u16 dsp_id;
>  		u64 val;
>  
> @@ -499,28 +497,27 @@ static int cdat_sslbis_handler(union acpi_subtable_headers *header, void *arg,
>  		val = cdat_normalize(le16_to_cpu(le_val), le64_to_cpu(le_base),
>  				     sslbis->data_type);
>  
> -		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);
> -			}
> +		if (dsp_id == ACPI_CDAT_SSLBIS_ANY_PORT ||
> +		    dsp_id == dport->port_id) {
> +			cxl_access_coordinate_set(dport->coord,
> +						  sslbis->data_type, val);
> +			return 0;
>  		}
>  	}
>  
>  	return 0;
>  }
>  
> -void cxl_switch_parse_cdat(struct cxl_port *port)
> +void cxl_switch_parse_cdat(struct cxl_dport *dport)
>  {
> +	struct cxl_port *port = dport->port;
>  	int rc;
>  
>  	if (!port->cdat.table)
>  		return;
>  
>  	rc = cdat_table_parse(ACPI_CDAT_TYPE_SSLBIS, cdat_sslbis_handler,
> -			      port, port->cdat.table, port->cdat.length);
> +			      dport, port->cdat.table, port->cdat.length);
>  	rc = cdat_table_parse_output(rc);
>  	if (rc)
>  		dev_dbg(&port->dev, "Failed to parse SSLBIS: %d\n", rc);
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index c1de6872e57f..afeebb0ce4ab 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -1614,7 +1614,7 @@ static int cxl_port_setup_with_dport(struct cxl_port *port,
>  {
>  	device_lock_assert(&port->dev);
>  
> -	cxl_switch_parse_cdat(port);
> +	cxl_switch_parse_cdat(dport);

Ah, I now realize I misread patch4 where I asked about the ordering of
this function relative to the update. The dport is already added at this
point.

>  
>  	return update_decoders_with_dport(port, dport);

More delayed realization... cxl_port_setup_with_dport() should be
"void", it never fails.

As for this patch, looks good.

Reviewed-by: Dan Williams <dan.j.williams@intel.com>