| Message ID | 20250228122752.2062-1-shiju.jose@huawei.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | ACPI: Add support for ACPI RAS2 feature table | expand |
On Fri, 28 Feb 2025 12:27:48 +0000 <shiju.jose@huawei.com> wrote: > From: Shiju Jose <shiju.jose@huawei.com> > > Add support for ACPI RAS2 feature table (RAS2) defined in the ACPI 6.5 > specification, section 5.2.21 and RAS2 HW based memory scrubbing feature. > > ACPI RAS2 patches were part of the EDAC series [1]. Whilst linux-next now contains the EDAC patches, we shouldn't base a feature submission on it. This should be the same as you did for the CXL tree with a statement that it depends on https://web.git.kernel.org/pub/scm/linux/kernel/git/ras/ras.git/log/?h=edac-cxl which is the immutable tag / branch Borislav provided. I doubt there is anything else hitting this code so shouldn't be any need to rebase (I could be wrong though!) Assuming everyone is happy with this series, who is going to pick it up? Borislav via ras.git, or Rafael via acpi.git? I don't really have any preference other than making sure it doesn't fall down the cracks! Jonathan > > 1. https://lore.kernel.org/linux-cxl/20250212143654.1893-1-shiju.jose@huawei.com/ > > Shiju Jose (2): > ACPI:RAS2: Add ACPI RAS2 driver > ras: mem: Add memory ACPI RAS2 driver > > Documentation/edac/scrub.rst | 73 ++++++ > drivers/acpi/Kconfig | 11 + > drivers/acpi/Makefile | 1 + > drivers/acpi/ras2.c | 417 +++++++++++++++++++++++++++++++++++ > drivers/ras/Kconfig | 11 + > drivers/ras/Makefile | 1 + > drivers/ras/acpi_ras2.c | 383 ++++++++++++++++++++++++++++++++ > include/acpi/ras2_acpi.h | 47 ++++ > 8 files changed, 944 insertions(+) > create mode 100755 drivers/acpi/ras2.c > create mode 100644 drivers/ras/acpi_ras2.c > create mode 100644 include/acpi/ras2_acpi.h >
>-----Original Message----- >From: Jonathan Cameron <jonathan.cameron@huawei.com> >Sent: 03 March 2025 09:36 >To: Shiju Jose <shiju.jose@huawei.com> >Cc: linux-edac@vger.kernel.org; linux-acpi@vger.kernel.org; rafael@kernel.org; >bp@alien8.de; tony.luck@intel.com; lenb@kernel.org; mchehab@kernel.org; >linux-mm@kvack.org; linux-kernel@vger.kernel.org; linux-cxl@vger.kernel.org; >j.williams@intel.com; dave@stgolabs.net; dave.jiang@intel.com; >alison.schofield@intel.com; vishal.l.verma@intel.com; ira.weiny@intel.com; >david@redhat.com; Vilas.Sridharan@amd.com; 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 ><tanxiaofei@huawei.com>; Zengtao (B) <prime.zeng@hisilicon.com>; Roberto >Sassu <roberto.sassu@huawei.com>; kangkang.shen@futurewei.com; >wanghuiqiang <wanghuiqiang@huawei.com>; Linuxarm ><linuxarm@huawei.com> >Subject: Re: [PATCH linux-next 0/2] ACPI: Add support for ACPI RAS2 feature >table > >On Fri, 28 Feb 2025 12:27:48 +0000 ><shiju.jose@huawei.com> wrote: > >> From: Shiju Jose <shiju.jose@huawei.com> >> >> Add support for ACPI RAS2 feature table (RAS2) defined in the ACPI 6.5 >> specification, section 5.2.21 and RAS2 HW based memory scrubbing feature. >> >> ACPI RAS2 patches were part of the EDAC series [1]. > >Whilst linux-next now contains the EDAC patches, we shouldn't base a feature >submission on it. This should be the same as you did for the CXL tree with a >statement that it depends on > >https://web.git.kernel.org/pub/scm/linux/kernel/git/ras/ras.git/log/?h=edac-cxl > >which is the immutable tag / branch Borislav provided. Hi Jonathan, These RAS2 patches are applied cleanly, built and tested fine in the immutable ras.git: 'edac-cxl' branch Borislav provided. (https://web.git.kernel.org/pub/scm/linux/kernel/git/ras/ras.git/log/?h=edac-cxl). Thanks, Shiju > >I doubt there is anything else hitting this code so shouldn't be any need to rebase >(I could be wrong though!) > >Assuming everyone is happy with this series, who is going to pick it up? > >Borislav via ras.git, or Rafael via acpi.git? I don't really have any preference >other than making sure it doesn't fall down the cracks! > >Jonathan > >> >> 1. >> https://lore.kernel.org/linux-cxl/20250212143654.1893-1-shiju.jose@hua >> wei.com/ >> >> Shiju Jose (2): >> ACPI:RAS2: Add ACPI RAS2 driver >> ras: mem: Add memory ACPI RAS2 driver >> >> Documentation/edac/scrub.rst | 73 ++++++ >> drivers/acpi/Kconfig | 11 + >> drivers/acpi/Makefile | 1 + >> drivers/acpi/ras2.c | 417 +++++++++++++++++++++++++++++++++++ >> drivers/ras/Kconfig | 11 + >> drivers/ras/Makefile | 1 + >> drivers/ras/acpi_ras2.c | 383 ++++++++++++++++++++++++++++++++ >> include/acpi/ras2_acpi.h | 47 ++++ >> 8 files changed, 944 insertions(+) >> create mode 100755 drivers/acpi/ras2.c create mode 100644 >> drivers/ras/acpi_ras2.c create mode 100644 include/acpi/ras2_acpi.h >>
On Mon, Mar 03, 2025 at 05:35:38PM +0800, Jonathan Cameron wrote: > Borislav via ras.git, or Rafael via acpi.git? I don't really > have any preference other than making sure it doesn't fall down > the cracks! It's probably easier if I take it. However, just from a cursory look, it would need some scrubbing. There's stuff like: + ps_sm->params.requested_address_range[0] = 0; + ps_sm->params.requested_address_range[1] = 0; + ps_sm->params.scrub_params_in &= ~RAS2_PATROL_SCRUB_SCHRS_IN_MASK; + ps_sm->params.scrub_params_in |= FIELD_PREP(RAS2_PATROL_SCRUB_SCHRS_IN_MASK, + ras2_ctx->scrub_cycle_hrs); + ps_sm->params.patrol_scrub_command = RAS2_START_PATROL_SCRUBBER; which definitely needs shortening. There's no need for a wholly written out "requested_address_range". I know variables should have meaningfull names but writing fiction shouldn't be either. +static int ras2_acpi_parse_table(struct acpi_table_header *pAcpiTable) Yuck, CamelCase?! And I'm pretty sure if I start looking more, I'll find more funky stuff. HTH.
>-----Original Message----- >From: Borislav Petkov <bp@alien8.de> >Sent: 03 March 2025 10:35 >To: Jonathan Cameron <jonathan.cameron@huawei.com> >Cc: Shiju Jose <shiju.jose@huawei.com>; linux-edac@vger.kernel.org; linux- >acpi@vger.kernel.org; rafael@kernel.org; tony.luck@intel.com; >lenb@kernel.org; mchehab@kernel.org; linux-mm@kvack.org; linux- >kernel@vger.kernel.org; linux-cxl@vger.kernel.org; j.williams@intel.com; >dave@stgolabs.net; dave.jiang@intel.com; alison.schofield@intel.com; >vishal.l.verma@intel.com; ira.weiny@intel.com; david@redhat.com; >Vilas.Sridharan@amd.com; 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 <tanxiaofei@huawei.com>; Zengtao (B) ><prime.zeng@hisilicon.com>; Roberto Sassu <roberto.sassu@huawei.com>; >kangkang.shen@futurewei.com; wanghuiqiang <wanghuiqiang@huawei.com>; >Linuxarm <linuxarm@huawei.com> >Subject: Re: [PATCH linux-next 0/2] ACPI: Add support for ACPI RAS2 feature >table > [...] > >However, just from a cursory look, it would need some scrubbing. There's stuff >like: > >+ ps_sm->params.requested_address_range[0] = 0; >+ ps_sm->params.requested_address_range[1] = 0; >+ ps_sm->params.scrub_params_in &= >~RAS2_PATROL_SCRUB_SCHRS_IN_MASK; >+ ps_sm->params.scrub_params_in |= >FIELD_PREP(RAS2_PATROL_SCRUB_SCHRS_IN_MASK, >+ ras2_ctx->scrub_cycle_hrs); >+ ps_sm->params.patrol_scrub_command = >+ RAS2_START_PATROL_SCRUBBER; > > >which definitely needs shortening. There's no need for a wholly written out >"requested_address_range". I know variables should have meaningfull names >but writing fiction shouldn't be either. Hi Borislav, Some of these variables, for e.g. requested_address_range are not defined in this patch, but in the 'include/acpi/actbl2.h'. My understanding is that those changes required to upstream first via https://github.com/acpica/acpica ? > >+static int ras2_acpi_parse_table(struct acpi_table_header *pAcpiTable) > >Yuck, CamelCase?! Fixed. > >And I'm pretty sure if I start looking more, I'll find more funky stuff. Will check and fix. > >HTH. > >-- >Regards/Gruss, > Boris. > >https://people.kernel.org/tglx/notes-about-netiquette Thanks, Shiju
On Tue, Mar 04, 2025 at 06:19:58PM +0000, Shiju Jose wrote: > Some of these variables, for e.g. requested_address_range are not defined > in this patch, but in the 'include/acpi/actbl2.h'. > My understanding is that those changes required to upstream first via > https://github.com/acpica/acpica ? Are you sure? ... * Additional ACPI Tables (2) * * These tables are not consumed directly by the ACPICA subsystem, but are * included here to support device drivers and the AML disassembler. ... In any case, if this goes through me, I will have to review it first as it looks funky. Your call guys.
> > Some of these variables, for e.g. requested_address_range are not defined > > in this patch, but in the 'include/acpi/actbl2.h'. > > My understanding is that those changes required to upstream first via > > https://github.com/acpica/acpica ? > > Are you sure? > > ... > * Additional ACPI Tables (2) > * > * These tables are not consumed directly by the ACPICA subsystem, but are > * included here to support device drivers and the AML disassembler. > ... > > In any case, if this goes through me, I will have to review it first as it > looks funky. That requested_address_range field has been in the acpi_rasf_patrol_scrub_parameter structure since 2018 Got moved/copied into the acpi_ras2_patrol_scrub_parameter structure in 2023. $ git blame include/acpi/actbl2.h | grep requested_address_range e62f8227851da (Erik Kaneda 2018-02-15 13:09:26 -0800 2722) u64 requested_address_range[2]; 2e94dc1189804 (Shiju Jose 2023-09-27 17:41:52 +0100 2829) u64 requested_address_range[2]; -Tony
From: Shiju Jose <shiju.jose@huawei.com> Add support for ACPI RAS2 feature table (RAS2) defined in the ACPI 6.5 specification, section 5.2.21 and RAS2 HW based memory scrubbing feature. ACPI RAS2 patches were part of the EDAC series [1]. 1. https://lore.kernel.org/linux-cxl/20250212143654.1893-1-shiju.jose@huawei.com/ Shiju Jose (2): ACPI:RAS2: Add ACPI RAS2 driver ras: mem: Add memory ACPI RAS2 driver Documentation/edac/scrub.rst | 73 ++++++ drivers/acpi/Kconfig | 11 + drivers/acpi/Makefile | 1 + drivers/acpi/ras2.c | 417 +++++++++++++++++++++++++++++++++++ drivers/ras/Kconfig | 11 + drivers/ras/Makefile | 1 + drivers/ras/acpi_ras2.c | 383 ++++++++++++++++++++++++++++++++ include/acpi/ras2_acpi.h | 47 ++++ 8 files changed, 944 insertions(+) create mode 100755 drivers/acpi/ras2.c create mode 100644 drivers/ras/acpi_ras2.c create mode 100644 include/acpi/ras2_acpi.h