Re: [PATCH 2/2] cxl: Add checks to access_coordinate calculation to fail missing data

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Wed, 28 Feb 2024 17:25:42 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> Jonathan noted that when the coordinates for host bridge and switches
> can be 0s if no actual data are retrieved and the calculation continues.
> The resulting number would be inaccurate. Add checks to ensure that the
> calculation would complete only if the numbers are valid.
> 
> Reported-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>

Hi Dave,

Whilst I think the fix is right, it is getting hard to read. Maybe
a rethink is needed on how that iteration works?

> ---
>  drivers/cxl/core/port.c | 31 +++++++++++++++++++++++++++----
>  1 file changed, 27 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index e1d30a885700..2c82fe24b789 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -2110,6 +2110,20 @@ static void combine_coordinates(struct access_coordinate *c1,
>  		c1->read_latency += c2->read_latency;
>  }
>  
> +static bool coordinates_invalid(struct access_coordinate *c)
> +{
> +	if (!c->read_bandwidth && !c->write_bandwidth &&
> +	    !c->read_latency && !c->write_latency)
> +		return true;
> +
> +	return false;
> +}
> +
> +static bool parent_port_is_cxl_root(struct cxl_port *port)
> +{
> +	return is_cxl_root(to_cxl_port(port->dev.parent));
> +}
> +
>  /**
>   * cxl_endpoint_get_perf_coordinates - Retrieve performance numbers stored in dports
>   *				   of CXL path
> @@ -2142,16 +2156,25 @@ int cxl_endpoint_get_perf_coordinates(struct cxl_port *port,
>  	 * port each iteration. If the parent is cxl root then there is
>  	 * nothing to gather.
>  	 */
> -	while (!is_cxl_root(to_cxl_port(iter->dev.parent))) {
> -		combine_coordinates(&c, &dport->sw_coord);
> +	while (!parent_port_is_cxl_root(iter)) {
> +		iter = to_cxl_port(iter->dev.parent);
> +
> +		/* There's no CDAT for the host bridge, so skip if so. */

Comment refers to skipping whereas code is 'doing more' for the other case
so this is confusing to me.

The inverse of this only occurs on the last iteration I think.

Possibly a do / while instead of a while will do it.
I'm far from confident though as all the levels of look up have
me too confused.


	do {
		if (coordinates_invalid(&dport->sw_coord))
			return -EINVAL;

		combine_coordinates(&c, &dport->sw_coord);
		iter = to_cxl_port(iter->dev.parent);
  		dport = iter->parent_dport;
	} while (!parent_port_is_cxl_root(iter));
	/* Do final link updates */
	c.write_latency += dport->link_latency;
	c.read_latency += dport->link_latency;

> +		if (!parent_port_is_cxl_root(iter)) {
> +			if (coordinates_invalid(&dport->sw_coord))
> +				return -EINVAL;
> +
> +			combine_coordinates(&c, &dport->sw_coord);
> +		}
> +
>  		c.write_latency += dport->link_latency;
>  		c.read_latency += dport->link_latency;
> -
> -		iter = to_cxl_port(iter->dev.parent);
>  		dport = iter->parent_dport;
>  	}
>  
>  	/* Augment with the generic port (host bridge) perf data */
> +	if (coordinates_invalid(&dport->hb_coord))
> +		return -EINVAL;
>  	combine_coordinates(&c, &dport->hb_coord);
>  
>  	/* Get the calculated PCI paths bandwidth */