[PATCH 0/4] iommu: Prevent oops in iommu_get() and while arch_iommu is in use

[PATCH 0/4] iommu: Prevent oops in iommu_get() and while arch_iommu is in use

[Sakari Ailus]

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,

-- 
Sakari Ailus
sakari.ailus@maxwell.research.nokia.com

[PATCH 0/4] iommu: Prevent oops in iommu_get() and while arch_iommu is in use

[Sakari Ailus]

Hi,

[Resend: the patches were accidentally sent to linux-media instead.
Apologies.]

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,

-- 
Sakari Ailus
sakari.ailus@maxwell.research.nokia.com

Re: [PATCH 0/4] iommu: Prevent oops in iommu_get() and while arch_iommu is in use

[David Cohen]

On Fri, Mar 25, 2011 at 5:17 PM, Sakari Ailus
<sakari.ailus@maxwell.research.nokia.com> wrote:
> Hi,

Hi Sakari,

>
> [Resend: the patches were accidentally sent to linux-media instead.
> Apologies.]
>
> 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.

Sounds nice.

>
> The patchset assumes the arch_iommu is uninstalled at module unload
> time. Is this an acceptable requirement?

I can't see a reason why this assumption could be wrong. In my point
of view it's acceptable. Let's see Hiroshi's.

Regards,

David Cohen

>
> 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,
>
> --
> Sakari Ailus
> sakari.ailus@maxwell.research.nokia.com
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> 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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] iommu: Prevent oops in iommu_get() and while arch_iommu is in use

[Ramirez Luna, Omar]

On Fri, Mar 25, 2011 at 10:13 AM, Sakari Ailus
<sakari.ailus@maxwell.research.nokia.com> wrote:
> 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.

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

---

diff --git a/arch/arm/mach-omap2/iommu2.c b/arch/arm/mach-omap2/iommu2.c
index adb083e..ab2f9a9 100644
--- a/arch/arm/mach-omap2/iommu2.c
+++ b/arch/arm/mach-omap2/iommu2.c
@@ -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);

diff --git a/arch/arm/plat-omap/include/plat/iommu.h
b/arch/arm/plat-omap/include/plat/iommu.h
index 174f1b9..1c8e7ee 100644
--- a/arch/arm/plat-omap/include/plat/iommu.h
+++ b/arch/arm/plat-omap/include/plat/iommu.h
@@ -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 *));

diff --git a/arch/arm/plat-omap/include/plat/iommu2.h
b/arch/arm/plat-omap/include/plat/iommu2.h
index 10ad05f..8189f58 100644
--- a/arch/arm/plat-omap/include/plat/iommu2.h
+++ b/arch/arm/plat-omap/include/plat/iommu2.h
@@ -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
  */
diff --git a/arch/arm/plat-omap/iommu.c b/arch/arm/plat-omap/iommu.c
index 8a51fd5..f088929 100644
--- a/arch/arm/plat-omap/iommu.c
+++ b/arch/arm/plat-omap/iommu.c
@@ -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);

Re: [PATCH 0/4] iommu: Prevent oops in iommu_get() and while arch_iommu is in use

[Sakari Ailus]

Ramirez Luna, Omar wrote:
> On Fri, Mar 25, 2011 at 10:13 AM, Sakari Ailus
> <sakari.ailus@maxwell.research.nokia.com> wrote:
>> 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.
> 
> 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,

-- 
Sakari Ailus
sakari.ailus@maxwell.research.nokia.com

Re: [PATCH 0/4] iommu: Prevent oops in iommu_get() and while arch_iommu is in use

[Ramirez Luna, Omar]

Hi,

On Sun, Mar 27, 2011 at 12:27 PM, Sakari Ailus
<sakari.ailus@maxwell.research.nokia.com> wrote:
> Ramirez Luna, Omar wrote:
>> On Fri, Mar 25, 2011 at 10:13 AM, Sakari Ailus
>> <sakari.ailus@maxwell.research.nokia.com> wrote:
>>> 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.
>>
>> 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).

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.

> 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

Re: [PATCH 0/4] iommu: Prevent oops in iommu_get() and while arch_iommu is in use

[David Cohen]

Hi Sakari,

On Sun, Mar 27, 2011 at 8:27 PM, Sakari Ailus
<sakari.ailus@maxwell.research.nokia.com> wrote:
> Ramirez Luna, Omar wrote:
>> On Fri, Mar 25, 2011 at 10:13 AM, Sakari Ailus
>> <sakari.ailus@maxwell.research.nokia.com> wrote:
>>> 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.
>>
>> 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.

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

>
> Regards,
>
> --
> Sakari Ailus
> sakari.ailus@maxwell.research.nokia.com
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> 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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] iommu: Prevent oops in iommu_get() and while arch_iommu is in use

[David Cohen]

On Mon, Mar 28, 2011 at 4:42 AM, Ramirez Luna, Omar <omar.ramirez@ti.com> wrote:
> Hi,
>
> On Sun, Mar 27, 2011 at 12:27 PM, Sakari Ailus
> <sakari.ailus@maxwell.research.nokia.com> wrote:
>> Ramirez Luna, Omar wrote:
>>> On Fri, Mar 25, 2011 at 10:13 AM, Sakari Ailus
>>> <sakari.ailus@maxwell.research.nokia.com> wrote:
>>>> 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.
>>>
>>> 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).
>
> 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.

