On Thu, Jan 26, 2023 at 10:27 AM Mark Brown 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
> Cc: Luca Di Stefano
> Cc: 993...@bugs.debian.org
> Cc: sta...@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;
