From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 92E01C28B20 for ; Fri, 28 Mar 2025 10:18:30 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 06E7E280134; Fri, 28 Mar 2025 06:18:28 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 01EFF280130; Fri, 28 Mar 2025 06:18:27 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id DDBC9280134; Fri, 28 Mar 2025 06:18:27 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0011.hostedemail.com [216.40.44.11]) by kanga.kvack.org (Postfix) with ESMTP id C374F280130 for ; Fri, 28 Mar 2025 06:18:27 -0400 (EDT) Received: from smtpin08.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay01.hostedemail.com (Postfix) with ESMTP id 55B3B1CE111 for ; Fri, 28 Mar 2025 10:18:29 +0000 (UTC) X-FDA: 83270560338.08.62F3A41 Received: from frasgout.his.huawei.com (frasgout.his.huawei.com [185.176.79.56]) by imf02.hostedemail.com (Postfix) with ESMTP id 7720580006 for ; Fri, 28 Mar 2025 10:18:26 +0000 (UTC) Authentication-Results: imf02.hostedemail.com; dkim=none; dmarc=pass (policy=quarantine) header.from=huawei.com; spf=pass (imf02.hostedemail.com: domain of shiju.jose@huawei.com designates 185.176.79.56 as permitted sender) smtp.mailfrom=shiju.jose@huawei.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1743157107; a=rsa-sha256; cv=none; b=eWdXzMs0zD0sl37zff4oslb+wZ1F/T99vFBDlP69dVBTqRn0LGx1WwVnbBEkiiWWC+o+nh kuBW24j6Rq2KlCY4mZEiHXFK7QGOUHfHj43jJER3eFhyV4Y7FdT2UJ5z4CMHqI25gAetN2 /9qB+L0EgFDLsJp2Hd8PXOkiIgKSTgA= ARC-Authentication-Results: i=1; imf02.hostedemail.com; dkim=none; dmarc=pass (policy=quarantine) header.from=huawei.com; spf=pass (imf02.hostedemail.com: domain of shiju.jose@huawei.com designates 185.176.79.56 as permitted sender) smtp.mailfrom=shiju.jose@huawei.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1743157107; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=XyLEJJXxpzNleNwA0IyNqxb2bypPbUI4xbATMYwB0bI=; b=J/IuDngMx1KBh8yJFMIwmx/dMYcPJBjV4GYvnIVUhjZUyKIjZsk+jc+RWRTOTFSXs/1RPO iM8xU/Y3vYA/m67fT5+6hir+SK3u3KqXQ6HPKU1OpqvOHMXzZmKEY4R05IB4u552pKbBfl tTdpZowWicFq41pjpsbFJtlCLsG/nYs= Received: from mail.maildlp.com (unknown [172.18.186.216]) by frasgout.his.huawei.com (SkyGuard) with ESMTP id 4ZPGgY5P0Gz6L53b; Fri, 28 Mar 2025 18:18:01 +0800 (CST) Received: from frapeml500006.china.huawei.com (unknown [7.182.85.219]) by mail.maildlp.com (Postfix) with ESMTPS id 70380140119; Fri, 28 Mar 2025 18:18:21 +0800 (CST) Received: from frapeml500007.china.huawei.com (7.182.85.172) by frapeml500006.china.huawei.com (7.182.85.219) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.1.2507.39; Fri, 28 Mar 2025 11:18:21 +0100 Received: from frapeml500007.china.huawei.com ([7.182.85.172]) by frapeml500007.china.huawei.com ([7.182.85.172]) with mapi id 15.01.2507.039; Fri, 28 Mar 2025 11:18:21 +0100 From: Shiju Jose To: Dan Williams , "linux-cxl@vger.kernel.org" , "dave@stgolabs.net" , Jonathan Cameron , "dave.jiang@intel.com" , "alison.schofield@intel.com" , "vishal.l.verma@intel.com" , "ira.weiny@intel.com" , "david@redhat.com" , "Vilas.Sridharan@amd.com" CC: "linux-edac@vger.kernel.org" , "linux-acpi@vger.kernel.org" , "linux-mm@kvack.org" , "linux-kernel@vger.kernel.org" , "bp@alien8.de" , "tony.luck@intel.com" , "rafael@kernel.org" , "lenb@kernel.org" , "mchehab@kernel.org" , "leo.duran@amd.com" , "Yazen.Ghannam@amd.com" , "rientjes@google.com" , "jiaqiyan@google.com" , "Jon.Grimm@amd.com" , "dave.hansen@linux.intel.com" , "naoya.horiguchi@nec.com" , "james.morse@arm.com" , "jthoughton@google.com" , "somasundaram.a@hpe.com" , "erdemaktas@google.com" , "pgonda@google.com" , "duenwen@google.com" , "gthelen@google.com" , "wschwartz@amperecomputing.com" , "dferguson@amperecomputing.com" , "wbs@os.amperecomputing.com" , "nifan.cxl@gmail.com" , tanxiaofei , "Zengtao (B)" , Roberto Sassu , "kangkang.shen@futurewei.com" , wanghuiqiang , Linuxarm Subject: RE: [PATCH v2 3/8] cxl/edac: Add CXL memory device patrol scrub control feature Thread-Topic: [PATCH v2 3/8] cxl/edac: Add CXL memory device patrol scrub control feature Thread-Index: AQHbmcKuLCBs/xi+XkeVn732q30wUbOGMAMAgAESV/A= Date: Fri, 28 Mar 2025 10:18:20 +0000 Message-ID: References: <20250320180450.539-1-shiju.jose@huawei.com> <20250320180450.539-4-shiju.jose@huawei.com> <67e4ae1cb0cdd_152c29462@dwillia2-mobl3.amr.corp.intel.com.notmuch> In-Reply-To: <67e4ae1cb0cdd_152c29462@dwillia2-mobl3.amr.corp.intel.com.notmuch> Accept-Language: en-GB, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.48.156.185] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-Rspam-User: X-Rspamd-Server: rspam03 X-Rspamd-Queue-Id: 7720580006 X-Stat-Signature: 8e5by5835rwj9pwqmj7t96q4qtwsootm X-HE-Tag: 1743157106-922232 X-HE-Meta: U2FsdGVkX1+ZFeCgM5jmN58wllxbRFi1lx38741pBp7DyV2fpvwJ7ktMWMjJ1Hw5dtVWUEW5lORtyje1F6Sfh3xst8FEyXUm3MKFKMKDrvvCFPPGEG1AFBhGBee/UlWyVK0Xm1qVozZEmrkCZTqVpfCMJYyULpEXFk96PdYNfdnIdQTKZrqn7llUesBkMys7PFCL2Bu6AuQCZLH6dYRvM7zzg6DggrKnccae9eu4HUt0ZXCNF6X5Tt1xK+7qHkvdK2+VRsMgfuwlE9JdrvuftHPhupzn4PhQkz+xRYYWX626XP6XN6z/3a0KKo51k+F7SLC2J3j9v6SA0sXcIHSe3ntyJzKNP/3/O1UqBMuir0bh8o8MycuUFTh5HB7ssZkKkHOvbvRCkRq0vOYSlU0VztpbvURu7uMT1OCMSz27MIOcc5JHxIyfsn/WYKQCKOOEKO1v95E5FixoILoiPOjSjIgJ8udEYFxdp2Thm/mfbapF21F+wqimMxQcqrnirInS2nv68IfEL2ZIyivUWZxh4zxqjMlEXsFFHPm+4Q50BgXFRJgVffkzDnq6Anu5MJgGt1BwH+zcsbS4Y/wKYobHrpamvzGh/kwYBqYDdVF6qTDVviTaoe4lpMIuEDRMEdLbG5vEeg1Pxxk+MOgvRTq3QO2qlC/zy0VUnkToFufom64iwx8dCGiA2idoZlWazSyV0uXbJNUDjMVfSkR7IWKa3rEyGw8PU+dnbS+mXGNhkYJz5GEG03zrgKcSIVw5In4aveM4BA2E1FLmZIZw8XDhcjY34BldHtzKOpUWmiO6QjZV+7GCVIKJyTgFjxaABK660Us9088kzgOdEQevy85HXKQYgfjBJI9HAZukZzRJECbZSaJweN4EBTg6m7KawJMGJGObGqYNwLU1/d/nUmyE6KOfd2T4/pKk8McXbcyD09h3/Tygqjr0X8gH34biudH/Xn1Xp+5f6W1x/+DCRbQ 1wbtgnRI +A5nVZv/BoxtbD709T6+/5d819zIW9aRWk6ImFz5z+QLI47Uniqo6s1CPPVEzaIl8fzcK+kgUslsWAq3jVvRAuCJBF1wgq5yowkPWq1x1FwESK10xGzdA1UZIns60j1+OYRd9t0Xo70+7mMpAsffWYAPHnnlaNtPIGdCAepf6dx0MK0wcWVBWMAxo2TQ5TkfdOtOZUawjX4PF0mVYtfPnf7YyrOsyR5rlc410L4eQBj3SjzVLAUr9PufkiB591nn+R9bSRyMsMYOdB8hIn7o8dXXZVd6igA6/I6n3av+dpfvd5Tvkuuwb9PolTlVor8JEZz0X8usCiTr0BSoMQPrDl5ob7e5F/bijeSZRs9+TWjLhz1S6tUlUOWMKlVNe4evw1sm81Hg9VKx4MqT4N9SamMPlb7PCphjl6nmJ9Bwl2WIsKNjXGmYAhs77WgTB/CsRIuStU4kVXD2oYnXHHlIkcWuhtZsUIBjmNfex8KT2tSftYF4w+8lCA0DPYlk4JNwrtnLhMt5KqjP/RBN5c7457ssg597KufyyxJ5zYmV7TYaI0piQMOc7uBTPzegsbXea8NvCvbvE7jWTRjXY/v1fjG8sz8r1wvk5uXA3zahDybTjRJ5BHqI16fTCaiiQqDoHGfGQSGS2lO1T5KLvuo+VmeSXS1VEpapw58kTWyYahduW7UxZngGl01EQ1z6OKsHXP4kVQQR7fl1bVUjH8eiz/FZEdC0t9TSlMCgkaihTezYRlFxjog7N0ik9PeW6QxuUzWZdo9mw8U7UyhrO8DPEgHDfzCZoPietst5PW5+PnmI5fAo50Y0gx0p7+HViBBWWeckiFPhDYAJZRxm2OL/xbaJwrg== X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: >-----Original Message----- >From: Dan Williams >Sent: 27 March 2025 01:47 >To: Shiju Jose ; linux-cxl@vger.kernel.org; >dan.j.williams@intel.com; dave@stgolabs.net; Jonathan Cameron >; dave.jiang@intel.com; >alison.schofield@intel.com; vishal.l.verma@intel.com; ira.weiny@intel.com; >david@redhat.com; Vilas.Sridharan@amd.com >Cc: linux-edac@vger.kernel.org; linux-acpi@vger.kernel.org; linux- >mm@kvack.org; linux-kernel@vger.kernel.org; bp@alien8.de; >tony.luck@intel.com; rafael@kernel.org; lenb@kernel.org; >mchehab@kernel.org; leo.duran@amd.com; Yazen.Ghannam@amd.com; >rientjes@google.com; jiaqiyan@google.com; Jon.Grimm@amd.com; >dave.hansen@linux.intel.com; naoya.horiguchi@nec.com; >james.morse@arm.com; jthoughton@google.com; somasundaram.a@hpe.com; >erdemaktas@google.com; pgonda@google.com; duenwen@google.com; >gthelen@google.com; wschwartz@amperecomputing.com; >dferguson@amperecomputing.com; wbs@os.amperecomputing.com; >nifan.cxl@gmail.com; tanxiaofei ; Zengtao (B) >; Roberto Sassu ; >kangkang.shen@futurewei.com; wanghuiqiang ; >Linuxarm ; Shiju Jose >Subject: Re: [PATCH v2 3/8] cxl/edac: Add CXL memory device patrol scrub >control feature > >shiju.jose@ wrote: >> From: Shiju Jose >> >> CXL spec 3.2 section 8.2.10.9.11.1 describes the device patrol scrub [...] >> diff --git a/drivers/cxl/core/edac.c b/drivers/cxl/core/edac.c new >> file mode 100644 index 000000000000..5ec3535785e1 >> --- /dev/null >> +++ b/drivers/cxl/core/edac.c >> @@ -0,0 +1,474 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> +/* >> + * CXL EDAC memory feature driver. >> + * >> + * Copyright (c) 2024-2025 HiSilicon Limited. >> + * >> + * - Supports functions to configure EDAC features of the >> + * CXL memory devices. >> + * - Registers with the EDAC device subsystem driver to expose >> + * the features sysfs attributes to the user for configuring >> + * CXL memory RAS feature. >> + */ >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include "core.h" >> + >> +#define CXL_NR_EDAC_DEV_FEATURES 1 >> + >> +static struct rw_semaphore *cxl_acquire(struct rw_semaphore *rwsem) { >> + if (down_read_interruptible(rwsem)) >> + return NULL; >> + >> + return rwsem; >> +} >> + >> +DEFINE_FREE(cxl_unlock, struct rw_semaphore *, if (_T) up_read(_T)) > >I know I suggested cxl_acquire() and cxl_unlock(), but this really is a ge= neric >facility. > >Let's call it rwsem_read_intr_acquire() and rwsem_read_release(), and I'll >follow up later with Peter to see if he wants this to graduate from CXL. > >Also, go ahead and define it in cxl.h for now as I think other places in t= he >subsystem could benefit from this approach. Hi Dan, Thanks for the comments. Sure. should these definitions in cxl.h require in a separate patch? > >> + >> +/* >> + * CXL memory patrol scrub control >> + */ >> +struct cxl_patrol_scrub_context { > >I like "patrol_scrub" spelled out here compared to "ps" used everywhere el= se. Will change. > >> + u8 instance; >> + u16 get_feat_size; >> + u16 set_feat_size; >> + u8 get_version; >> + u8 set_version; >> + u16 effects; >> + struct cxl_memdev *cxlmd; >> + struct cxl_region *cxlr; >> +}; >> + >> +/** >> + * struct cxl_memdev_ps_params - CXL memory patrol scrub parameter data >structure. >> + * @enable: [IN & OUT] enable(1)/disable(0) patrol scrub. >> + * @scrub_cycle_changeable: [OUT] scrub cycle attribute of patrol scrub= is >changeable. >> + * @scrub_cycle_hrs: [IN] Requested patrol scrub cycle in hours. >> + * [OUT] Current patrol scrub cycle in hours. >> + * @min_scrub_cycle_hrs:[OUT] minimum patrol scrub cycle in hours >supported. >> + */ >> +struct cxl_memdev_ps_params { >> + bool enable; >> + bool scrub_cycle_changeable; > >This is set but unused. Even if it were to be used I would expect it to be= set in the >cxl_patrol_scrub_context. I will add to cxl_patrol_scrub_context and will add an extra check against= this when user request to change the scrub rate. > >> + u8 scrub_cycle_hrs; >> + u8 min_scrub_cycle_hrs; >> +}; > >I do not understand the point of this extra object and would prefer to kee= p >intermediate data structures to a minimum. > >It looks like all this does is provide for short lived parsed caching of t= he raw >hardware patrol scrube attributes. Just pass those raw objects around and >provide helpers to do the conversion. Sure. Will do. Was added to avoid more number of function parameters.=20 > >The less data structures the less confusion for the next person that has t= o read >this code a few years down the road. > >> + >> +enum cxl_scrub_param { >> + CXL_PS_PARAM_ENABLE, >> + CXL_PS_PARAM_SCRUB_CYCLE, >> +}; > >This seems unforuntate, why not make non-zero scrub rate an implied enable >and zero to disable? A non-zero sentinel value like U32_MAX can indicate "= keep >scrub rate unchanged". These enums can be removed with remove using cxl_memdev_ps_params. > >> +#define CXL_MEMDEV_PS_SCRUB_CYCLE_CHANGE_CAP_MASK BIT(0) > >This CXL_MEMDEV_PS prefix is awkward due to overload with 'struct >cxl_memdev'. Minimize it to something like: > >CXL_SCRUB_CONTROL_CHANGEABLE >CXL_SCRUB_CONTROL_REALTIME >CXL_SCRUB_CONTROL_CYCLE_MASK >CXL_SCRUB_CONTROL_MIN_CYCLE_MASK Will change. > >> +#define CXL_MEMDEV_PS_SCRUB_CYCLE_REALTIME_REPORT_CAP_MASK >BIT(1) >> +#define CXL_MEMDEV_PS_CUR_SCRUB_CYCLE_MASK GENMASK(7, 0) >#define >> +CXL_MEMDEV_PS_MIN_SCRUB_CYCLE_MASK GENMASK(15, 8) #define >> +CXL_MEMDEV_PS_FLAG_ENABLED_MASK BIT(0) > >CXL_SCRUB_CONTROL_ENABLE > >...no need to call it a mask when it is just a single-bit, and when it is = both the >status and the control just call it "enable". Sure. Will change. > >> + >> +/* >> + * See CXL spec rev 3.2 @8.2.10.9.11.1 Table 8-222 Device Patrol >> +Scrub Control >> + * Feature Readable Attributes. >> + */ >> +struct cxl_memdev_ps_rd_attrs { >> + u8 scrub_cycle_cap; >> + __le16 scrub_cycle_hrs; > >"hours" is just 2 more characters than "hrs", I think we can afford the ex= tra >bytes. Will change. > >[..] >> +int devm_cxl_memdev_edac_register(struct cxl_memdev *cxlmd) { >> + struct edac_dev_feature ras_features[CXL_NR_EDAC_DEV_FEATURES]; >> + int num_ras_features =3D 0; >> + u8 scrub_inst =3D 0; >> + int rc; >> + >> + rc =3D cxl_memdev_scrub_init(cxlmd, &ras_features[num_ras_features], >> + scrub_inst); >> + if (rc < 0 && rc !=3D -EOPNOTSUPP) >> + return rc; >> + >> + if (rc !=3D -EOPNOTSUPP) >> + num_ras_features++; >> + >> + char *cxl_dev_name __free(kfree) =3D >> + kasprintf(GFP_KERNEL, "cxl_%s", dev_name(&cxlmd->dev)); > >if (!cxl_dev_name) > return -ENOMEM; Will add. > >> + >> + return edac_dev_register(&cxlmd->dev, cxl_dev_name, NULL, >> + num_ras_features, ras_features); } >> +EXPORT_SYMBOL_NS_GPL(devm_cxl_memdev_edac_register, "CXL"); >> + >> +int devm_cxl_region_edac_register(struct cxl_region *cxlr) { >> + struct edac_dev_feature ras_features[CXL_NR_EDAC_DEV_FEATURES]; >> + int num_ras_features =3D 0; >> + u8 scrub_inst =3D 0; >> + int rc; >> + >> + rc =3D cxl_region_scrub_init(cxlr, &ras_features[num_ras_features], >> + scrub_inst); >> + if (rc < 0) >> + return rc; >> + >> + num_ras_features++; >> + >> + char *cxl_dev_name __free(kfree) =3D >> + kasprintf(GFP_KERNEL, "cxl_%s", dev_name(&cxlr->dev)); >> + >> + return edac_dev_register(&cxlr->dev, cxl_dev_name, NULL, >> + num_ras_features, ras_features); } >> +EXPORT_SYMBOL_NS_GPL(devm_cxl_region_edac_register, "CXL"); >> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c >> index b3260d433ec7..2aa6eb675fdf 100644 >> --- a/drivers/cxl/core/region.c >> +++ b/drivers/cxl/core/region.c >> @@ -3542,6 +3542,11 @@ static int cxl_region_probe(struct device *dev) >> case CXL_PARTMODE_PMEM: >> return devm_cxl_add_pmem_region(cxlr); >> case CXL_PARTMODE_RAM: >> + rc =3D devm_cxl_region_edac_register(cxlr); > >Why do only volatile regions get EDAC support? PMEM patrol scrub seems >equally valid. Will add devm_cxl_region_edac_register () for PMEM as well. Thanks, Shiju