| Message ID | 20200407183742.4344-32-joro@8bytes.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | iommu: Move iommu_group setup to IOMMU core code | expand |
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
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
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 --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)