Re: [PATCH] iommu/exynos: Get rid of 'struct exynos_iommu_owner' exynos_iommu_owner

[Marek Szyprowski]

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