>
> 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.

>
> 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.
>
>> 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

So, every new specific implementation should modify this piece of
code? Are you sure it's a good idea?

Regards,

David Cohen

>
> 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
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> 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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] iommu: Prevent oops in iommu_get() and while arch_iommu is in use

[Laurent Pinchart]

On Friday 25 March 2011 20:37:55 Ramirez Luna, Omar wrote:
> On Fri, Mar 25, 2011 at 10:13 AM, Sakari Ailus
> 
> <sakari.ailus@maxwell.research.nokia.com> wrote:
> > 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.
> 
> 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.

> 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,

Laurent Pinchart

Re: [PATCH 0/4] iommu: Prevent oops in iommu_get() and while arch_iommu is in use

[Laurent Pinchart]

Hi,

On Monday 28 March 2011 03:42:23 Ramirez Luna, Omar wrote:
> On Sun, Mar 27, 2011 at 12:27 PM, Sakari Ailus wrote:
> > Ramirez Luna, Omar wrote:
> >> On Fri, Mar 25, 2011 at 10:13 AM, Sakari Ailus wrote:
> >>> 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.
> >> 
> >> 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).
> 
> 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.

> 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.
> 
> > 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,

Laurent Pinchart

Re: [PATCH 0/4] iommu: Prevent oops in iommu_get() and while arch_iommu is in use

[Sakari Ailus]

Hi Laurent and Omar,

Laurent Pinchart wrote:
> On Friday 25 March 2011 20:37:55 Ramirez Luna, Omar wrote:
>> On Fri, Mar 25, 2011 at 10:13 AM, Sakari Ailus
>>
>> <sakari.ailus@maxwell.research.nokia.com> wrote:
>>> 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.
>>
>> 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 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.

> 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,

-- 
Sakari Ailus
sakari.ailus@maxwell.research.nokia.com

Re: [PATCH 0/4] iommu: Prevent oops in iommu_get() and while arch_iommu is in use

[Laurent Pinchart]

Hi Sakari,

On Wednesday 30 March 2011 10:16:56 Sakari Ailus wrote:
> Laurent Pinchart wrote:
> > On Friday 25 March 2011 20:37:55 Ramirez Luna, Omar wrote:
> >> On Fri, Mar 25, 2011 at 10:13 AM, Sakari Ailus wrote:
> >>> 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.
> >> 
> >> 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 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.

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.

> In a generic case there would have to be a list of modules implementing
> iommu in the platform data.
> 
> > 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,

Laurent Pinchart

Re: [PATCH 0/4] iommu: Prevent oops in iommu_get() and while arch_iommu is in use

[Sakari Ailus]

Laurent Pinchart wrote:
> Hi Sakari,

Hi Laurent,

> On Wednesday 30 March 2011 10:16:56 Sakari Ailus wrote:
>> Laurent Pinchart wrote:
>>> On Friday 25 March 2011 20:37:55 Ramirez Luna, Omar wrote:
>>>> On Fri, Mar 25, 2011 at 10:13 AM, Sakari Ailus wrote:
>>>>> 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.
>>>>
>>>> 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 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.
> 
> 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.

> The iommu implementation should be automatically selected based on the machine 
> time.

Machine type?

I agree, but where is the selection made?

-- 
Sakari Ailus
sakari.ailus@maxwell.research.nokia.com

Re: [PATCH 0/4] iommu: Prevent oops in iommu_get() and while arch_iommu is in use

[Laurent Pinchart]

On Wednesday 30 March 2011 15:50:37 Sakari Ailus wrote:
> Laurent Pinchart wrote:
> > Hi Sakari,
> 
> Hi Laurent,
> 
> > On Wednesday 30 March 2011 10:16:56 Sakari Ailus wrote:
> >> Laurent Pinchart wrote:
> >>> On Friday 25 March 2011 20:37:55 Ramirez Luna, Omar wrote:
> >>>> On Fri, Mar 25, 2011 at 10:13 AM, Sakari Ailus wrote:
> >>>>> 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.
> >>>> 
> >>>> 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 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.
> > 
> > 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.
> 
> > 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.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 0/4] iommu: Prevent oops in iommu_get() and while arch_iommu is in use

[David Cohen]

On Wed, Mar 30, 2011 at 4:56 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Wednesday 30 March 2011 15:50:37 Sakari Ailus wrote:
>> Laurent Pinchart wrote:
>> > Hi Sakari,
>>
>> Hi Laurent,
>>
>> > On Wednesday 30 March 2011 10:16:56 Sakari Ailus wrote:
>> >> Laurent Pinchart wrote:
>> >>> On Friday 25 March 2011 20:37:55 Ramirez Luna, Omar wrote:
>> >>>> On Fri, Mar 25, 2011 at 10:13 AM, Sakari Ailus wrote:
>> >>>>> 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.
>> >>>>
>> >>>> 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 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.
>> >
>> > 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.
>>
>> > 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.

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

