iommu: Prevent oops in iommu_get() and while arch_iommu is in use
Subsystems (4)
cover iommu: Prevent oops in iommu_get() and while arch_iommu is in use
Hi,
This patchset is aimed to fix a problem in arch_iommu implementation references. When an actual arch_iommu implementation is not loaded while iommu_get() is being called results to a kernel oops, as well as removing an arch_iommu implementation which is in use.
This patchset fixes both issues.
The patchset assumes the arch_iommu is uninstalled at module unload time. Is this an acceptable requirement?
Serialisation of the access to arch_iommu is done using mutex called arch_iommu_mutex.
module_put() doesn't need to have the arch_iommu_mutex since when this gets called there won't be any users on the arch_iommu anyway.
Comments are welcome. :-)
Cheers,
Signature · 2 lines
Sakari Ailus [email protected]
PATCH 1/4 omap iommu: Check existence of arch_iommu
Check that the arch_iommu has been installed before trying to use it. This will lead to kernel oops if the arch_iommu isn't there.
--- arch/arm/plat-omap/iommu.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)
arch/arm/plat-omap/iommu.c +3 -0
@@ -104,6 +104,9 @@ static int iommu_enable(struct iommu *obj)if (!obj)return -EINVAL ; +if (!arch_iommu) +return -ENOENT ; +clk_enable (obj->clk); err = arch_iommu->enable (obj);
Signature · 1 line
1.7.2.3
Re: [PATCH 1/4] omap iommu: Check existence of arch_iommu
Quoted 2 lines · Sakari Ailus
Check that the
arch_iommuhas been installed before trying to use it. This will lead to kernel oops if thearch_iommuisn't there.
This was intended to go to linux-omap. Sorry for the noise.
Signature · 2 lines
Sakari Ailus [email protected]
RE: [PATCH 1/4] omap iommu: Check existence of arch_iommu
In reply to Sakari AilusQuoted 22 lines
-----Original Message-----
From: [email protected] [mailto:linux-media- [email protected]] On Behalf Of Sakari Ailus
Sent: Friday, March 25, 2011 8:43 PM
To: [email protected]
Cc: [email protected]; [email protected]; [email protected]
Subject: [PATCH 1/4] omap iommu: Check existence ofarch_iommuCheck that the
arch_iommuhas been installed before trying to use it. This will lead to kernel oops if thearch_iommuisn't there.Signed-off-by Sakari Ailus <[email protected]>--- arch/arm/plat-omap/iommu.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)arch/arm/plat-omap/iommu.c +0
-0
arch/arm/plat-omap/iommu.c static int iommu_enable(struct iommu *obj) · line 109
Quoted 6 lines
arch/arm/plat-omap/iommu.c +3
-0@@ -104,6 +104,9 @@ static int iommu_enable(struct iommu *obj)if (!obj)return -EINVAL ; +if (!arch_iommu) +return -ENOENT ; +
[Hiremath, Vaibhav] Similar patch has already been submitted and accepted in community, not sure which baseline you are using. Please refer to below commit -
commit ef4815ab1ff10d642c21ef92faa6544934bc78d1
Author: Martin Hostettler <[email protected]> Date: Thu Feb 24 12:51:31 2011 -0800
omap: iommu: Gracefully fail iommu_enable if no arch_iommu is registered
In a modular build of the iommu code it's possible that the arch iommu code isn't loaded when trying to enable the iommu. Instead of blindly following a null pointer return -NODEV in that case.
arch/arm/plat-omap/iommu.c +3 -0
@@ -104,6 +104,9 @@ static int iommu_enable(struct iommu *obj)if (!obj)return -EINVAL ; +if (!arch_iommu) +return -ENODEV ; +clk_enable (obj->clk); err = arch_iommu->enable (obj);
Thanks,
Vaibhav
Quoted 10 lines
arch/arm/plat-omap/iommu.c +0
-1clk_enable (obj->clk); err = arch_iommu->enable (obj); --1.7.2.3
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] omap iommu: Check existence of arch_iommu
Quoted 25 lines · Vaibhav
Quoted 25 lines
-----Original Message-----
From: [email protected] [mailto:linux-media- [email protected]] On Behalf Of Sakari Ailus
Sent: Friday, March 25, 2011 8:43 PM
To: [email protected]
Cc: [email protected]; [email protected]; [email protected]
Subject: [PATCH 1/4] omap iommu: Check existence ofarch_iommuCheck that the
arch_iommuhas been installed before trying to use it. This will lead to kernel oops if thearch_iommuisn't there.Signed-off-by Sakari Ailus <[email protected]>--- arch/arm/plat-omap/iommu.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)arch/arm/plat-omap/iommu.c +0
-0@@ -104,6 +104,9 @@ static int iommu_enable(struct iommu *obj)if (!obj)return -EINVAL ;
Quoted 4 lines
arch/arm/plat-omap/iommu.c static int iommu_enable(struct iommu *obj) · line 109
Quoted 3 lines
arch/arm/plat-omap/iommu.c +3
-0+if (!arch_iommu) +return -ENOENT ; +[Hiremath, Vaibhav] Similar patch has already been submitted and
accepted in community, not sure which baseline you are using. Please refer to below commit -
Quoted 1 line
Thanks, Vaibhav. I guess I need to update my tree and resend the set...
Cheers,
Signature · 2 lines
Sakari Ailus [email protected]
PATCH 2/4 omap iommu: Add module information to struct iommu_functions
In reply to Sakari AilusWhichever module that implements the struct, may not be unloaded while it's in use. Prepare to this by adding module reference to the structure.
--- arch/arm/plat-omap/include/plat/iommu.h | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
arch/arm/plat-omap/include/plat/iommu.h +1 -0
@@ -79,6 +79,7 @@ struct iotlb_lock {/* architecture specific functions */ struct iommu_functions {unsigned long version; +struct module *module;int (*enable)(struct iommu *obj);void (*disable)(struct iommu *obj);
Signature · 1 line
1.7.2.3
PATCH 3/4 omap2 iommu: Set module information in omap2_iommu_ops
In reply to Sakari AilusSet omap2_iommu_ops.module to point to THIS_MODULE.
--- arch/arm/mach-omap2/iommu2.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)
arch/arm/mach-omap2/iommu2.c +2 -0
@@ -342,6 +342,8 @@ static const struct iommu_functions omap2_iommu_ops = { .save_ctx = omap2_iommu_save_ctx, .restore_ctx = omap2_iommu_restore_ctx, .dump_ctx = omap2_iommu_dump_ctx, + + .module = THIS_MODULE, };static int __init omap2_iommu_init (void )
Signature · 1 line
1.7.2.3
PATCH 4/4 omap iommu: Prevent iommu implementations from being unloaded while in use
In reply to Sakari AilusPrevent module implementing arch_iommu from being unloaded while the arch_iommu is in use, essentially while the iommu has been acquired using iommu_get() by increasing module use count.
This assumes that the arch_iommu has to be uninstalled at module unload time.
--- arch/arm/plat-omap/iommu.c | 31 +++++++++++++++++++++++++++++-- 1 files changed, 29 insertions(+), 2 deletions(-)
arch/arm/plat-omap/iommu.c +29 -2
@@ -35,6 +35,7 @@ static const struct iommu_functions *arch_iommu;static struct platform_driver omap_iommu_driver;static struct kmem_cache *iopte_cachep; +static struct mutex arch_iommu_mutex;/** * install_iommu_arch - Install archtecure specific iommu functions
@@ -45,10 +46,17 @@ static struct kmem_cache *iopte_cachep;**/ int install_iommu_arch (const struct iommu_functions *ops) { -if (arch_iommu) +mutex_lock (&arch_iommu_mutex); + +if (arch_iommu) { +mutex_unlock (&arch_iommu_mutex);return -EBUSY ; + } arch_iommu = ops; + +mutex_unlock (&arch_iommu_mutex); +return 0 ; }EXPORT_SYMBOL_GPL (install_iommu_arch);
@@ -58,13 +66,22 @@ EXPORT_SYMBOL_GPL(install_iommu_arch);* @ops: a pointer to architecture specific iommu functions * * This interface uninstalls the iommu algorighm installed previously. +* +* Note: This function may only be called at module_exit() function of +* a module which implements arch_iommu. Otherwise another mechanism +* from the module use count only to ensure the arch_iommu stays there +* has to be implemented. **/ void uninstall_iommu_arch (const struct iommu_functions *ops) { +mutex_lock (&arch_iommu_mutex); +if (arch_iommu != ops)pr_err ("%s: not your arch\n" , __func__); arch_iommu =NULL ; + +mutex_unlock (&arch_iommu_mutex); }EXPORT_SYMBOL_GPL (uninstall_iommu_arch);
@@ -104,14 +121,20 @@ static int iommu_enable(struct iommu *obj)if (!obj)return -EINVAL ; -if (!arch_iommu) +mutex_lock (&arch_iommu_mutex); +if (!arch_iommu || !try_module_get (arch_iommu->module)) { +mutex_unlock (&arch_iommu_mutex);return -ENOENT ; + }clk_enable (obj->clk); err = arch_iommu->enable (obj);clk_disable (obj->clk); + +mutex_unlock (&arch_iommu_mutex); +return err; }
@@ -125,6 +148,8 @@ static void iommu_disable(struct iommu *obj) arch_iommu->disable (obj);clk_disable (obj->clk); + +module_put (arch_iommu->module); }/*
@@ -1058,6 +1083,8 @@ static int __init omap_iommu_init(void)return -ENOMEM ; iopte_cachep = p; +mutex_init (&arch_iommu_mutex); +return platform_driver_register (&omap_iommu_driver); }module_init (omap_iommu_init);
Signature · 1 line
1.7.2.3
Quoted 6 lines · Sakari Ailus
Hi,
This patchset is aimed to fix a problem in
arch_iommuimplementation references. When an actualarch_iommuimplementation is not loaded whileiommu_get()is being called results to a kernel oops, as well as removing anarch_iommuimplementation which is in use.
How about fixing the dependency instead? Right now iommu2 depends on iommu because of the calls to install_iommu_arch/uninstall_iommu_arch... we should change that dependency to iommu depend on iommu2. Something like iommu (plat) querying iommu2 (mach) for devices to install.
This way depmod (if I'm not mistaken) can do its job, you won't be able to remove iommu2 in the middle of execution nor install iommu without its mach counterpart being there first, it should also fix clients depending on this modules, e.g "modprobe bridgedriver" would only install iommu and bridgedriver, with this new dependency iommu2 should be installed as well. BTW same happens with omap mailbox.
$ lsmod iovmm 7225 1 bridgedriver iommu 11084 2 bridgedriver,iovmm iommu2 4783 1 iommu
I can send as a patch if the mailer screws the spacing, also just copy-pasted and played with the pointers, if needed we can give better naming.
Regards,
Omar
---
arch/arm/mach-omap2/iommu2.c +34 -2
@@ -341,15 +341,47 @@ static const struct iommu_functions omap2_iommu_ops = { .dump_ctx = omap2_iommu_dump_ctx, }; +/** +* install_iommu_arch - Install archtecure specific iommu functions +* @ops: a pointer to architecture specific iommu functions +* +* There are several kind of iommu algorithm(tlb, pagetable) among +* omap series. This interface installs such an iommu algorighm. +**/ +int install_iommu_arch (const struct iommu_functions **ops) +{ +if (*ops) +return -EBUSY ; + *ops = &omap2_iommu_ops; + +return 0 ; +} +EXPORT_SYMBOL_GPL (install_iommu_arch); + +/** +* uninstall_iommu_arch - Uninstall archtecure specific iommu functions +* @ops: a pointer to architecture specific iommu functions +* +* This interface uninstalls the iommu algorighm installed previously. +**/ +void uninstall_iommu_arch (const struct iommu_functions **ops) +{ +if (*ops != &omap2_iommu_ops) +pr_err ("%s: not your arch\n" , __func__); + + *ops =NULL ; +} +EXPORT_SYMBOL_GPL (uninstall_iommu_arch); +static int __init omap2_iommu_init (void ) { -return install_iommu_arch (&omap2_iommu_ops); +return 0 ; }module_init (omap2_iommu_init); static void __exit omap2_iommu_exit(void) { -uninstall_iommu_arch (&omap2_iommu_ops); +/* Do nothing */ }module_exit (omap2_iommu_exit);
(quoted diff) +0 -0
b/arch/arm/plat-omap/include/plat/iommu.h
index 174f1b9..1c8e7ee 100644
arch/arm/plat-omap/include/plat/iommu.h +0 -3
@@ -177,9 +177,6 @@ extern int iommu_set_isr(const char *name,extern void iommu_save_ctx (struct iommu *obj);extern void iommu_restore_ctx (struct iommu *obj); -extern int install_iommu_arch (const struct iommu_functions *ops); -extern void uninstall_iommu_arch (const struct iommu_functions *ops); -extern int foreach_iommu_device (void *data,int (*fn)(struct device *,void *));
(quoted diff) +0 -0
b/arch/arm/plat-omap/include/plat/iommu2.h
index 10ad05f..8189f58 100644
arch/arm/plat-omap/include/plat/iommu2.h +3 -0
@@ -80,6 +80,9 @@#define MMU_RAM_MIXED_MASK (1 << MMU_RAM_MIXED_SHIFT)#define MMU_RAM_MIXED MMU_RAM_MIXED_MASK +extern int install_iommu_arch (const struct iommu_functions **ops); +extern void uninstall_iommu_arch (const struct iommu_functions **ops); +/* * register accessors */
arch/arm/plat-omap/iommu.c +7 -32
@@ -37,38 +37,6 @@ static struct platform_driver omap_iommu_driver;static struct kmem_cache *iopte_cachep;/** -* install_iommu_arch - Install archtecure specific iommu functions -* @ops: a pointer to architecture specific iommu functions -* -* There are several kind of iommu algorithm(tlb, pagetable) among -* omap series. This interface installs such an iommu algorighm. -**/ -int install_iommu_arch (const struct iommu_functions *ops) -{ -if (arch_iommu) -return -EBUSY ; - - arch_iommu = ops; -return 0 ; -} -EXPORT_SYMBOL_GPL (install_iommu_arch); - -/** -* uninstall_iommu_arch - Uninstall archtecure specific iommu functions -* @ops: a pointer to architecture specific iommu functions -* -* This interface uninstalls the iommu algorighm installed previously. -**/ -void uninstall_iommu_arch (const struct iommu_functions *ops) -{ -if (arch_iommu != ops) -pr_err ("%s: not your arch\n" , __func__); - - arch_iommu =NULL ; -} -EXPORT_SYMBOL_GPL (uninstall_iommu_arch); - -/** * iommu_save_ctx - Save registers for pm off-mode support * @obj: target iommu **/
@@ -1072,10 +1040,15 @@ static void iopte_cachep_ctor(void *iopte)static int __init omap_iommu_init (void ) { +int err;struct kmem_cache *p;const unsigned long flags =SLAB_HWCACHE_ALIGN ;size_t align =1 <<10 ;/* L2 pagetable alignement */ + err =install_iommu_arch (&arch_iommu); +if (err) +return err; + p =kmem_cache_create ("iopte_cache" ,IOPTE_TABLE_SIZE , align, flags, iopte_cachep_ctor);if (!p)
@@ -1090,6 +1063,8 @@ static void __exit omap_iommu_exit(void) {kmem_cache_destroy (iopte_cachep); +uninstall_iommu_arch (&arch_iommu); +platform_driver_unregister (&omap_iommu_driver); }module_exit (omap_iommu_exit);
Quoted 9 lines · Omar
Quoted 6 lines · Sakari Ailus
Hi,
This patchset is aimed to fix a problem in
arch_iommuimplementation references. When an actualarch_iommuimplementation is not loaded whileiommu_get()is being called results to a kernel oops, as well as removing anarch_iommuimplementation which is in use.
Quoted 5 lines
How about fixing the dependency instead? Right now iommu2 depends on iommu because of the calls to install_iommu_arch/uninstall_iommu_arch... we should change that dependency to iommu depend on iommu2. Something like iommu (plat) querying iommu2 (mach) for devices to install.
There is no direct dependency from a driver using the generic API to a particular implementation of the iommu. This comes from the design of the iommu framework. The generic layer shouldn't depend on particular implementation(s).
What comes to the patch, it works as long as there's only one iommu implementation loaded / compiled to the kernel. I wonder if this kind of limitation can be accepted.
Regards,
Signature · 2 lines
Sakari Ailus [email protected]
Hi,
Quoted 14 lines · Sakari Ailus
Quoted 7 lines · Omar
Quoted 4 lines · Sakari Ailus
This patchset is aimed to fix a problem in
arch_iommuimplementation references. When an actualarch_iommuimplementation is not loaded whileiommu_get()is being called results to a kernel oops, as well as removing anarch_iommuimplementation which is in use.Quoted 5 lines
How about fixing the dependency instead? Right now iommu2 depends on iommu because of the calls to install_iommu_arch/uninstall_iommu_arch... we should change that dependency to iommu depend on iommu2. Something like iommu (plat) querying iommu2 (mach) for devices to install.
Quoted 4 lines
There is no direct dependency from a driver using the generic API to a particular implementation of the iommu. This comes from the design of the iommu framework. The generic layer shouldn't depend on particular implementation(s).
IMHO there is, take as an example bridgedriver (it is arm/omap dependent), so it depends on iommu providing the mach-omap2 implementation. I imagine isp for omap imposes the same dependency, even more your patchset enforces that dependency.
Basically, if there is no "arch_iommu" the iommu driver does not work, and if there was an "arch_iommu" but it was removed then the driver crashes.
Now, there could be architectures that does not depend on a particular implementation but this iommu driver doesn't support them because if there is no arch_iommu operations, it does nothing or crashes.
Quoted 3 lines
What comes to the patch, it works as long as there's only one iommu implementation loaded / compiled to the kernel. I wonder if this kind of limitation can be accepted.
Which is the way iommu choose to work, like I said if there is no arch_iommu nothing works, most of APIs in iommu depend on an machine specific implementation. To fix that it is not the scope of my proposal.
If indeed iommu can function without a machine specific implementation then a redesign needs to be made, but to me the same approach as I did needs to be followed: if there is mach implementation (e.g.: iommu2.c) the generic API needs to depend on it, otherwise the module can be removed and crash the kernel; OTOH if there is no mach implementation, then iommu should not depend on it to be installed as you point out, this could be handled in plat/iommu.h among with:
#if defined(CONFIG_ARCH_OMAP1)
#error "iommu for this processor not implemented yet" #else
#include <plat/iommu2.h>
#endif
A new else defining the install/uninstall_arch_iommu functions or simply reversing the check to be OMAP2+ and error on anything else.
Regards,
Omar
Quoted 23 lines · Omar
Hi,
Quoted 14 lines · Sakari Ailus
Quoted 7 lines · Omar
Quoted 4 lines · Sakari Ailus
This patchset is aimed to fix a problem in
arch_iommuimplementation references. When an actualarch_iommuimplementation is not loaded whileiommu_get()is being called results to a kernel oops, as well as removing anarch_iommuimplementation which is in use.Quoted 5 lines
How about fixing the dependency instead? Right now iommu2 depends on iommu because of the calls to install_iommu_arch/uninstall_iommu_arch... we should change that dependency to iommu depend on iommu2. Something like iommu (plat) querying iommu2 (mach) for devices to install.
Quoted 4 lines
There is no direct dependency from a driver using the generic API to a particular implementation of the iommu. This comes from the design of the iommu framework. The generic layer shouldn't depend on particular implementation(s).
Quoted 4 lines
IMHO there is, take as an example bridgedriver (it is arm/omap dependent), so it depends on iommu providing the mach-omap2 implementation. I imagine isp for omap imposes the same dependency, even more your patchset enforces that dependency.
The generic layer can exist without a specific implementation. It should accept later arch install/uninstall without any problem.
Quoted 4 lines
Basically, if there is no "
arch_iommu" the iommu driver does not work, and if there was an "arch_iommu" but it was removed then the driver crashes.
The loaded module *must* uninstall arch when existing, then kernel will see no crash. And yes, generic layer won't work anymore but that's an expected and correct situation.
Quoted 22 lines
Now, there could be architectures that does not depend on a particular implementation but this iommu driver doesn't support them because if there is no
arch_iommuoperations, it does nothing or crashes.Quoted 3 lines
What comes to the patch, it works as long as there's only one iommu implementation loaded / compiled to the kernel. I wonder if this kind of limitation can be accepted.
Which is the way iommu choose to work, like I said if there is no
arch_iommunothing works, most of APIs in iommu depend on an machine specific implementation. To fix that it is not the scope of my proposal.If indeed iommu can function without a machine specific implementation then a redesign needs to be made, but to me the same approach as I did needs to be followed: if there is mach implementation (e.g.:
iommu2.c) the generic API needs to depend on it, otherwise the module can be removed and crash the kernel; OTOH if there is no mach implementation, then iommu should not depend on it to be installed as you point out, this could be handled inplat/iommu.hamong with:
Quoted 5 lines
#if
defined(CONFIG_ARCH_OMAP1)
#error "iommu for this processor not implemented yet" #else
#include <plat/iommu2.h>
#endif
So, every new specific implementation should modify this piece of code? Are you sure it's a good idea?
Regards,
David Cohen
Quoted 4 lines
A new else defining the install/uninstall_arch_iommu functions or simply reversing the check to be OMAP2+ and error on anything else.
Quoted 8 lines
Regards,
Omar
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi,
Quoted 19 lines · Omar
Quoted 13 lines · Sakari Ailus
Quoted 11 lines · Omar
Quoted 4 lines · Sakari Ailus
This patchset is aimed to fix a problem in
arch_iommuimplementation references. When an actualarch_iommuimplementation is not loaded whileiommu_get()is being called results to a kernel oops, as well as removing anarch_iommuimplementation which is in use.How about fixing the dependency instead? Right now iommu2 depends on iommu because of the calls to install_iommu_arch/uninstall_iommu_arch... we should change that dependency to iommu depend on iommu2. Something like iommu (plat) querying iommu2 (mach) for devices to install.
Quoted 4 lines
There is no direct dependency from a driver using the generic API to a particular implementation of the iommu. This comes from the design of the iommu framework. The generic layer shouldn't depend on particular implementation(s).
Quoted 4 lines
IMHO there is, take as an example bridgedriver (it is arm/omap dependent), so it depends on iommu providing the mach-omap2 implementation. I imagine isp for omap imposes the same dependency, even more your patchset enforces that dependency.
The problem is that drivers don't depend on particular iommu implementations (they only use symbols from the generic iommu layer), but they require the implementation to be present. Maybe the implementation should be registered at runtime by board code, or should be compiled in (bool instead of tristate) and check if the current mach supports the implementation (with cpu_is_* or similar) before registering it.
Quoted 33 lines
Basically, if there is no "
arch_iommu" the iommu driver does not work, and if there was an "arch_iommu" but it was removed then the driver crashes.Now, there could be architectures that does not depend on a particular implementation but this iommu driver doesn't support them because if there is no
arch_iommuoperations, it does nothing or crashes.Quoted 3 lines
What comes to the patch, it works as long as there's only one iommu implementation loaded / compiled to the kernel. I wonder if this kind of limitation can be accepted.
Which is the way iommu choose to work, like I said if there is no
arch_iommunothing works, most of APIs in iommu depend on an machine specific implementation. To fix that it is not the scope of my proposal.If indeed iommu can function without a machine specific implementation then a redesign needs to be made, but to me the same approach as I did needs to be followed: if there is mach implementation (e.g.:
iommu2.c) the generic API needs to depend on it, otherwise the module can be removed and crash the kernel; OTOH if there is no mach implementation, then iommu should not depend on it to be installed as you point out, this could be handled inplat/iommu.hamong with:#if
defined(CONFIG_ARCH_OMAP1)
#error "iommu for this processor not implemented yet" #else
#include <plat/iommu2.h>
#endifA new else defining the install/uninstall_arch_iommu functions or simply reversing the check to be OMAP2+ and error on anything else.
Signature · 3 lines
Regards, Laurent Pinchart
Hi Sakari,
Quoted 16 lines · Sakari Ailus
Quoted 9 lines · Omar
Quoted 6 lines · Sakari Ailus
Hi,
This patchset is aimed to fix a problem in
arch_iommuimplementation references. When an actualarch_iommuimplementation is not loaded whileiommu_get()is being called results to a kernel oops, as well as removing anarch_iommuimplementation which is in use.Quoted 5 lines
How about fixing the dependency instead? Right now iommu2 depends on iommu because of the calls to install_iommu_arch/uninstall_iommu_arch... we should change that dependency to iommu depend on iommu2. Something like iommu (plat) querying iommu2 (mach) for devices to install.
Quoted 8 lines
There is no direct dependency from a driver using the generic API to a particular implementation of the iommu. This comes from the design of the iommu framework. The generic layer shouldn't depend on particular implementation(s).
What comes to the patch, it works as long as there's only one iommu implementation loaded / compiled to the kernel. I wonder if this kind of limitation can be accepted.
The generic iommu driver cannot support more than one implementation loaded at the same time, so your patch is correct by assuming it.
Regards,
David Cohen
Quoted 11 lines
Regards,
--
Sakari Ailus
[email protected]
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 10 lines · Omar
On Fri, Mar 25, 2011 at 10:13 AM, Sakari Ailus
Quoted 6 lines · <[email protected]> wrote:
Hi,
This patchset is aimed to fix a problem in
arch_iommuimplementation references. When an actualarch_iommuimplementation is not loaded whileiommu_get()is being called results to a kernel oops, as well as removing anarch_iommuimplementation which is in use.
Quoted 5 lines
How about fixing the dependency instead? Right now iommu2 depends on iommu because of the calls to install_iommu_arch/uninstall_iommu_arch... we should change that dependency to iommu depend on iommu2. Something like iommu (plat) querying iommu2 (mach) for devices to install.
The reason why iommu depends on iommu2 and not the other way around is because several mach-specific iommu implementations should be able to coexist in the same kernel. The right one should be loaded at runtime.
I think that Sakari's patches correcty fix the problems he noticed. However, they won't fix one basic issue, which is that the iommu2 module won't be automatically pulled in when the omap3isp module is loaded. The omap3isp driver will then fail to probe the device. That's better than crashing though.
One possible solution for that is to turn the tristate option for iommu2 into a bool option. I've also read a couple of times that the kernel provides a standard iommu API. Maybe switching to it would help.
Quoted 15 lines
This way depmod (if I'm not mistaken) can do its job, you won't be able to remove iommu2 in the middle of execution nor install iommu without its mach counterpart being there first, it should also fix clients depending on this modules, e.g "modprobe bridgedriver" would only install iommu and bridgedriver, with this new dependency iommu2 should be installed as well. BTW same happens with omap mailbox.
$ lsmod iovmm 7225 1 bridgedriver iommu 11084 2 bridgedriver,iovmm iommu2 4783 1 iommuI can send as a patch if the mailer screws the spacing, also just copy-pasted and played with the pointers, if needed we can give better naming.
Signature · 3 lines
Regards, Laurent Pinchart
Hi Laurent and Omar,
Quoted 17 lines · Laurent Pinchart
Quoted 10 lines · Omar
On Fri, Mar 25, 2011 at 10:13 AM, Sakari Ailus
Quoted 6 lines · <[email protected]> wrote:
Hi,
This patchset is aimed to fix a problem in
arch_iommuimplementation references. When an actualarch_iommuimplementation is not loaded whileiommu_get()is being called results to a kernel oops, as well as removing anarch_iommuimplementation which is in use.Quoted 5 lines
How about fixing the dependency instead? Right now iommu2 depends on iommu because of the calls to install_iommu_arch/uninstall_iommu_arch... we should change that dependency to iommu depend on iommu2. Something like iommu (plat) querying iommu2 (mach) for devices to install.
Quoted 8 lines
The reason why iommu depends on iommu2 and not the other way around is because several mach-specific iommu implementations should be able to coexist in the same kernel. The right one should be loaded at runtime.
I think that Sakari's patches correcty fix the problems he noticed. However, they won't fix one basic issue, which is that the iommu2 module won't be automatically pulled in when the omap3isp module is loaded. The omap3isp driver will then fail to probe the device. That's better than crashing though.
One option would be to specify the name of the module in the platform data and request_module() that in omap_iommu_probe(). This would solve the issue, not sure how pretty is this though.
In a generic case there would have to be a list of modules implementing iommu in the platform data.
Quoted 3 lines
One possible solution for that is to turn the tristate option for iommu2 into a bool option. I've also read a couple of times that the kernel provides a standard iommu API. Maybe switching to it would help.
That would solve it as well, but having it as a module would be nice, too.
Regards,
Signature · 2 lines
Sakari Ailus [email protected]
Hi Sakari,
Quoted 26 lines · On Wednesday 30 March 2011 10:16:56 Sakari Ailus wrote:
Quoted 19 lines · Laurent Pinchart
Quoted 8 lines · Omar
Quoted 6 lines · Sakari Ailus
Hi,
This patchset is aimed to fix a problem in
arch_iommuimplementation references. When an actualarch_iommuimplementation is not loaded whileiommu_get()is being called results to a kernel oops, as well as removing anarch_iommuimplementation which is in use.Quoted 5 lines
How about fixing the dependency instead? Right now iommu2 depends on iommu because of the calls to install_iommu_arch/uninstall_iommu_arch... we should change that dependency to iommu depend on iommu2. Something like iommu (plat) querying iommu2 (mach) for devices to install.
The reason why iommu depends on iommu2 and not the other way around is because several mach-specific iommu implementations should be able to coexist in the same kernel. The right one should be loaded at runtime.
Quoted 5 lines
I think that Sakari's patches correcty fix the problems he noticed. However, they won't fix one basic issue, which is that the iommu2 module won't be automatically pulled in when the omap3isp module is loaded. The omap3isp driver will then fail to probe the device. That's better than crashing though.
Quoted 3 lines
One option would be to specify the name of the module in the platform data and
request_module()that inomap_iommu_probe(). This would solve the issue, not sure how pretty is this though.
Do we need that ? My understanding is that a machine will need a single mach- specific iommu implementation only. Drivers shouldn't need to care about that. The iommu implementation should be automatically selected based on the machine time.
Quoted 8 lines
In a generic case there would have to be a list of modules implementing iommu in the platform data.
Quoted 3 lines
One possible solution for that is to turn the tristate option for iommu2 into a bool option. I've also read a couple of times that the kernel provides a standard iommu API. Maybe switching to it would help.
That would solve it as well, but having it as a module would be nice, too.
Signature · 3 lines
Regards, Laurent Pinchart
Quoted 1 line · Laurent Pinchart
Hi Sakari,
Hi Laurent,
Quoted 27 lines · On Wednesday 30 March 2011 10:16:56 Sakari Ailus wrote:
Quoted 26 lines
Quoted 24 lines · Laurent Pinchart
Quoted 8 lines · Omar
Quoted 6 lines · Sakari Ailus
Hi,
This patchset is aimed to fix a problem in
arch_iommuimplementation references. When an actualarch_iommuimplementation is not loaded whileiommu_get()is being called results to a kernel oops, as well as removing anarch_iommuimplementation which is in use.Quoted 5 lines
How about fixing the dependency instead? Right now iommu2 depends on iommu because of the calls to install_iommu_arch/uninstall_iommu_arch... we should change that dependency to iommu depend on iommu2. Something like iommu (plat) querying iommu2 (mach) for devices to install.
The reason why iommu depends on iommu2 and not the other way around is because several mach-specific iommu implementations should be able to coexist in the same kernel. The right one should be loaded at runtime.
I think that Sakari's patches correcty fix the problems he noticed. However, they won't fix one basic issue, which is that the iommu2 module won't be automatically pulled in when the omap3isp module is loaded. The omap3isp driver will then fail to probe the device. That's better than crashing though.
Quoted 6 lines
Quoted 3 lines
One option would be to specify the name of the module in the platform data and
request_module()that inomap_iommu_probe(). This would solve the issue, not sure how pretty is this though.Do we need that ? My understanding is that a machine will need a single mach- specific iommu implementation only. Drivers shouldn't need to care about that.
Well, no more than that there would have to be a driver for the IOMMU for that very hardware.
Quoted 2 lines
The iommu implementation should be automatically selected based on the machine time.
Machine type?
I agree, but where is the selection made?
Signature · 2 lines
Sakari Ailus [email protected]
Quoted 43 lines · On Wednesday 30 March 2011 15:50:37 Sakari Ailus wrote:
Quoted 1 line · Laurent Pinchart
Hi Sakari,
Hi Laurent,
Quoted 27 lines · On Wednesday 30 March 2011 10:16:56 Sakari Ailus wrote:
Quoted 26 lines
Quoted 24 lines · Laurent Pinchart
Quoted 8 lines · Omar
Quoted 6 lines · Sakari Ailus
Hi,
This patchset is aimed to fix a problem in
arch_iommuimplementation references. When an actualarch_iommuimplementation is not loaded whileiommu_get()is being called results to a kernel oops, as well as removing anarch_iommuimplementation which is in use.Quoted 5 lines
How about fixing the dependency instead? Right now iommu2 depends on iommu because of the calls to install_iommu_arch/uninstall_iommu_arch... we should change that dependency to iommu depend on iommu2. Something like iommu (plat) querying iommu2 (mach) for devices to install.
The reason why iommu depends on iommu2 and not the other way around is because several mach-specific iommu implementations should be able to coexist in the same kernel. The right one should be loaded at runtime.
I think that Sakari's patches correcty fix the problems he noticed. However, they won't fix one basic issue, which is that the iommu2 module won't be automatically pulled in when the omap3isp module is loaded. The omap3isp driver will then fail to probe the device. That's better than crashing though.
Quoted 7 lines
Quoted 3 lines
One option would be to specify the name of the module in the platform data and
request_module()that inomap_iommu_probe(). This would solve the issue, not sure how pretty is this though.Do we need that ? My understanding is that a machine will need a single mach- specific iommu implementation only. Drivers shouldn't need to care about that.
Well, no more than that there would have to be a driver for the IOMMU for that very hardware.
Quoted 6 lines
Quoted 2 lines
The iommu implementation should be automatically selected based on the machine time.
Machine type?
I agree, but where is the selection made?
The selection can be made by board code, or by the iommu implementations themselves if they're compiled in.
Signature · 3 lines
Regards, Laurent Pinchart
Quoted 47 lines · Laurent Pinchart
Quoted 46 lines · On Wednesday 30 March 2011 15:50:37 Sakari Ailus wrote:
Quoted 1 line · Laurent Pinchart
Hi Sakari,
Hi Laurent,
Quoted 27 lines · On Wednesday 30 March 2011 10:16:56 Sakari Ailus wrote:
Quoted 26 lines
Quoted 24 lines · Laurent Pinchart
Quoted 8 lines · Omar
Quoted 6 lines · Sakari Ailus
Hi,
This patchset is aimed to fix a problem in
arch_iommuimplementation references. When an actualarch_iommuimplementation is not loaded whileiommu_get()is being called results to a kernel oops, as well as removing anarch_iommuimplementation which is in use.Quoted 5 lines
How about fixing the dependency instead? Right now iommu2 depends on iommu because of the calls to install_iommu_arch/uninstall_iommu_arch... we should change that dependency to iommu depend on iommu2. Something like iommu (plat) querying iommu2 (mach) for devices to install.
The reason why iommu depends on iommu2 and not the other way around is because several mach-specific iommu implementations should be able to coexist in the same kernel. The right one should be loaded at runtime.
I think that Sakari's patches correcty fix the problems he noticed. However, they won't fix one basic issue, which is that the iommu2 module won't be automatically pulled in when the omap3isp module is loaded. The omap3isp driver will then fail to probe the device. That's better than crashing though.
Quoted 7 lines
Quoted 3 lines
One option would be to specify the name of the module in the platform data and
request_module()that inomap_iommu_probe(). This would solve the issue, not sure how pretty is this though.Do we need that ? My understanding is that a machine will need a single mach- specific iommu implementation only. Drivers shouldn't need to care about that.
Well, no more than that there would have to be a driver for the IOMMU for that very hardware.
Quoted 2 lines
The iommu implementation should be automatically selected based on the machine time.
Quoted 6 lines
Quoted 3 lines
Machine type?
I agree, but where is the selection made?
The selection can be made by board code, or by the iommu implementations themselves if they're compiled in.
I prefer the first option. The second one will make the current implementation be even more OMAP-only.
We have basically 3 layers:
IOVMM, IOMMU_GENERIC and IOMMU_SPECIFIC. The middle one should be generic and don't care about machine types. The later one can be handled by board code as it's machine specific and, for most of the cases, I see no reason to let any other implementation besides the machine type's to be loaded.
But the generic layer should not depend on any specific one. If somebody decides to load the specific layer after the generic one, it cannot be a problem.
Regards,
David
Quoted 10 lines
--
Regards,Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi David,
Quoted 62 lines · On Wednesday 30 March 2011 17:50:17 David Cohen wrote:
Quoted 45 lines · Laurent Pinchart
Quoted 44 lines · On Wednesday 30 March 2011 15:50:37 Sakari Ailus wrote:
Quoted 29 lines · Laurent Pinchart
Quoted 28 lines · On Wednesday 30 March 2011 10:16:56 Sakari Ailus wrote:
Quoted 26 lines · Laurent Pinchart
Quoted 9 lines · Omar
Quoted 7 lines · Sakari Ailus
Hi,
This patchset is aimed to fix a problem in
arch_iommuimplementation references. When an actualarch_iommuimplementation is not loaded whileiommu_get()is being called results to a kernel oops, as well as removing anarch_iommuimplementation which is in use.Quoted 5 lines
How about fixing the dependency instead? Right now iommu2 depends on iommu because of the calls to install_iommu_arch/uninstall_iommu_arch... we should change that dependency to iommu depend on iommu2. Something like iommu (plat) querying iommu2 (mach) for devices to install.
The reason why iommu depends on iommu2 and not the other way around is because several mach-specific iommu implementations should be able to coexist in the same kernel. The right one should be loaded at runtime.
I think that Sakari's patches correcty fix the problems he noticed. However, they won't fix one basic issue, which is that the iommu2 module won't be automatically pulled in when the omap3isp module is loaded. The omap3isp driver will then fail to probe the device. That's better than crashing though.
Quoted 7 lines
Quoted 3 lines
One option would be to specify the name of the module in the platform data and
request_module()that inomap_iommu_probe(). This would solve the issue, not sure how pretty is this though.Do we need that ? My understanding is that a machine will need a single mach- specific iommu implementation only. Drivers shouldn't need to care about that.
Well, no more than that there would have to be a driver for the IOMMU for that very hardware.
Quoted 2 lines
The iommu implementation should be automatically selected based on the machine time.
Quoted 6 lines
Quoted 3 lines
Machine type?
I agree, but where is the selection made?
The selection can be made by board code, or by the iommu implementations themselves if they're compiled in.
I prefer the first option. The second one will make the current implementation be even more OMAP-only.
We have basically 3 layers:
IOVMM,IOMMU_GENERICandIOMMU_SPECIFIC. The middle one should be generic and don't care about machine types. The later one can be handled by board code as it's machine specific and, for most of the cases, I see no reason to let any other implementation besides the machine type's to be loaded.
Quoted 3 lines
But the generic layer should not depend on any specific one. If somebody decides to load the specific layer after the generic one, it cannot be a problem.
Let me try to summarize the issue and the requirements.
IOMMU support on OMAP platforms uses an OMAP-specific implementation, divided into 3 layers:
- the IOVMM layer (arch/arm/plat-omap/iovmm.ko) deals with virtual address space management
- the IOMMU layer (arch/arm/plat-omap/iommu.ko) controls the IOMMU hardware, and deals with TLB and page tables
- the IOMMU platform-specific layers (arch/arm/mach-omap2/iommu2.ko) handles the IOMMU implementation variants between various OMAP platforms
Drivers depend on iovmm and iommu. They must not depend on iommu2.ko.
The only existing platform-specific IOMMU layer is iommu2.ko for OMAP2+. An OMAP1 implementation is being worked on, and other implementations might be needed for OMAP4 and/or OMAP5.
Building a kernel image that will run on all OMAP platforms isn't possible at the moment but is being worked on. Such a kernel image will need to include all the platform-specific IOMMU layers, and the correct layer will need to be selected at runtime.
If a driver tries to request and use an IOMMU before the platform-specific IOMMU layer is initialized, the request will fail. We thus need to ensure that the correct platform-specific IOMMU layer is initialized before IOMMU users are initialized.
I can see several ways to fix the problem.
- Turn the iommu and iommu2 options from tristate to bool. The downside is that the kernel image will get slightly bigger.
- Merge the iommu and iommu2 modules together. This will temporarily fix the problem, but a proper solution will still be needed for the OMAP1 (and maybe OMAP5) IOMMU layers.
- Auto-load the correct platform-specific IOMMU module based on modaliases created from the platform name. The platform-specific modules will need to check at runtime whether they support the current platform to avoid conflicts when several of those modules will be compiled in.
The second solution is the simplest, but it's a workaround. On the other hand, it's hard to design a proper solution before we know the requirements of the other OMAP platforms that have an IOMMU incompatible with iommu2.ko, so it might be better to postpone the decision until we have a real use case.
Signature · 3 lines
Regards, Laurent Pinchart
Quoted 1 line · Laurent Pinchart
Hi David,
Hi Laurent, David, others,
Quoted 90 lines · On Wednesday 30 March 2011 17:50:17 David Cohen wrote:
Quoted 62 lines · Laurent Pinchart
Quoted 45 lines · On Wednesday 30 March 2011 15:50:37 Sakari Ailus wrote:
Quoted 44 lines
Quoted 29 lines · Laurent Pinchart
Quoted 28 lines · On Wednesday 30 March 2011 10:16:56 Sakari Ailus wrote:
Quoted 26 lines · Laurent Pinchart
Quoted 9 lines · Omar
Quoted 7 lines · Sakari Ailus
Hi,
This patchset is aimed to fix a problem in
arch_iommuimplementation references. When an actualarch_iommuimplementation is not loaded whileiommu_get()is being called results to a kernel oops, as well as removing anarch_iommuimplementation which is in use.Quoted 5 lines
How about fixing the dependency instead? Right now iommu2 depends on iommu because of the calls to install_iommu_arch/uninstall_iommu_arch... we should change that dependency to iommu depend on iommu2. Something like iommu (plat) querying iommu2 (mach) for devices to install.
The reason why iommu depends on iommu2 and not the other way around is because several mach-specific iommu implementations should be able to coexist in the same kernel. The right one should be loaded at runtime.
I think that Sakari's patches correcty fix the problems he noticed. However, they won't fix one basic issue, which is that the iommu2 module won't be automatically pulled in when the omap3isp module is loaded. The omap3isp driver will then fail to probe the device. That's better than crashing though.
Quoted 7 lines
Quoted 3 lines
One option would be to specify the name of the module in the platform data and
request_module()that inomap_iommu_probe(). This would solve the issue, not sure how pretty is this though.Do we need that ? My understanding is that a machine will need a single mach- specific iommu implementation only. Drivers shouldn't need to care about that.
Well, no more than that there would have to be a driver for the IOMMU for that very hardware.
Quoted 2 lines
The iommu implementation should be automatically selected based on the machine time.
Quoted 6 lines
Quoted 3 lines
Machine type?
I agree, but where is the selection made?
The selection can be made by board code, or by the iommu implementations themselves if they're compiled in.
I prefer the first option. The second one will make the current implementation be even more OMAP-only.
We have basically 3 layers:
IOVMM,IOMMU_GENERICandIOMMU_SPECIFIC. The middle one should be generic and don't care about machine types. The later one can be handled by board code as it's machine specific and, for most of the cases, I see no reason to let any other implementation besides the machine type's to be loaded.Quoted 3 lines
But the generic layer should not depend on any specific one. If somebody decides to load the specific layer after the generic one, it cannot be a problem.
Let me try to summarize the issue and the requirements.
IOMMU support on OMAP platforms uses an OMAP-specific implementation, divided into 3 layers:
- the IOVMM layer (
arch/arm/plat-omap/iovmm.ko) deals with virtual address space management
- the IOMMU layer (arch/arm/plat-omap/iommu.ko) controls the IOMMU hardware, and deals with TLB and page tables
- the IOMMU platform-specific layers (arch/arm/mach-omap2/iommu2.ko) handles the IOMMU implementation variants between various OMAP platformsDrivers depend on iovmm and iommu. They must not depend on iommu2.ko.
The only existing platform-specific IOMMU layer is iommu2.ko for OMAP2+. An OMAP1 implementation is being worked on, and other implementations might be needed for OMAP4 and/or OMAP5.
Building a kernel image that will run on all OMAP platforms isn't possible at the moment but is being worked on. Such a kernel image will need to include all the platform-specific IOMMU layers, and the correct layer will need to be selected at runtime.
Quoted 4 lines
If a driver tries to request and use an IOMMU before the platform-specific IOMMU layer is initialized, the request will fail. We thus need to ensure that the correct platform-specific IOMMU layer is initialized before IOMMU users are initialized.
Thanks for the summary!
Quoted 5 lines
I can see several ways to fix the problem.
- Turn the iommu and iommu2 options from tristate to bool. The downside is that the kernel image will get slightly bigger.
Quoted 8 lines
- Merge the iommu and iommu2 modules together. This will temporarily fix the problem, but a proper solution will still be needed for the OMAP1 (and maybe OMAP5) IOMMU layers.
- Auto-load the correct platform-specific IOMMU module based on modaliases created from the platform name. The platform-specific modules will need to check at runtime whether they support the current platform to avoid conflicts when several of those modules will be compiled in.
I'd like to add option to auto-load the module based on the type of the IOMMU. This is more generic since there could be several types of IOMMUs in the same system, although in the scope of OMAPs we are likely to have always just one.
Extending the scope of the OMAP IOMMU would be nice, or to add functionality to the current generic layer which doesn't do much at the moment.
This is probably a bigger task and something to consider in the future, though.
I'd go with the third option you suggested since this one
1) solves the problem,
2) doesn't appear to create new ones,
3) doesn't add anything that would be incompatible with probable future developments and
4) is easy to implement.
Btw. There should be no devices created by the board code on those platforms either. Wrong iommu device drivers may be loaded in addition, but this does no more harm than compiling those in to the kernel in the first option.
Quoted 4 lines
The second solution is the simplest, but it's a workaround. On the other hand, it's hard to design a proper solution before we know the requirements of the other OMAP platforms that have an IOMMU incompatible with iommu2.ko, so it might be better to postpone the decision until we have a real use case.
There are two options that I can think of: either a SoC-wide IOMMU implementation or
The problem of loading that module exists right now so it should have some kind of solution. If we go with the second option right now it does push this to direction I don't like too much. The next implementer has to solve the problem instead, and it might be easier to implement this right now, as we are all up-to-date with the issue.
Just my 0,05 euros. :-)
Cheers,
Signature · 2 lines
Sakari Ailus [email protected]
Hi Sakari,
Quoted 1 line · On Tuesday 05 April 2011 11:03:21 Sakari Ailus wrote:
Laurent Pinchart wrote:
[snip]
Quoted 39 lines
Quoted 23 lines
Let me try to summarize the issue and the requirements.
IOMMU support on OMAP platforms uses an OMAP-specific implementation, divided into 3 layers:
- the IOVMM layer (
arch/arm/plat-omap/iovmm.ko) deals with virtual address space management
- the IOMMU layer (arch/arm/plat-omap/iommu.ko) controls the IOMMU hardware, and deals with TLB and page tables
- the IOMMU platform-specific layers (arch/arm/mach-omap2/iommu2.ko) handles the IOMMU implementation variants between various OMAP platformsDrivers depend on iovmm and iommu. They must not depend on iommu2.ko.
The only existing platform-specific IOMMU layer is iommu2.ko for OMAP2+. An OMAP1 implementation is being worked on, and other implementations might be needed for OMAP4 and/or OMAP5.
Building a kernel image that will run on all OMAP platforms isn't possible at the moment but is being worked on. Such a kernel image will need to include all the platform-specific IOMMU layers, and the correct layer will need to be selected at runtime.
Quoted 4 lines
If a driver tries to request and use an IOMMU before the platform-specific IOMMU layer is initialized, the request will fail. We thus need to ensure that the correct platform-specific IOMMU layer is initialized before IOMMU users are initialized.
Thanks for the summary!
Quoted 9 lines
I can see several ways to fix the problem.
- Turn the iommu and iommu2 options from tristate to bool. The downside is that the kernel image will get slightly bigger.
- Merge the iommu and iommu2 modules together. This will temporarily fix the problem, but a proper solution will still be needed for the OMAP1 (and maybe OMAP5) IOMMU layers.
Quoted 7 lines
Quoted 4 lines
- Auto-load the correct platform-specific IOMMU module based on modaliases created from the platform name. The platform-specific modules will need to check at runtime whether they support the current platform to avoid conflicts when several of those modules will be compiled in.
I'd like to add option to auto-load the module based on the type of the IOMMU.
Could you elaborate on that ?
Quoted 23 lines
This is more generic since there could be several types of IOMMUs in the same system, although in the scope of OMAPs we are likely to have always just one.
Extending the scope of the OMAP IOMMU would be nice, or to add functionality to the current generic layer which doesn't do much at the moment.
This is probably a bigger task and something to consider in the future, though.
I'd go with the third option you suggested since this one
1) solves the problem,
2) doesn't appear to create new ones,
3) doesn't add anything that would be incompatible with probable future developments and
4) is easy to implement.Btw. There should be no devices created by the board code on those platforms either. Wrong iommu device drivers may be loaded in addition, but this does no more harm than compiling those in to the kernel in the first option.
Quoted 8 lines
Quoted 5 lines
The second solution is the simplest, but it's a workaround. On the other hand, it's hard to design a proper solution before we know the requirements of the other OMAP platforms that have an IOMMU incompatible with iommu2.ko, so it might be better to postpone the decision until we have a real use case.
There are two options that I can think of: either a SoC-wide IOMMU implementation or
That's one option, unless you
:-)
Quoted 5 lines
The problem of loading that module exists right now so it should have some kind of solution. If we go with the second option right now it does push this to direction I don't like too much. The next implementer has to solve the problem instead, and it might be easier to implement this right now, as we are all up-to-date with the issue.
We only have iommu2.ko at the moment. I've heard about an iommu1.ko being worked on, but I don't have more information. We don't know whether the OMAP5 will be able to use the same IOMMU implementation. Without more information, I'm quite reluctant to design and implement a generic solution that will end up being useless because we missed information in the design stage.
Signature · 3 lines
Regards, Laurent Pinchart
Quoted 1 line · Laurent Pinchart
Hi Sakari,
Hi Laurent, Sakari,
Quoted 20 lines · On Tuesday 05 April 2011 11:03:21 Sakari Ailus wrote:
Quoted 1 line
Laurent Pinchart wrote:
[snip]
Quoted 14 lines
Quoted 14 lines
Let me try to summarize the issue and the requirements.
IOMMU support on OMAP platforms uses an OMAP-specific implementation, divided into 3 layers:
- the IOVMM layer (
arch/arm/plat-omap/iovmm.ko) deals with virtual address space management
- the IOMMU layer (arch/arm/plat-omap/iommu.ko) controls the IOMMU hardware, and deals with TLB and page tables
- the IOMMU platform-specific layers (arch/arm/mach-omap2/iommu2.ko) handles the IOMMU implementation variants between various OMAP platformsDrivers depend on iovmm and iommu. They must not depend on iommu2.ko.
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
The only existing platform-specific IOMMU layer is iommu2.ko for OMAP2+. An OMAP1 implementation is being worked on, and other implementations might be needed for OMAP4 and/or OMAP5.
Building a kernel image that will run on all OMAP platforms isn't possible at the moment but is being worked on. Such a kernel image will need to include all the platform-specific IOMMU layers, and the correct layer will need to be selected at runtime.
That exists for all omap2+.
Quoted 7 lines
Quoted 7 lines
Quoted 5 lines
If a driver tries to request and use an IOMMU before the platform-specific IOMMU layer is initialized, the request will fail. We thus need to ensure that the correct platform-specific IOMMU layer is initialized before IOMMU users are initialized.
Thanks for the summary!
You've done it very well.
Quoted 15 lines
Quoted 15 lines
Quoted 14 lines
I can see several ways to fix the problem.
- Turn the iommu and iommu2 options from tristate to bool. The downside is that the kernel image will get slightly bigger.
- Merge the iommu and iommu2 modules together. This will temporarily fix the problem, but a proper solution will still be needed for the OMAP1 (and maybe OMAP5) IOMMU layers.
- Auto-load the correct platform-specific IOMMU module based on modaliases created from the platform name. The platform-specific modules will need to check at runtime whether they support the current platform to avoid conflicts when several of those modules will be compiled in.
Quoted 4 lines
Quoted 2 lines
I'd like to add option to auto-load the module based on the type of the IOMMU.
Could you elaborate on that ?
I think it might be a good solution for near future in order to keep the driver generic for OMAP. But we need to keep in mind nothing related to specific implementation should be added to generic IOMMU layer, so board code could be the right place. In this case I may agree with the third option from Laurent.
Quoted 43 lines
Quoted 24 lines
This is more generic since there could be several types of IOMMUs in the same system, although in the scope of OMAPs we are likely to have always just one.
Extending the scope of the OMAP IOMMU would be nice, or to add functionality to the current generic layer which doesn't do much at the moment.
This is probably a bigger task and something to consider in the future, though.
I'd go with the third option you suggested since this one
1) solves the problem,
2) doesn't appear to create new ones,
3) doesn't add anything that would be incompatible with probable future developments and
4) is easy to implement.Btw. There should be no devices created by the board code on those platforms either. Wrong iommu device drivers may be loaded in addition, but this does no more harm than compiling those in to the kernel in the first option.
Quoted 8 lines
Quoted 5 lines
The second solution is the simplest, but it's a workaround. On the other hand, it's hard to design a proper solution before we know the requirements of the other OMAP platforms that have an IOMMU incompatible with iommu2.ko, so it might be better to postpone the decision until we have a real use case.
There are two options that I can think of: either a SoC-wide IOMMU implementation or
That's one option, unless you
:-)
Quoted 5 lines
The problem of loading that module exists right now so it should have some kind of solution. If we go with the second option right now it does push this to direction I don't like too much. The next implementer has to solve the problem instead, and it might be easier to implement this right now, as we are all up-to-date with the issue.
Quoted 5 lines
We only have iommu2.ko at the moment. I've heard about an iommu1.ko being worked on, but I don't have more information. We don't know whether the OMAP5 will be able to use the same IOMMU implementation. Without more information, I'm quite reluctant to design and implement a generic solution that will end up being useless because we missed information in the design stage.
One implementation belongs to mach-omap1 and other to mach-omap2. Not sure if it's a good plan to get them together. IMO the third option from Laurent solves the issue for now and don't make it more difficult to implement a better standard to OMAP.
Regards,
David Cohen
Hi David,
Quoted 1 line · On Tuesday 05 April 2011 13:54:09 David Cohen wrote:
On Tue, Apr 5, 2011 at 2:23 PM, Laurent Pinchart wrote:
[snip]
Quoted 9 lines
Quoted 6 lines
We only have iommu2.ko at the moment. I've heard about an iommu1.ko being worked on, but I don't have more information. We don't know whether the OMAP5 will be able to use the same IOMMU implementation. Without more information, I'm quite reluctant to design and implement a generic solution that will end up being useless because we missed information in the design stage.
One implementation belongs to mach-omap1 and other to mach-omap2. Not sure if it's a good plan to get them together.
My point is that we have a single implementation at the moment. Not one in mach-omap1 and one in mach-omap2, just one. I don't like solving problems with generic solutions when there's a single use case.
Quoted 2 lines
IMO the third option from Laurent solves the issue for now and don't make it more difficult to implement a better standard to OMAP.
Signature · 3 lines
Regards, Laurent Pinchart
Quoted 1 line · Laurent Pinchart
Hi Sakari,
Hi Laurent,
Quoted 49 lines · On Tuesday 05 April 2011 11:03:21 Sakari Ailus wrote:
Quoted 1 line
Laurent Pinchart wrote:
[snip]
Quoted 44 lines
Quoted 23 lines
Let me try to summarize the issue and the requirements.
IOMMU support on OMAP platforms uses an OMAP-specific implementation, divided into 3 layers:
- the IOVMM layer (
arch/arm/plat-omap/iovmm.ko) deals with virtual address space management
- the IOMMU layer (arch/arm/plat-omap/iommu.ko) controls the IOMMU hardware, and deals with TLB and page tables
- the IOMMU platform-specific layers (arch/arm/mach-omap2/iommu2.ko) handles the IOMMU implementation variants between various OMAP platformsDrivers depend on iovmm and iommu. They must not depend on iommu2.ko.
The only existing platform-specific IOMMU layer is iommu2.ko for OMAP2+. An OMAP1 implementation is being worked on, and other implementations might be needed for OMAP4 and/or OMAP5.
Building a kernel image that will run on all OMAP platforms isn't possible at the moment but is being worked on. Such a kernel image will need to include all the platform-specific IOMMU layers, and the correct layer will need to be selected at runtime.
Quoted 4 lines
If a driver tries to request and use an IOMMU before the platform-specific IOMMU layer is initialized, the request will fail. We thus need to ensure that the correct platform-specific IOMMU layer is initialized before IOMMU users are initialized.
Thanks for the summary!
Quoted 13 lines
I can see several ways to fix the problem.
- Turn the iommu and iommu2 options from tristate to bool. The downside is that the kernel image will get slightly bigger.
- Merge the iommu and iommu2 modules together. This will temporarily fix the problem, but a proper solution will still be needed for the OMAP1 (and maybe OMAP5) IOMMU layers.
- Auto-load the correct platform-specific IOMMU module based on modaliases created from the platform name. The platform-specific modules will need to check at runtime whether they support the current platform to avoid conflicts when several of those modules will be compiled in.
Quoted 4 lines
Quoted 2 lines
I'd like to add option to auto-load the module based on the type of the IOMMU.
Could you elaborate on that ?
The module name could be specified somewhere, but the question is then where. There needs to be iommu name -> module name mapping somewhere (can be distributed) and it must not be the end user of the iommu framework.
Quoted 29 lines
Quoted 29 lines
This is more generic since there could be several types of IOMMUs in the same system, although in the scope of OMAPs we are likely to have always just one.
Extending the scope of the OMAP IOMMU would be nice, or to add functionality to the current generic layer which doesn't do much at the moment.
This is probably a bigger task and something to consider in the future, though.
I'd go with the third option you suggested since this one
1) solves the problem,
2) doesn't appear to create new ones,
3) doesn't add anything that would be incompatible with probable future developments and
4) is easy to implement.Btw. There should be no devices created by the board code on those platforms either. Wrong iommu device drivers may be loaded in addition, but this does no more harm than compiling those in to the kernel in the first option.
Quoted 5 lines
The second solution is the simplest, but it's a workaround. On the other hand, it's hard to design a proper solution before we know the requirements of the other OMAP platforms that have an IOMMU incompatible with iommu2.ko, so it might be better to postpone the decision until we have a real use case.
Quoted 4 lines
Quoted 2 lines
There are two options that I can think of: either a SoC-wide IOMMU implementation or
That's one option, unless you
I sent that too early. :-P
... a device which does have an IOMMU, connected to e.g. a bus that uses the IOMMU framework. This way there could be different types of IOMMUs in a system.
But we don't have that yet.
So either we have just one or multiple types or IOMMUs in the system.
Quoted 11 lines
Quoted 5 lines
The problem of loading that module exists right now so it should have some kind of solution. If we go with the second option right now it does push this to direction I don't like too much. The next implementer has to solve the problem instead, and it might be easier to implement this right now, as we are all up-to-date with the issue.
We only have iommu2.ko at the moment. I've heard about an iommu1.ko being worked on, but I don't have more information. We don't know whether the OMAP5 will be able to use the same IOMMU implementation. Without more information, I'm quite reluctant to design and implement a generic solution that will end up being useless because we missed information in the design stage.
For the scope of OMAPs the first and third solutions would be enough.
I have no strong opinion on this either way.
I think that for the fully generic case it's that there are multiple different IOMMUs in the system and those are being used by different devices. Some could be used by many, as the OMAP 4 auxiliary ARMs.
But making the loading fully generic within the scope of OMAPs makes no sense.
Regards,
Signature · 2 lines
Sakari Ailus [email protected]
Quoted 8 lines · Laurent Pinchart
I think that Sakari's patches correcty fix the problems he noticed. However, they won't fix one basic issue, which is that the iommu2 module won't be automatically pulled in when the omap3isp module is loaded. The omap3isp driver will then fail to probe the device. That's better than crashing though.
One possible solution for that is to turn the tristate option for iommu2 into a bool option. I've also read a couple of times that the kernel provides a standard iommu API. Maybe switching to it would help.
I'm attempting to get support for the Gumstix Caspa image sensor (based on mt9v032) working with the 3.0 release.
I'm running into the issue described above -- the omap3-isp module loads but fails to probe the device.
I've tried the tristate->bool option change and find that it does allow me to proceed further, but the omap3-isp module is no longer loaded automatically when I call omap3_init_camera in the board file. I have to manually modprobe it. I then see the sensor module successfully probed and the video devices created (though not functioning yet).
Are you aware of a better way to do this? I see lots of patches for iommu since this thread. Would I be better off waiting for 3.1 and trying that, or will final resolution of this issue come even later?
Best regards,
Steve
Hi Steve,
Quoted 16 lines · On Saturday 24 September 2011 15:44:52 Steve Sakoman wrote:
On Tue, Mar 29, 2011 at 8:32 AM, Laurent Pinchart
Quoted 9 lines · <[email protected]> wrote:
I think that Sakari's patches correcty fix the problems he noticed. However, they won't fix one basic issue, which is that the iommu2 module won't be automatically pulled in when the omap3isp module is loaded. The omap3isp driver will then fail to probe the device. That's better than crashing though.
One possible solution for that is to turn the tristate option for iommu2 into a bool option. I've also read a couple of times that the kernel provides a standard iommu API. Maybe switching to it would help.
I'm attempting to get support for the Gumstix Caspa image sensor (based on mt9v032) working with the 3.0 release.
Quoted 6 lines
I'm running into the issue described above -- the omap3-isp module loads but fails to probe the device.
I've tried the
tristate->booloption change and find that it does allow me to proceed further, but the omap3-isp module is no longer loaded automatically when I callomap3_init_camerain the board file.
That's weird, it should if udev is setup properly.
Quoted 7 lines
I have to manually modprobe it. I then see the sensor module successfully probed and the video devices created (though not functioning yet).
Are you aware of a better way to do this? I see lots of patches for iommu since this thread. Would I be better off waiting for 3.1 and trying that, or will final resolution of this issue come even later?
The tristate -> bool change (which will likely be part of a much bigger change set) will come later.
Signature · 3 lines
Regards, Laurent Pinchart