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 B0CEFC28B22 for ; Thu, 6 Mar 2025 11:22:01 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 9D11A280002; Thu, 6 Mar 2025 06:22:00 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 98153280001; Thu, 6 Mar 2025 06:22:00 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 82213280002; Thu, 6 Mar 2025 06:22:00 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0013.hostedemail.com [216.40.44.13]) by kanga.kvack.org (Postfix) with ESMTP id 6379A280001 for ; Thu, 6 Mar 2025 06:22:00 -0500 (EST) Received: from smtpin25.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay07.hostedemail.com (Postfix) with ESMTP id 731461614D4 for ; Thu, 6 Mar 2025 11:22:00 +0000 (UTC) X-FDA: 83190886800.25.9272EE1 Received: from frasgout.his.huawei.com (frasgout.his.huawei.com [185.176.79.56]) by imf03.hostedemail.com (Postfix) with ESMTP id AC26F20006 for ; Thu, 6 Mar 2025 11:21:56 +0000 (UTC) Authentication-Results: imf03.hostedemail.com; dkim=none; dmarc=pass (policy=quarantine) header.from=huawei.com; spf=pass (imf03.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=1741260117; a=rsa-sha256; cv=none; b=mV4snUzp2jZ+rsShX4uj0rvoNaWhwZqVoeKzoSfUWhKT/SRolqzkAB+LGqYBpmSxiD+kWS KmbTBnYTlvvxuC/QyldSuqh5ihrZkpq7RGeEMMzjYxIDBbhBikkl3XXEa3WsLuJHpMiXEq eYCuaXWmP9ADfL6ZuTJpi4zNXPVXIbQ= ARC-Authentication-Results: i=1; imf03.hostedemail.com; dkim=none; dmarc=pass (policy=quarantine) header.from=huawei.com; spf=pass (imf03.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=1741260117; 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=GU9RHLZgoJJlz06HHcvboylqRnvv6KjHaLfSZjHT2F8=; b=yUh0TwCfyNxzObQvUTAYmwjA8UkOt5Wj6lg8dIiFd1hQDuyPeJt6f3x4YhDLK4hhbzDs59 NoC3tpsDDkeAMkExozS2NNqh99vQW6+TggL17+USfozsHOp86RXO25USBFYORh04URef5p 4gSAf+hzT2C7bz5gGC2FKpzuEpBedpM= Received: from mail.maildlp.com (unknown [172.18.186.216]) by frasgout.his.huawei.com (SkyGuard) with ESMTP id 4Z7n2Z3c3mz6L5Qc; Thu, 6 Mar 2025 19:17:42 +0800 (CST) Received: from frapeml100008.china.huawei.com (unknown [7.182.85.131]) by mail.maildlp.com (Postfix) with ESMTPS id C1C11140593; Thu, 6 Mar 2025 19:21:51 +0800 (CST) Received: from frapeml500007.china.huawei.com (7.182.85.172) by frapeml100008.china.huawei.com (7.182.85.131) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.1.2507.39; Thu, 6 Mar 2025 12:21:51 +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; Thu, 6 Mar 2025 12:21:51 +0100 From: Shiju Jose To: Jonathan Cameron CC: "linux-edac@vger.kernel.org" , "linux-acpi@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" , "linux-cxl@vger.kernel.org" , "dan.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" , "linux-mm@kvack.org" , "linux-kernel@vger.kernel.org" , "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 2/3] ACPI:RAS2: Add ACPI RAS2 driver Thread-Topic: [PATCH v2 2/3] ACPI:RAS2: Add ACPI RAS2 driver Thread-Index: AQHbjfjU3SdB6rbarkWzBYMyJX3PgbNlxP+AgAAg7jA= Date: Thu, 6 Mar 2025 11:21:51 +0000 Message-ID: References: <20250305180225.1226-1-shiju.jose@huawei.com> <20250305180225.1226-3-shiju.jose@huawei.com> <20250306171925.00002721@huawei.com> In-Reply-To: <20250306171925.00002721@huawei.com> Accept-Language: en-GB, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.195.35.177] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-Rspamd-Server: rspam08 X-Rspamd-Queue-Id: AC26F20006 X-Stat-Signature: g7jnoean77t9e4s89oweq5ckstkffu36 X-Rspam-User: X-HE-Tag: 1741260116-49714 X-HE-Meta: U2FsdGVkX1+nXRaxXIIxE15vwPq7M779JkdRHKmrQyAoCHS8NniRf8Xqy2x0ElLcnr9ovVDznZWCtgisZOgaKd7lwcb505AdcPncjyk/BZKcyNW2geTlXL0WW7HtwnfyKCCnpH+HVOyCztQblBfxlDjQKVxLpZetBuEk50HX/AUtwcr1w2QyxIh1l4KDMryhiWnlhzrYAgaPUZb1lf6fYAxFWkupfIMyUm8bGU+hJnYN4atmAhks3O8IzezGjdK9Kw+pDwe+0YLTVtGMRaZrK1Vbn8HKApmdXwyvSkm6AbJD6k4lJsNBkSnHJoLYRSTrdc3v3kDgPZUe/WBBBXnPMxl+DHzi01pYebLA54B9rhMcaZa0pU2lzUbTef6PeITebbsYD28K1gxKQzXYKCXb8EKNrQZVNXNQLo/oMUkMBsuk4Qx+Armc8lgX5/ijlpC1yh7UhOVC7s4rAgv6SHen046mNT9LE4Rd0uBLNtGKoGSO8NC7OzASFlewGmQfBUWlsTol3suvcdLD4psIe9JCH3X+c9D5OIQAMC4P/asvfAfcSvXHybo/zWW/fxw0EOiIpcmbz3Y80zwlk73+hCX2UcZ9WLe/7OjVOvhk0ek2Tw9z72DZrt+tdmRIzvJ1Zo+rxslyCkghqxzFbhZREcFPJDg1VuUyyapJTMcKGvKQuiHBfkceyahG1RFFVll4hthJiCaFmWnNWxYvozmCpuEHZzM+BSwMdvXD7tLBauAK27B1JCvDJ/bwA+dllU/jtsomJNKS3hdLQEewYaXJws8eryqIpmIoSPG5gx0NeyWHUMbB4q5MDdZ5jDysFkgVsScr+K8TXOzqnwSCHsOp5bL3LEj+84MqeNsy1Ry95GuQrRjyogIudxiuIT5iQIF2WOui9OflsB0K73wAqGKBjPRgFygi2mYBJ20SKiAi2p2zMXmAxoHvyVcV5RknNF/7DYEzmk4xsAN6cLxg/ZJXCKM fFactuOz 6XmZ6buM20HG7PSZQ8Mi7sEusSQEWFj5T297v2Ik1rK1pbEs/LgE83NwH5gemr8tHyHu+Eu6g8OhcsO5lIsJweMxxV0m9ndGFDGDQw8Xh9Gc10XxOA9QMSrT3HIgK7TgGZ4qLRKFb+5faxb3A46dRPzS6YcTcsKfweq7av3rizUtPTp7CWKD14L4klNVJrxgFM/H12FyTtGaK9gbjxqZ16F+CeR0U6ts9eTv+yoaNO+LKHi8CDpzbCFicALzfomV0X2tiMOLT3KO9+6WG3cJF4UjCoaDMYTiTLZ+RhVNJVWgn0G+H7Qpyuf9gI8XQML08eiGG0ld39RVl5byVE0h/9lu2uxmkrxsyDTII4YCFxieGdlKMi2naeyrkSqUXO68LhvkLWh9sWPBmJNcLOoVcDFVNcAZNOiq+XJlPTg08y3aL0QokDD3is6EQVbskevT2PlXakZt7qBS6pmwEstn9U8DVG7YtU82V0MMzrx6mUXHF8RkUm0uiwVfUByUc3+Hs2gfKQkg1knJS9PmjtLDfEjw2ldfmzWBX4xc1s14JJHIDg8hSYBH6D7Q4O4Z1quXTKCjs3EefOhal0tHm6R+cLcuRGnVRriWztRAhhRwfh4OsCATKhOyZKYh/OJqOnod5piWseVnbrpGo7fdCcwelri8m93TPG21vmEam5Gzpj8yASM0IgERCcinpZ4S6NcPJaCFEWcNdco8W7KbkRa2KqXdUrWChjSkSE8FqNhEeYA686tmW8yo9R8O1NT6rKsPLAgyhJYP43SGwR9ONdWkIy3F+sRPhFmvayX+V/ZZ+zMYaT71n3nv+kOoRQChcRA1Jc8xbirUyJWplTM3WX7YhQND1gg== 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: Jonathan Cameron >Sent: 06 March 2025 09:19 >To: Shiju Jose >Cc: linux-edac@vger.kernel.org; linux-acpi@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; linux- >cxl@vger.kernel.org; dan.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; linux- >mm@kvack.org; linux-kernel@vger.kernel.org; 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 2/3] ACPI:RAS2: Add ACPI RAS2 driver > >On Wed, 5 Mar 2025 18:02:23 +0000 > wrote: > >> From: Shiju Jose >> >> Add support for ACPI RAS2 feature table (RAS2) defined in the ACPI 6.5 >> Specification, section 5.2.21. >> Driver defines RAS2 Init, which extracts the RAS2 table and driver >> adds auxiliary device for each memory feature which binds to the >> RAS2 memory driver. >> >> Driver uses PCC mailbox to communicate with the ACPI HW and the driver >> adds OSPM interfaces to send RAS2 commands. >> >> Acked-by: Rafael J. Wysocki >> Co-developed-by: A Somasundaram >> Signed-off-by: A Somasundaram >> Co-developed-by: Jonathan Cameron >> Signed-off-by: Jonathan Cameron >> Tested-by: Daniel Ferguson >> Signed-off-by: Shiju Jose > >Hi Shiju, > >I took another look through as it's been a while and I've pretty much forg= otten >this code :( > >Anyhow, a few minor comments inline. Hi Jonathan, Thanks for the feedbacks. Please find reply inline. Thanks, Shiju > >Thanks, > >Jonathan > >> diff --git a/drivers/acpi/ras2.c b/drivers/acpi/ras2.c new file mode >> 100755 index 000000000000..8831a2bd5fab >> --- /dev/null >> +++ b/drivers/acpi/ras2.c > > > > >> +static int ras2_register_pcc_channel(struct ras2_mem_ctx *ras2_ctx, int >pcc_id) >> +{ >> + struct ras2_pcc_subspace *pcc_subspace; >> + struct pcc_mbox_chan *pcc_chan; >> + struct mbox_client *mbox_cl; >> + >> + if (pcc_id < 0) >> + return -EINVAL; >> + >> + mutex_lock(&ras2_pcc_lock); >> + list_for_each_entry(pcc_subspace, &ras2_pcc_subspaces, elem) { >> + if (pcc_subspace->pcc_id !=3D pcc_id) >> + continue; >> + ras2_ctx->pcc_subspace =3D pcc_subspace; >> + pcc_subspace->ref_count++; >> + mutex_unlock(&ras2_pcc_lock); >> + return 0; >> + } >> + mutex_unlock(&ras2_pcc_lock); >> + >> + pcc_subspace =3D kcalloc(1, sizeof(*pcc_subspace), GFP_KERNEL); > >if allocating a count of 1, why not kzalloc? Will use kzalloc. > >> + if (!pcc_subspace) >> + return -ENOMEM; > > > > >> +static int acpi_ras2_parse(void) >> +{ >> + struct acpi_ras2_pcc_desc *pcc_desc_list; >> + int pcc_id; >> + u8 count =3D 0; >> + int rc, i; >> + >> + if (ras2_tab->header.length < sizeof(struct acpi_table_ras2)) { > >extra space before < Will fix.=20 > >Maybe sizeof(*ras2_tab) is cleaner. Sure. > >> + pr_warn(FW_WARN "ACPI RAS2 table present but broken (too >short #1)\n"); >> + return -EINVAL; >> + } >> + >> + if (!ras2_tab->num_pcc_descs) { >> + pr_warn(FW_WARN "No PCC descs in ACPI RAS2 table\n"); >> + return -EINVAL; >> + } >> + >> + pcc_desc_list =3D (struct acpi_ras2_pcc_desc *)(ras2_tab + 1); >> + /* Double scan for the case of only one actual controller */ >> + pcc_id =3D -1; >> + count =3D 0; > >Already set above, so no need to do it again. I'd do it just here. Can >put it in the loop init though. Will change. > >> + for (i =3D 0; i < ras2_tab->num_pcc_descs; i++, pcc_desc_list++) { >> + if (pcc_desc_list->feature_type !=3D RAS2_FEAT_TYPE_MEMORY) >> + continue; >> + if (pcc_id =3D=3D -1) { >> + pcc_id =3D pcc_desc_list->channel_id; >> + count++; >> + } >> + if (pcc_desc_list->channel_id !=3D pcc_id) >> + count++; >> + } >> + >> + /* >> + * Workaround for the client platform with multiple scrub devices >> + * but uses single PCC subspace for communication. >> + */ >> + if (count =3D=3D 1) { >> + /* Add auxiliary device and bind ACPI RAS2 memory driver */ >> + rc =3D ras2_add_aux_device(RAS2_MEM_DEV_ID_NAME, pcc_id); >> + if (rc) >> + return rc; >> + >> + return 0; >> + } >> + >> + pcc_desc_list =3D (struct acpi_ras2_pcc_desc *)(ras2_tab + 1); >> + count =3D 0; > >Maybe set in loop init. Sure. > >> + for (i =3D 0; i < ras2_tab->num_pcc_descs; i++, pcc_desc_list++) { >> + if (pcc_desc_list->feature_type !=3D RAS2_FEAT_TYPE_MEMORY) >> + continue; >> + pcc_id =3D pcc_desc_list->channel_id; >> + /* Add auxiliary device and bind ACPI RAS2 memory driver */ >obvious enough to drop the comment I think. Will do. > >> + rc =3D ras2_add_aux_device(RAS2_MEM_DEV_ID_NAME, pcc_id); > pcc_desc_list->channel_id); >and no local variable. Ok. > >> + if (rc) >> + return rc; >> + } >> + >> + return 0; >> +} >> + >> +void __init acpi_ras2_init(void) >> +{ >> + acpi_status status; >> + int rc; >> + >> + status =3D acpi_get_table(ACPI_SIG_RAS2, 0, >> + (struct acpi_table_header **)&ras2_tab); >> + if (ACPI_FAILURE(status) || !ras2_tab) { >> + const char *msg =3D acpi_format_exception(status); >> + >> + pr_err("Failed to get table, %s\n", msg); > >If only going to use it here maybe > pr_err("Failed to get table, %s\n", > acpi_format_exception(status)); >and save on the local variable. Will change. > >> + return; >> + } >> + >> + rc =3D acpi_ras2_parse(); >> + if (rc) { >> + acpi_put_table((struct acpi_table_header *)ras2_tab); >> + pr_err("Failed to parse RAS2 table\n"); >> + } >> +} >> diff --git a/include/acpi/ras2.h b/include/acpi/ras2.h >> new file mode 100644 >> index 000000000000..5b27c1f30096 >> --- /dev/null >> +++ b/include/acpi/ras2.h >> @@ -0,0 +1,48 @@ >> +/* SPDX-License-Identifier: GPL-2.0-only */ >> +/* >> + * ACPI RAS2 driver header file >> + * >> + * Copyright (c) 2024-2025 HiSilicon Limited >> + */ >> + >> +#ifndef _ACPI_RAS2_H >> +#define _ACPI_RAS2_H >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#define RAS2_PCC_CMD_COMPLETE BIT(0) >> +#define RAS2_PCC_CMD_ERROR BIT(2) >> + >I think these bits are from table 14.11 and >generic to all PCC status registers? Should these >have more generic names rather than ras2 specific ones? Yes. Instead will use PCC_STATUS_CMD_ COMPLETE and PCC_STATUS_ ERROR from include/acpi/pcc.h.=20 > >> +/* RAS2 specific PCC commands */ >> +#define RAS2_PCC_CMD_EXEC 0x01 >Are we mixing commands and field definitions both >with prefix RAS2_PCC_CMD_ ? That is somewhat >confusing. Will add Table 5.82: .. here in the comment and=20 Is rename to PCC_CMD_ EXEC_RAS2 better? > >> + >> +#define RAS2_AUX_DEV_NAME "ras2" >> +#define RAS2_MEM_DEV_ID_NAME "acpi_ras2_mem" >> + >I would add a forwards def >struct device; Sure. > >whilst it is really unlikely that headers would ever be reorganized >such that auxiliary_bus.h would not include device.h given the embedded >device we shouldn't rely on that here. > >> +/* Data structure RAS2 table */ >> +struct ras2_mem_ctx { >> + struct auxiliary_device adev; >> + /* Lock to provide mutually exclusive access to PCC channel */ >> + struct mutex lock; >> + struct device *dev; >> + struct acpi_ras2_shmem __iomem *comm_addr; >> + void *pcc_subspace; >> + int id; >> +}; >> + >> +#ifdef CONFIG_ACPI_RAS2 >> +void __init acpi_ras2_init(void); >> +int ras2_send_pcc_cmd(struct ras2_mem_ctx *ras2_ctx, u16 cmd); >> +#else >> +static inline void acpi_ras2_init(void) { } >> + >> +static inline int ras2_send_pcc_cmd(struct ras2_mem_ctx *ras2_ctx, u16 >cmd) > >Is this stub ever needed? To me it seems unlikely >we would have a user that is built without a dependency >on CONFIG_ACPI_RAS2. This is different from acpi_ras2_init() >which makes much more sense to me. Ok will remove. > >> +{ >> + return -EOPNOTSUPP; >> +} >> +#endif >> +#endif /* _ACPI_RAS2_H */ Thanks, Shiju