diff mbox series

[RFC,31/34] iommu/exynos: Create iommu_device in struct exynos_iommu_owner

Message ID 20200407183742.4344-32-joro@8bytes.org (mailing list archive)
State New, archived
Headers show
Series iommu: Move iommu_group setup to IOMMU core code | expand

Commit Message

Joerg Roedel April 7, 2020, 6:37 p.m. UTC 
From: Joerg Roedel <jroedel@suse.de>

The 'struct exynos_iommu_owner' is an umbrella for multiple SYSMMU
instances attached to one master. As such all these instances are
handled the same, they are all configured with the same iommu_domain,
for example.

The IOMMU core code expects each device to have only one IOMMU
attached, so create the IOMMU-device for the umbrella instead of each
hardware SYSMMU.

Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 drivers/iommu/exynos-iommu.c | 96 +++++++++++++++++++++++++++---------
 1 file changed, 73 insertions(+), 23 deletions(-)

Comments

Marek Szyprowski April 8, 2020, 12:23 p.m. UTC | #1 
Hi Joerg,

On 07.04.2020 20:37, Joerg Roedel wrote:
> From: Joerg Roedel <jroedel@suse.de>
>
> The 'struct exynos_iommu_owner' is an umbrella for multiple SYSMMU
> instances attached to one master. As such all these instances are
> handled the same, they are all configured with the same iommu_domain,
> for example.
>
> The IOMMU core code expects each device to have only one IOMMU
> attached, so create the IOMMU-device for the umbrella instead of each
> hardware SYSMMU.
>
> Signed-off-by: Joerg Roedel <jroedel@suse.de>
> ---
>   drivers/iommu/exynos-iommu.c | 96 +++++++++++++++++++++++++++---------
>   1 file changed, 73 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
> index 186ff5cc975c..86ecccbf0438 100644
> --- a/drivers/iommu/exynos-iommu.c
> +++ b/drivers/iommu/exynos-iommu.c
> @@ -235,6 +235,8 @@ 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 */
> +
> +	struct iommu_device iommu;	/* IOMMU core handle */
>   };
>   
>   /*
> @@ -274,8 +276,6 @@ struct sysmmu_drvdata {
>   	struct list_head owner_node;	/* node for owner controllers list */
>   	phys_addr_t pgtable;		/* assigned page table structure */
>   	unsigned int version;		/* our version */
> -
> -	struct iommu_device iommu;	/* IOMMU core handle */
>   };
>   
>   static struct exynos_iommu_domain *to_exynos_domain(struct iommu_domain *dom)
> @@ -625,18 +625,6 @@ static int exynos_sysmmu_probe(struct platform_device *pdev)
>   	data->sysmmu = dev;
>   	spin_lock_init(&data->lock);
>   
> -	ret = iommu_device_sysfs_add(&data->iommu, &pdev->dev, NULL,
> -				     dev_name(data->sysmmu));
> -	if (ret)
> -		return ret;
> -
> -	iommu_device_set_ops(&data->iommu, &exynos_iommu_ops);
> -	iommu_device_set_fwnode(&data->iommu, &dev->of_node->fwnode);

The iommu_device_set_fwnode() call is lost during this conversion, what breaks driver operation. Most of the above IOMMU fw calls you have moved to xlate function. I've checked briefly but it looks that there is a chicken-egg problem here. The owner structure is allocated and initialized from of_xlate(), which won't be called without linking the problem iommu structure with the fwnode first, what might be done only in sysmmu_probe(). I will check how to handle this in a different way.

