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 gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (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 DA4E9C02196 for ; Wed, 5 Feb 2025 20:37:43 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 8828F10E3E8; Wed, 5 Feb 2025 20:37:43 +0000 (UTC) Authentication-Results: gabe.freedesktop.org; dkim=pass (2048-bit key; unprotected) header.d=kernel.org header.i=@kernel.org header.b="CZc4ASco"; dkim-atps=neutral Received: from nyc.source.kernel.org (nyc.source.kernel.org [147.75.193.91]) by gabe.freedesktop.org (Postfix) with ESMTPS id 3A30310E3E5; Wed, 5 Feb 2025 20:36:29 +0000 (UTC) Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by nyc.source.kernel.org (Postfix) with ESMTP id 1A055A43823; Wed, 5 Feb 2025 20:34:42 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 1F6C6C4CED1; Wed, 5 Feb 2025 20:36:27 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1738787787; bh=ELRCctPBnVLkgoDBoJ3670v6AboAOxNEir9mJ6kHZCo=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=CZc4AScoUtsDsLDDzdtIID6g7vTi03YGoyEQX5K47rq22JJF66TA372ENAjREHLTL cAGO+1I/FA3CRf22QHNWmUp2GdKb8Fy4pao1BY7+THzk12ZmadmB5n8pk7WLXLtDUB +ZwC5m0BxlggKmQb170fDGLNaIff41YOoPkFlZPOzheg5inOFJRlLmHo0UNnVUScXO PyeSgRa/iV8YSDhnaeccRJYQdgSURdlJtV3N0xOQp9Z2wUIDa//ddgC3QdkrEp7fj6 4HXt73fneV2S0nRSE7HxEawmnwkjF1hnEkcXlQqBqepEhgWjcypOREUEXtxJajknXU N4SXe/kJf8cXA== Message-ID: <1a046955-de2e-4d5f-8b47-ca90d7aebf9f@kernel.org> Date: Wed, 5 Feb 2025 14:36:26 -0600 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] drm/amd: Refactor find_system_memory() To: Felix Kuehling , mario.limonciello@amd.com, alexander.deucher@amd.com, christian.koenig@amd.com, Xinhui.Pan@amd.com, airlied@gmail.com, simona@ffwll.ch Cc: amd-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org References: <20250204222140.3883013-1-superm1@kernel.org> <65b47db9-883b-46c6-919a-e84c3ffde401@amd.com> <0451e510-f8e4-4539-a3ce-cc9fbe71964f@kernel.org> <30fbb0d6-b911-4a99-bcdc-38e2ede5eaa0@amd.com> Content-Language: en-US From: Mario Limonciello In-Reply-To: <30fbb0d6-b911-4a99-bcdc-38e2ede5eaa0@amd.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-BeenThere: amd-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Discussion list for AMD gfx List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: amd-gfx-bounces@lists.freedesktop.org Sender: "amd-gfx" On 2/5/2025 14:33, Felix Kuehling wrote: > > > On 2025-02-05 14:31, Mario Limonciello wrote: >> On 2/4/2025 17:19, Felix Kuehling wrote: >>> >>> On 2025-02-04 17:21, Mario Limonciello wrote: >>>> From: Mario Limonciello >>>> >>>> find_system_memory() pulls out two fields from an SMBIOS type 17 >>>> device and sets them on KFD devices. This however is pulling from >>>> the middle of the field in the SMBIOS device and leads to an unaligned >>>> access. >>>> >>>> Instead use a struct representation to access the members and pull >>>> out the two specific fields. >>> >>> Isn't that still an unaligned access? I don't understand the purpose of this patch. >> >> Unless I added wrong, it looked to me that the offset it was pulling from previously was incorrect.  So I was expecting it should be aligned (and less error prone) to pull from the correct offset from a struct. > > The way I see it, the offsets that were used before were correct and match the offsets in the packed structure definition. I'm annotating the offsets from the end of the header in the structure definition below as proof. > The problem I saw was that the dmi_data field starts a byte late though. That's why I was thinking this is the source of the unaligned access and the mistake. Let me annotate below. >> >>> >>> One more comment inline. >>> >>>> >>>> Link: https://www.dmtf.org/sites/default/files/standards/documents/DSP0134_3.8.0.pdf p99 >>>> Signed-off-by: Mario Limonciello >>>> --- >>>>   drivers/gpu/drm/amd/amdkfd/kfd_topology.c | 27 +++++++++++------------ >>>>   drivers/gpu/drm/amd/amdkfd/kfd_topology.h | 17 ++++++++++++++ >>>>   2 files changed, 30 insertions(+), 14 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c >>>> index ceb9fb475ef13..93d3924dfcba0 100644 >>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c >>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c >>>> @@ -968,24 +968,23 @@ static void kfd_update_system_properties(void) >>>>       up_read(&topology_lock); >>>>   } >>>>   -static void find_system_memory(const struct dmi_header *dm, >>>> -    void *private) >>>> +static void find_system_memory(const struct dmi_header *dm, void *private) >>>>   { >>>> +    struct dmi_mem_device *memdev = (struct dmi_mem_device *)(dm); >>> >>> I think it would be cleaner to use container_of to get a pointer to the structure containing the header. >> >> Ack. >> >>> >>> Regards, >>>    Felix >>> >>>>       struct kfd_mem_properties *mem; >>>> -    u16 mem_width, mem_clock; >>>>       struct kfd_topology_device *kdev = >>>>           (struct kfd_topology_device *)private; >>>> -    const u8 *dmi_data = (const u8 *)(dm + 1); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ >>>> - >>>> -    if (dm->type == DMI_ENTRY_MEM_DEVICE && dm->length >= 0x15) { >>>> -        mem_width = (u16)(*(const u16 *)(dmi_data + 0x6)); >>>> -        mem_clock = (u16)(*(const u16 *)(dmi_data + 0x11)); >>>> -        list_for_each_entry(mem, &kdev->mem_props, list) { >>>> -            if (mem_width != 0xFFFF && mem_width != 0) >>>> -                mem->width = mem_width; >>>> -            if (mem_clock != 0) >>>> -                mem->mem_clk_max = mem_clock; >>>> -        } >>>> + >>>> +    if (memdev->header.type != DMI_ENTRY_MEM_DEVICE) >>>> +        return; >>>> +    if (memdev->header.length < sizeof(struct dmi_mem_device)) >>>> +        return; >>>> + >>>> +    list_for_each_entry(mem, &kdev->mem_props, list) { >>>> +        if (memdev->total_width != 0xFFFF && memdev->total_width != 0) >>>> +            mem->width = memdev->total_width; >>>> +        if (memdev->speed != 0) >>>> +            mem->mem_clk_max = memdev->speed; >>>>       } >>>>   } >>>>   diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_topology.h b/drivers/gpu/drm/amd/amdkfd/kfd_topology.h >>>> index 155b5c410af16..f06c9db7ddde9 100644 >>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_topology.h >>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_topology.h >>>> @@ -24,6 +24,7 @@ >>>>   #ifndef __KFD_TOPOLOGY_H__ >>>>   #define __KFD_TOPOLOGY_H__ >>>>   +#include >>>>   #include >>>>   #include >>>>   #include >>>> @@ -179,6 +180,22 @@ struct kfd_system_properties { >>>>       struct attribute    attr_props; >>>>   }; >>>>   +struct dmi_mem_device { >>>> +    struct dmi_header header; > > Comments below to annotate the byte offset of each field from the end of the header. > >>>> +    u16 physical_handle; // 0x0 >>>> +    u16 error_handle; // 0x2 >>>> +    u16 total_width; // 0x4 >>>> +    u16 data_width; // 0x6 (matches the original code) >>>> +    u16 size; // 0x8 >>>> +    u8 form_factor; // 0xa >>>> +    u8 device_set; // 0xb >>>> +    u8 device_locator; // 0xc >>>> +    u8 bank_locator; // 0xd >>>> +    u8 memory_type; // 0xe >>>> +    u16 type_detail; // 0xf >>>> +    u16 speed; // 0x11 (matches the original code) >>>> +} __packed; > > The bottom line is, this patch doesn't change anything about which DMI data is accessed. It's still an unaligned access. Now I think your patch is still a decent cleanup. But the justification in the commit description doesn't make sense. > > Regards, > Felix > >>>> + >>>>   struct kfd_topology_device *kfd_create_topology_device( >>>>           struct list_head *device_list); >>>>   void kfd_release_topology_device_list(struct list_head *device_list); >>> >> >