2020-02-25 09:05:57

by Heyi Guo

Subject: [PATCH] acpi/iort: check output reference for the real used mapping

The function iort_node_map_id() does the sanity check against the
first mapping in the node, but not the one which we really use.

Logically we need check the mapping we use, or check every mapping in
the node. Choose the first fix for we are not firmware tester.

Signed-off-by: Heyi Guo <[email protected]>

---
Cc: Lorenzo Pieralisi <[email protected]>
Cc: Hanjun Guo <[email protected]>
Cc: Sudeep Holla <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: Len Brown <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
---
drivers/acpi/arm64/iort.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
index ed3d2d1a7ae9..d0fe8d673240 100644
--- a/drivers/acpi/arm64/iort.c
+++ b/drivers/acpi/arm64/iort.c
@@ -470,13 +470,6 @@ static struct acpi_iort_node *iort_node_map_id(struct acpi_iort_node *node,
map = ACPI_ADD_PTR(struct acpi_iort_id_mapping, node,
node->mapping_offset);

- /* Firmware bug! */
- if (!map->output_reference) {
- pr_err(FW_BUG "[node %p type %d] ID map has NULL parent reference\n",
- node, node->type);
- goto fail_map;
- }
-
/*
* Get the special ID mapping index (if any) and skip its
* associated ID map to prevent erroneous multi-stage
@@ -497,6 +490,13 @@ static struct acpi_iort_node *iort_node_map_id(struct acpi_iort_node *node,
if (i == node->mapping_count)
goto fail_map;

+ /* Firmware bug! */
+ if (!map->output_reference) {
+ pr_err(FW_BUG "[node %p type %d] ID map has NULL parent reference\n",
+ node, node->type);
+ goto fail_map;
+ }
+
node = ACPI_ADD_PTR(struct acpi_iort_node, iort_table,
map->output_reference);
}
--
2.19.1

2020-02-25 15:07:55

by Lorenzo Pieralisi

Subject: Re: [PATCH] acpi/iort: check output reference for the real used mapping

On Tue, Feb 25, 2020 at 05:01:36PM +0800, Heyi Guo wrote:
> The function iort_node_map_id() does the sanity check against the
> first mapping in the node, but not the one which we really use.
>
> Logically we need check the mapping we use, or check every mapping in
> the node. Choose the first fix for we are not firmware tester.

Yes, I agree with you, I will think about what's best to do, can
I pick up this patch and resend it on your behalf please ?

Thanks,
Lorenzo

> Signed-off-by: Heyi Guo <[email protected]>
>
> ---
> Cc: Lorenzo Pieralisi <[email protected]>
> Cc: Hanjun Guo <[email protected]>
> Cc: Sudeep Holla <[email protected]>
> Cc: "Rafael J. Wysocki" <[email protected]>
> Cc: Len Brown <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> ---
> drivers/acpi/arm64/iort.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> index ed3d2d1a7ae9..d0fe8d673240 100644
> --- a/drivers/acpi/arm64/iort.c
> +++ b/drivers/acpi/arm64/iort.c
> @@ -470,13 +470,6 @@ static struct acpi_iort_node *iort_node_map_id(struct acpi_iort_node *node,
> map = ACPI_ADD_PTR(struct acpi_iort_id_mapping, node,
> node->mapping_offset);
>
> - /* Firmware bug! */
> - if (!map->output_reference) {
> - pr_err(FW_BUG "[node %p type %d] ID map has NULL parent reference\n",
> - node, node->type);
> - goto fail_map;
> - }
> -
> /*
> * Get the special ID mapping index (if any) and skip its
> * associated ID map to prevent erroneous multi-stage
> @@ -497,6 +490,13 @@ static struct acpi_iort_node *iort_node_map_id(struct acpi_iort_node *node,
> if (i == node->mapping_count)
> goto fail_map;
>
> + /* Firmware bug! */
> + if (!map->output_reference) {
> + pr_err(FW_BUG "[node %p type %d] ID map has NULL parent reference\n",
> + node, node->type);
> + goto fail_map;
> + }
> +
> node = ACPI_ADD_PTR(struct acpi_iort_node, iort_table,
> map->output_reference);
> }
> --
> 2.19.1
>

2020-02-26 03:44:35

by Hanjun Guo

Subject: Re: [PATCH] acpi/iort: check output reference for the real used mapping

On 2020/2/25 17:01, Heyi Guo wrote:
> The function iort_node_map_id() does the sanity check against the
> first mapping in the node, but not the one which we really use.
>
> Logically we need check the mapping we use, or check every mapping in
> the node. Choose the first fix for we are not firmware tester.
>
> Signed-off-by: Heyi Guo <[email protected]>
>
> ---
> Cc: Lorenzo Pieralisi <[email protected]>
> Cc: Hanjun Guo <[email protected]>
> Cc: Sudeep Holla <[email protected]>
> Cc: "Rafael J. Wysocki" <[email protected]>
> Cc: Len Brown <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> ---
> drivers/acpi/arm64/iort.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> index ed3d2d1a7ae9..d0fe8d673240 100644
> --- a/drivers/acpi/arm64/iort.c
> +++ b/drivers/acpi/arm64/iort.c
> @@ -470,13 +470,6 @@ static struct acpi_iort_node *iort_node_map_id(struct acpi_iort_node *node,
> map = ACPI_ADD_PTR(struct acpi_iort_id_mapping, node,
> node->mapping_offset);
>
> - /* Firmware bug! */
> - if (!map->output_reference) {
> - pr_err(FW_BUG "[node %p type %d] ID map has NULL parent reference\n",
> - node, node->type);
> - goto fail_map;
> - }
> -
> /*
> * Get the special ID mapping index (if any) and skip its
> * associated ID map to prevent erroneous multi-stage
> @@ -497,6 +490,13 @@ static struct acpi_iort_node *iort_node_map_id(struct acpi_iort_node *node,
> if (i == node->mapping_count)
> goto fail_map;
>
> + /* Firmware bug! */
> + if (!map->output_reference) {
> + pr_err(FW_BUG "[node %p type %d] ID map has NULL parent reference\n",
> + node, node->type);
> + goto fail_map;
> + }

I think we can warning on the NULL parent reference when
scanning the mappings in the node, but don't bail out for
the mappings we are not using.

Thanks
Hanjun