| Message ID | 1387284818-28739-2-git-send-email-florian.vaussard@epfl.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Florian, On 12/17/2013 06:53 AM, Florian Vaussard wrote: > Currently, bus_set_iommu() is done in omap_iommu_init(). However, > omap_iommu_probe() can fail in a number of ways, leaving the platform > bus with a dangling reference to a non-initialized iommu. Perform > bus_set_iommu() only if omap_iommu_probe() succeed. Can you clarify a bit more on what kind of issues you were seeing specifically? In general, there can be multiple instances of the iommu, so setting it in probe may not be fixing whatever issue you were seeing. The current OMAP3 code has only the ISP MMU enabled, but there is also another one for the IVA MMU (currently not configured by default). Moving the bus_set_iommu to probe makes sense if only one iommu is present, so this patch may not be needed at all. Also, the main change in this patch is moving the bus_set_iommu from omap_iommu_init to omap_iommu_probe, so you should probably leave out moving the function. The omap_iommu_probe function would anyway need conversion to using devm_ functions. regards Suman > > Signed-off-by: Florian Vaussard <florian.vaussard@epfl.ch> > --- > drivers/iommu/omap-iommu.c | 207 +++++++++++++++++++++++---------------------- > 1 file changed, 104 insertions(+), 103 deletions(-) > > diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c > index bcd78a7..ee83bcc 100644 > --- a/drivers/iommu/omap-iommu.c > +++ b/drivers/iommu/omap-iommu.c > @@ -930,107 +930,6 @@ static void omap_iommu_detach(struct omap_iommu *obj) > dev_dbg(obj->dev, "%s: %s\n", __func__, obj->name); > } > > -/* > - * OMAP Device MMU(IOMMU) detection > - */ > -static int omap_iommu_probe(struct platform_device *pdev) > -{ > - int err = -ENODEV; > - int irq; > - struct omap_iommu *obj; > - struct resource *res; > - struct iommu_platform_data *pdata = pdev->dev.platform_data; > - > - obj = kzalloc(sizeof(*obj) + MMU_REG_SIZE, GFP_KERNEL); > - if (!obj) > - return -ENOMEM; > - > - obj->nr_tlb_entries = pdata->nr_tlb_entries; > - obj->name = pdata->name; > - obj->dev = &pdev->dev; > - obj->ctx = (void *)obj + sizeof(*obj); > - obj->da_start = pdata->da_start; > - obj->da_end = pdata->da_end; > - > - spin_lock_init(&obj->iommu_lock); > - mutex_init(&obj->mmap_lock); > - spin_lock_init(&obj->page_table_lock); > - INIT_LIST_HEAD(&obj->mmap); > - > - res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > - if (!res) { > - err = -ENODEV; > - goto err_mem; > - } > - > - res = request_mem_region(res->start, resource_size(res), > - dev_name(&pdev->dev)); > - if (!res) { > - err = -EIO; > - goto err_mem; > - } > - > - obj->regbase = ioremap(res->start, resource_size(res)); > - if (!obj->regbase) { > - err = -ENOMEM; > - goto err_ioremap; > - } > - > - irq = platform_get_irq(pdev, 0); > - if (irq < 0) { > - err = -ENODEV; > - goto err_irq; > - } > - err = request_irq(irq, iommu_fault_handler, IRQF_SHARED, > - dev_name(&pdev->dev), obj); > - if (err < 0) > - goto err_irq; > - platform_set_drvdata(pdev, obj); > - > - pm_runtime_irq_safe(obj->dev); > - pm_runtime_enable(obj->dev); > - > - dev_info(&pdev->dev, "%s registered\n", obj->name); > - return 0; > - > -err_irq: > - iounmap(obj->regbase); > -err_ioremap: > - release_mem_region(res->start, resource_size(res)); > -err_mem: > - kfree(obj); > - return err; > -} > - > -static int omap_iommu_remove(struct platform_device *pdev) > -{ > - int irq; > - struct resource *res; > - struct omap_iommu *obj = platform_get_drvdata(pdev); > - > - iopgtable_clear_entry_all(obj); > - > - irq = platform_get_irq(pdev, 0); > - free_irq(irq, obj); > - res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > - release_mem_region(res->start, resource_size(res)); > - iounmap(obj->regbase); > - > - pm_runtime_disable(obj->dev); > - > - dev_info(&pdev->dev, "%s removed\n", obj->name); > - kfree(obj); > - return 0; > -} > - > -static struct platform_driver omap_iommu_driver = { > - .probe = omap_iommu_probe, > - .remove = omap_iommu_remove, > - .driver = { > - .name = "omap-iommu", > - }, > -}; > - > static void iopte_cachep_ctor(void *iopte) > { > clean_dcache_area(iopte, IOPTE_TABLE_SIZE); > @@ -1265,6 +1164,110 @@ static struct iommu_ops omap_iommu_ops = { > .pgsize_bitmap = OMAP_IOMMU_PGSIZES, > }; > > +/* > + * OMAP Device MMU(IOMMU) detection > + */ > +static int omap_iommu_probe(struct platform_device *pdev) > +{ > + int err = -ENODEV; > + int irq; > + struct omap_iommu *obj; > + struct resource *res; > + struct iommu_platform_data *pdata = pdev->dev.platform_data; > + > + obj = kzalloc(sizeof(*obj) + MMU_REG_SIZE, GFP_KERNEL); > + if (!obj) > + return -ENOMEM; > + > + obj->nr_tlb_entries = pdata->nr_tlb_entries; > + obj->name = pdata->name; > + obj->dev = &pdev->dev; > + obj->ctx = (void *)obj + sizeof(*obj); > + obj->da_start = pdata->da_start; > + obj->da_end = pdata->da_end; > + > + spin_lock_init(&obj->iommu_lock); > + mutex_init(&obj->mmap_lock); > + spin_lock_init(&obj->page_table_lock); > + INIT_LIST_HEAD(&obj->mmap); > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + if (!res) { > + err = -ENODEV; > + goto err_mem; > + } > + > + res = request_mem_region(res->start, resource_size(res), > + dev_name(&pdev->dev)); > + if (!res) { > + err = -EIO; > + goto err_mem; > + } > + > + obj->regbase = ioremap(res->start, resource_size(res)); > + if (!obj->regbase) { > + err = -ENOMEM; > + goto err_ioremap; > + } > + > + irq = platform_get_irq(pdev, 0); > + if (irq < 0) { > + err = -ENODEV; > + goto err_irq; > + } > + > + err = request_irq(irq, iommu_fault_handler, IRQF_SHARED, > + dev_name(&pdev->dev), obj); > + if (err < 0) > + goto err_irq; > + platform_set_drvdata(pdev, obj); > + > + bus_set_iommu(&platform_bus_type, &omap_iommu_ops); > + > + pm_runtime_irq_safe(obj->dev); > + pm_runtime_enable(obj->dev); > + > + dev_info(&pdev->dev, "%s registered\n", obj->name); > + return 0; > + > +err_irq: > + iounmap(obj->regbase); > +err_ioremap: > + release_mem_region(res->start, resource_size(res)); > +err_mem: > + kfree(obj); > + return err; > +} > + > +static int omap_iommu_remove(struct platform_device *pdev) > +{ > + int irq; > + struct resource *res; > + struct omap_iommu *obj = platform_get_drvdata(pdev); > + > + iopgtable_clear_entry_all(obj); > + > + irq = platform_get_irq(pdev, 0); > + free_irq(irq, obj); > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + release_mem_region(res->start, resource_size(res)); > + iounmap(obj->regbase); > + > + pm_runtime_disable(obj->dev); > + > + dev_info(&pdev->dev, "%s removed\n", obj->name); > + kfree(obj); > + return 0; > +} > + > +static struct platform_driver omap_iommu_driver = { > + .probe = omap_iommu_probe, > + .remove = omap_iommu_remove, > + .driver = { > + .name = "omap-iommu", > + }, > +}; > + > static int __init omap_iommu_init(void) > { > struct kmem_cache *p; > @@ -1277,8 +1280,6 @@ static int __init omap_iommu_init(void) > return -ENOMEM; > iopte_cachep = p; > > - bus_set_iommu(&platform_bus_type, &omap_iommu_ops); > - > return platform_driver_register(&omap_iommu_driver); > } > /* must be ready before omap3isp is probed */ >
Hi Suman, On 12/23/2013 08:02 PM, Anna, Suman wrote: > Hi Florian, > > On 12/17/2013 06:53 AM, Florian Vaussard wrote: >> Currently, bus_set_iommu() is done in omap_iommu_init(). However, >> omap_iommu_probe() can fail in a number of ways, leaving the platform >> bus with a dangling reference to a non-initialized iommu. Perform >> bus_set_iommu() only if omap_iommu_probe() succeed. > > Can you clarify a bit more on what kind of issues you were seeing > specifically? In general, there can be multiple instances of the iommu, > so setting it in probe may not be fixing whatever issue you were seeing. > The current OMAP3 code has only the ISP MMU enabled, but there is also > another one for the IVA MMU (currently not configured by default). > Moving the bus_set_iommu to probe makes sense if only one iommu is > present, so this patch may not be needed at all. > If omap_iommu_probe() fails, the init will have called bus_set_iommu() anyways. Thus, when a driver request the iommu by calling iommu_domain_alloc(), it will succeed (but iommu_attach_device() will fail if I remember). Leaving a driver with a dangling reference to a phantom iommu is not good IMHO. It will lead to strange behaviours one day or another. As for the multiple iommu case, as I do not think it is currently possible, as bus_type (in this case &platform_bus_type) has only one struct iommu_ops. bus_set_iommu() will return EBUSY on the second call. Am I missing something? > Also, the main change in this patch is moving the bus_set_iommu from > omap_iommu_init to omap_iommu_probe, so you should probably leave out > moving the function. The omap_iommu_probe function would anyway need > conversion to using devm_ functions. > This was my first try also, but as bus_set_iommu() needs omap_iommu_ops (itself depending on several functions), its call must come after the declaration of omap_iommu_ops. Thus I moved omap_iommu_probe() after the declaration of omap_iommu_ops. But I can probably use a forward declaration for omap_iommu_ops, this would be better. Indeed, we can also convert to devm_. Regards, Florian
Hi Florian, >> >> On 12/17/2013 06:53 AM, Florian Vaussard wrote: >>> Currently, bus_set_iommu() is done in omap_iommu_init(). However, >>> omap_iommu_probe() can fail in a number of ways, leaving the platform >>> bus with a dangling reference to a non-initialized iommu. Perform >>> bus_set_iommu() only if omap_iommu_probe() succeed. >> >> Can you clarify a bit more on what kind of issues you were seeing >> specifically? In general, there can be multiple instances of the iommu, >> so setting it in probe may not be fixing whatever issue you were seeing. >> The current OMAP3 code has only the ISP MMU enabled, but there is also >> another one for the IVA MMU (currently not configured by default). >> Moving the bus_set_iommu to probe makes sense if only one iommu is >> present, so this patch may not be needed at all. >> > > If omap_iommu_probe() fails, the init will have called bus_set_iommu() > anyways. Thus, when a driver request the iommu by calling > iommu_domain_alloc(), it will succeed (but iommu_attach_device() will > fail if I remember). Yeah, thats the behavior I expected anyway. > Leaving a driver with a dangling reference to > a phantom iommu is not good IMHO. It will lead to strange behaviours > one day or another. > > As for the multiple iommu case, as I do not think it is currently > possible, as bus_type (in this case &platform_bus_type) has only > one struct iommu_ops. bus_set_iommu() will return EBUSY on the > second call. Am I missing something? What I meant was the problem you cited will still exist, say if ISP MMU probe failed, but the IVA MMU probe succeeded. The bus_set_iommu() can only be called once anyway, so moving it from init to probe would not help much. regards Suman > >> Also, the main change in this patch is moving the bus_set_iommu from >> omap_iommu_init to omap_iommu_probe, so you should probably leave out >> moving the function. The omap_iommu_probe function would anyway need >> conversion to using devm_ functions. >> > > This was my first try also, but as bus_set_iommu() needs omap_iommu_ops > (itself depending on several functions), its call must come after the > declaration of omap_iommu_ops. Thus I moved omap_iommu_probe() after > the declaration of omap_iommu_ops. But I can probably use a forward > declaration for omap_iommu_ops, this would be better. > > Indeed, we can also convert to devm_. > > Regards, > > Florian >
Hi Suman, So back to this... On 12/24/2013 12:35 AM, Anna, Suman wrote: > Hi Florian, > [...] >> >> If omap_iommu_probe() fails, the init will have called bus_set_iommu() >> anyways. Thus, when a driver request the iommu by calling >> iommu_domain_alloc(), it will succeed (but iommu_attach_device() will >> fail if I remember). > > Yeah, thats the behavior I expected anyway. > >> Leaving a driver with a dangling reference to >> a phantom iommu is not good IMHO. It will lead to strange behaviours >> one day or another. >> >> As for the multiple iommu case, as I do not think it is currently >> possible, as bus_type (in this case &platform_bus_type) has only >> one struct iommu_ops. bus_set_iommu() will return EBUSY on the >> second call. Am I missing something? > > What I meant was the problem you cited will still exist, say if ISP MMU > probe failed, but the IVA MMU probe succeeded. The bus_set_iommu() can > only be called once anyway, so moving it from init to probe would not > help much. > Ok I see your point. Similar IPs share the same ops, but with different omap_iommu_arch_data, even if currently we only have one registered IOMMU for OMAP3. So I will drop this patch. Regards, Florian
diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c index bcd78a7..ee83bcc 100644 --- a/drivers/iommu/omap-iommu.c +++ b/drivers/iommu/omap-iommu.c @@ -930,107 +930,6 @@ static void omap_iommu_detach(struct omap_iommu *obj) dev_dbg(obj->dev, "%s: %s\n", __func__, obj->name); } -/* - * OMAP Device MMU(IOMMU) detection - */ -static int omap_iommu_probe(struct platform_device *pdev) -{ - int err = -ENODEV; - int irq; - struct omap_iommu *obj; - struct resource *res; - struct iommu_platform_data *pdata = pdev->dev.platform_data; - - obj = kzalloc(sizeof(*obj) + MMU_REG_SIZE, GFP_KERNEL); - if (!obj) - return -ENOMEM; - - obj->nr_tlb_entries = pdata->nr_tlb_entries; - obj->name = pdata->name; - obj->dev = &pdev->dev; - obj->ctx = (void *)obj + sizeof(*obj); - obj->da_start = pdata->da_start; - obj->da_end = pdata->da_end; - - spin_lock_init(&obj->iommu_lock); - mutex_init(&obj->mmap_lock); - spin_lock_init(&obj->page_table_lock); - INIT_LIST_HEAD(&obj->mmap); - - res = platform_get_resource(pdev, IORESOURCE_MEM, 0); - if (!res) { - err = -ENODEV; - goto err_mem; - } - - res = request_mem_region(res->start, resource_size(res), - dev_name(&pdev->dev)); - if (!res) { - err = -EIO; - goto err_mem; - } - - obj->regbase = ioremap(res->start, resource_size(res)); - if (!obj->regbase) { - err = -ENOMEM; - goto err_ioremap; - } - - irq = platform_get_irq(pdev, 0); - if (irq < 0) { - err = -ENODEV; - goto err_irq; - } - err = request_irq(irq, iommu_fault_handler, IRQF_SHARED, - dev_name(&pdev->dev), obj); - if (err < 0) - goto err_irq; - platform_set_drvdata(pdev, obj); - - pm_runtime_irq_safe(obj->dev); - pm_runtime_enable(obj->dev); - - dev_info(&pdev->dev, "%s registered\n", obj->name); - return 0; - -err_irq: - iounmap(obj->regbase); -err_ioremap: - release_mem_region(res->start, resource_size(res)); -err_mem: - kfree(obj); - return err; -} - -static int omap_iommu_remove(struct platform_device *pdev) -{ - int irq; - struct resource *res; - struct omap_iommu *obj = platform_get_drvdata(pdev); - - iopgtable_clear_entry_all(obj); - - irq = platform_get_irq(pdev, 0); - free_irq(irq, obj); - res = platform_get_resource(pdev, IORESOURCE_MEM, 0); - release_mem_region(res->start, resource_size(res)); - iounmap(obj->regbase); - - pm_runtime_disable(obj->dev); - - dev_info(&pdev->dev, "%s removed\n", obj->name); - kfree(obj); - return 0; -} - -static struct platform_driver omap_iommu_driver = { - .probe = omap_iommu_probe, - .remove = omap_iommu_remove, - .driver = { - .name = "omap-iommu", - }, -}; - static void iopte_cachep_ctor(void *iopte) { clean_dcache_area(iopte, IOPTE_TABLE_SIZE); @@ -1265,6 +1164,110 @@ static struct iommu_ops omap_iommu_ops = { .pgsize_bitmap = OMAP_IOMMU_PGSIZES, }; +/* + * OMAP Device MMU(IOMMU) detection + */ +static int omap_iommu_probe(struct platform_device *pdev) +{ + int err = -ENODEV; + int irq; + struct omap_iommu *obj; + struct resource *res; + struct iommu_platform_data *pdata = pdev->dev.platform_data; + + obj = kzalloc(sizeof(*obj) + MMU_REG_SIZE, GFP_KERNEL); + if (!obj) + return -ENOMEM; + + obj->nr_tlb_entries = pdata->nr_tlb_entries; + obj->name = pdata->name; + obj->dev = &pdev->dev; + obj->ctx = (void *)obj + sizeof(*obj); + obj->da_start = pdata->da_start; + obj->da_end = pdata->da_end; + + spin_lock_init(&obj->iommu_lock); + mutex_init(&obj->mmap_lock); + spin_lock_init(&obj->page_table_lock); + INIT_LIST_HEAD(&obj->mmap); + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + if (!res) { + err = -ENODEV; + goto err_mem; + } + + res = request_mem_region(res->start, resource_size(res), + dev_name(&pdev->dev)); + if (!res) { + err = -EIO; + goto err_mem; + } + + obj->regbase = ioremap(res->start, resource_size(res)); + if (!obj->regbase) { + err = -ENOMEM; + goto err_ioremap; + } + + irq = platform_get_irq(pdev, 0); + if (irq < 0) { + err = -ENODEV; + goto err_irq; + } + + err = request_irq(irq, iommu_fault_handler, IRQF_SHARED, + dev_name(&pdev->dev), obj); + if (err < 0) + goto err_irq; + platform_set_drvdata(pdev, obj); + + bus_set_iommu(&platform_bus_type, &omap_iommu_ops); + + pm_runtime_irq_safe(obj->dev); + pm_runtime_enable(obj->dev); + + dev_info(&pdev->dev, "%s registered\n", obj->name); + return 0; + +err_irq: + iounmap(obj->regbase); +err_ioremap: + release_mem_region(res->start, resource_size(res)); +err_mem: + kfree(obj); + return err; +} + +static int omap_iommu_remove(struct platform_device *pdev) +{ + int irq; + struct resource *res; + struct omap_iommu *obj = platform_get_drvdata(pdev); + + iopgtable_clear_entry_all(obj); + + irq = platform_get_irq(pdev, 0); + free_irq(irq, obj); + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + release_mem_region(res->start, resource_size(res)); + iounmap(obj->regbase); + + pm_runtime_disable(obj->dev); + + dev_info(&pdev->dev, "%s removed\n", obj->name); + kfree(obj); + return 0; +} + +static struct platform_driver omap_iommu_driver = { + .probe = omap_iommu_probe, + .remove = omap_iommu_remove, + .driver = { + .name = "omap-iommu", + }, +}; + static int __init omap_iommu_init(void) { struct kmem_cache *p; @@ -1277,8 +1280,6 @@ static int __init omap_iommu_init(void) return -ENOMEM; iopte_cachep = p; - bus_set_iommu(&platform_bus_type, &omap_iommu_ops); - return platform_driver_register(&omap_iommu_driver); } /* must be ready before omap3isp is probed */
Currently, bus_set_iommu() is done in omap_iommu_init(). However, omap_iommu_probe() can fail in a number of ways, leaving the platform bus with a dangling reference to a non-initialized iommu. Perform bus_set_iommu() only if omap_iommu_probe() succeed. Signed-off-by: Florian Vaussard <florian.vaussard@epfl.ch> --- drivers/iommu/omap-iommu.c | 207 +++++++++++++++++++++++---------------------- 1 file changed, 104 insertions(+), 103 deletions(-)