Re: [PATCH v3 2/4] of: move of_dma_configure() to device,c to help re-use

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 01/09/2015 10:34 AM, Rob Herring wrote:
On Thu, Jan 8, 2015 at 4:24 PM, Arnd Bergmann<arnd@xxxxxxxx>  wrote:
On Thursday 08 January 2015 14:26:36 Murali Karicheri wrote:
On 01/08/2015 03:40 AM, Arnd Bergmann wrote:
On Wednesday 07 January 2015 17:37:56 Rob Herring wrote:
On Wed, Jan 7, 2015 at 12:49 PM, Murali Karicheri<m-karicheri2@xxxxxx>   wrote:

+       ret = of_dma_get_range(np,&dma_addr,&paddr,&size);
+       if (ret<   0) {
+               dma_addr = offset = 0;
+               size = dev->coherent_dma_mask + 1;

If coherent_dma_mask is DMA_BIT_MASK(64), then you will overflow and
have a size of 0. There may also be a problem when the mask is only
32-bit type.

The mask is always a 64-bit type, it's not optional. But you are right,
the 64-bit mask case is broken, so I guess we have to fix it differently
by always passing the smaller value into arch_setup_dma_ops and
adapting that function instead.
Arnd,

What is the smaller value you are referring to in the below code?
between *dev->dma_mask and size from DT? But overflow can still happen
when size is to be calculated in arch_setup_dma_ops() for Non DT case or
when DT size is configured to be equivalent of DMA_BIT_MASK(64) + 1. Can
we discuss the code change you have in mind when you get a chance?

I meant changing every function that the size values gets passed into
to take a mask like 0xffffffff instead of a size like 0x100000000, so
we can represent a 64-bit capable bus correctly.

Or you could special case a size of 0 to mean all/max? I'm not sure if
we need to handle size=0 for other reasons beyond just wrong DT data.

This means we also need to adapt the value returned from of_dma_get_range.
A minor complication here is that the DT properties sometimes already
contain the mask value, in particular when we want to represent a
full mapping like

         bus {
                 #address-cells =<1>;
                 #size-cells =<1>;
                 dma-ranges =<0 0 0xffffffff>; /* all 4 GB, DMA_BIT_MASK(32) */

This is wrong though, right? The DT should be size. Certainly, this
could be a valid size, but that would not make the mask 0xfffffffe. We
would still want it to be 0xffffffff.

We could do a fixup for these cases adding 1 if bit 0 is set (or not
subtracting 1 if we want the mask).

Rob
Arnd, Rob, et all,

Do we have preference one way or other for the size format? If we need to follow the mask format, all of the calling functions below and the arm_iommu_create_mapping() has to change as well to use this changed format.

drivers/gpu/drm/rockchip/rockchip_drm_drv.c: mapping = arm_iommu_create_mapping(&platform_bus_type, 0x00000000, drivers/gpu/drm/exynos/exynos_drm_iommu.c: mapping = arm_iommu_create_mapping(&platform_bus_type, priv->da_start, drivers/media/platform/omap3isp/isp.c: mapping = arm_iommu_create_mapping(&platform_bus_type, SZ_1G, SZ_2G); drivers/iommu/shmobile-iommu.c: mapping = arm_iommu_create_mapping(&platform_bus_type, 0, drivers/iommu/ipmmu-vmsa.c: mapping = arm_iommu_create_mapping(&platform_bus_type,

So IMO, keeping current convention of size and taking care of exception in DT handling is the right thing to do instead of changing all of the above functions. i.e in of_dma_configure(), if DT provide a mask for size (lsb set), we will check that and add 1 to it. Only case in DTS that I can see this usage is

arch/arm64/boot/dts/amd/amd-seattle-soc.dtsi: dma-ranges = <0x80 0x0 0x80 0x0 0x7f 0xffffffff>; arch/arm64/boot/dts/amd/amd-seattle-soc.dtsi: dma-ranges = <0x43000000 0x80 0x0 0x80 0x0 0x7f 0xffffffff>;

This logic should take care of setting the size to ox80_00000000 for these cases. if dma_coherent_mask is set to max of u64, then this will result in a zero size (both DT case and non DT case). So treat a size of zero as error being overflow.

Also arm_iommu_create_mapping() currently accept a size of type size_t which means, this function expect the size to be max of 0xffffffff. So in arm_setup_iommu_dma_ops(), we need to check if size if >0xffffffff and return an error. If in future this function support u64 for size, this check can be removed.

I will update the series with this change and post it if we have an agreement on this. Please repond.

Thanks

--
Murali Karicheri
Linux Kernel, Texas Instruments
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]
  Powered by Linux