Re: [RFC 2/6] omap: iovmm: generic iommu api migration

Laurent Pinchart
2011-06-07T09:05:06Z

Hi Ohad,

Thanks for the patch.

On Friday 03 June 2011 00:27:39 Ohad Ben-Cohen wrote:
> Migrate omap's iovmm (virtual memory manager) to the generic iommu api.
>
> This brings iovmm a step forward towards being completely non
> omap-specific (it's still assuming omap's iommu page sizes, and also
> maintaining state inside omap's internal iommu structure, but it no
> longer calls omap-specific iommu map/unmap api).
>
> Further generalizing of iovmm (or complete removal) should take place
> together with broader plans of providing a generic virtual memory manager
> and allocation framework (de-coupled from specific mappers).
>
> Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>

[snip]

> diff --git a/arch/arm/plat-omap/iovmm.c b/arch/arm/plat-omap/iovmm.c
> index 51ef43e..80bb2b6 100644
> --- a/arch/arm/plat-omap/iovmm.c
> +++ b/arch/arm/plat-omap/iovmm.c

[snip]

> @@ -473,22 +475,22 @@ static int map_iovm_area(struct iommu *obj, struct
> iovm_struct *new, u32 pa;
> int pgsz;
> size_t bytes;
> - struct iotlb_entry e;
>
> pa = sg_phys(sg);
> bytes = sg_dma_len(sg);
>
> flags &= ~IOVMF_PGSZ_MASK;
> +
> pgsz = bytes_to_iopgsz(bytes);
> if (pgsz < 0)
> goto err_out;
> - flags |= pgsz;

pgsz isn't used anymore, you can remove it.

> +
> + order = get_order(bytes);

Does iommu_map() handle offsets correctly, or does it expect pa to be aligned
to an order (or other) boundary ? Same comment for iommu_unmap() in
unmap_iovm_area().

> pr_debug("%s: [%d] %08x %08x(%x)\n", __func__,
> i, da, pa, bytes);
>
> - iotlb_init_entry(&e, da, pa, flags);
> - err = iopgtable_store_entry(obj, &e);
> + err = iommu_map(domain, da, pa, order, flags);
> if (err)
> goto err_out;
>
> @@ -502,9 +504,11 @@ err_out:
> for_each_sg(sgt->sgl, sg, i, j) {
> size_t bytes;
>
> - bytes = iopgtable_clear_entry(obj, da);
> + bytes = sg_dma_len(sg);

As Russell pointed out, we should use sg->length instead of sg_dma_length(sg).
sg_dma_length(sg) is only valid after the scatter list has been DMA-mapped,
which doesn't happen in the iovmm driver. This applies to all sg_dma_len(sg)
calls.

> + order = get_order(bytes);
>
> - BUG_ON(!iopgsz_ok(bytes));
> + /* ignore failures.. we're already handling one */
> + iommu_unmap(domain, da, order);
>
> da += bytes;
> }

--
Regards,

Laurent Pinchart