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 vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id C34F4C77B7C for ; Fri, 12 May 2023 11:59:04 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S240936AbjELL7E (ORCPT ); Fri, 12 May 2023 07:59:04 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:42836 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S241105AbjELL7B (ORCPT ); Fri, 12 May 2023 07:59:01 -0400 Received: from frasgout.his.huawei.com (frasgout.his.huawei.com [185.176.79.56]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 80F7414379; Fri, 12 May 2023 04:58:35 -0700 (PDT) Received: from lhrpeml500005.china.huawei.com (unknown [172.18.147.201]) by frasgout.his.huawei.com (SkyGuard) with ESMTP id 4QHnLC1qQcz67lJ3; Fri, 12 May 2023 19:56:51 +0800 (CST) Received: from localhost (10.202.227.76) by lhrpeml500005.china.huawei.com (7.191.163.240) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.23; Fri, 12 May 2023 12:58:32 +0100 Date: Fri, 12 May 2023 12:58:31 +0100 From: Jonathan Cameron To: Dave Jiang CC: , , , , , , , , Subject: Re: [PATCH 1/4] acpi: tables: Add CDAT table parsing support Message-ID: <20230512125831.00000eb0@Huawei.com> In-Reply-To: <168330797672.2042604.12339416734589446371.stgit@djiang5-mobl3> References: <168330787964.2042604.17648905811002211147.stgit@djiang5-mobl3> <168330797672.2042604.12339416734589446371.stgit@djiang5-mobl3> Organization: Huawei Technologies Research and Development (UK) Ltd. X-Mailer: Claws Mail 4.1.0 (GTK 3.24.33; x86_64-w64-mingw32) MIME-Version: 1.0 Content-Type: text/plain; charset="US-ASCII" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.202.227.76] X-ClientProxiedBy: lhrpeml500002.china.huawei.com (7.191.160.78) To lhrpeml500005.china.huawei.com (7.191.163.240) X-CFilter-Loop: Reflected Precedence: bulk List-ID: X-Mailing-List: linux-acpi@vger.kernel.org On Fri, 05 May 2023 10:32:56 -0700 Dave Jiang wrote: > The CDAT table is very similar to ACPI tables when it comes to sub-table > and entry structures. The helper functions can be also used to parse the > CDAT table. Add support to the helper functions to deal with an external > CDAT table, and also handle the endieness since CDAT can be processed by a > BE host. Export a function acpi_table_parse_cdat() for CXL driver to parse > a CDAT table. > > In order to minimize ACPI code changes, __force is being utilized to deal > with the case of a big endien (BE) host parsing a CDAT. All CDAT data > structure variables are being force casted to __leX as appropriate. Hi Dave, This falls into the annoyance that CDAT doesn't have a standard table header. Whilst I understand that was done deliberately it means some odd things happen in this code. Just how bad is the duplication if we don't do this at all, but instead roll a version for CDAT that doesn't force things through pointers of the wrong types? Otherwise, maybe we need some unions so that the type mashups don't happen. > > Cc: Rafael J. Wysocki > Cc: Len Brown > Signed-off-by: Dave Jiang > --- > drivers/acpi/tables.c | 47 +++++++++++++++++++++++++++++++++++++++++++++-- > include/acpi/actbl1.h | 3 +++ > include/linux/acpi.h | 4 ++++ > 3 files changed, 52 insertions(+), 2 deletions(-) > > diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c > index 7b4680da57d7..08486f6df442 100644 > --- a/drivers/acpi/tables.c > +++ b/drivers/acpi/tables.c > @@ -42,6 +42,7 @@ enum acpi_subtable_type { > ACPI_SUBTABLE_HMAT, > ACPI_SUBTABLE_PRMT, > ACPI_SUBTABLE_CEDT, > + ACPI_SUBTABLE_CDAT, > }; > > struct acpi_subtable_entry { > @@ -239,6 +240,8 @@ acpi_get_entry_type(struct acpi_subtable_entry *entry) > return 0; > case ACPI_SUBTABLE_CEDT: > return entry->hdr->cedt.type; > + case ACPI_SUBTABLE_CDAT: > + return entry->hdr->cdat.type; > } > return 0; > } > @@ -255,6 +258,8 @@ acpi_get_entry_length(struct acpi_subtable_entry *entry) > return entry->hdr->prmt.length; > case ACPI_SUBTABLE_CEDT: > return entry->hdr->cedt.length; > + case ACPI_SUBTABLE_CDAT: > + return le16_to_cpu((__force __le16)entry->hdr->cdat.length); > } > return 0; > } > @@ -271,6 +276,8 @@ acpi_get_subtable_header_length(struct acpi_subtable_entry *entry) > return sizeof(entry->hdr->prmt); > case ACPI_SUBTABLE_CEDT: > return sizeof(entry->hdr->cedt); > + case ACPI_SUBTABLE_CDAT: > + return sizeof(entry->hdr->cdat); > } > return 0; > } > @@ -284,9 +291,22 @@ acpi_get_subtable_type(char *id) > return ACPI_SUBTABLE_PRMT; > if (strncmp(id, ACPI_SIG_CEDT, 4) == 0) > return ACPI_SUBTABLE_CEDT; > + if (strncmp(id, ACPI_SIG_CDAT, 4) == 0) > + return ACPI_SUBTABLE_CDAT; I'm not super keen on inventing a SIG when the CDAT 'table' doesn't actually have one. > return ACPI_SUBTABLE_COMMON; > } > > +static unsigned long __init_or_acpilib > +acpi_table_get_length(enum acpi_subtable_type type, > + struct acpi_table_header *hdr) I don't like parsing in an acpi_table_header type here when it may not be one. I think this length decision needs to be pushed up a level to where we can see if we have a CDAT table or not. > +{ > + if (type == ACPI_SUBTABLE_CDAT) > + return le32_to_cpu( > + (__force __le32)((struct acpi_table_cdat *)hdr)->length); Perhaps a local variable in here somewhere would make it more readable. __le32 length = (__force__le32)((struct acpi_table_cdat *)hdr)->length; return le32_to_cpu(length)? > + > + return hdr->length; > +} > + > static __init_or_acpilib bool has_handler(struct acpi_subtable_proc *proc) > { > return proc->handler || proc->handler_arg; > @@ -332,16 +352,19 @@ static int __init_or_acpilib acpi_parse_entries_array( > int proc_num, unsigned int max_entries) > { > struct acpi_subtable_entry entry; > + enum acpi_subtable_type type; > unsigned long table_end, subtable_len, entry_len; > int count = 0; > int errs = 0; > int i; > > - table_end = (unsigned long)table_header + table_header->length; > + type = acpi_get_subtable_type(id); > + table_end = (unsigned long)table_header + > + acpi_table_get_length(type, table_header); As above, I don't like carrying CDAT which doesn't have an acpi_table_header section around as that type of pointer. > > /* Parse all entries looking for a match. */ > > - entry.type = acpi_get_subtable_type(id); > + entry.type = type; > entry.hdr = (union acpi_subtable_headers *) > ((unsigned long)table_header + table_size); > subtable_len = acpi_get_subtable_header_length(&entry); > @@ -464,6 +487,26 @@ int __init acpi_table_parse_madt(enum acpi_madt_type id, > handler, max_entries); > } > > +int acpi_table_parse_cdat(enum acpi_cdat_type type, > + acpi_tbl_entry_handler_arg handler_arg, void *arg, > + struct acpi_table_cdat *table_header) > +{ > + struct acpi_subtable_proc proc = { > + .id = type, > + .handler_arg = handler_arg, > + .arg = arg, > + }; > + > + if (!table_header) > + return -EINVAL; > + > + return acpi_parse_entries_array(ACPI_SIG_CDAT, > + sizeof(struct acpi_table_cdat), > + (struct acpi_table_header *)table_header, > + &proc, 1, 0); > +} > +EXPORT_SYMBOL_NS_GPL(acpi_table_parse_cdat, CXL); > + > /** > * acpi_table_parse - find table with @id, run @handler on it > * @id: table id to find > diff --git a/include/acpi/actbl1.h b/include/acpi/actbl1.h > index 81b9e794424d..3119be093cfe 100644 > --- a/include/acpi/actbl1.h > +++ b/include/acpi/actbl1.h > @@ -66,6 +66,9 @@ > #define ACPI_SIG_IEIT "IEIT" > #endif > > +/* External to ACPI */ > +#define ACPI_SIG_CDAT "CDAT" /* Coherent Device Attribute Table */ Worse that that, fictional signature :) It's the nameof the 'table', but it's not a signature as it's never used as they are in ACPI and doesn't appear anywhere in the table. > + > /* > * All tables must be byte-packed to match the ACPI specification, since > * the tables are provided by the system BIOS. > diff --git a/include/linux/acpi.h b/include/linux/acpi.h > index efff750f326d..4c3dfe7587e9 100644 > --- a/include/linux/acpi.h > +++ b/include/linux/acpi.h > @@ -135,6 +135,7 @@ union acpi_subtable_headers { > struct acpi_hmat_structure hmat; > struct acpi_prmt_module_header prmt; > struct acpi_cedt_header cedt; > + struct acpi_cdat_header cdat; > }; > > typedef int (*acpi_tbl_table_handler)(struct acpi_table_header *table); > @@ -266,6 +267,9 @@ acpi_table_parse_cedt(enum acpi_cedt_type id, > > int acpi_parse_mcfg (struct acpi_table_header *header); > void acpi_table_print_madt_entry (struct acpi_subtable_header *madt); > +int acpi_table_parse_cdat(enum acpi_cdat_type type, > + acpi_tbl_entry_handler_arg handler, void *arg, > + struct acpi_table_cdat *table_header); How did we end up with an 'acpi_' table that isn't in ACPI? (I'm not looking as I fear I might be responsible :) Should perhaps consider renaming all the CDAT entries so it doesn't looks like they are. > > /* the following numa functions are architecture-dependent */ > void acpi_numa_slit_init (struct acpi_table_slit *slit); > > >