RE: [PATCH v7 3/5] cxl: Fix incorrect region perf data calculation

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

Dave Jiang wrote:
> Current math in cxl_region_perf_data_calculate divides the latency by 1000
> every time the function gets called. This causes the region latency to be
> divided by 1000 per memory device and the math is incorrect. This is user
> visible as the latency access_coordinate exposed via sysfs will show
> incorrect latency data.
> 
> Normalize values from CDAT to nanoseconds. Adjust sub-nanoseconds latency
> to at least 1. Remove adjustment of perf numbers from the generic target
> since hmat handling code has already normalized those numbers. Now all
> computation and stored numbers should be in nanoseconds.
> 
> cxl_hb_get_perf_coordinates() is removed and HB coords are calculated
> in the port access_coordinate calculation path since it no longer need
> to be treated special.
> 
> Fixes: 3d9f4a197230 ("cxl/region: Calculate performance data for a region")
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
> v7:
> - Remove min_not_zero(). Incorrectly set everything to 1. DIV_ROUNDUP()
>   will ensure sub-nanoseconds values not 0 unless value 0 to begin with.
>   (Jonathan)
> - Reflowed patch order
> - Remove cxl_hb_get_perf_coordinates() as change made function unnessary.
> - Add hb access_coordinate back to port caclculation.
> ---
>  drivers/cxl/acpi.c      | 13 +-----
>  drivers/cxl/core/cdat.c | 89 ++++++++++++++++-------------------------
>  drivers/cxl/core/port.c | 36 ++---------------
>  drivers/cxl/cxl.h       |  2 -
>  4 files changed, 40 insertions(+), 100 deletions(-)

yum... bug fix that removes more than double the code it adds.

[..]
> diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
> index eddbbe21450c..48704976693e 100644
> --- a/drivers/cxl/core/cdat.c
> +++ b/drivers/cxl/core/cdat.c
> @@ -20,6 +20,31 @@ struct dsmas_entry {
>  	int qos_class;
>  };
>  
> +static u32 cdat_normalize(u16 entry, u64 base, u8 type)
> +{
> +	u32 value;
> +
> +	/*
> +	 * Check for invalid and overflow values
> +	 */
> +	if (entry == 0xffff || !entry)
> +		return 0;
> +	else if (base > (UINT_MAX / (entry)))
> +		return 0;
> +
> +	value = entry * base;

Might be worth a reminder comment here that CDAT fields follow the
format of HMAT fields when a future reader wonders why these type names
are not CDAT_ACCESS_LATENCY, etc. Bonus points for a "see Table 5
Device Scoped Latency and Bandwidth Information Structure in Coherent
Device Attribute Table (CDAT) Specification v1.01"

Either way you can add:

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