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 bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id AF4ACCED27F for ; Tue, 8 Oct 2024 09:00:03 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:Content-Type:MIME-Version: References:In-Reply-To:Subject:Cc:To:From:Message-ID:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=vjOdrJ0NpWtcOGYxymSj+vEES04vvxURtfCAQITGUqQ=; b=AoXBMZ8+1plAGzKIna7wvYncTc o08WdE1xLKoknCJMqqfi54lCwK4J91uPj3I3gvr+rCn+bRIW8maYKl8ELBRD/P3W60exwqpUYrZWj plJDNaml0aLkBjpvIO/E+e8B2U7clpCZUQ4FMv3yOujqDYQhaEed2YjmpVFsrf9Bw9Em9jwzYx3XH /CgO/A2QfcBPDeq4XArO/Efq2FTUUvuri4SDZDmH5HecNxzAaW4ZdlQcq9eBUMJXcrOVUMT6RrM0M eBfDJIKW37n/C/Xdtm+opI/iRQ2yB1tV0R1Nk0SiT013hQ1lobYGVM0eHaZ7QsJGd1zA1fdGuwHZ3 RgNlpsFw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.98 #2 (Red Hat Linux)) id 1sy64L-00000005BcD-1Mxq; Tue, 08 Oct 2024 08:59:49 +0000 Received: from nyc.source.kernel.org ([147.75.193.91]) by bombadil.infradead.org with esmtps (Exim 4.98 #2 (Red Hat Linux)) id 1sy5zo-00000005ABB-2Eni for linux-arm-kernel@lists.infradead.org; Tue, 08 Oct 2024 08:55:10 +0000 Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by nyc.source.kernel.org (Postfix) with ESMTP id B261CA433C0; Tue, 8 Oct 2024 08:54:58 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id E7D05C4CEC7; Tue, 8 Oct 2024 08:55:06 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1728377707; bh=dzsCD0bOgufP2zzatT6Djh1eOmuug+qqhpBIirk2fkI=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=b2pun7Sfw15Ull7ICokZ0OX86X5Q/VjK9Nqp5eEmmQqGrDoGFC5/r7zTb+gYZDiRZ lEdPXvfAseOteH8yCIdg39lij/ALnjGz1EYaB0QJa8f4qDe7t9eHLmt30LdTsUqU/B WbWpiiYHespUcN21yFrnFEgfkMmHdjsZE6JYHIV3edFJrV4HjdqVpdBXMUM6bCKwdy pZh2WVxmwW8plf+5lo3LvVpuxpUmxwjQc0ZRHyYRfrfkEbiClkhnQnqSId/FG43aJl EGKq1NwGrYu4qiHd+VY7ZB1JmDCTH7YGNg2zL1o36+sZ1OILfb5DCJqWSUdhwYgtqR 6qNLTX6qBZgXA== Received: from sofa.misterjones.org ([185.219.108.64] helo=goblin-girl.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.95) (envelope-from ) id 1sy5zk-001IA4-J4; Tue, 08 Oct 2024 09:55:04 +0100 Date: Tue, 08 Oct 2024 09:55:04 +0100 Message-ID: <86v7y355zr.wl-maz@kernel.org> From: Marc Zyngier To: Zheng Zengkai Cc: , , , , , , , , , , Subject: Re: [PATCH v2] ACPI: GTDT: simplify acpi_gtdt_init() implementation In-Reply-To: <20241008082429.33646-1-zhengzengkai@huawei.com> References: <20241008082429.33646-1-zhengzengkai@huawei.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/29.4 (aarch64-unknown-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII X-SA-Exim-Connect-IP: 185.219.108.64 X-SA-Exim-Rcpt-To: zhengzengkai@huawei.com, lpieralisi@kernel.org, guohanjun@huawei.com, sudeep.holla@arm.com, mark.rutland@arm.com, rafael@kernel.org, lenb@kernel.org, daniel.lezcano@linaro.org, tglx@linutronix.de, linux-acpi@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20241008_015508_770970_B4946183 X-CRM114-Status: GOOD ( 34.43 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Tue, 08 Oct 2024 09:24:29 +0100, Zheng Zengkai wrote: > > According to GTDT Table Structure of ACPI specification, the result of > expression '(void *)gtdt + gtdt->platform_timer_offset' will be same > with the expression '(void *)table + sizeof(struct acpi_table_gtdt)' There is no such language in the spec. It simply says "Offset to the Platform Timer Structure[] array from the start of this table". > in function acpi_gtdt_init(), so the condition of the "invalid timer > data" check will never be true, remove the EINVAL error check branch > and change to void return type for acpi_gtdt_init() to simplify the > function implementation and error handling by callers. And ACPI tables are well known to be always correct, right? > > Besides, after commit c2743a36765d ("clocksource: arm_arch_timer: add > GTDT support for memory-mapped timer"), acpi_gtdt_init() currently will > not be called with parameter platform_timer_count set to NULL and we > can explicitly initialize the integer variable which is used for storing > the number of platform timers by caller to zero, so there is no need to > do null pointer check for platform_timer_count in acpi_gtdt_init(), > remove it to make code a bit more concise. > > Signed-off-by: Zheng Zengkai > --- > Changes in v2: > - initialize 'ret' in gtdt_sbsa_gwdt_init() to silence build warning > > v1: https://lore.kernel.org/all/20240930030716.179992-1-zhengzengkai@huawei.com/ > --- > drivers/acpi/arm64/gtdt.c | 31 +++++++--------------------- > drivers/clocksource/arm_arch_timer.c | 6 ++---- > include/linux/acpi.h | 2 +- > 3 files changed, 11 insertions(+), 28 deletions(-) > > diff --git a/drivers/acpi/arm64/gtdt.c b/drivers/acpi/arm64/gtdt.c > index c0e77c1c8e09..7fe27c0edde7 100644 > --- a/drivers/acpi/arm64/gtdt.c > +++ b/drivers/acpi/arm64/gtdt.c > @@ -147,45 +147,30 @@ bool __init acpi_gtdt_c3stop(int type) > * @table: The pointer to GTDT table. > * @platform_timer_count: It points to a integer variable which is used > * for storing the number of platform timers. > - * This pointer could be NULL, if the caller > - * doesn't need this info. > - * > - * Return: 0 if success, -EINVAL if error. > */ > -int __init acpi_gtdt_init(struct acpi_table_header *table, > +void __init acpi_gtdt_init(struct acpi_table_header *table, > int *platform_timer_count) > { > - void *platform_timer; > struct acpi_table_gtdt *gtdt; > > gtdt = container_of(table, struct acpi_table_gtdt, header); > acpi_gtdt_desc.gtdt = gtdt; > acpi_gtdt_desc.gtdt_end = (void *)table + table->length; > acpi_gtdt_desc.platform_timer = NULL; > - if (platform_timer_count) > - *platform_timer_count = 0; > > if (table->revision < 2) { > pr_warn("Revision:%d doesn't support Platform Timers.\n", > table->revision); > - return 0; > + return; > } > > if (!gtdt->platform_timer_count) { > pr_debug("No Platform Timer.\n"); > - return 0; > + return; > } > > - platform_timer = (void *)gtdt + gtdt->platform_timer_offset; > - if (platform_timer < (void *)table + sizeof(struct acpi_table_gtdt)) { > - pr_err(FW_BUG "invalid timer data.\n"); > - return -EINVAL; > - } > - acpi_gtdt_desc.platform_timer = platform_timer; > - if (platform_timer_count) > - *platform_timer_count = gtdt->platform_timer_count; > - > - return 0; > + acpi_gtdt_desc.platform_timer = (void *)gtdt + gtdt->platform_timer_offset; And now you are trusting something that potentially points to some unexpected location, blindly using it. It is bad enough that the current checks are pretty poor (no check against the end of the table for the first timer entry), but you are making it worse. M. -- Without deviation from the norm, progress is not possible.