lkml.org 
[lkml]   [2008]   [Jul]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 25/34] AMD IOMMU: add dma_ops mapping functions for single mappings
On Thu, 26 Jun 2008 21:28:01 +0200 Joerg Roedel <joerg.roedel@amd.com> wrote:

> This patch adds the dma_ops specific mapping functions for single mappings.
>
> Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
> ---
> arch/x86/kernel/amd_iommu.c | 59 +++++++++++++++++++++++++++++++++++++++++++
> 1 files changed, 59 insertions(+), 0 deletions(-)
>
> diff --git a/arch/x86/kernel/amd_iommu.c b/arch/x86/kernel/amd_iommu.c
> index e00a3e7..b4079f6 100644
> --- a/arch/x86/kernel/amd_iommu.c
> +++ b/arch/x86/kernel/amd_iommu.c
> @@ -40,6 +40,11 @@ struct command {
> static int dma_ops_unity_map(struct dma_ops_domain *dma_dom,
> struct unity_map_entry *e);
>
> +static int iommu_has_npcache(struct amd_iommu *iommu)
> +{
> + return iommu->cap & IOMMU_CAP_NPCACHE;
> +}
> +
> static int __iommu_queue_command(struct amd_iommu *iommu, struct command *cmd)
> {
> u32 tail, head;
> @@ -641,3 +646,57 @@ static void __unmap_single(struct amd_iommu *iommu,
> dma_ops_free_addresses(dma_dom, dma_addr, pages);
> }
>
> +static dma_addr_t map_single(struct device *dev, phys_addr_t paddr,
> + size_t size, int dir)
> +{
> + unsigned long flags;
> + struct amd_iommu *iommu;
> + struct protection_domain *domain;
> + u16 devid;
> + dma_addr_t addr;
> +
> + get_device_resources(dev, &iommu, &domain, &devid);
> +
> + if (iommu == NULL || domain == NULL)
> + return (dma_addr_t)paddr;

OK, a test case. A reader of your code (ie: me) wants to find out what
this code is doing. What is the *meaning* of iommu == NULL || domain ==
NULL here?
I go look at the (undocumented) get_device_resources() and I see that this:

if (_bdf >= amd_iommu_last_bdf) {
happened. I don't know what that semantically means and I gave up.

I'm not saying that the code is unmaintainable, but I would assert that it
is a heck of a lot harder to maintain than it could be, and than it should
be.

get_device_resources() has an open-coded copy of your DEVID() macro, btw.

> + spin_lock_irqsave(&domain->lock, flags);
> + addr = __map_single(dev, iommu, domain->priv, paddr, size, dir);
> + if (addr == bad_dma_address)
> + goto out;
> +
> + if (iommu_has_npcache(iommu))
> + iommu_flush_pages(iommu, domain->id, addr, size);
> +
> + if (iommu->need_sync)
> + iommu_completion_wait(iommu);
> +
> +out:
> + spin_unlock_irqrestore(&domain->lock, flags);
> +
> + return addr;
> +}
> +
> +static void unmap_single(struct device *dev, dma_addr_t dma_addr,
> + size_t size, int dir)
> +{
> + unsigned long flags;
> + struct amd_iommu *iommu;
> + struct protection_domain *domain;
> + u16 devid;
> +
> + if (!get_device_resources(dev, &iommu, &domain, &devid))
> + return;
> +
> + spin_lock_irqsave(&domain->lock, flags);
> +
> + __unmap_single(iommu, domain->priv, dma_addr, size, dir);
> +
> + iommu_flush_pages(iommu, domain->id, dma_addr, size);
> +
> + if (iommu->need_sync)
> + iommu_completion_wait(iommu);
> +
> + spin_unlock_irqrestore(&domain->lock, flags);
> +}



\
 
 \ /
  Last update: 2008-07-10 04:35    [from the cache]
©2003-2011 Jasper Spaans