Re: [PATCH] of: restricted dma: Don't fail device probe on rmem init failure

[Robin Murphy]

On 2021-08-05 10:47, Will Deacon wrote:
> If CONFIG_DMA_RESTRICTED_POOL=n then probing a device with a reference
> to a "restricted-dma-pool" will fail with a reasonably cryptic error:
>
>    | pci-host-generic: probe of 10000.pci failed with error -22
>
> Print a more helpful message in this case and try to continue probing
> the device as we do if the kernel doesn't have the restricted DMA patches
> applied or either CONFIG_OF_ADDRESS or CONFIG_HAS_DMA =n.

Makes sense to me;

Reviewed-by: Robin Murphy <robin.murphy@xxxxxxx>

Although if we allow probe to succeed when a pool really was there for a 
reason, it may end up being much more fatal if the driver then tries to 
do a DMA transfer to any old memory and the device access causes an 
SError, or the VM to be killed, or whatever. That's not quite the same 
as the stubbed cases where the respective platforms couldn't have a 
genuine pool to parse either way, but as you say it is what could happen 
already if the user tried to use an older kernel, and I think the chance 
of of_reserved_mem_device_init_by_idx() failing without something being 
terminally wrong anyway - invalid DT, not enough RAM, etc. - is low 
enough that it's probably not a major concern. Plus I'd hope that the 
memory protection schemes people do actually implement don't take such 
such a zero-tolerance approach anyway - allowing a malicious or 
malfunctioning device to take down the system because it tried to make a 
rogue access which *was* already contained seems a bit silly.

Robin.

> Cc: Claire Chang <tientzu@xxxxxxxxxxxx>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> Cc: Robin Murphy <robin.murphy@xxxxxxx>
> Cc: Christoph Hellwig <hch@xxxxxx>
> Cc: Rob Herring <robh+dt@xxxxxxxxxx>
> Signed-off-by: Will Deacon <will@xxxxxxxxxx>
> ---
>   drivers/of/address.c    | 8 ++++----
>   drivers/of/device.c     | 2 +-
>   drivers/of/of_private.h | 8 +++-----
>   3 files changed, 8 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/of/address.c b/drivers/of/address.c
> index 973257434398..f6bf4b423c2a 100644
> --- a/drivers/of/address.c
> +++ b/drivers/of/address.c
> @@ -997,7 +997,7 @@ int of_dma_get_range(struct device_node *np, const struct bus_dma_region **map)
>   	return ret;
>   }
>   
> -int of_dma_set_restricted_buffer(struct device *dev, struct device_node *np)
> +void of_dma_set_restricted_buffer(struct device *dev, struct device_node *np)
>   {
>   	struct device_node *node, *of_node = dev->of_node;
>   	int count, i;
> @@ -1022,11 +1022,11 @@ int of_dma_set_restricted_buffer(struct device *dev, struct device_node *np)
>   		 */
>   		if (of_device_is_compatible(node, "restricted-dma-pool") &&
>   		    of_device_is_available(node))
> -			return of_reserved_mem_device_init_by_idx(dev, of_node,
> -								  i);
> +			break;
>   	}
>   
> -	return 0;
> +	if (i != count && of_reserved_mem_device_init_by_idx(dev, of_node, i))
> +		dev_warn(dev, "failed to initialise \"restricted-dma-pool\" memory node\n");
>   }
>   #endif /* CONFIG_HAS_DMA */
>   
> diff --git a/drivers/of/device.c b/drivers/of/device.c
> index 2defdca418ec..258a2b099410 100644
> --- a/drivers/of/device.c
> +++ b/drivers/of/device.c
> @@ -166,7 +166,7 @@ int of_dma_configure_id(struct device *dev, struct device_node *np,
>   	arch_setup_dma_ops(dev, dma_start, size, iommu, coherent);
>   
>   	if (!iommu)
> -		return of_dma_set_restricted_buffer(dev, np);
> +		of_dma_set_restricted_buffer(dev, np);
>   
>   	return 0;
>   }
> diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
> index f557bd22b0cf..bc883f69496b 100644
> --- a/drivers/of/of_private.h
> +++ b/drivers/of/of_private.h
> @@ -163,18 +163,16 @@ struct bus_dma_region;
>   #if defined(CONFIG_OF_ADDRESS) && defined(CONFIG_HAS_DMA)
>   int of_dma_get_range(struct device_node *np,
>   		const struct bus_dma_region **map);
> -int of_dma_set_restricted_buffer(struct device *dev, struct device_node *np);
> +void of_dma_set_restricted_buffer(struct device *dev, struct device_node *np);
>   #else
>   static inline int of_dma_get_range(struct device_node *np,
>   		const struct bus_dma_region **map)
>   {
>   	return -ENODEV;
>   }
> -static inline int of_dma_set_restricted_buffer(struct device *dev,
> -					       struct device_node *np)
> +static inline void of_dma_set_restricted_buffer(struct device *dev,
> +						struct device_node *np)
>   {
> -	/* Do nothing, successfully. */
> -	return 0;
>   }
>   #endif
>