Received-SPF: pass (google.com: domain of linux-kernel+bounces-123831-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <0309f5f9-9a95-485c-a442-e9fba603d676@os.amperecomputing.com>
Date: Thu, 28 Mar 2024 16:39:44 -0700
User-Agent: Mozilla Thunderbird
From: Daniel Ferguson <danielf@os.amperecomputing.com>
Subject: Re: [RFC PATCH v7 00/12] memory: scrub: introduce subsystem +
 CXL/ACPI-RAS2 drivers
To: shiju.jose@huawei.com, linux-cxl@vger.kernel.org,
 linux-acpi@vger.kernel.org, linux-mm@kvack.org, dan.j.williams@intel.com,
 dave@stgolabs.net, jonathan.cameron@huawei.com, dave.jiang@intel.com,
 alison.schofield@intel.com, vishal.l.verma@intel.com, ira.weiny@intel.com
Cc: linux-edac@vger.kernel.org, linux-kernel@vger.kernel.org,
 david@redhat.com, Vilas.Sridharan@amd.com, leo.duran@amd.com,
 Yazen.Ghannam@amd.com, rientjes@google.com, jiaqiyan@google.com,
 tony.luck@intel.com, Jon.Grimm@amd.com, dave.hansen@linux.intel.com,
 rafael@kernel.org, lenb@kernel.org, naoya.horiguchi@nec.com,
 james.morse@arm.com, jthoughton@google.com, somasundaram.a@hpe.com,
 erdemaktas@google.com, pgonda@google.com, duenwen@google.com,
 mike.malvestuto@intel.com, gthelen@google.com,
 wschwartz@amperecomputing.com, dferguson@amperecomputing.com,
 tanxiaofei@huawei.com, prime.zeng@hisilicon.com,
 kangkang.shen@futurewei.com, wanghuiqiang@huawei.com, linuxarm@huawei.com,
 wbs@os.amperecomputing.com
References: <20240223143723.1574-1-shiju.jose@huawei.com>
Content-Language: en-US
In-Reply-To: <20240223143723.1574-1-shiju.jose@huawei.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?RDFKK2lqbCtXYTI0N1l2Z2ZlUnhNemJOZjNxOE91LzBUcFU2VkVwNkRPN2gv?=
 =?utf-8?B?aTQxUlZOYm00RE85OGRpV2J1blg5Uk02VnJUYm5hSkllUzBnYWZVSnBaUlV3?=
 =?utf-8?B?aEVHTGRYd3oydER4bFpKYWtDNjdJM1pLNnFVNGpMNzdOSzBzSytHcmw4K0p6?=
 =?utf-8?B?cTFGSmNDaVdncm1PNzFUNW9xRFg5SldUOFpEMDlVS3dCVkNKWlBoRWdVRlZ0?=
 =?utf-8?B?azEvSnlWMHJPNkpUVGxzOCtsQ0gwZDBvWGlHdXRYemxYZ29DUWFpUHhuK1V2?=
 =?utf-8?B?eE5pbEtzRERpc0dkOTVKeUFoU3RJK2o3Y2FidTJXOFhYbHUxUmtaQ1Rmc3Vl?=
 =?utf-8?B?NGZoVTFkbmdqTnFwcXExVHZ1ckplOTh3bFFYYmVRakdHa09DM2k4MXdtWG8z?=
 =?utf-8?B?aGRlaEhtRy9oTkszTWhSMzJmUkRJN0FDTWkrVnZhN3BEU013UitrWW9HUlRT?=
 =?utf-8?B?TTczZFROUGtvNEoyY3QyVDNBMVk4OEUxWVE2Y1owZng2Q1NZcjEvQ3JLbFBt?=
 =?utf-8?B?VVZHeTVKVGFZc1cwbmFFYk5tR1E2aGVhVW1NQTNLdWltYVV2WDRwNWlHczRH?=
 =?utf-8?B?QmVZUVhaMXMzNm5zOCtnVkR3Q2hxV2R4NFM2enJYb09MZ2t5MStiZ0M4ZUY3?=
 =?utf-8?B?K0JERHlSYUdrSDZyL0ZwUjVCb1g4bjIveTBLWFhRdlhwT3RoVm9iOU9rQ1NF?=
 =?utf-8?B?NDVoUG9nWGhIYUdEUEtzd2tmNjExbXhVc05QLzN4cFl6S2QvaVdVekVUajNX?=
 =?utf-8?B?Z2RQNHVLenlWbmJ1bGprTDliUTdaMGtUOGJFU0l3UDVQR05FSlZKY3E1bjJv?=
 =?utf-8?B?ZzUyZFVyOWRRSXAwUTZIWjM3K2cyNlZnanlFQnFNaWtVbEdIb1BBa29NSkVJ?=
 =?utf-8?B?cXB0NzJlZWUrVDQ0NzYwczI2TWw5RFltbmszMnE5WkVFTjZZNnRNN3ZsNWc5?=
 =?utf-8?B?aGU0Vmg4UTBYaStYVWNXSlJFZnZaMUU3S29BTENRS3dxUnVCK1FhdVZRMWUw?=
 =?utf-8?B?NEswOTl4K05UaDlVK0pkUDJnaEpaV1pGdlgxVHNXVHRQbko1RkpFL0NENWhR?=
 =?utf-8?B?QnFjQWtsaDhwN243dThRKzkrYlJoMTc2QVd1dUJFZmxHd2o0cnlUV25kL2dl?=
 =?utf-8?B?ZFBrNHhEUHNTNG02Ty9HWll6c2t0MWozejU5bFo0bGNPbEFKMWE0dFJWRlZJ?=
 =?utf-8?B?ZVNCby82QmVBdVZVUzRxdXhsbmxSVVZreC95ZWl6SWxocWZSYjgxQjVPQnVv?=
 =?utf-8?B?ZXhmWThrMnJic3FYaHQzY0RhVFNod3g0UUxrWmtRNVd0dlAyeTEzUnd1UEgx?=
 =?utf-8?B?d0N6Ny9tU25TNXNnTlJqSDdZaDhGRU9sU0pXZ3NabVo1RkIwN0p5ZXRFNDZ6?=
 =?utf-8?B?NURPU3FLWXphVmVwd0ZkeTdCK2FNajVHeERUdHNlNnp4bTdvdHpRdmZhcjVD?=
 =?utf-8?B?d1pUQVArMjNWMjNmWW5oM28rTklrOWs1NG5POEQ1aUVhT2xwZUtnU0JNdGxQ?=
 =?utf-8?B?dDJ2dU9HZURSSS9aUit4Uzh1UHl6b1pQa2gwUzJrRSswNHVCTVBxcnZGbjRi?=
 =?utf-8?B?RG4ybUVZTGNCWU5mdVdCUUFCVWNoUGJKKzhFb1ZtanhwOTdJcGpxRVRUVVV2?=
 =?utf-8?B?S0lpSTNvdFloV1ZnSW5EQ2RKczN1RzJ3ZHVxdExUdU15SkF0R1pkZ21neUIv?=
 =?utf-8?B?YnM1alhPcXRsbmRmUFlqMXRCRUJIa1dReExHSEhySWVpR2VCRDlaYUZrcDVT?=
 =?utf-8?B?OHFNWEZuTmlqUlI0N1BJZXFLcXZIOXRrQUl0ZXpwdzdwY3d1dVdIdGxvY0ZJ?=
 =?utf-8?B?cmlnNkRnQVpRNmVFRjZXUGUwQ1YxQkw5bDhVR3d2SFFoUC9uY3ZzWm1oT2FY?=
 =?utf-8?B?TFJhZzkwUStoQ2JoczVWNnpzd0JMUWMydzJ0enFES1VrVjZnUEZqK3FTK2x1?=
 =?utf-8?B?Y3NLa2M5N3dJa0lGOEhaZDMwVXdtUnFGUjllRTZKRS9PNTZOa0kycGsrOXpO?=
 =?utf-8?B?V2xMTm1jalYxb2N6TXJkSExmWVJuNkh0Tks5Q0liWFlSWnAxdlZ6ekJya3RP?=
 =?utf-8?B?OGZPbFRXZU5DTDFqVXFzVHRRUGlGL2xZdmh6UGdJYkZ5blJyNkJ4S3RjR0Vm?=
 =?utf-8?B?UjFROVM4QmpzSzJsQkdSN25tdWx2emdyU1JKY3o0Ky9TZW13Q2hRYzMxeHV5?=
 =?utf-8?Q?jkK9cBixD2L39SRuKsnVQHg=3D?=
X-OriginatorOrg: os.amperecomputing.com
X-MS-Exchange-CrossTenant-Network-Message-Id: b5788c87-846e-4c83-521c-08dc4f805b6c
X-MS-Exchange-CrossTenant-AuthSource: CO1PR01MB7370.prod.exchangelabs.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 28 Mar 2024 23:39:49.1017
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3bc2b170-fd94-476d-b0ce-4229bdc904a7
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 3fJ2w6/Rxzljqz1bm3XokxkbhadSOdxlRVzQrfCVzxHd6R7fy9xrjiwTe+VVKC0mEO5ZCzVBdvbiTt2wnRAGXOR+fdH2GRnsYUtDlSoW29T7tUBnviWW3LaP/nQZ5A8N
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR01MB6400

> RAS2 HW based memory patrol scrub needs RAS2 PCC interfaces
> and ACPI RAS2 driver for communication b/w kernel and firmware.
> ACPI RAS2 Driver adds platform device, for each memory feature,
> which binds to the RAS2 memory driver.
> Memory RAS2 driver registers with the memory scrub subsystem to
> expose the RAS2 scrub controls to the user.

Hi Shiju,
Thanks for this work. This has been very useful for us, as we've
been using it to test a RAS2 implementation here at Ampere
Computing.

In general, the pieces implementing RAS2 are useful and functional. We did,
however, encounter a few places where we had to make some bug fixes.
I'll reply to specific patches with changes I can recommend. Also, there are
a few implementation choices that I will have questions about.

Here is an outline containing the generalized comments I plan on making
within/near relevant patches:
1) Found a bug in ras2.c in how the pcc_desc_list pointer is incremented.
2) Executing a RAS2 command seems unnecessary for feature detection.
specifically, in the functions ras2_is_patrol_scrub_support() and 
ras2_get_patrol_scrub_params()
3) Consider adding more error detection/reporting in ras2_check_pcc_chan()
4) Concerns with multiple scrub devices attempting to map the same PCC 
channel.

-Daniel