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@xxxxxxx>
> 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@xxxxxxx>
> ---
> 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
