RE: [RFC PATCH 1/1] cxl/edac: Fix the min_scrub_cycle of a region miscalculation

[Shiju Jose <shiju.jose@huawei.com>]

>-----Original Message-----
>From: Alison Schofield <alison.schofield@intel.com>
>Sent: 30 May 2025 19:28
>To: Li Ming <ming.li@zohomail.com>
>Cc: dave@stgolabs.net; Jonathan Cameron <jonathan.cameron@huawei.com>;
>dave.jiang@intel.com; vishal.l.verma@intel.com; ira.weiny@intel.com;
>dan.j.williams@intel.com; Shiju Jose <shiju.jose@huawei.com>; linux-
>cxl@vger.kernel.org; linux-kernel@vger.kernel.org
>Subject: Re: [RFC PATCH 1/1] cxl/edac: Fix the min_scrub_cycle of a region
>miscalculation
>
>On Fri, May 30, 2025 at 08:28:52PM +0800, Li Ming wrote:
>> When trying to update the scrub_cycle value of a cxl region, which
>> means updating the scrub_cycle value of each memdev under a cxl
>> region. cxl driver needs to guarantee the new scrub_cycle value is
>> greater than the min_scrub_cycle value of a memdev, otherwise the
>> updating operation will fail(Per Table 8-223 in CXL r3.2 section 8.2.10.9.11.1).
>>
>> Current implementation logic of getting the min_scrub_cycle value of a
>> cxl region is that getting the min_scrub_cycle value of each memdevs
>> under the cxl region, then using the minimum min_scrub_cycle value as
>> the region's min_scrub_cycle. Checking if the new scrub_cycle value is
>> greater than this value. If yes, updating the new scrub_cycle value to
>> each memdevs. The issue is that the new scrub_cycle value is possibly
>> greater than the minimum min_scrub_cycle value of all memdevs but less
>> than the maximum min_scrub_cycle value of all memdevs if memdevs have
>> a different min_scrub_cycle value. The updating operation will always
>> fail on these memdevs which have a greater min_scrub_cycle than the
>> new scrub_cycle.
>>
>> The correct implementation logic is to get the maximum value of these
>> memdevs' min_scrub_cycle, check if the new scrub_cycle value is
>> greater than the value. If yes, the new scrub_cycle value is fit for the region.
>>
>> The change also impacts the result of
>> cxl_patrol_scrub_get_min_scrub_cycle(), the interface returned the
>> minimum min_scrub_cycle value among all memdevs under the region
>> before the change. The interface will return the maximum
>> min_scrub_cycle value among all memdevs under the region with the change.
>>
>> Signed-off-by: Li Ming <ming.li@zohomail.com>
>> ---
>> I made this change based on my understanding on the SPEC and current
>> CXL EDAC code, but I am not sure if it is a bug or it is designed this way.
>
>The attribute is defined to show (per Documentation/ABI/testing/sysfs-edac-
>scrub)
>   "Supported minimum scrub cycle duration in seconds by the memory
>scrubber."
>
>Your fix, making the min the max of the mins, looks needed.
>
>I took a look at the max attribute. If the min is the max on the mins, then the
>max should be the max of the maxes. But, not true. We do this:
>
>instead: *max = U8_MAX * 3600; /* Max set by register size */
>
>The comment isn't helping me, esp since the sysfs description doesn't explain
>that we are using a constant max.
CXL spec r3.2 Table 8-222. Device Patrol Scrub Control Feature Readable Attributes
does not define a field for "max scrub cycle supported".  Thus for max scrub 
cycle, returning max value of (U8_MAX) of patrol scrub cycle field. 

Thanks,
Shiju
>
>
>>
>> base-commit: 9f153b7fb5ae45c7d426851f896487927f40e501 cxl/next
>> ---
>>  drivers/cxl/core/edac.c | 8 ++++++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/cxl/core/edac.c b/drivers/cxl/core/edac.c index
>> 2cbc664e5d62..ad243cfe00e7 100644
>> --- a/drivers/cxl/core/edac.c
>> +++ b/drivers/cxl/core/edac.c
>> @@ -103,10 +103,10 @@ static int cxl_scrub_get_attrbs(struct
>cxl_patrol_scrub_context *cxl_ps_ctx,
>>  				u8 *cap, u16 *cycle, u8 *flags, u8 *min_cycle)
>{
>>  	struct cxl_mailbox *cxl_mbox;
>> -	u8 min_scrub_cycle = U8_MAX;
>>  	struct cxl_region_params *p;
>>  	struct cxl_memdev *cxlmd;
>>  	struct cxl_region *cxlr;
>> +	u8 min_scrub_cycle = 0;
>>  	int i, ret;
>>
>>  	if (!cxl_ps_ctx->cxlr) {
>> @@ -133,8 +133,12 @@ static int cxl_scrub_get_attrbs(struct
>cxl_patrol_scrub_context *cxl_ps_ctx,
>>  		if (ret)
>>  			return ret;
>>
>> +		/*
>> +		 * The min_scrub_cycle of a region is the maximum value
>among
>> +		 * the min_scrub_cycle of all the memdevs under the region.
>> +		 */
>>  		if (min_cycle)
>> -			min_scrub_cycle = min(*min_cycle, min_scrub_cycle);
>> +			min_scrub_cycle = max(*min_cycle, min_scrub_cycle);
>>  	}
>>
>>  	if (min_cycle)
>> --
>> 2.34.1
>>