[PATCH] ACPI/IORT: Fix iort_node_get_id() mapping entries indexing

[PATCH] ACPI/IORT: Fix iort_node_get_id() mapping entries indexing

[Lorenzo Pieralisi]

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 <hanjun.guo@linaro.org>
Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Hanjun Guo <hanjun.guo@linaro.org>
Cc: Sinan Kaya <okaya@codeaurora.org>
Cc: Tomasz Nowicki <tn@semihalf.com>
Cc: Nate Watterson <nwatters@codeaurora.org>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
---
 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));
 
 	/* 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;
 			return parent;
 		}
 	}
-- 
2.10.0

Re: [PATCH] ACPI/IORT: Fix iort_node_get_id() mapping entries indexing

[Sinan Kaya]

On 1/5/2017 1:29 PM, Lorenzo Pieralisi wrote:
> 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.

Maybe, irrelevant but I'm going to throw it out.

Can we somehow break these multiple input and multiple output single
function into smaller and manageable pieces while we are fixing a bug
on it?

These functions seem to do N different things when an input is present
or not. 

I got lost in these function when implementing ITS->SMMU->NC use case.

-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH] ACPI/IORT: Fix iort_node_get_id() mapping entries indexing

[Lorenzo Pieralisi]

On Thu, Jan 05, 2017 at 01:37:22PM -0500, Sinan Kaya wrote:
> On 1/5/2017 1:29 PM, Lorenzo Pieralisi wrote:
> > 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.
> 
> Maybe, irrelevant but I'm going to throw it out.
> 
> Can we somehow break these multiple input and multiple output single
> function into smaller and manageable pieces while we are fixing a bug
> on it?

No. This is a fix for code currently in the kernel that is going
to be released in a few weeks, it is not a clean-up, please let me
know if it works on your platforms.

> These functions seem to do N different things when an input is present
> or not. 
> 
> I got lost in these function when implementing ITS->SMMU->NC use case.

Hanjun is carrying out the clean-up of the mapping functions as part of
its platform MSIs patchset, I followed up on that already to achieve
what you request above.

Thanks,
Lorenzo

Re: [PATCH] ACPI/IORT: Fix iort_node_get_id() mapping entries indexing

[Sinan Kaya]

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 <hanjun.guo@linaro.org>
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Hanjun Guo <hanjun.guo@linaro.org>
> Cc: Sinan Kaya <okaya@codeaurora.org>
> Cc: Tomasz Nowicki <tn@semihalf.com>
> Cc: Nate Watterson <nwatters@codeaurora.org>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> ---
>  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?

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?

>  			return parent;
>  		}
>  	}
> 

If we are just doing a housekeeping, this is fine. I couldn't see an actual bug getting fixed.

-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH] ACPI/IORT: Fix iort_node_get_id() mapping entries indexing

[Hanjun Guo]

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 <hanjun.guo@linaro.org>
>> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>> Cc: Hanjun Guo <hanjun.guo@linaro.org>
>> Cc: Sinan Kaya <okaya@codeaurora.org>
>> Cc: Tomasz Nowicki <tn@semihalf.com>
>> Cc: Nate Watterson <nwatters@codeaurora.org>
>> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
>> ---
>>  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.

>
>>  			return parent;
>>  		}
>>  	}
>>
>
> If we are just doing a housekeeping, this is fine. I couldn't see an actual bug getting fixed.

Although we may don't have such use cases for now, but I think we
need to prepare for it, it worth a bugfix I think :)

Thanks
Hanjun

Re: [PATCH] ACPI/IORT: Fix iort_node_get_id() mapping entries indexing

[Hanjun Guo]

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 <hanjun.guo@linaro.org>
>>> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>>> Cc: Hanjun Guo <hanjun.guo@linaro.org>
>>> Cc: Sinan Kaya <okaya@codeaurora.org>
>>> Cc: Tomasz Nowicki <tn@semihalf.com>
>>> Cc: Nate Watterson <nwatters@codeaurora.org>
>>> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
>>> ---
>>>  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

Re: [PATCH] ACPI/IORT: Fix iort_node_get_id() mapping entries indexing

[Hanjun Guo]

On 2017/1/6 2:29, 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 <hanjun.guo@linaro.org>
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Hanjun Guo <hanjun.guo@linaro.org>
> Cc: Sinan Kaya <okaya@codeaurora.org>
> Cc: Tomasz Nowicki <tn@semihalf.com>
> Cc: Nate Watterson <nwatters@codeaurora.org>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> ---
>  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));
>
>  	/* 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;
>  			return parent;
>  		}
>  	}

Reviewed-by: Hanjun Guo <hanjun.guo@linaro.org>

Re: [PATCH] ACPI/IORT: Fix iort_node_get_id() mapping entries indexing

[okaya]

On 2017-01-09 01: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 <hanjun.guo@linaro.org>
>>> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>>> Cc: Hanjun Guo <hanjun.guo@linaro.org>
>>> Cc: Sinan Kaya <okaya@codeaurora.org>
>>> Cc: Tomasz Nowicki <tn@semihalf.com>
>>> Cc: Nate Watterson <nwatters@codeaurora.org>
>>> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
>>> ---
>>>  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)

I see. This wasn't obvious from the commit message. Thanks for taking 
time to explain it.

I think the commit message needs to include some of your description.

> 
> 
>> 
>> 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.

Ok, I was judt looking at the patch. I didn't realize ww are changing 
the return value.

This could have been mentioned.


> 
>> 
>>>  			return parent;
>>>  		}
>>>  	}
>>> 
>> 
>> If we are just doing a housekeeping, this is fine. I couldn't see an 
>> actual bug getting fixed.
> 
> Although we may don't have such use cases for now, but I think we
> need to prepare for it, it worth a bugfix I think :)
> 
> Thanks
> Hanjun