>
> --
> Regards,
>
> Laurent Pinchart
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> 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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] iommu: Prevent oops in iommu_get() and while arch_iommu is in use

[Laurent Pinchart]

Hi David,

On Wednesday 30 March 2011 17:50:17 David Cohen wrote:
> On Wed, Mar 30, 2011 at 4:56 PM, Laurent Pinchart wrote:
> > On Wednesday 30 March 2011 15:50:37 Sakari Ailus wrote:
> >> Laurent Pinchart wrote:
> >> > On Wednesday 30 March 2011 10:16:56 Sakari Ailus wrote:
> >> >> Laurent Pinchart wrote:
> >> >>> On Friday 25 March 2011 20:37:55 Ramirez Luna, Omar wrote:
> >> >>>> On Fri, Mar 25, 2011 at 10:13 AM, Sakari Ailus wrote:
> >> >>>>> 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.
> >> >>>> 
> >> >>>> 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 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.
> >> > 
> >> > 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.
> >> 
> >> > 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.
> 
> 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.

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.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 0/4] iommu: Prevent oops in iommu_get() and while arch_iommu is in use

[Sakari Ailus]

Laurent Pinchart wrote:
> Hi David,

Hi Laurent, David, others,

> On Wednesday 30 March 2011 17:50:17 David Cohen wrote:
>> On Wed, Mar 30, 2011 at 4:56 PM, Laurent Pinchart wrote:
>>> On Wednesday 30 March 2011 15:50:37 Sakari Ailus wrote:
>>>> Laurent Pinchart wrote:
>>>>> On Wednesday 30 March 2011 10:16:56 Sakari Ailus wrote:
>>>>>> Laurent Pinchart wrote:
>>>>>>> On Friday 25 March 2011 20:37:55 Ramirez Luna, Omar wrote:
>>>>>>>> On Fri, Mar 25, 2011 at 10:13 AM, Sakari Ailus wrote:
>>>>>>>>> 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.
>>>>>>>>
>>>>>>>> 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 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.
>>>>>
>>>>> 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.
>>>>
>>>>> 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.
>>
>> 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.
> 
> 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.

Thanks for the summary!

> 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.

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.

> 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,

-- 
Sakari Ailus
sakari.ailus@maxwell.research.nokia.com

Re: [PATCH 0/4] iommu: Prevent oops in iommu_get() and while arch_iommu is in use

[Laurent Pinchart]

Hi Sakari,

On Tuesday 05 April 2011 11:03:21 Sakari Ailus wrote:
> Laurent Pinchart wrote:

[snip]

> > 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.
> 
> Thanks for the summary!
> 
> > 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.
> 
> I'd like to add option to auto-load the module based on the type of the
> IOMMU.

Could you elaborate on that ?

> 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.
> 
> > 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

:-)

> 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.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 0/4] iommu: Prevent oops in iommu_get() and while arch_iommu is in use

[David Cohen]

On Tue, Apr 5, 2011 at 2:23 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Sakari,

Hi Laurent, Sakari,

>
> On Tuesday 05 April 2011 11:03:21 Sakari Ailus wrote:
>> Laurent Pinchart wrote:
>
> [snip]
>
>> > 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.

That exists for all omap2+.

>> >
>> > 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.

>>
>> > 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.
>>
>> 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.

>
>> 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.
>>
>> > 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
>
> :-)
>
>> 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.

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

Re: [PATCH 0/4] iommu: Prevent oops in iommu_get() and while arch_iommu is in use

[Sakari Ailus]

Laurent Pinchart wrote:
> Hi Sakari,

Hi Laurent,

> On Tuesday 05 April 2011 11:03:21 Sakari Ailus wrote:
>> Laurent Pinchart wrote:
> 
> [snip]
> 
>>> 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.
>>
>> Thanks for the summary!
>>
>>> 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.
>>
>> 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.

>> 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.
>>
>>> 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

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.

>> 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,

-- 
Sakari Ailus
sakari.ailus@maxwell.research.nokia.com

Re: [PATCH 0/4] iommu: Prevent oops in iommu_get() and while arch_iommu is in use

[Laurent Pinchart]

Hi David,

On Tuesday 05 April 2011 13:54:09 David Cohen wrote:
> On Tue, Apr 5, 2011 at 2:23 PM, Laurent Pinchart wrote:

[snip]

> > 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.

> 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,

Laurent Pinchart

Re: [PATCH 0/4] iommu: Prevent oops in iommu_get() and while arch_iommu is in use

[Steve Sakoman]

On Tue, Mar 29, 2011 at 8:32 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> 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.

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

Re: [PATCH 0/4] iommu: Prevent oops in iommu_get() and while arch_iommu is in use

[Laurent Pinchart]

Hi Steve,

On Saturday 24 September 2011 15:44:52 Steve Sakoman wrote:
> On Tue, Mar 29, 2011 at 8:32 AM, Laurent Pinchart
> 
> <laurent.pinchart@ideasonboard.com> 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.
> 
> 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.

That's weird, it should if udev is setup properly.

> 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.

-- 
Regards,

Laurent Pinchart