Re: [PATCH 25/34] AMD IOMMU: add dma_ops mapping functions for single mappings

[Andrew Morton <akpm@linux-foundation.org>]

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);
> +}