RE: [PATCH v2 3/3] ras: mem: Add memory ACPI RAS2 driver

[Shiju Jose]

>-----Original Message-----
>From: Daniel Ferguson <danielf@xxxxxxxxxxxxxxxxxxxxxx>
>Sent: 07 March 2025 21:52
>To: Shiju Jose <shiju.jose@xxxxxxxxxx>; linux-edac@xxxxxxxxxxxxxxx; linux-
>acpi@xxxxxxxxxxxxxxx; bp@xxxxxxxxx; tony.luck@xxxxxxxxx; rafael@xxxxxxxxxx;
>lenb@xxxxxxxxxx; mchehab@xxxxxxxxxx; leo.duran@xxxxxxx;
>Yazen.Ghannam@xxxxxxx
>Cc: linux-cxl@xxxxxxxxxxxxxxx; dan.j.williams@xxxxxxxxx; dave@xxxxxxxxxxxx;
>Jonathan Cameron <jonathan.cameron@xxxxxxxxxx>; dave.jiang@xxxxxxxxx;
>alison.schofield@xxxxxxxxx; vishal.l.verma@xxxxxxxxx; ira.weiny@xxxxxxxxx;
>david@xxxxxxxxxx; Vilas.Sridharan@xxxxxxx; linux-mm@xxxxxxxxx; linux-
>kernel@xxxxxxxxxxxxxxx; rientjes@xxxxxxxxxx; jiaqiyan@xxxxxxxxxx;
>Jon.Grimm@xxxxxxx; dave.hansen@xxxxxxxxxxxxxxx;
>naoya.horiguchi@xxxxxxx; james.morse@xxxxxxx; jthoughton@xxxxxxxxxx;
>somasundaram.a@xxxxxxx; erdemaktas@xxxxxxxxxx; pgonda@xxxxxxxxxx;
>duenwen@xxxxxxxxxx; gthelen@xxxxxxxxxx;
>wschwartz@xxxxxxxxxxxxxxxxxxx; dferguson@xxxxxxxxxxxxxxxxxxx;
>wbs@xxxxxxxxxxxxxxxxxxxxxx; nifan.cxl@xxxxxxxxx; tanxiaofei
><tanxiaofei@xxxxxxxxxx>; Zengtao (B) <prime.zeng@xxxxxxxxxxxxx>; Roberto
>Sassu <roberto.sassu@xxxxxxxxxx>; kangkang.shen@xxxxxxxxxxxxx;
>wanghuiqiang <wanghuiqiang@xxxxxxxxxx>; Linuxarm
><linuxarm@xxxxxxxxxx>
>Subject: Re: [PATCH v2 3/3] ras: mem: Add memory ACPI RAS2 driver
>
>
>> +static int ras2_hw_scrub_read_size(struct device *dev, void
>> +*drv_data, u64 *size) {
>> +	struct ras2_mem_ctx *ras2_ctx = drv_data;
>> +	int ret;
>> +
>> +	if (ras2_ctx->bg_scrub)
>> +		return -EBUSY;
>> +
>> +	ret = ras2_update_patrol_scrub_params_cache(ras2_ctx);
>> +	if (ret)
>> +		return ret;
>> +
>> +	*size = ras2_ctx->size;
>> +
>> +	return 0;
>> +}
>
>Calling ras2_update_patrol_scrub_params_cache here is problematic.
>
>Imagine:
>  echo 0x1000 > size
>  cat size
>  echo 0x2000000000 > addr
>
>What happens here? What happens is the scrub range is not what you expect it
>to be.  Once you cat size, you reset the size from what you initially set it to.
>I don't think that is what anyone will expect. It certainly caused us to stumble
>while testing.

This is an expected behavior and this extra call was added here when changed using attribute 'addr' to start
the on-demand  scrub operation instead of previous separate attribute ' enable_on_demand' to start
the on-demand scrub operation, according to Borislav's suggestion in v13.

 Please see the following comment in the ras2_hw_scrub_read_addr() fnction,
"Userspace will get the status of the demand scrubbing through the address range
read from the firmware. When the demand scrubbing is finished firmware must reset actual 
address range to 0. Otherwise userspace assumes demand scrubbing is in progress."

Here sysfs attributes 'addr' and 'size' is reading the field: Actual Address Range of 
Table 5.87: Parameter Block Structure for PATROL_SCRUB, written by the firmware.
  
In my opinion, reading back the address range size set in the sysfs before actually writing
the address range to the firmware and starting the on-demand scrub operation doesn't 
hold much significance?

>
>Regards,
>~Daniel

Thanks,
Shiju