Date: Thu, 10 Jul 2008 18:46:44 +0200
From: Joerg Roedel <joerg.roedel@amd.com>
To: Andrew Morton <akpm@linux-foundation.org>
CC: tglx@linutronix.de, mingo@redhat.com, linux-kernel@vger.kernel.org,
       iommu@lists.linux-foundation.org, bhavna.sarathy@amd.com,
       Sebastian.Biemueller@amd.com, robert.richter@amd.com, joro@8bytes.org
Subject: Re: [PATCH 23/34] AMD IOMMU: add functions to find IOMMU device resources
Message-ID: <20080710164644.GR14977@amd.com>
References: <1214508490-29683-1-git-send-email-joerg.roedel@amd.com> <1214508490-29683-24-git-send-email-joerg.roedel@amd.com> <20080709191827.dbdfcd96.akpm@linux-foundation.org>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20080709191827.dbdfcd96.akpm@linux-foundation.org>
User-Agent: mutt-ng/devel-r804 (Linux)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2578
Lines: 66

On Wed, Jul 09, 2008 at 07:18:27PM -0700, Andrew Morton wrote:
> On Thu, 26 Jun 2008 21:27:59 +0200 Joerg Roedel <joerg.roedel@amd.com> wrote:
> 
> > This patch adds functions necessary to find the IOMMU resources for a specific
> > device.
> > 
> > Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
> > ---
> >  arch/x86/kernel/amd_iommu.c |   75 +++++++++++++++++++++++++++++++++++++++++++
> >  1 files changed, 75 insertions(+), 0 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/amd_iommu.c b/arch/x86/kernel/amd_iommu.c
> > index c43d15d..47e80b5 100644
> > --- a/arch/x86/kernel/amd_iommu.c
> > +++ b/arch/x86/kernel/amd_iommu.c
> > @@ -461,3 +461,78 @@ free_dma_dom:
> >  	return NULL;
> >  }
> >  
> > +static struct protection_domain *domain_for_device(u16 devid)
> > +{
> > +	struct protection_domain *dom;
> > +	unsigned long flags;
> > +
> > +	read_lock_irqsave(&amd_iommu_devtable_lock, flags);
> 
> Why is this cheerfully undocumented lock irq-safe?  Is it ever taken from
> IRQ context?

This function is called from the dma-mapping path. As far as I know the
DMA mapping functions can be called from interrupt context.

> 
> > +	dom = amd_iommu_pd_table[devid];
> > +	read_unlock_irqrestore(&amd_iommu_devtable_lock, flags);
> > +
> > +	return dom;
> > +}
> 
> The locking in this function makes no sense.  We drop the lock then return
> a value which the caller cannot use in a race-free fashion, because the
> lock is no longer held.

The lock only protects the protection domain table (and the device
table) itself. It does not protect the values the pointers in that list
point to. In this case its also not racy because a value to that list is
only written once and then never changed again (currently). If this
changes in the future (and it will) I will change the locking too. This
will also need reference counting for 'struct protection_domain' which
is not implemented yet.

Joerg

-- 
           |           AMD Saxony Limited Liability Company & Co. KG
 Operating |         Wilschdorfer Landstr. 101, 01109 Dresden, Germany
 System    |                  Register Court Dresden: HRA 4896
 Research  |              General Partner authorized to represent:
 Center    |             AMD Saxony LLC (Wilmington, Delaware, US)
           | General Manager of AMD Saxony LLC: Dr. Hans-R. Deppe, Thomas McCoy

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/