linux-kernel - Re: [PATCH v5 2/2] iommu/exynos: Add iommu driver for Exynos Platforms

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Sat, 15 Oct 2011 10:55:52 +0900
From:	KyongHo Cho <pullip.cho@...sung.com>
To:	"Roedel, Joerg" <Joerg.Roedel@....com>
Cc:	Linux ARM Kernel <linux-arm-kernel@...ts.infradead.org>,
	"linux-samsung-soc@...r.kernel.org" 
	<linux-samsung-soc@...r.kernel.org>,
	"iommu@...ts.linux-foundation.org" <iommu@...ts.linux-foundation.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Russell King <rmk+kernel@....linux.org.uk>,
	Ohad Ben-Cohen <ohad@...ery.com>,
	Sanghyun Lee <sanghyun75.lee@...sung.com>,
	"younglak1004.kim@...sung.com" <younglak1004.kim@...sung.com>,
	Kukjin Kim <kgene.kim@...sung.com>
Subject: Re: [PATCH v5 2/2] iommu/exynos: Add iommu driver for Exynos Platforms

On Fri, Oct 14, 2011 at 11:17 PM, Roedel, Joerg <Joerg.Roedel@....com> wrote:
> On Thu, Oct 13, 2011 at 04:13:23AM -0400, KyongHo Cho wrote:
>> +struct exynos_iommu_domain {
>> +       struct device *dev;
>> +       unsigned long *pgtable;
>> +       spinlock_t lock;
>> +       spinlock_t pgtablelock;
>> +};
>
> Can you please add comments to document what these spinlocks protect?
>
'lock' protects race condition on the domain private data.
'pgtablelock' does it on the page table of the domain.
Since updating page table does not need to be synchronized with
domain private data, I defined those 2 locks seperately.

I will add this information in the source code as comments.

>> +static int exynos_iommu_attach_device(struct iommu_domain *domain,
>> +                                  struct device *dev)
>> +{
>> +       struct exynos_iommu_domain *priv = domain->priv;
>> +       int ret;
>> +
>> +       spin_lock(&priv->lock);
>> +
>> +       priv->dev = dev;
>> +
>> +       spin_unlock(&priv->lock);
>> +
>> +       ret = exynos_iommu_enable(domain);
>> +
>> +       return ret;
>> +}
>> +
>> +static void exynos_iommu_detach_device(struct iommu_domain *domain,
>> +                                   struct device *dev)
>> +{
>> +       struct exynos_iommu_domain *priv = domain->priv;
>> +
>> +       spin_lock(&priv->lock);
>> +
>> +       if (priv->dev == dev) {
>> +               priv->dev = NULL;
>> +
>> +               spin_unlock(&priv->lock);
>> +
>> +               exynos_iommu_disable(domain);
>> +       } else {
>> +               spin_unlock(&priv->lock);
>> +       }
>> +}
>
> That looks weird. As I read this code there is a 1-1 mapping between a
> device and a domain. This breaks semantics of the iommu-api where a
> domain can contain multiple devices.

I agree.
I will implement it soon.


Thank you

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