From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1761309AbdAIGs7 (ORCPT ); Mon, 9 Jan 2017 01:48:59 -0500 Received: from mail-pf0-f175.google.com ([209.85.192.175]:33788 "EHLO mail-pf0-f175.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758057AbdAIGs4 (ORCPT ); Mon, 9 Jan 2017 01:48:56 -0500 Subject: Re: [PATCH] ACPI/IORT: Fix iort_node_get_id() mapping entries indexing To: Sinan Kaya , Lorenzo Pieralisi , linux-acpi@vger.kernel.org References: <20170105182921.8167-1-lorenzo.pieralisi@arm.com> <8d046cbd-d650-41c0-f0a5-96c2ffecd299@linaro.org> Cc: linux-kernel@vger.kernel.org, Tomasz Nowicki , Nate Watterson , "Rafael J. Wysocki" From: Hanjun Guo Message-ID: <7a03a869-ac31-72aa-337a-c962edc99381@linaro.org> Date: Mon, 9 Jan 2017 14:48:51 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.5.1 MIME-Version: 1.0 In-Reply-To: <8d046cbd-d650-41c0-f0a5-96c2ffecd299@linaro.org> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2017/1/9 14:34, Hanjun Guo wrote: > Hi Sinan, > > On 2017/1/8 5:09, Sinan Kaya wrote: >> On 1/5/2017 1:29 PM, Lorenzo Pieralisi wrote: >>> Commit 618f535a6062 ("ACPI/IORT: Add single mapping function") >>> introduced a function (iort_node_get_id()) to retrieve ids for IORT >>> named components. >>> >>> iort_node_get_id() takes an index as input to refer to a specific >>> mapping entry in the mapping array to retrieve the id at a specific >>> index provided the index is below the total mapping count; currently the >>> index is used to retrieve the mapping value from the correct entry but >>> not to dereference the correct entry while retrieving the mapping >>> output_reference (ie IORT parent pointer), which consequently always >>> resolves to the output_reference of the first entry in the mapping >>> array. >>> >>> Update the map array entry pointer computation in iort_node_get_id() to >>> take into account the index value, fixing the issue. >>> >>> Fixes: 618f535a6062 ("ACPI/IORT: Add single mapping function") >>> Reported-by: Hanjun Guo >>> Signed-off-by: Lorenzo Pieralisi >>> Cc: Hanjun Guo >>> Cc: Sinan Kaya >>> Cc: Tomasz Nowicki >>> Cc: Nate Watterson >>> Cc: "Rafael J. Wysocki" >>> --- >>> drivers/acpi/arm64/iort.c | 6 +++--- >>> 1 file changed, 3 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c >>> index e0d2e6e..ba156c5 100644 >>> --- a/drivers/acpi/arm64/iort.c >>> +++ b/drivers/acpi/arm64/iort.c >>> @@ -333,7 +333,7 @@ struct acpi_iort_node *iort_node_get_id(struct >>> acpi_iort_node *node, >>> return NULL; >>> >>> map = ACPI_ADD_PTR(struct acpi_iort_id_mapping, node, >>> - node->mapping_offset); >>> + node->mapping_offset + index * sizeof(*map)); >> >> What does this give us that the previous code didn't do? > > Fir example, if you have multi mappings ids under platform device: > > |-------------| > | SMMU 2 |<------- > |-------------| | > | > | > |-------------| | > | SMMU 1 |<---- | > |-------------| | | > | | > | | > |-------------| | | > | platform | | | > | device | | | > |-------------| | | > | stream id | | | > | 1 | | | > | parent------|----| | > |-------------| | > | stream id | | > | 2 | | > | parent-----|-------| > |-------------| > > For now, we just use the first entry in the mapping entry to get > the parent, and always point to the same parent, as above, we will > always map to SMMU 1 even if you connect to different SMMUs. (Although > we may don't have such device topology yet) > > >> >> You are using map as a pointer and returning the offset of the first >> map entry above >> and then accessing the map at the indexed offset with map[index] >> >> The new code is using map as a plain pointer, calculating the pointer >> location with ACPI_ADD_PTR >> instead and then collecting the output parameter with map->output_base. >> >>> >>> /* Firmware bug! */ >>> if (!map->output_reference) { >>> @@ -348,10 +348,10 @@ struct acpi_iort_node *iort_node_get_id(struct >>> acpi_iort_node *node, >>> if (!(IORT_TYPE_MASK(parent->type) & type_mask)) >>> return NULL; >>> >>> - if (map[index].flags & ACPI_IORT_ID_SINGLE_MAPPING) { >>> + if (map->flags & ACPI_IORT_ID_SINGLE_MAPPING) { >>> if (node->type == ACPI_IORT_NODE_NAMED_COMPONENT || >>> node->type == ACPI_IORT_NODE_PCI_ROOT_COMPLEX) { >>> - *id_out = map[index].output_base; >>> + *id_out = map->output_base; >> >> You are claiming that the existing code is collecting the output >> parameter from the first mapping. >> I don't see this happening above. >> >> What am I missing? > > It's not about the output id but it's about the parent returned > by this function, it always return the first entry's parent in the > mapping entry. And as Lorenzo said in the commit message: "always resolves to the output_reference of the first entry in the mapping array." Then always judge the same entry for: if (!map->output_reference) pr_err(); ... Thanks Hanjun