Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 18 Jul 2023 09:52:11 +0200
From:   Lorenzo Pieralisi <lpieralisi@kernel.org>
To:     Guanghui Feng <guanghuifeng@linux.alibaba.com>
Cc:     guohanjun@huawei.com, sudeep.holla@arm.com, rafael@kernel.org,
        linux-acpi@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-kernel@vger.kernel.org, baolin.wang@linux.alibaba.com,
        alikernel-developer@linux.alibaba.com, will@kernel.org,
        catalin.marinas@arm.com, shameerali.kolothum.thodi@huawei.com
Subject: Re: [PATCH v3] ACPI/IORT: Remove erroneous id_count check in
 iort_node_get_rmr_info()
Message-ID: <ZLZEq0QBBW4rcxJM@lpieralisi>
References: <1689593625-45213-1-git-send-email-guanghuifeng@linux.alibaba.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1689593625-45213-1-git-send-email-guanghuifeng@linux.alibaba.com>
Precedence: bulk

[+Catalin, Will, Shameer]

On Mon, Jul 17, 2023 at 07:33:45PM +0800, Guanghui Feng wrote:
> According to the ARM IORT specifications DEN 0049 issue E,
> the "Number of IDs" field in the ID mapping format reports
> the number of IDs in the mapping range minus one.
> 
> In iort_node_get_rmr_info(), we erroneously skip ID mappings
> whose "Number of IDs" equal to 0, resulting in valid mapping
> nodes with a single ID to map being skipped, which is wrong.
> 
> Fix iort_node_get_rmr_info() by removing the bogus id_count
> check.
> 
> Fixes: 491cf4a6735a ("ACPI/IORT: Add support to retrieve IORT RMR reserved regions")
> Signed-off-by: Guanghui Feng <guanghuifeng@linux.alibaba.com>
> ---
>  drivers/acpi/arm64/iort.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> index 3631230..56d8873 100644
> --- a/drivers/acpi/arm64/iort.c
> +++ b/drivers/acpi/arm64/iort.c
> @@ -1007,9 +1007,6 @@ static void iort_node_get_rmr_info(struct acpi_iort_node *node,
>  	for (i = 0; i < node->mapping_count; i++, map++) {
>  		struct acpi_iort_node *parent;
>  
> -		if (!map->id_count)
> -			continue;
> -
>  		parent = ACPI_ADD_PTR(struct acpi_iort_node, iort_table,
>  				      map->output_reference);
>  		if (parent != iommu)

Shameer, I know this may look like overkill since the hunk we are
removing is buggy but can you please test this patch on platforms
with RMR to make sure we are not triggering regressions by removing
it (by the specs that's what should be done but current firmware
is always something to reckon with) ?

Thanks,
Lorenzo