Re: [PATCH v11 07/11] device-mapping: Introduce DMA range map, supplanting dma_pfn_offset

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Mon, Aug 24, 2020 at 03:30:20PM -0400, Jim Quinlan wrote:
> The new field 'dma_range_map' in struct device is used to facilitate the
> use of single or multiple offsets between mapping regions of cpu addrs and
> dma addrs.  It subsumes the role of "dev->dma_pfn_offset" which was only
> capable of holding a single uniform offset and had no region bounds
> checking.
> 
> The function of_dma_get_range() has been modified so that it takes a single
> argument -- the device node -- and returns a map, NULL, or an error code.
> The map is an array that holds the information regarding the DMA regions.
> Each range entry contains the address offset, the cpu_start address, the
> dma_start address, and the size of the region.
> 
> of_dma_configure() is the typical manner to set range offsets but there are
> a number of ad hoc assignments to "dev->dma_pfn_offset" in the kernel
> driver code.  These cases now invoke the function
> dma_attach_offset_range(dev, cpu_addr, dma_addr, size).

...

> +	/*
> +	 * Record all info in the generic DMA ranges array for struct device.
> +	 */
> +	*map = r;
> +	for_each_of_range(&parser, &range) {
> +		pr_debug("dma_addr(%llx) cpu_addr(%llx) size(%llx)\n",
> +			 range.bus_addr, range.cpu_addr, range.size);
> +		r->cpu_start = range.cpu_addr;
> +		r->dma_start = range.bus_addr;
> +		r->size = range.size;

> +		r->offset = (u64)range.cpu_addr - (u64)range.bus_addr;

What's the point in explicit castings to the same type?

> +		r++;
> +	}

...

> +		phys_addr_t	paddr;
> +		dma_addr_t	dma_addr;
> +		struct device	dev_bogus;

>  		unittest(paddr == expect_paddr,
> -			 "of_dma_get_range wrong phys addr (%llx) on node %pOF", paddr, np);
> +			 "of_dma_get_range: wrong phys addr %llx (expecting %llx) on node %pOF\n",
> +			 (u64)paddr, expect_paddr, np);

%llx -> %pap

>  		unittest(dma_addr == expect_dma_addr,
> -			 "of_dma_get_range wrong DMA addr (%llx) on node %pOF", dma_addr, np);
> +			 "of_dma_get_range: wrong DMA addr %llx (expecting %llx) on node %pOF\n",
> +			 (u64)dma_addr, expect_dma_addr, np);

%llx -> %pad

...

> +	if (mem->use_dev_dma_pfn_offset) {
> +		u64 base_addr = PFN_PHYS((u64)mem->pfn_base);

Do we need explicit casting here?

> +
> +		return base_addr - dma_offset_from_phys_addr(dev, base_addr);
> +	}

...

> +int dma_set_offset_range(struct device *dev, phys_addr_t cpu_start,
> +			 dma_addr_t dma_start, u64 size)
> +{
> +	struct bus_dma_region *map;
> +	u64 offset = (u64)cpu_start - (u64)dma_start;
> +
> +	if (dev->dma_range_map) {
> +		dev_err(dev, "attempt to add DMA range to existing map\n");
> +		return -EINVAL;
> +	}

Wouldn't be better to do an assignment of offset here?

> +	if (!offset)
> +		return 0;
> +
> +	map = kcalloc(2, sizeof(*map), GFP_KERNEL);
> +	if (!map)
> +		return -ENOMEM;
> +	map[0].cpu_start = cpu_start;
> +	map[0].dma_start = dma_start;
> +	map[0].offset = offset;
> +	map[0].size = size;
> +	dev->dma_range_map = map;
> +
> +	return 0;
> +}

-- 
With Best Regards,
Andy Shevchenko


_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu