Message-ID: <7282d074-15ba-4fe7-bf62-6a4dd6089817@redhat.com>
Date:   Mon, 18 Sep 2023 14:15:33 +0200
From:   David Hildenbrand <david@...hat.com>
To:     Shiju Jose <shiju.jose@...wei.com>,
        "linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>,
        "linux-mm@...ck.org" <linux-mm@...ck.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Cc:     "rafael@...nel.org" <rafael@...nel.org>,
        "lenb@...nel.org" <lenb@...nel.org>,
        "naoya.horiguchi@....com" <naoya.horiguchi@....com>,
        "tony.luck@...el.com" <tony.luck@...el.com>,
        "james.morse@....com" <james.morse@....com>,
        "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
        "jiaqiyan@...gle.com" <jiaqiyan@...gle.com>,
        "jthoughton@...gle.com" <jthoughton@...gle.com>,
        "somasundaram.a@....com" <somasundaram.a@....com>,
        "erdemaktas@...gle.com" <erdemaktas@...gle.com>,
        "pgonda@...gle.com" <pgonda@...gle.com>,
        "rientjes@...gle.com" <rientjes@...gle.com>,
        "duenwen@...gle.com" <duenwen@...gle.com>,
        "Vilas.Sridharan@....com" <Vilas.Sridharan@....com>,
        "mike.malvestuto@...el.com" <mike.malvestuto@...el.com>,
        "gthelen@...gle.com" <gthelen@...gle.com>,
        Linuxarm <linuxarm@...wei.com>,
        Jonathan Cameron <jonathan.cameron@...wei.com>,
        tanxiaofei <tanxiaofei@...wei.com>,
        "Zengtao (B)" <prime.zeng@...ilicon.com>
Subject: Re: [RFC PATCH 3/9] Documentation/scrub-configure.rst: Add
 documentation for scrub driver

On 18.09.23 12:25, Shiju Jose wrote:
> Hi David,
> 
> Thanks for looking into this.
> 
>> -----Original Message-----
>> From: David Hildenbrand <david@...hat.com>
>> Sent: 18 September 2023 08:24
>> To: Shiju Jose <shiju.jose@...wei.com>; linux-acpi@...r.kernel.org; linux-
>> mm@...ck.org; linux-kernel@...r.kernel.org
>> Cc: rafael@...nel.org; lenb@...nel.org; naoya.horiguchi@....com;
>> tony.luck@...el.com; james.morse@....com; dave.hansen@...ux.intel.com;
>> jiaqiyan@...gle.com; jthoughton@...gle.com; somasundaram.a@....com;
>> erdemaktas@...gle.com; pgonda@...gle.com; rientjes@...gle.com;
>> duenwen@...gle.com; Vilas.Sridharan@....com; mike.malvestuto@...el.com;
>> gthelen@...gle.com; Linuxarm <linuxarm@...wei.com>; Jonathan Cameron
>> <jonathan.cameron@...wei.com>; tanxiaofei <tanxiaofei@...wei.com>;
>> Zengtao (B) <prime.zeng@...ilicon.com>
>> Subject: Re: [RFC PATCH 3/9] Documentation/scrub-configure.rst: Add
>> documentation for scrub driver
>>
>> On 15.09.23 19:28, shiju.jose@...wei.com wrote:
>>> From: Shiju Jose <shiju.jose@...wei.com>
>>>
>>> Add documentation for scrub driver, supports configure scrub
>>> parameters, in Documentation/scrub-configure.rst
>>>
>>> Signed-off-by: Shiju Jose <shiju.jose@...wei.com>
>>> ---
>>>    Documentation/scrub-configure.rst | 55
>> +++++++++++++++++++++++++++++++
>>>    1 file changed, 55 insertions(+)
>>>    create mode 100644 Documentation/scrub-configure.rst
>>>
>>> diff --git a/Documentation/scrub-configure.rst
>>> b/Documentation/scrub-configure.rst
>>> new file mode 100644
>>> index 000000000000..9f8581b88788
>>> --- /dev/null
>>> +++ b/Documentation/scrub-configure.rst
>>> @@ -0,0 +1,55 @@
>>> +==========================
>>> +Scrub subsystem driver
>>> +==========================
>>> +
>>> +Copyright (c) 2023 HiSilicon Limited.
>>> +
>>> +:Author:   Shiju Jose <shiju.jose@...wei.com>
>>> +:License:  The GNU Free Documentation License, Version 1.2
>>> +          (dual licensed under the GPL v2) :Original Reviewers:
>>> +
>>> +- Written for: 6.7
>>> +- Updated for:
>>> +
>>> +Introduction
>>> +------------
>>> +The scrub subsystem driver provides the interface for configure the
>>
>> "... interface for configuring memory scrubbers in the system."
>>
>> are we only configuring firmware/hw-based memory scrubbing? I assume so.
> The scrub control could be used for the SW  based memory scrubbing too.

Okay, looks like there is not too much hw/firmware specific in there 
(besides these weird range changes).
[...]

>>> +-------
>>> +
>>> +  The usage takes the form shown in this example::
>>> +
>>> +    # echo 0x300000 > /sys/class/scrub/scrub0/region0/addr_base
>>> +    # echo 0x100000 > /sys/class/scrub/scrub0/region0/addr_size
>>> +    # cat /sys/class/scrub/scrub0/region0/speed_available
>>> +    # 1-60
>>> +    # echo 25 > /sys/class/scrub/scrub0/region0/speed
>>> +    # echo 1 > /sys/class/scrub/scrub0/region0/enable
>>> +
>>> +    # cat /sys/class/scrub/scrub0/region0/speed
>>> +    # 0x19
>>
>> Is it reasonable to return the speed as hex? You set it as dec.
> Presently return speed  as hex to reduce the number of callback function needed
> for reading the hex/dec data because the values for the address range
> need to be in hex.

If speed_available returns dec, speed better also return dec IMHO.

> 
>>
>>> +    # cat /sys/class/scrub/scrub0/region0/addr_base
>>> +    # 0x100000
>>
>> But didn't we set it to 0x300000 ...
> This is an emulated example for testing the RASF/RAS2 definition.
> According to the RASF & RAS2 definition, the actual address range in the
> platform could vary from the requested address range for the patrol scrubbing.
> "The platform calculates the nearest patrol scrub boundary address
> from where it can start". The platform returns the actual address range
> in response to GET_PATROL_PARAMETERS command to the firmware.
> Please see section 5.2.21.2.1 Hardware-based Memory Scrubbing ,
> Table 5.87: Parameter Block Structure for PATROL_SCRUB in the
> ACPI 6.5 specification.
> 

So you configure [0x300000 - 0x400000] and you get [0x100000 - 0x300000]

How does that make any sense? :)

Shouldn't we rather return an error when setting a range that is 
impossible, instead of the hardware deciding to scrub something 
completely different (as can be seen in the example)?

-- 
Cheers,

David / dhildenb

Powered by blists - more mailing lists