Hi Ohad,

Thanks for the patch.

On Friday 03 June 2011 00:27:38 Ohad Ben-Cohen wrote:
> Migrate OMAP's iommu to the generic iommu api, so users can stay
> generic, and non-omap-specific code can be removed and eventually
> consolidated into a generic framework.
> 
> Tested on both OMAP3 and OMAP4.
> 
> Signed-off-by: Ohad Ben-Cohen <o...@wizery.com>

[snip]

> diff --git a/arch/arm/plat-omap/iommu.c b/arch/arm/plat-omap/iommu.c
> index 34fc31e..f06e99c 100644
> --- a/arch/arm/plat-omap/iommu.c
> +++ b/arch/arm/plat-omap/iommu.c

[snip]

> +static int omap_iommu_domain_init(struct iommu_domain *domain)
> +{
> +     struct omap_iommu_domain *omap_domain;
> +
> +     omap_domain = kzalloc(sizeof(*omap_domain), GFP_KERNEL);
> +     if (!omap_domain) {
> +             pr_err("kzalloc failed\n");
> +             goto fail_nomem;
> +     }
> +
> +     omap_domain->pgtable = (u32 *)__get_free_pages(GFP_KERNEL,
> +                                     get_order(IOPGD_TABLE_SIZE));
> +     if (!omap_domain->pgtable) {
> +             pr_err("__get_free_pages failed\n");
> +             goto fail_nomem;
> +     }
> +
> +     BUG_ON(!IS_ALIGNED((long)omap_domain->pgtable, IOPGD_TABLE_SIZE));

Either __get_free_pages() guarantees that the allocated memory will be aligned 
on an IOPGD_TABLE_SIZE boundary, in which case the BUG_ON() is unnecessary, or 
doesn't offer such guarantee, in which case the BUG_ON() will oops randomly. 
In both cases BUG_ON() should probably be avoided.

> +     memset(omap_domain->pgtable, 0, IOPGD_TABLE_SIZE);
> +     clean_dcache_area(omap_domain->pgtable, IOPGD_TABLE_SIZE);
> +     mutex_init(&omap_domain->lock);
> +
> +     domain->priv = omap_domain;
> +
> +     return 0;
> +
> +fail_nomem:
> +     kfree(omap_domain);
> +     return -ENOMEM;
> +}
> +
> +/* assume device was already detached */
> +static void omap_iommu_domain_destroy(struct iommu_domain *domain)
> +{
> +     struct omap_iommu_domain *omap_domain = domain->priv;
> +
> +     domain->priv = NULL;
> +
> +     kfree(omap_domain);

This leaks omap_domain->pgtable.

The free_pages() call in omap_iommu_remove() should be removed, as 
omap_iommu_probe() doesn't allocate the pages table anymore. You can also 
remove the the struct iommu::iopgd field.

> +}


> +
> +static phys_addr_t omap_iommu_iova_to_phys(struct iommu_domain *domain,
> +                                       unsigned long da)
> +{
> +     struct omap_iommu_domain *omap_domain = domain->priv;
> +     struct iommu *oiommu = omap_domain->iommu_dev;
> +     struct device *dev = oiommu->dev;
> +     u32 *pgd, *pte;
> +     phys_addr_t ret = 0;
> +
> +     iopgtable_lookup_entry(oiommu, da, &pgd, &pte);
> +
> +     if (pte) {
> +             if (iopte_is_small(*pte))
> +                     ret = omap_iommu_translate(*pte, da, IOPTE_MASK);
> +             else if (iopte_is_large(*pte))
> +                     ret = omap_iommu_translate(*pte, da, IOLARGE_MASK);
> +             else
> +                     dev_err(dev, "bogus pte 0x%x", *pte);
> +     } else {
> +             if (iopgd_is_section(*pgd))
> +                     ret = omap_iommu_translate(*pgd, da, IOSECTION_MASK);
> +             else if (iopgd_is_super(*pgd))
> +                     ret = omap_iommu_translate(*pgd, da, IOSUPER_MASK);
> +             else
> +                     dev_err(dev, "bogus pgd 0x%x", *pgd);
> +     }
> +
> +     return ret;

You return 0 in the bogus pte/pgd cases. Is that intentional ?

> +}

-- 
Regards,

Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to