[PATCH v2] ACPI: GTDT: simplify acpi_gtdt_init() implementation
Zheng Zengkai
zhengzengkai at huawei.com
Thu Oct 10 05:37:51 PDT 2024
在 2024/10/9 19:33, Marc Zyngier 写道:
> On Tue, 08 Oct 2024 15:04:52 +0100,
> Zheng Zengkai <zhengzengkai at huawei.com> wrote:
>>
>> 在 2024/10/8 16:55, Marc Zyngier 写道:
>>> On Tue, 08 Oct 2024 09:24:29 +0100,
>>> Zheng Zengkai <zhengzengkai at huawei.com> 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".
>> OK, I mean that in current code, the condition of this check is redundant.
> That's not my reading if it. Where do you see another validity check
> that makes this one superfluous?
>
>>>> 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?
>> Not always, check is needed, but should be changed.
> You are not changing it, you are getting rid of it, and I don't see
> you replacing it with anything else.
>
>>>> 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 <zhengzengkai at huawei.com>
>>>> ---
>>>> 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.
>> Can I use the second and third bytes (the length) of platform timer
>> structure to check against the end of the table ?
> That's how it is supposed to be done indeed.
OK, I will send another patch to add check against the end of the table
for the first platform timer entry.
Thanks!
> M.
>
More information about the linux-arm-kernel
mailing list