MIME-Version: 1.0
References: <20230126-synquacer-boot-v1-1-94ed0eb1011f@kernel.org>
In-Reply-To: <20230126-synquacer-boot-v1-1-94ed0eb1011f@kernel.org>
From:   Rob Herring <robh+dt@kernel.org>
Date:   Fri, 27 Jan 2023 12:37:35 -0600
Message-ID: <CAL_JsqLPUKS2+8-sShADSYxAgxjP3mh=TcZPszFeYbMTiPvjYQ@mail.gmail.com>
Subject: Re: [PATCH] of/address: Return an error when no valid dma-ranges are found
To:     Mark Brown <broonie@kernel.org>
Cc:     Frank Rowand <frowand.list@gmail.com>, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        Luca Di Stefano <luca.distefano@linaro.org>,
        993612@bugs.debian.org, stable@kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Jan 26, 2023 at 10:27 AM Mark Brown <broonie@kernel.org> wrote:
>
> Commit 7a8b64d17e35 ("of/address: use range parser for of_dma_get_range")
> converted the parsing of dma-range properties to use code shared with the
> PCI range parser. The intent was to introduce no functional changes however
> in the case where we fail to translate the first resource instead of
> returning -EINVAL the new code we return 0. Restore the previous behaviour
> by returning an error if we find no valid ranges, the original code only
> handled the first range but subsequently support for parsing all supplied
> ranges was added.
>
> This avoids confusing code using the parsed ranges which doesn't expect to
> successfully parse ranges but have only a list terminator returned, this
> fixes breakage with so far as I can tell all DMA for on SoC devices on the
> Socionext Synquacer platform which has a firmware supplied DT. A bisect
> identified the original conversion as triggering the issues there.

Looks like maybe it was fixed by Colin in commit f49c7faf776f
("of/address: check for invalid range.cpu_addr") as that commit refers
to Synquacer. But then was it possibly reintroduced by commit
e0d072782c73 ("dma-mapping: introduce DMA range map, supplanting
dma_pfn_offset")?

> Fixes: 7a8b64d17e35 ("of/address: use range parser for of_dma_get_range")
> Signed-off-by: Mark Brown <broonie@kernel.org>
> Cc: Luca Di Stefano <luca.distefano@linaro.org>
> Cc: 993612@bugs.debian.org
> Cc: stable@kernel.org
> ---
>  drivers/of/address.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/of/address.c b/drivers/of/address.c
> index c34ac33b7338..21342223b8e5 100644
> --- a/drivers/of/address.c
> +++ b/drivers/of/address.c
> @@ -975,10 +975,12 @@ int of_dma_get_range(struct device_node *np, const struct bus_dma_region **map)
>         }
>
>         /*
> -        * Record all info in the generic DMA ranges array for struct device.
> +        * Record all info in the generic DMA ranges array for struct device,
> +        * returning an error if we don't find any parsable ranges.
>          */
>         *map = r;
>         of_dma_range_parser_init(&parser, node);
> +       ret = -EINVAL;

Looks to me like we are leaking 'r' with this change.

Wouldn't this change work:

diff --git a/drivers/of/address.c b/drivers/of/address.c
index c34ac33b7338..f43311f01c32 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -968,6 +968,11 @@ int of_dma_get_range(struct device_node *np,
const struct bus_dma_region **map)
        for_each_of_range(&parser, &range)
                num_ranges++;

+       if (!num_ranges) {
+               ret = -EINVAL;
+               goto out;
+       }
+
        r = kcalloc(num_ranges + 1, sizeof(*r), GFP_KERNEL);
        if (!r) {
                ret = -ENOMEM;