>>>> +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."
Why not just use Bit[0] in the Flags register of the Parameter Block Structure
for PATROL_SCRUB? It seems having firmware reset the actual address range is
extra complexity for something we already have a facility for.
>>
>> 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?
>
> After further discussion, I will add a fix for this case to return the 'size' which the user set in the sysfs
> until the scrubbing is started.
I think fixing this will make the interface less confusing, but I also agree
that it doesn't hold much significance technically.
Regards,
Daniel
>
> Thanks,
> Shiju
>>
>