> -
> -	ret = iommu_device_register(&data->iommu);
> -	if (ret)
> -		return ret;
> -
>   	platform_set_drvdata(pdev, data);
>   
>   	__sysmmu_get_version(data);
> @@ -1261,6 +1249,8 @@ static int exynos_iommu_add_device(struct device *dev)
>   	}
>   	iommu_group_put(group);
>   
> +	iommu_device_link(&owner->iommu, dev);
> +
>   	return 0;
>   }
>   
> @@ -1282,18 +1272,82 @@ static void exynos_iommu_remove_device(struct device *dev)
>   			iommu_group_put(group);
>   		}
>   	}
> +	iommu_device_unlink(&owner->iommu, dev);
>   	iommu_group_remove_device(dev);
>   
>   	list_for_each_entry(data, &owner->controllers, owner_node)
>   		device_link_del(data->link);
>   }
>   
> +static int exynos_iommu_device_init(struct exynos_iommu_owner *owner)
> +{
> +	static u32 counter = 0;
> +	int ret;
> +
> +	/*
> +	 * Create a virtual IOMMU device. In reality it is an umbrella for a
> +	 * number of SYSMMU platform devices, but that also means that any
> +	 * master can have more than one real IOMMU device. This drivers handles
> +	 * all the real devices for one master synchronously, so they appear as
> +	 * one anyway.
> +	 */
> +	ret = iommu_device_sysfs_add(&owner->iommu, NULL, NULL,
> +				     "sysmmu-owner-%d", counter++);
> +	if (ret)
> +		return ret;
> +
> +	iommu_device_set_ops(&owner->iommu, &exynos_iommu_ops);
> +
> +	return 0;
> +}
> +
> +static void exynos_iommu_device_remove(struct exynos_iommu_owner *owner)
> +{
> +	iommu_device_set_ops(&owner->iommu, NULL);
> +	iommu_device_sysfs_remove(&owner->iommu);
> +}
> +
> +static int exynos_owner_init(struct device *dev)
> +{
> +	struct exynos_iommu_owner *owner = dev->archdata.iommu;
> +	int ret;
> +
> +	if (owner)
> +		return 0;
> +
> +	owner = kzalloc(sizeof(*owner), GFP_KERNEL);
> +	if (!owner)
> +		return -ENOMEM;
> +
> +	ret = exynos_iommu_device_init(owner);
> +	if (ret)
> +		goto out_free_owner;
> +
> +	ret = iommu_device_register(&owner->iommu);
> +	if (ret)
> +		goto out_remove_iommu_device;
> +
> +	INIT_LIST_HEAD(&owner->controllers);
> +	mutex_init(&owner->rpm_lock);
> +	dev->archdata.iommu = owner;
> +
> +	return 0;
> +
> +out_remove_iommu_device:
> +	exynos_iommu_device_remove(owner);
> +out_free_owner:
> +	kfree(owner);
> +
> +	return ret;
> +}
> +
>   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;
> +	struct exynos_iommu_owner *owner;
> +	int ret;
>   
>   	if (!sysmmu)
>   		return -ENODEV;
> @@ -1302,15 +1356,11 @@ 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;
> +	ret = exynos_owner_init(dev);
> +	if (ret)
> +		return ret;
>   
> -		INIT_LIST_HEAD(&owner->controllers);
> -		mutex_init(&owner->rpm_lock);
> -		dev->archdata.iommu = owner;
> -	}
> +	owner = dev->archdata.iommu;
>   
>   	list_for_each_entry(entry, &owner->controllers, owner_node)
>   		if (entry == data)

Best regards
Marek Szyprowski April 8, 2020, 2:23 p.m. UTC | #2 
Hi again,

On 08.04.2020 14:23, Marek Szyprowski wrote:
> Hi Joerg,
>
> On 07.04.2020 20:37, Joerg Roedel wrote:
>> From: Joerg Roedel <jroedel@suse.de>
>>
>> The 'struct exynos_iommu_owner' is an umbrella for multiple SYSMMU
>> instances attached to one master. As such all these instances are
>> handled the same, they are all configured with the same iommu_domain,
>> for example.
>>
>> The IOMMU core code expects each device to have only one IOMMU
>> attached, so create the IOMMU-device for the umbrella instead of each
>> hardware SYSMMU.
>>
>> Signed-off-by: Joerg Roedel <jroedel@suse.de>
>> ---
>>   drivers/iommu/exynos-iommu.c | 96 +++++++++++++++++++++++++++---------
>>   1 file changed, 73 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
>> index 186ff5cc975c..86ecccbf0438 100644
>> --- a/drivers/iommu/exynos-iommu.c
>> +++ b/drivers/iommu/exynos-iommu.c
>> @@ -235,6 +235,8 @@ 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 */
>> +
>> +    struct iommu_device iommu;    /* IOMMU core handle */
>>   };
>>     /*
>> @@ -274,8 +276,6 @@ struct sysmmu_drvdata {
>>       struct list_head owner_node;    /* node for owner controllers 
>> list */
>>       phys_addr_t pgtable;        /* assigned page table structure */
>>       unsigned int version;        /* our version */
>> -
>> -    struct iommu_device iommu;    /* IOMMU core handle */
>>   };
>>     static struct exynos_iommu_domain *to_exynos_domain(struct 
>> iommu_domain *dom)
>> @@ -625,18 +625,6 @@ static int exynos_sysmmu_probe(struct 
>> platform_device *pdev)
>>       data->sysmmu = dev;
>>       spin_lock_init(&data->lock);
>>   -    ret = iommu_device_sysfs_add(&data->iommu, &pdev->dev, NULL,
>> -                     dev_name(data->sysmmu));
>> -    if (ret)
>> -        return ret;
>> -
>> -    iommu_device_set_ops(&data->iommu, &exynos_iommu_ops);
>> -    iommu_device_set_fwnode(&data->iommu, &dev->of_node->fwnode);
>
> The iommu_device_set_fwnode() call is lost during this conversion, 
> what breaks driver operation. Most of the above IOMMU fw calls you 
> have moved to xlate function. I've checked briefly but it looks that 
> there is a chicken-egg problem here. The owner structure is allocated 
> and initialized from of_xlate(), which won't be called without linking 
> the problem iommu structure with the fwnode first, what might be done 
> only in sysmmu_probe(). I will check how to handle this in a different 
> way.

