# \[PATCH v2 7/7\] OF: Don't set default coherent DMA mask

- From: Robin Murphy \<robin.murphy@arm.com\>
- Date: 2018-07-27 11:36:00 
- List: linux-arm-kernel
- Page: https://ratatoskr.run/linux-arm-kernel/2018/07/8771687
- Thread: https://ratatoskr.run/linux-arm-kernel/2018/07/8771456/t.md

```text
On 27/07/18 01:22, Grygorii Strashko wrote:
[...]
>> the result of this change is pretty strange as for me :(
>> Resulted code:
>>
>> 	/*
>> 	 * If @dev is expected to be DMA-capable then the bus code that created
>> 	 * it should have initialised its dma_mask pointer by this point. For
>> 	 * now, we'll continue the legacy behaviour of coercing it to the
>> 	 * coherent mask if not, but we'll no longer do so quietly.
>> 	 */
>> 	if (!dev->dma_mask) {
>> 		dev_warn(dev, "DMA mask not set\n");
>> 		dev->dma_mask = &dev->coherent_dma_mask;
>> ^this will always produce warning in case of platform-bus or if there are no bus driver.
>> even if DT contains correct definition of dma-range
>> 	}
>>
>> 	if (!size && dev->coherent_dma_mask)
>>
>> ^ coherent_dma_mask is zero, so size will not be calculated
> pls, ignore this particular comment
> 
>> 		size = max(dev->coherent_dma_mask, dev->coherent_dma_mask + 1);
>> 	else if (!size)
>> 		size = 1ULL << 32;
>>
>> 	dev->dma_pfn_offset = offset;
>>
>> 	/*
>> 	 * Limit coherent and dma mask based on size and default mask
>> 	 * set by the driver.
>> 	 */
>> 	mask = DMA_BIT_MASK(ilog2(dma_addr + size - 1) + 1);
>> 	dev->bus_dma_mask = mask;
>> 	dev->coherent_dma_mask &= mask;
>>
>> ^^ if nobody set coherent_dma_mask before it will stay null forever unless drivers
>> will overwrite it. Again even if DT has correct definition for dma-range.

That is intentional. Consider a 32-bit device on a bus with an upstream 
DMA range of 40 bits (which could easily happen with e.g. PCI). If the 
firmware code gives that device 40-bit DMA masks and the driver doesn't 
change them, then DMA is not going to work correctly. Therefore the 
firmware code cannot safely make {coherent_}dma_mask larger than the 
default value passed in, and if the default is 0 then that should be 
fixed in whatever code created the device, not worked around later.

Robin.

>>
>> 	*dev->dma_mask &= mask;
>>
>> 	coherent = of_dma_is_coherent(np);
>> 	dev_dbg(dev, "device is%sdma coherent\n",
>> 		coherent ? " " : " not ");
>>
>>
> 
> FYI, with below diff i can boot at least:
> 
> diff --git a/drivers/of/device.c b/drivers/of/device.c
> index 5957cd4..f7dc121 100644
> --- a/drivers/of/device.c
> +++ b/drivers/of/device.c
> @@ -150,8 +150,8 @@ int of_dma_configure(struct device *dev, struct device_node *np, bool force_dma)
>           */
>          mask = DMA_BIT_MASK(ilog2(dma_addr + size - 1) + 1);
>          dev->bus_dma_mask = mask;
> -       dev->coherent_dma_mask &= mask;
> -       *dev->dma_mask &= mask;
> +       dev->coherent_dma_mask = mask;
> +       *dev->dma_mask = mask;
>   
>          coherent = of_dma_is_coherent(np);
>          dev_dbg(dev, "device is%sdma coherent\n",
> 
> 
> 
```