Re: [RFC PATCH v4 6/8] dma-mapping: set dma segment properties in of_dma_configure

[Robin Murphy <robin.murphy@arm.com>]

Hi Will,

On 14/11/14 18:56, Will Deacon wrote:
> of_dma_configure determines the size of the DMA range for a device by
> either parsing the dma-ranges property or inspecting the coherent DMA
> mask. This same information can be used to initialise the max segment
> size and boundary_mask to a default value.
>
> Signed-off-by: Will Deacon <will.deacon@arm.com>

Sadly, NAK on this one. After a thorough investigation and testing, 
generic code really shouldn't be touching dma_parms like this, and I 
have some corrupted files on my USB filesystem to prove it ;) It seems 
it's a bad idea to effectively change the default segment size 
universally unless you really want to go through the entire block layer 
(and who-knows-what-else) to find and fix everything that relies on it 
being 64k.

Simply dropping this patch and letting the existing defaults in 
dma_set_max_seg_size and dma_get_seg_boundary stand seems like the 
correct action.


Marek, I believe you may have some use case for this - if so, as I 
understand it your individual drivers should be setting up their own 
dma_parms directly, to tell the IOMMU they can handle it merging 
scatterlists into arbitrarily long segments, without affecting the other 
drivers that assume they'll never see >64k segments and go wrong if they 
do (including the generic USB stack, apparently).


Robin.

> ---
>   drivers/of/platform.c           | 4 ++++
>   include/linux/amba/bus.h        | 1 +
>   include/linux/platform_device.h | 2 ++
>   3 files changed, 7 insertions(+)
>
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index b89caf8c7586..0a2842d91db4 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -191,6 +191,8 @@ static void of_dma_configure(struct device *dev)
>   		dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", dev->dma_pfn_offset);
>   	}
>   	dev->dma_pfn_offset = offset;
> +	dma_set_max_seg_size(dev, size);
> +	dma_set_seg_boundary(dev, size);
>
>   	coherent = of_dma_is_coherent(dev->of_node);
>   	dev_dbg(dev, "device is%sdma coherent\n",
> @@ -236,6 +238,7 @@ static struct platform_device *of_platform_device_create_pdata(
>
>   	dev->dev.bus = &platform_bus_type;
>   	dev->dev.platform_data = platform_data;
> +	dev->dev.dma_parms = &dev->dma_parms;
>   	of_dma_configure(&dev->dev);
>
>   	if (of_device_add(dev) != 0) {
> @@ -295,6 +298,7 @@ static struct amba_device *of_amba_device_create(struct device_node *node,
>   	dev->dev.of_node = of_node_get(node);
>   	dev->dev.parent = parent;
>   	dev->dev.platform_data = platform_data;
> +	dev->dev.dma_parms = &dev->dma_parms;
>   	if (bus_id)
>   		dev_set_name(&dev->dev, "%s", bus_id);
>   	else
> diff --git a/include/linux/amba/bus.h b/include/linux/amba/bus.h
> index c324f5700d1a..9b232eeb6c20 100644
> --- a/include/linux/amba/bus.h
> +++ b/include/linux/amba/bus.h
> @@ -32,6 +32,7 @@ struct amba_device {
>   	struct clk		*pclk;
>   	unsigned int		periphid;
>   	unsigned int		irq[AMBA_NR_IRQS];
> +	struct device_dma_parameters dma_parms;
>   };
>
>   struct amba_driver {
> diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
> index 153d303af7eb..8dc48487b34b 100644
> --- a/include/linux/platform_device.h
> +++ b/include/linux/platform_device.h
> @@ -27,6 +27,8 @@ struct platform_device {
>   	u32		num_resources;
>   	struct resource	*resource;
>
> +	struct device_dma_parameters dma_parms;
> +
>   	const struct platform_device_id	*id_entry;
>   	char *driver_override; /* Driver name to force a match */
>
>