I've played with this a bit and it looks that won't be easy to make it 
working on ARM 32bit.

I need a place to initialize properly all the structures for the given 
master (IOMMU client device, the one which performs DMA operations).

I tried to move all the initialization from xlate() to device_probe(), 
but such approach doesn't work.

On ARM32bit exynos_iommu_device_probe() is called by the device core and 
IOMMU framework very early, before the Exynos SYSMMU platform devices 
(controllers) are probed yet. Even iommu class is not yet initialized 
that time, so returning anything successful from it causes following 
NULL ptr dereference:

Unable to handle kernel NULL pointer dereference at virtual address 0000004c
...

(__iommu_probe_device) from [<c055b334>] (iommu_probe_device+0x18/0x114)
(iommu_probe_device) from [<c055b4ac>] (iommu_bus_notifier+0x7c/0x94)
(iommu_bus_notifier) from [<c014e8ec>] (notifier_call_chain+0x44/0x84)
(notifier_call_chain) from [<c014e9ec>] 
(__blocking_notifier_call_chain+0x48/0x60)
(__blocking_notifier_call_chain) from [<c014ea1c>] 
(blocking_notifier_call_chain+0x18/0x20)
(blocking_notifier_call_chain) from [<c05c8d1c>] (device_add+0x3e8/0x704)
(device_add) from [<c07bafc4>] (of_platform_device_create_pdata+0x90/0xc4)
(of_platform_device_create_pdata) from [<c07bb138>] 
(of_platform_bus_create+0x134/0x48c)
(of_platform_bus_create) from [<c07bb1a4>] 
(of_platform_bus_create+0x1a0/0x48c)
(of_platform_bus_create) from [<c07bb63c>] (of_platform_populate+0x84/0x114)
(of_platform_populate) from [<c1125e9c>] 
(of_platform_default_populate_init+0x90/0xac)
(of_platform_default_populate_init) from [<c010326c>] 
(do_one_initcall+0x80/0x42c)
(do_one_initcall) from [<c1101074>] (kernel_init_freeable+0x15c/0x208)
(kernel_init_freeable) from [<c0afdde0>] (kernel_init+0x8/0x118)
(kernel_init) from [<c01010b4>] (ret_from_fork+0x14/0x20)

I've tried returning ERR_PTR(-EPROBE_DEFER) from device_probe(), but I 
doesn't work there. Some more changes in the framework are needed...

 > ...

Best regards
Joerg Roedel April 8, 2020, 3 p.m. UTC | #3 
Hi Marek,

On Wed, Apr 08, 2020 at 04:23:05PM +0200, Marek Szyprowski wrote:
> I need a place to initialize properly all the structures for the given 
> master (IOMMU client device, the one which performs DMA operations).

That could be in the probe_device() call-back, no?

> I tried to move all the initialization from xlate() to device_probe(), 
> but such approach doesn't work.

device_probe() is exynos_sysmmu_probe(), then yes, this is called before
any of the xlate() calls are made.

Would it work to keep the iommu_device structures in the sysmmus and
also create them for the owners? This isn't really a nice solution but
should work the the IOMMU driver until there is a better way to fix
this.

Regards,

	Joerg
diff mbox series

Patch

diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
index 186ff5cc975c..86ecccbf0438 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -235,6 +235,8 @@  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 */
+
+	struct iommu_device iommu;	/* IOMMU core handle */
 };
 
 /*
@@ -274,8 +276,6 @@  struct sysmmu_drvdata {
 	struct list_head owner_node;	/* node for owner controllers list */
 	phys_addr_t pgtable;		/* assigned page table structure */
 	unsigned int version;		/* our version */
-
-	struct iommu_device iommu;	/* IOMMU core handle */
 };
 
 static struct exynos_iommu_domain *to_exynos_domain(struct iommu_domain *dom)
@@ -625,18 +625,6 @@  static int exynos_sysmmu_probe(struct platform_device *pdev)
 	data->sysmmu = dev;
 	spin_lock_init(&data->lock);
 
-	ret = iommu_device_sysfs_add(&data->iommu, &pdev->dev, NULL,
-				     dev_name(data->sysmmu));
-	if (ret)
-		return ret;
-
-	iommu_device_set_ops(&data->iommu, &exynos_iommu_ops);
-	iommu_device_set_fwnode(&data->iommu, &dev->of_node->fwnode);
-
-	ret = iommu_device_register(&data->iommu);
-	if (ret)
-		return ret;
-
 	platform_set_drvdata(pdev, data);
 
 	__sysmmu_get_version(data);
@@ -1261,6 +1249,8 @@  static int exynos_iommu_add_device(struct device *dev)
 	}
 	iommu_group_put(group);
 
+	iommu_device_link(&owner->iommu, dev);
+
 	return 0;
 }
 
@@ -1282,18 +1272,82 @@  static void exynos_iommu_remove_device(struct device *dev)
 			iommu_group_put(group);
 		}
 	}
+	iommu_device_unlink(&owner->iommu, dev);
 	iommu_group_remove_device(dev);
 
 	list_for_each_entry(data, &owner->controllers, owner_node)
 		device_link_del(data->link);
 }
 
+static int exynos_iommu_device_init(struct exynos_iommu_owner *owner)
+{
+	static u32 counter = 0;
+	int ret;
+
+	/*
+	 * Create a virtual IOMMU device. In reality it is an umbrella for a
+	 * number of SYSMMU platform devices, but that also means that any
+	 * master can have more than one real IOMMU device. This drivers handles
+	 * all the real devices for one master synchronously, so they appear as
+	 * one anyway.
+	 */
+	ret = iommu_device_sysfs_add(&owner->iommu, NULL, NULL,
+				     "sysmmu-owner-%d", counter++);
+	if (ret)
+		return ret;
+
+	iommu_device_set_ops(&owner->iommu, &exynos_iommu_ops);
+
+	return 0;
+}
+
+static void exynos_iommu_device_remove(struct exynos_iommu_owner *owner)
+{
+	iommu_device_set_ops(&owner->iommu, NULL);
+	iommu_device_sysfs_remove(&owner->iommu);
+}
+
+static int exynos_owner_init(struct device *dev)
+{
+	struct exynos_iommu_owner *owner = dev->archdata.iommu;
+	int ret;
+
+	if (owner)
+		return 0;
+
+	owner = kzalloc(sizeof(*owner), GFP_KERNEL);
+	if (!owner)
+		return -ENOMEM;
+
+	ret = exynos_iommu_device_init(owner);
+	if (ret)
+		goto out_free_owner;
+
+	ret = iommu_device_register(&owner->iommu);
+	if (ret)
+		goto out_remove_iommu_device;
+
+	INIT_LIST_HEAD(&owner->controllers);
+	mutex_init(&owner->rpm_lock);
+	dev->archdata.iommu = owner;
+
+	return 0;
+
+out_remove_iommu_device:
+	exynos_iommu_device_remove(owner);
+out_free_owner:
+	kfree(owner);
+
+	return ret;
+}
+
 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;
+	struct exynos_iommu_owner *owner;
+	int ret;
 
 	if (!sysmmu)
 		return -ENODEV;
@@ -1302,15 +1356,11 @@  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;
+	ret = exynos_owner_init(dev);
+	if (ret)
+		return ret;
 
-		INIT_LIST_HEAD(&owner->controllers);
-		mutex_init(&owner->rpm_lock);
-		dev->archdata.iommu = owner;
-	}
+	owner = dev->archdata.iommu;
 
 	list_for_each_entry(entry, &owner->controllers, owner_node)
 		if (entry == data)