From: Marek Szyprowski <m.szyprowski@samsung.com>
To: Joerg Roedel <joro@8bytes.org>
Cc: Will Deacon <will@kernel.org>,
Robin Murphy <robin.murphy@arm.com>,
Kukjin Kim <kgene@kernel.org>,
Krzysztof Kozlowski <krzk@kernel.org>,
David Woodhouse <dwmw2@infradead.org>,
Lu Baolu <baolu.lu@linux.intel.com>,
Andy Gross <agross@kernel.org>,
Bjorn Andersson <bjorn.andersson@linaro.org>,
Matthias Brugger <matthias.bgg@gmail.com>,
Rob Clark <robdclark@gmail.com>, Heiko Stuebner <heiko@sntech.de>,
Gerald Schaefer <gerald.schaefer@de.ibm.com>,
Thierry Reding <thierry.reding@gmail.com>,
Jonathan Hunter <jonathanh@nvidia.com>,
Jean-Philippe Brucker <jean-philippe@linaro.org>,
iommu@lists.linux-foundation.org, linux-kernel@vger.kernel.org,
linux-samsung-soc@vger.kernel.org, linux-arm-msm@vger.kernel.org,
linux-mediatek@lists.infradead.org,
linux-rockchip@lists.infradead.org, linux-s390@vger.kernel.org,
linux-tegra@vger.kernel.org,
virtualization@lists.linux-foundation.org,
Joerg Roedel <jroedel@suse.de>
Subject: Re: [PATCH] iommu/exynos: Get rid of 'struct exynos_iommu_owner' exynos_iommu_owner
Date: Thu, 9 Apr 2020 15:58:00 +0200 [thread overview]
Message-ID: <40af831b-d00c-0cf9-0a06-e60c048a9ab8@samsung.com> (raw)
In-Reply-To: <20200409114620.GA16298@8bytes.org>
Hi Joerg,
On 09.04.2020 13:46, Joerg Roedel wrote:
> Hi Marek,
>
> I had some more thoughts and discussions with Robin about how to make
> this work with the Exynos driver. The result is the patch below, can you
> please test it and report back? Even better if you can fix up any
> breakage it might cause :)
I've checked and it works fine on top of
ff68eb23308e6538ec7864c83d39540f423bbe90. However I'm not a fan of
removing this 'owner' structure. It gave a nice abstraction for the all
SYSMMU controllers for the given device (although most devices in the
system have only one SYSMMU). Why this structure is a problem for your
rework?
I've also spent some time trying to fix exynos-iommu on top of your
iommu-probe-device branch. I really wonder if it works on any ARM 32bit
or 64bit systems for other IOMMUs.
I got something working on ARM32bit, but I have to move all the
initialization from exynos_iommu_probe_device/exynos_iommu_of_xlate to
exynos_sysmmu_probe(). I don't like such approach, because I had to use
bus_find_device() and manually find the owner for the every SYSMMU
controller in the system. This approach also lack a proper symmetry and
release path.
The main problem after your conversion is the fact that ->probe_device()
is called very early, before any other platform device (thus IOMMU
controller) is is probed. It doesn't handle EPROBE_DEFER too.
The other issue I've noticed is that iommu_device_set_ops() is not
enough to assign ops properly. I had to add iommu_fwspec_init(dev,
&dev->of_node->fwnode, &exynos_iommu_ops) to ensure that the 'dev' gets
proper iommu ops.
I will send my patch in a few minutes to show you the changes.
> >From 60a288509baa34df6a0bf437c977925a0a617c72 Mon Sep 17 00:00:00 2001
> From: Joerg Roedel <jroedel@suse.de>
> Date: Thu, 9 Apr 2020 13:38:18 +0200
> Subject: [PATCH] iommu/exynos: Get rid of 'struct exynos_iommu_owner'
>
> Remove 'struct exynos_iommu_owner' and replace it with a single-linked
> list of 'struct sysmmu_drvdata'. The first item in the list acts as a
> replacement for the previous exynos_iommu_owner structure. The
> iommu_device member of the first list item is reported to the IOMMU
> core code for the master device.
>
> Signed-off-by: Joerg Roedel <jroedel@suse.de>
> ---
> drivers/iommu/exynos-iommu.c | 155 ++++++++++++++++++++---------------
> 1 file changed, 88 insertions(+), 67 deletions(-)
>
> diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
> index 186ff5cc975c..e70eb360093f 100644
> --- a/drivers/iommu/exynos-iommu.c
> +++ b/drivers/iommu/exynos-iommu.c
> @@ -225,18 +225,6 @@ static const struct sysmmu_fault_info sysmmu_v5_faults[] = {
> { 20, REG_V5_FAULT_AW_VA, "AW SECURITY PROTECTION", IOMMU_FAULT_WRITE },
> };
>
> -/*
> - * This structure is attached to dev.archdata.iommu of the master device
> - * on device add, contains a list of SYSMMU controllers defined by device tree,
> - * which are bound to given master device. It is usually referenced by 'owner'
> - * pointer.
> -*/
> -struct exynos_iommu_owner {
> - struct list_head controllers; /* list of sysmmu_drvdata.owner_node */
> - struct iommu_domain *domain; /* domain this device is attached */
> - struct mutex rpm_lock; /* for runtime pm of all sysmmus */
> -};
> -
> /*
> * This structure exynos specific generalization of struct iommu_domain.
> * It contains list of SYSMMU controllers from all master devices, which has
> @@ -271,13 +259,23 @@ struct sysmmu_drvdata {
> bool active; /* current status */
> struct exynos_iommu_domain *domain; /* domain we belong to */
> struct list_head domain_node; /* node for domain clients list */
> - struct list_head owner_node; /* node for owner controllers list */
> + struct sysmmu_drvdata *next; /* Single-linked list to group SMMUs for
> + one master. NULL means not in any
> + list, ERR_PTR(-ENODEV) means end of
> + list */
> + struct mutex rpm_lock; /* for runtime pm of all sysmmus */
> phys_addr_t pgtable; /* assigned page table structure */
> unsigned int version; /* our version */
>
> struct iommu_device iommu; /* IOMMU core handle */
> };
>
> +/* Helper to iterate over all SYSMMUs for a given platform device */
> +#define for_each_sysmmu(dev, drvdata) \
> + for (drvdata = (dev)->archdata.iommu; \
> + drvdata != ERR_PTR(-ENODEV); \
> + drvdata = drvdata->next)
> +
> static struct exynos_iommu_domain *to_exynos_domain(struct iommu_domain *dom)
> {
> return container_of(dom, struct exynos_iommu_domain, domain);
> @@ -624,6 +622,8 @@ static int exynos_sysmmu_probe(struct platform_device *pdev)
>
> data->sysmmu = dev;
> spin_lock_init(&data->lock);
> + data->next = NULL;
> + mutex_init(&data->rpm_lock);
>
> ret = iommu_device_sysfs_add(&data->iommu, &pdev->dev, NULL,
> dev_name(data->sysmmu));
> @@ -668,17 +668,20 @@ static int __maybe_unused exynos_sysmmu_suspend(struct device *dev)
> {
> struct sysmmu_drvdata *data = dev_get_drvdata(dev);
> struct device *master = data->master;
> + struct sysmmu_drvdata *master_data;
>
> - if (master) {
> - struct exynos_iommu_owner *owner = master->archdata.iommu;
> + if (!master)
> + return 0;
>
> - mutex_lock(&owner->rpm_lock);
> - if (data->domain) {
> - dev_dbg(data->sysmmu, "saving state\n");
> - __sysmmu_disable(data);
> - }
> - mutex_unlock(&owner->rpm_lock);
> + master_data = master->archdata.iommu;
> +
> + mutex_lock(&master_data->rpm_lock);
> + if (data->domain) {
> + dev_dbg(data->sysmmu, "saving state\n");
> + __sysmmu_disable(data);
> }
> + mutex_unlock(&master_data->rpm_lock);
> +
> return 0;
> }
>
> @@ -686,17 +689,20 @@ static int __maybe_unused exynos_sysmmu_resume(struct device *dev)
> {
> struct sysmmu_drvdata *data = dev_get_drvdata(dev);
> struct device *master = data->master;
> + struct sysmmu_drvdata *master_data;
>
> - if (master) {
> - struct exynos_iommu_owner *owner = master->archdata.iommu;
> + if (!master)
> + return 0;
>
> - mutex_lock(&owner->rpm_lock);
> - if (data->domain) {
> - dev_dbg(data->sysmmu, "restoring state\n");
> - __sysmmu_enable(data);
> - }
> - mutex_unlock(&owner->rpm_lock);
> + master_data = master->archdata.iommu;
> +
> + mutex_lock(&master_data->rpm_lock);
> + if (data->domain) {
> + dev_dbg(data->sysmmu, "restoring state\n");
> + __sysmmu_enable(data);
> }
> + mutex_unlock(&master_data->rpm_lock);
> +
> return 0;
> }
>
> @@ -834,21 +840,21 @@ static void exynos_iommu_domain_free(struct iommu_domain *iommu_domain)
> kfree(domain);
> }
>
> -static void exynos_iommu_detach_device(struct iommu_domain *iommu_domain,
> - struct device *dev)
> +static void __exynos_iommu_detach_device(struct exynos_iommu_domain *domain,
> + struct device *dev)
> {
> - struct exynos_iommu_owner *owner = dev->archdata.iommu;
> - struct exynos_iommu_domain *domain = to_exynos_domain(iommu_domain);
> phys_addr_t pagetable = virt_to_phys(domain->pgtable);
> - struct sysmmu_drvdata *data, *next;
> + struct sysmmu_drvdata *dev_data, *data, *next;
> unsigned long flags;
>
> - if (!has_sysmmu(dev) || owner->domain != iommu_domain)
> + dev_data = dev->archdata.iommu;
> +
> + if (!has_sysmmu(dev) || dev_data->domain != domain)
> return;
>
> - mutex_lock(&owner->rpm_lock);
> + mutex_lock(&dev_data->rpm_lock);
>
> - list_for_each_entry(data, &owner->controllers, owner_node) {
> + for_each_sysmmu(dev, data) {
> pm_runtime_get_noresume(data->sysmmu);
> if (pm_runtime_active(data->sysmmu))
> __sysmmu_disable(data);
> @@ -863,51 +869,59 @@ static void exynos_iommu_detach_device(struct iommu_domain *iommu_domain,
> list_del_init(&data->domain_node);
> spin_unlock(&data->lock);
> }
> - owner->domain = NULL;
> spin_unlock_irqrestore(&domain->lock, flags);
>
> - mutex_unlock(&owner->rpm_lock);
> + mutex_unlock(&dev_data->rpm_lock);
>
> dev_dbg(dev, "%s: Detached IOMMU with pgtable %pa\n", __func__,
> &pagetable);
> }
>
> +static void exynos_iommu_detach_device(struct iommu_domain *iommu_domain,
> + struct device *dev)
> +{
> + struct exynos_iommu_domain *domain = to_exynos_domain(iommu_domain);
> +
> + __exynos_iommu_detach_device(domain, dev);
> +}
> +
> static int exynos_iommu_attach_device(struct iommu_domain *iommu_domain,
> struct device *dev)
> {
> - struct exynos_iommu_owner *owner = dev->archdata.iommu;
> struct exynos_iommu_domain *domain = to_exynos_domain(iommu_domain);
> - struct sysmmu_drvdata *data;
> + struct sysmmu_drvdata *dev_data, *data;
> phys_addr_t pagetable = virt_to_phys(domain->pgtable);
> unsigned long flags;
>
> if (!has_sysmmu(dev))
> return -ENODEV;
>
> - if (owner->domain)
> - exynos_iommu_detach_device(owner->domain, dev);
> + dev_data = dev->archdata.iommu;
>
> - mutex_lock(&owner->rpm_lock);
> + if (dev_data->domain)
> + __exynos_iommu_detach_device(dev_data->domain, dev);
> +
> + mutex_lock(&dev_data->rpm_lock);
>
> spin_lock_irqsave(&domain->lock, flags);
> - list_for_each_entry(data, &owner->controllers, owner_node) {
> + for_each_sysmmu(dev, data) {
> spin_lock(&data->lock);
> data->pgtable = pagetable;
> data->domain = domain;
> list_add_tail(&data->domain_node, &domain->clients);
> spin_unlock(&data->lock);
> }
> - owner->domain = iommu_domain;
> spin_unlock_irqrestore(&domain->lock, flags);
>
> - list_for_each_entry(data, &owner->controllers, owner_node) {
> +
> + for_each_sysmmu(dev, data) {
> pm_runtime_get_noresume(data->sysmmu);
> if (pm_runtime_active(data->sysmmu))
> __sysmmu_enable(data);
> pm_runtime_put(data->sysmmu);
> }
>
> - mutex_unlock(&owner->rpm_lock);
> + mutex_unlock(&dev_data->rpm_lock);
>
> dev_dbg(dev, "%s: Attached IOMMU with pgtable %pa\n", __func__,
> &pagetable);
> @@ -1237,7 +1251,6 @@ static phys_addr_t exynos_iommu_iova_to_phys(struct iommu_domain *iommu_domain,
>
> static int exynos_iommu_add_device(struct device *dev)
> {
> - struct exynos_iommu_owner *owner = dev->archdata.iommu;
> struct sysmmu_drvdata *data;
> struct iommu_group *group;
>
> @@ -1249,7 +1262,7 @@ static int exynos_iommu_add_device(struct device *dev)
> if (IS_ERR(group))
> return PTR_ERR(group);
>
> - list_for_each_entry(data, &owner->controllers, owner_node) {
> + for_each_sysmmu(dev, data) {
> /*
> * SYSMMU will be runtime activated via device link
> * (dependency) to its master device, so there are no
> @@ -1261,37 +1274,39 @@ static int exynos_iommu_add_device(struct device *dev)
> }
> iommu_group_put(group);
>
> + data = dev->archdata.iommu;
> + iommu_device_link(&data->iommu, dev);
> +
> return 0;
> }
>
> static void exynos_iommu_remove_device(struct device *dev)
> {
> - struct exynos_iommu_owner *owner = dev->archdata.iommu;
> - struct sysmmu_drvdata *data;
> + struct sysmmu_drvdata *data = dev->archdata.iommu;
>
> if (!has_sysmmu(dev))
> return;
>
> - if (owner->domain) {
> + if (data->domain) {
> struct iommu_group *group = iommu_group_get(dev);
>
> if (group) {
> - WARN_ON(owner->domain !=
> + WARN_ON(&data->domain->domain !=
> iommu_group_default_domain(group));
> - exynos_iommu_detach_device(owner->domain, dev);
> + __exynos_iommu_detach_device(data->domain, dev);
> iommu_group_put(group);
> }
> }
> + iommu_device_unlink(&data->iommu, dev);
> iommu_group_remove_device(dev);
>
> - list_for_each_entry(data, &owner->controllers, owner_node)
> + for_each_sysmmu(dev, data)
> device_link_del(data->link);
> }
>
> static int exynos_iommu_of_xlate(struct device *dev,
> struct of_phandle_args *spec)
> {
> - struct exynos_iommu_owner *owner = dev->archdata.iommu;
> struct platform_device *sysmmu = of_find_device_by_node(spec->np);
> struct sysmmu_drvdata *data, *entry;
>
> @@ -1302,22 +1317,28 @@ static int exynos_iommu_of_xlate(struct device *dev,
> if (!data)
> return -ENODEV;
>
> - if (!owner) {
> - owner = kzalloc(sizeof(*owner), GFP_KERNEL);
> - if (!owner)
> - return -ENOMEM;
> + data->master = dev;
>
> - INIT_LIST_HEAD(&owner->controllers);
> - mutex_init(&owner->rpm_lock);
> - dev->archdata.iommu = owner;
> + if (!dev->archdata.iommu) {
> + WARN_ON(data->next != NULL);
> +
> + /* SYSMMU list is empty - add drvdata and return */
> + data->next = ERR_PTR(-ENODEV);
> + dev->archdata.iommu = data;
> +
> + return 0;
> }
>
> - list_for_each_entry(entry, &owner->controllers, owner_node)
> + /* Check if SYSMMU is already in the list */
> + for_each_sysmmu(dev, entry)
> if (entry == data)
> return 0;
>
> - list_add_tail(&data->owner_node, &owner->controllers);
> - data->master = dev;
> + /* Not in the list yet */
> + WARN_ON(data->next != NULL);
> + entry = dev->archdata.iommu;
> + data->next = entry->next;
> + entry->next = data;
>
> return 0;
> }
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
next prev parent reply other threads:[~2020-04-09 13:58 UTC|newest]
Thread overview: 51+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-04-07 18:37 [RFC PATCH 00/34] iommu: Move iommu_group setup to IOMMU core code Joerg Roedel
2020-04-07 18:37 ` [RFC PATCH 01/34] iommu: Move default domain allocation to separate function Joerg Roedel
2020-04-07 18:37 ` [RFC PATCH 02/34] iommu: Add def_domain_type() callback in iommu_ops Joerg Roedel
2020-04-07 18:37 ` [RFC PATCH 03/34] iommu/amd: Implement iommu_ops->def_domain_type call-back Joerg Roedel
2020-04-07 18:37 ` [RFC PATCH 04/34] iommu/vt-d: Wire up iommu_ops->def_domain_type Joerg Roedel
2020-04-07 18:37 ` [RFC PATCH 05/34] iommu/amd: Remove dma_mask check from check_device() Joerg Roedel
2020-04-07 18:37 ` [RFC PATCH 06/34] iommu/amd: Return -ENODEV in add_device when device is not handled by IOMMU Joerg Roedel
2020-04-07 18:37 ` [RFC PATCH 07/34] iommu: Add probe_device() and remove_device() call-backs Joerg Roedel
2020-04-07 18:37 ` [RFC PATCH 08/34] iommu: Move default domain allocation to iommu_probe_device() Joerg Roedel
2020-04-07 18:37 ` [RFC PATCH 09/34] iommu: Keep a list of allocated groups in __iommu_probe_device() Joerg Roedel
2020-04-07 18:37 ` [RFC PATCH 10/34] iommu: Move new probe_device path to separate function Joerg Roedel
2020-04-07 18:37 ` [RFC PATCH 11/34] iommu: Split off default domain allocation from group assignment Joerg Roedel
2020-04-13 22:10 ` Derrick, Jonathan
2020-04-14 15:27 ` joro
2020-04-07 18:37 ` [RFC PATCH 12/34] iommu: Move iommu_group_create_direct_mappings() out of iommu_group_add_device() Joerg Roedel
2020-04-07 18:37 ` [RFC PATCH 13/34] iommu: Export bus_iommu_probe() and make is safe for re-probing Joerg Roedel
2020-04-07 18:37 ` [RFC PATCH 14/34] iommu/amd: Remove dev_data->passthrough Joerg Roedel
2020-04-07 18:37 ` [RFC PATCH 15/34] iommu/amd: Convert to probe/release_device() call-backs Joerg Roedel
2020-04-07 18:37 ` [RFC PATCH 16/34] iommu/vt-d: " Joerg Roedel
2020-04-07 18:37 ` [RFC PATCH 17/34] iommu/arm-smmu: Store device instead of group in arm_smmu_s2cr Joerg Roedel
2020-04-08 12:09 ` Robin Murphy
2020-04-08 14:37 ` Joerg Roedel
2020-04-08 15:07 ` Robin Murphy
2020-04-08 19:11 ` Joerg Roedel
2020-04-07 18:37 ` [RFC PATCH 18/34] iommu/arm-smmu: Convert to probe/release_device() call-backs Joerg Roedel
2020-04-07 18:37 ` [RFC PATCH 19/34] iommu/pamu: " Joerg Roedel
2020-04-07 18:37 ` [RFC PATCH 20/34] iommu/s390: " Joerg Roedel
2020-04-07 18:37 ` [RFC PATCH 21/34] iommu/virtio: " Joerg Roedel
2020-04-07 18:37 ` [RFC PATCH 22/34] iommu/msm: " Joerg Roedel
2020-04-07 18:37 ` [RFC PATCH 23/34] iommu/mediatek: " Joerg Roedel
2020-04-07 18:37 ` [RFC PATCH 24/34] iommu/mediatek-v1 " Joerg Roedel
2020-04-07 18:37 ` [RFC PATCH 25/34] iommu/qcom: " Joerg Roedel
2020-04-07 18:37 ` [RFC PATCH 26/34] iommu/rockchip: " Joerg Roedel
2020-04-07 18:37 ` [RFC PATCH 27/34] iommu/tegra: " Joerg Roedel
2020-04-07 18:37 ` [RFC PATCH 28/34] iommu/renesas: " Joerg Roedel
2020-04-07 18:37 ` [RFC PATCH 29/34] iommu/omap: Remove orphan_dev tracking Joerg Roedel
2020-04-07 18:37 ` [RFC PATCH 30/34] iommu/omap: Convert to probe/release_device() call-backs Joerg Roedel
2020-04-07 18:37 ` [RFC PATCH 31/34] iommu/exynos: Create iommu_device in struct exynos_iommu_owner Joerg Roedel
2020-04-08 12:23 ` Marek Szyprowski
2020-04-08 14:23 ` Marek Szyprowski
2020-04-08 15:00 ` Joerg Roedel
2020-04-09 11:46 ` [PATCH] iommu/exynos: Get rid of 'struct exynos_iommu_owner' exynos_iommu_owner Joerg Roedel
2020-04-09 13:58 ` Marek Szyprowski [this message]
[not found] ` <CGME20200409140939eucas1p190daac74c0d5dda4627314c49c1a5b50@eucas1p1.samsung.com>
2020-04-09 14:09 ` [PATCH] iommu/exynos: Rework intialization Marek Szyprowski
2020-04-09 14:30 ` [PATCH] iommu/exynos: Get rid of 'struct exynos_iommu_owner' exynos_iommu_owner Joerg Roedel
2020-04-14 13:20 ` Joerg Roedel
2020-04-07 18:37 ` [RFC PATCH 32/34] iommu/exynos: Convert to probe/release_device() call-backs Joerg Roedel
2020-04-07 18:37 ` [RFC PATCH 33/34] iommu: Remove add_device()/remove_device() code-paths Joerg Roedel
2020-04-10 10:39 ` Marek Szyprowski
2020-04-14 13:17 ` Joerg Roedel
2020-04-07 18:37 ` [RFC PATCH 34/34] iommu: Unexport iommu_group_get_for_dev() Joerg Roedel
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=40af831b-d00c-0cf9-0a06-e60c048a9ab8@samsung.com \
--to=m.szyprowski@samsung.com \
--cc=agross@kernel.org \
--cc=baolu.lu@linux.intel.com \
--cc=bjorn.andersson@linaro.org \
--cc=dwmw2@infradead.org \
--cc=gerald.schaefer@de.ibm.com \
--cc=heiko@sntech.de \
--cc=iommu@lists.linux-foundation.org \
--cc=jean-philippe@linaro.org \
--cc=jonathanh@nvidia.com \
--cc=joro@8bytes.org \
--cc=jroedel@suse.de \
--cc=kgene@kernel.org \
--cc=krzk@kernel.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mediatek@lists.infradead.org \
--cc=linux-rockchip@lists.infradead.org \
--cc=linux-s390@vger.kernel.org \
--cc=linux-samsung-soc@vger.kernel.org \
--cc=linux-tegra@vger.kernel.org \
--cc=matthias.bgg@gmail.com \
--cc=robdclark@gmail.com \
--cc=robin.murphy@arm.com \
--cc=thierry.reding@gmail.com \
--cc=virtualization@lists.linux-foundation.org \
--cc=will@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).