[PATCH 0/2] IOMMU fault callback support

Hi,

This patch set adds fault callback support to allow IOMMU users to debug or
react when a fault happens.
IOMMU faults might be very difficult to reproduce and then to figure out
the source of the problem. Currently IOMMU driver prints not so useful
debug message and does not notice user about such issue.
With a fault callback, IOMMU user may debug much more useful information
and/or react to go back to a valid state.

Br,

David
---

David Cohen (2):
  OMAP2+: IOMMU: change OMAP2+ error message to dev_dbg()
  OMAP: IOMMU: add support to callback during fault handling

 arch/arm/mach-omap2/iommu2.c            |   27 ++++++++++++++++----
 arch/arm/plat-omap/include/plat/iommu.h |   15 ++++++++++-
 arch/arm/plat-omap/iommu.c              |   41 ++++++++++++++++++++++++++++---
 3 files changed, 72 insertions(+), 11 deletions(-)

-- 
1.7.2.3

IOMMU upper layer is already printing error message. OMAP2+ specific
layer may print error message only for debug purpose.

Signed-off-by: David Cohen <redacted>
---
 arch/arm/mach-omap2/iommu2.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mach-omap2/iommu2.c b/arch/arm/mach-omap2/iommu2.c
index 14ee686..4244a07 100644
--- a/arch/arm/mach-omap2/iommu2.c
+++ b/arch/arm/mach-omap2/iommu2.c

@@ -163,13 +163,13 @@ static u32 omap2_iommu_fault_isr(struct iommu *obj, u32 *ra)
 	da = iommu_read_reg(obj, MMU_FAULT_AD);
 	*ra = da;
 
-	dev_err(obj->dev, "%s:\tda:%08x ", __func__, da);
+	dev_dbg(obj->dev, "%s:\tda:%08x ", __func__, da);
 
 	for (i = 0; i < ARRAY_SIZE(err_msg); i++) {
 		if (stat & (1 << i))
-			printk("%s ", err_msg[i]);
+			printk(KERN_DEBUG "%s ", err_msg[i]);
 	}
-	printk("\n");
+	printk(KERN_DEBUG "\n");
 
 	iommu_write_reg(obj, stat, MMU_IRQSTATUS);
 

-- 
1.7.2.3

Hello.

On 15-02-2011 16:20, David Cohen wrote:

IOMMU upper layer is already printing error message. OMAP2+ specific
layer may print error message only for debug purpose.

    Note that dev_dbg() will only print something if either DEBUG or 
CONFIG_DYNAMIC_DEBUG are defined...

  	for (i = 0; i<  ARRAY_SIZE(err_msg); i++) {
  		if (stat & (1<<  i))
-			printk("%s ", err_msg[i]);
+			printk(KERN_DEBUG "%s ", err_msg[i]);

    ... unlike printk(KERN_DEBUG...). You probably want to use pr_debug() instead.

  	}
-	printk("\n");
+	printk(KERN_DEBUG "\n");

    Here too... Although wait, it should be KERN_CONT instead! Debug levels 
are only attributed to the whole lines.

WBR, Sergei

On Tue, Feb 15, 2011 at 3:38 PM, Sergei Shtylyov [off-list ref] wrote:

Hello.

Hi,

On 15-02-2011 16:20, David Cohen wrote:

quoted

IOMMU upper layer is already printing error message. OMAP2+ specific
layer may print error message only for debug purpose.

quoted

Signed-off-by: David Cohen<redacted>
---
?arch/arm/mach-omap2/iommu2.c | ? ?6 +++---
?1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mach-omap2/iommu2.c b/arch/arm/mach-omap2/iommu2.c
index 14ee686..4244a07 100644
--- a/arch/arm/mach-omap2/iommu2.c
+++ b/arch/arm/mach-omap2/iommu2.c

@@ -163,13 +163,13 @@ static u32 omap2_iommu_fault_isr(struct iommu *obj,

u32 *ra)
? ? ? ?da = iommu_read_reg(obj, MMU_FAULT_AD);
? ? ? ?*ra = da;

- ? ? ? dev_err(obj->dev, "%s:\tda:%08x ", __func__, da);
+ ? ? ? dev_dbg(obj->dev, "%s:\tda:%08x ", __func__, da);

? Note that dev_dbg() will only print something if either DEBUG or
CONFIG_DYNAMIC_DEBUG are defined...

That's my plan.

quoted

? ? ? ?for (i = 0; i< ?ARRAY_SIZE(err_msg); i++) {
? ? ? ? ? ? ? ?if (stat & (1<< ?i))
- ? ? ? ? ? ? ? ? ? ? ? printk("%s ", err_msg[i]);
+ ? ? ? ? ? ? ? ? ? ? ? printk(KERN_DEBUG "%s ", err_msg[i]);

? ... unlike printk(KERN_DEBUG...). You probably want to use pr_debug()
instead.

quoted

? ? ? ?}
- ? ? ? printk("\n");
+ ? ? ? printk(KERN_DEBUG "\n");

? Here too... Although wait, it should be KERN_CONT instead! Debug levels
are only attributed to the whole lines.

But your observation is correct. I'll resend it with KERN_CONT instead.

Regards,

David

WBR, Sergei

On 15-02-2011 16:44, David Cohen wrote:

quoted

quoted

IOMMU upper layer is already printing error message. OMAP2+ specific
layer may print error message only for debug purpose.

quoted

quoted

Signed-off-by: David Cohen<redacted>
---
  arch/arm/mach-omap2/iommu2.c |    6 +++---
  1 files changed, 3 insertions(+), 3 deletions(-)

quoted

quoted

diff --git a/arch/arm/mach-omap2/iommu2.c b/arch/arm/mach-omap2/iommu2.c
index 14ee686..4244a07 100644
--- a/arch/arm/mach-omap2/iommu2.c
+++ b/arch/arm/mach-omap2/iommu2.c

@@ -163,13 +163,13 @@ static u32 omap2_iommu_fault_isr(struct iommu *obj,

u32 *ra)
        da = iommu_read_reg(obj, MMU_FAULT_AD);
        *ra = da;

-       dev_err(obj->dev, "%s:\tda:%08x ", __func__, da);
+       dev_dbg(obj->dev, "%s:\tda:%08x ", __func__, da);

quoted

   Note that dev_dbg() will only print something if either DEBUG or
CONFIG_DYNAMIC_DEBUG are defined...

That's my plan.

quoted

quoted

        for (i = 0; i < ARRAY_SIZE(err_msg); i++) {
                if (stat&  (1<<    i))
-                       printk("%s ", err_msg[i]);
+                       printk(KERN_DEBUG "%s ", err_msg[i]);

quoted

   ... unlike printk(KERN_DEBUG...). You probably want to use pr_debug()
instead.

quoted

quoted

        }
-       printk("\n");
+       printk(KERN_DEBUG "\n");

quoted

   Here too... Although wait, it should be KERN_CONT instead! Debug levels
are only attributed to the whole lines.

But your observation is correct. I'll resend it with KERN_CONT instead.

    This won't play out correctly anyway. If DEBUG is not #define'd, the 
beginning of line won't be printed but the continuations will. You just can't 
do it correctly with dev_dbg(), unless you break the single line into several 
ones.

Regards,

David

WBR, Sergei

On Tue, Feb 15, 2011 at 3:56 PM, Sergei Shtylyov [off-list ref] wrote:

On 15-02-2011 16:44, David Cohen wrote:

quoted

quoted

quoted

IOMMU upper layer is already printing error message. OMAP2+ specific
layer may print error message only for debug purpose.

quoted

quoted

quoted

Signed-off-by: David Cohen<redacted>
---
?arch/arm/mach-omap2/iommu2.c | ? ?6 +++---
?1 files changed, 3 insertions(+), 3 deletions(-)

quoted

quoted

quoted

diff --git a/arch/arm/mach-omap2/iommu2.c b/arch/arm/mach-omap2/iommu2.c
index 14ee686..4244a07 100644
--- a/arch/arm/mach-omap2/iommu2.c
+++ b/arch/arm/mach-omap2/iommu2.c

@@ -163,13 +163,13 @@ static u32 omap2_iommu_fault_isr(struct iommu

*obj,
u32 *ra)
? ? ? ?da = iommu_read_reg(obj, MMU_FAULT_AD);
? ? ? ?*ra = da;

- ? ? ? dev_err(obj->dev, "%s:\tda:%08x ", __func__, da);
+ ? ? ? dev_dbg(obj->dev, "%s:\tda:%08x ", __func__, da);

quoted

quoted

? Note that dev_dbg() will only print something if either DEBUG or
CONFIG_DYNAMIC_DEBUG are defined...

quoted

That's my plan.

quoted

quoted

quoted

? ? ? ?for (i = 0; i < ARRAY_SIZE(err_msg); i++) {
? ? ? ? ? ? ? ?if (stat& ?(1<< ? ?i))
- ? ? ? ? ? ? ? ? ? ? ? printk("%s ", err_msg[i]);
+ ? ? ? ? ? ? ? ? ? ? ? printk(KERN_DEBUG "%s ", err_msg[i]);

quoted

quoted

? ... unlike printk(KERN_DEBUG...). You probably want to use pr_debug()
instead.

quoted

quoted

quoted

? ? ? ?}
- ? ? ? printk("\n");
+ ? ? ? printk(KERN_DEBUG "\n");

quoted

quoted

? Here too... Although wait, it should be KERN_CONT instead! Debug levels
are only attributed to the whole lines.

quoted

But your observation is correct. I'll resend it with KERN_CONT instead.

? This won't play out correctly anyway. If DEBUG is not #define'd, the
beginning of line won't be printed but the continuations will. You just
can't do it correctly with dev_dbg(), unless you break the single line into
several ones.

Yes, I got this situation. I'm coming with a proper solution on next version.

Br,

David

quoted

Regards,

quoted

David

WBR, Sergei

On Tue, 15 Feb 2011 15:44:27 +0200
David Cohen [off-list ref] wrote:

quoted

quoted

@@ -163,13 +163,13 @@ static u32 omap2_iommu_fault_isr(struct iommu *obj,

u32 *ra)
? ? ? ?da = iommu_read_reg(obj, MMU_FAULT_AD);
? ? ? ?*ra = da;

- ? ? ? dev_err(obj->dev, "%s:\tda:%08x ", __func__, da);
+ ? ? ? dev_dbg(obj->dev, "%s:\tda:%08x ", __func__, da);

? Note that dev_dbg() will only print something if either DEBUG or
CONFIG_DYNAMIC_DEBUG are defined...

That's my plan.

So it's sure that a developer won't need these error dumps when
receiving an error report? I.e. IOMMU upper level errors give enough
information to start doing own debugging?

Just my 2 cents.

-- 
Jarkko

On Tue, Feb 15, 2011 at 3:59 PM, Jarkko Nikula [off-list ref] wrote:

On Tue, 15 Feb 2011 15:44:27 +0200
David Cohen [off-list ref] wrote:

quoted

quoted

quoted

@@ -163,13 +163,13 @@ static u32 omap2_iommu_fault_isr(struct iommu *obj,

u32 *ra)
? ? ? ?da = iommu_read_reg(obj, MMU_FAULT_AD);
? ? ? ?*ra = da;

- ? ? ? dev_err(obj->dev, "%s:\tda:%08x ", __func__, da);
+ ? ? ? dev_dbg(obj->dev, "%s:\tda:%08x ", __func__, da);

? Note that dev_dbg() will only print something if either DEBUG or
CONFIG_DYNAMIC_DEBUG are defined...

That's my plan.

So it's sure that a developer won't need these error dumps when
receiving an error report? I.e. IOMMU upper level errors give enough
information to start doing own debugging?

Yes, developers do need this information.
But it's a bit useless tell only we've got an iommu fault, due to many
places might be causing it. My purpose is to let the debug
responsibility to IOMMU users. They have access to the iovmm layer as
well and can provide a much more useful information.
e.g. OMAP3 ISP has many submodules using IOMMU. With a fault callback,
it can dump all the iovm areas and the faulty 'da' too. It might
indicate which submodule was responsible for the issue.

Of course we can just let this debug messages the way they are and
print this redundant information. But IMO it's not necessary.

Regards,

David

Just my 2 cents.

--
Jarkko

On Tue, Feb 15, 2011 at 4:08 PM, David Cohen [off-list ref] wrote:

On Tue, Feb 15, 2011 at 3:59 PM, Jarkko Nikula [off-list ref] wrote:

quoted

On Tue, 15 Feb 2011 15:44:27 +0200
David Cohen [off-list ref] wrote:

quoted

quoted

quoted

@@ -163,13 +163,13 @@ static u32 omap2_iommu_fault_isr(struct iommu *obj,

u32 *ra)
? ? ? ?da = iommu_read_reg(obj, MMU_FAULT_AD);
? ? ? ?*ra = da;

- ? ? ? dev_err(obj->dev, "%s:\tda:%08x ", __func__, da);
+ ? ? ? dev_dbg(obj->dev, "%s:\tda:%08x ", __func__, da);

? Note that dev_dbg() will only print something if either DEBUG or
CONFIG_DYNAMIC_DEBUG are defined...

That's my plan.

So it's sure that a developer won't need these error dumps when
receiving an error report? I.e. IOMMU upper level errors give enough
information to start doing own debugging?

Yes, developers do need this information.
But it's a bit useless tell only we've got an iommu fault, due to many
places might be causing it. My purpose is to let the debug
responsibility to IOMMU users. They have access to the iovmm layer as
well and can provide a much more useful information.
e.g. OMAP3 ISP has many submodules using IOMMU. With a fault callback,
it can dump all the iovm areas and the faulty 'da' too. It might
indicate which submodule was responsible for the issue.

Of course we can just let this debug messages the way they are and
print this redundant information. But IMO it's not necessary.

Indeed, we can leave this discussion for future. My main purpose now
is the fault callback. I'll drop this patch 1/2 for now.

Regards,

David

Regards,

David

quoted

Just my 2 cents.

--
Jarkko

On Tue, 15 Feb 2011 16:08:32 +0200
David Cohen [off-list ref] wrote:

quoted

So it's sure that a developer won't need these error dumps when
receiving an error report? I.e. IOMMU upper level errors give enough
information to start doing own debugging?

Yes, developers do need this information.
But it's a bit useless tell only we've got an iommu fault, due to many
places might be causing it. My purpose is to let the debug
responsibility to IOMMU users. They have access to the iovmm layer as
well and can provide a much more useful information.
e.g. OMAP3 ISP has many submodules using IOMMU. With a fault callback,
it can dump all the iovm areas and the faulty 'da' too. It might
indicate which submodule was responsible for the issue.

Of course we can just let this debug messages the way they are and
print this redundant information. But IMO it's not necessary.

Sounds fair enough and if I understood correctly this is not something
what end user will hit but more like another developer. In that case
the debug messages are the right thing.

-- 
Jarkko

On Tue, Feb 15, 2011 at 04:38:32PM +0300, Sergei Shtylyov wrote:

quoted

diff --git a/arch/arm/mach-omap2/iommu2.c b/arch/arm/mach-omap2/iommu2.c
index 14ee686..4244a07 100644
--- a/arch/arm/mach-omap2/iommu2.c
+++ b/arch/arm/mach-omap2/iommu2.c

@@ -163,13 +163,13 @@ static u32 omap2_iommu_fault_isr(struct iommu *obj, u32 *ra)
  	da = iommu_read_reg(obj, MMU_FAULT_AD);
  	*ra = da;

-	dev_err(obj->dev, "%s:\tda:%08x ", __func__, da);
+	dev_dbg(obj->dev, "%s:\tda:%08x ", __func__, da);

   Note that dev_dbg() will only print something if either DEBUG or  
CONFIG_DYNAMIC_DEBUG are defined...

quoted

  	for (i = 0; i<  ARRAY_SIZE(err_msg); i++) {
  		if (stat & (1<<  i))
-			printk("%s ", err_msg[i]);
+			printk(KERN_DEBUG "%s ", err_msg[i]);

   ... unlike printk(KERN_DEBUG...). You probably want to use pr_debug() instead.

No - this isn't starting a new line.  pr_cont() here.

quoted

  	}
-	printk("\n");
+	printk(KERN_DEBUG "\n");

   Here too... Although wait, it should be KERN_CONT instead! Debug 
levels are only attributed to the whole lines.

And pr_cont() here too.  If you care about using KERN_CONT which is
just a static marker to allow automated printk level checking easier.

On Tue, Feb 15, 2011 at 4:29 PM, Russell King - ARM Linux
[off-list ref] wrote:

On Tue, Feb 15, 2011 at 04:38:32PM +0300, Sergei Shtylyov wrote:

quoted

quoted

diff --git a/arch/arm/mach-omap2/iommu2.c b/arch/arm/mach-omap2/iommu2.c
index 14ee686..4244a07 100644
--- a/arch/arm/mach-omap2/iommu2.c
+++ b/arch/arm/mach-omap2/iommu2.c

@@ -163,13 +163,13 @@ static u32 omap2_iommu_fault_isr(struct iommu *obj, u32 *ra)

? ? ?da = iommu_read_reg(obj, MMU_FAULT_AD);
? ? ?*ra = da;

- ? ?dev_err(obj->dev, "%s:\tda:%08x ", __func__, da);
+ ? ?dev_dbg(obj->dev, "%s:\tda:%08x ", __func__, da);

? ?Note that dev_dbg() will only print something if either DEBUG or
CONFIG_DYNAMIC_DEBUG are defined...

quoted

? ? ?for (i = 0; i< ?ARRAY_SIZE(err_msg); i++) {
? ? ? ? ? ? ?if (stat & (1<< ?i))
- ? ? ? ? ? ? ? ? ? ?printk("%s ", err_msg[i]);
+ ? ? ? ? ? ? ? ? ? ?printk(KERN_DEBUG "%s ", err_msg[i]);

? ?... unlike printk(KERN_DEBUG...). You probably want to use pr_debug() instead.

No - this isn't starting a new line. ?pr_cont() here.

But pr_cont() would be wrong in case of DEBUG isn't set, isn't it?

quoted

quoted

? ? ?}
- ? ?printk("\n");
+ ? ?printk(KERN_DEBUG "\n");

? ?Here too... Although wait, it should be KERN_CONT instead! Debug
levels are only attributed to the whole lines.

And pr_cont() here too. ?If you care about using KERN_CONT which is
just a static marker to allow automated printk level checking easier.

The same situation here.

But this patch was dropped in the next version.

Br,

David

On Tue, Feb 15, 2011 at 04:36:26PM +0200, David Cohen wrote:

But pr_cont() would be wrong in case of DEBUG isn't set, isn't it?

Yes.  One other solution you could do is:

	char buf[80], *p = buf;

	buf[0] = '\0';
	for (i = 0; i < ARRAY_SIZE(err_msg); i++)
		if (stat & (1 << i))
			p += scnprintf(p, sizeof(buf) - (p - buf),
					" %s", err_msg[i]);

	dev_dbg(obj->dev, "%s: da:%08x%s\n", __func__, da, buf);

which means that you're then printing the entire string in one go -
and there's no chance for another message to come in the middle of it.

Note that I've placed the ' ' at the beginning of the format string so
that we don't end up with useless trailing space in messages.  Minor
point but it's easy to do here.

On Tue, Feb 15, 2011 at 4:44 PM, Russell King - ARM Linux
[off-list ref] wrote:

On Tue, Feb 15, 2011 at 04:36:26PM +0200, David Cohen wrote:

quoted

But pr_cont() would be wrong in case of DEBUG isn't set, isn't it?

Yes. ?One other solution you could do is:

? ? ? ?char buf[80], *p = buf;

? ? ? ?buf[0] = '\0';
? ? ? ?for (i = 0; i < ARRAY_SIZE(err_msg); i++)
? ? ? ? ? ? ? ?if (stat & (1 << i))
? ? ? ? ? ? ? ? ? ? ? ?p += scnprintf(p, sizeof(buf) - (p - buf),
? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?" %s", err_msg[i]);

? ? ? ?dev_dbg(obj->dev, "%s: da:%08x%s\n", __func__, da, buf);

which means that you're then printing the entire string in one go -
and there's no chance for another message to come in the middle of it.

Note that I've placed the ' ' at the beginning of the format string so
that we don't end up with useless trailing space in messages. ?Minor
point but it's easy to do here.

That could be my choice.
I'm not planing to resend this patch, but how good/bad it sounds to
you to have dev_dbg_cont() for such situation?

Br,

David

On Tue, Feb 15, 2011 at 04:50:29PM +0200, David Cohen wrote:

That could be my choice.
I'm not planing to resend this patch, but how good/bad it sounds to
you to have dev_dbg_cont() for such situation?

That doesn't help when the message gets corrupted by another thread,
so I'd much prefer the "format line then print" approach rather than
the blah_CONT stuff.

Add support to register a callback for IOMMU fault situations. Drivers using
IOMMU module might want to be informed when such errors happen in order to
debug it or react.

Signed-off-by: David Cohen <redacted>
---
 arch/arm/mach-omap2/iommu2.c            |   21 +++++++++++++--
 arch/arm/plat-omap/include/plat/iommu.h |   15 ++++++++++-
 arch/arm/plat-omap/iommu.c              |   41 ++++++++++++++++++++++++++++---
 3 files changed, 69 insertions(+), 8 deletions(-)

diff --git a/arch/arm/mach-omap2/iommu2.c b/arch/arm/mach-omap2/iommu2.c
index 4244a07..559a066 100644
--- a/arch/arm/mach-omap2/iommu2.c
+++ b/arch/arm/mach-omap2/iommu2.c

@@ -143,10 +143,10 @@ static void omap2_iommu_set_twl(struct iommu *obj, bool on)
 	__iommu_set_twl(obj, false);
 }
 
-static u32 omap2_iommu_fault_isr(struct iommu *obj, u32 *ra)
+static u32 omap2_iommu_fault_isr(struct iommu *obj, u32 *ra, u32 *iommu_errs)
 {
 	int i;
-	u32 stat, da;
+	u32 stat, da, errs;
 	const char *err_msg[] =	{
 		"tlb miss",
 		"translation fault",

@@ -157,8 +157,10 @@ static u32 omap2_iommu_fault_isr(struct iommu *obj, u32 *ra)
 
 	stat = iommu_read_reg(obj, MMU_IRQSTATUS);
 	stat &= MMU_IRQ_MASK;
-	if (!stat)
+	if (!stat) {
+		*iommu_errs = 0;
 		return 0;
+	}
 
 	da = iommu_read_reg(obj, MMU_FAULT_AD);
 	*ra = da;

@@ -171,6 +173,19 @@ static u32 omap2_iommu_fault_isr(struct iommu *obj, u32 *ra)
 	}
 	printk(KERN_DEBUG "\n");
 
+	errs = 0;
+	if (stat & MMU_IRQ_TLBMISS)
+		errs |= OMAP_IOMMU_ERR_TLB_MISS;
+	if (stat & MMU_IRQ_TRANSLATIONFAULT)
+		errs |= OMAP_IOMMU_ERR_TRANS_FAULT;
+	if (stat & MMU_IRQ_EMUMISS)
+		errs |= OMAP_IOMMU_ERR_EMU_MISS;
+	if (stat & MMU_IRQ_TABLEWALKFAULT)
+		errs |= OMAP_IOMMU_ERR_TBLWALK_FAULT;
+	if (stat & MMU_IRQ_MULTIHITFAULT)
+		errs |= OMAP_IOMMU_ERR_MULTIHIT_FAULT;
+	*iommu_errs = errs;
+
 	iommu_write_reg(obj, stat, MMU_IRQSTATUS);
 
 	return stat;

diff --git a/arch/arm/plat-omap/include/plat/iommu.h b/arch/arm/plat-omap/include/plat/iommu.h
index 19cbb5e..5a2475f 100644
--- a/arch/arm/plat-omap/include/plat/iommu.h
+++ b/arch/arm/plat-omap/include/plat/iommu.h

@@ -31,6 +31,7 @@ struct iommu {
 	struct clk	*clk;
 	void __iomem	*regbase;
 	struct device	*dev;
+	void		*fault_cb_priv;
 
 	unsigned int	refcount;
 	struct mutex	iommu_lock;	/* global for this whole object */

@@ -48,6 +49,7 @@ struct iommu {
 	struct mutex		mmap_lock; /* protect mmap */
 
 	int (*isr)(struct iommu *obj);
+	void (*fault_cb)(struct iommu *obj, u32 da, u32 iommu_errs, void *priv);
 
 	void *ctx; /* iommu context: registres saved area */
 	u32 da_start;

@@ -83,7 +85,7 @@ struct iommu_functions {
 	int (*enable)(struct iommu *obj);
 	void (*disable)(struct iommu *obj);
 	void (*set_twl)(struct iommu *obj, bool on);
-	u32 (*fault_isr)(struct iommu *obj, u32 *ra);
+	u32 (*fault_isr)(struct iommu *obj, u32 *ra, u32 *iommu_errs);
 
 	void (*tlb_read_cr)(struct iommu *obj, struct cr_regs *cr);
 	void (*tlb_load_cr)(struct iommu *obj, struct cr_regs *cr);

@@ -109,6 +111,13 @@ struct iommu_platform_data {
 	u32 da_end;
 };
 
+/* IOMMU errors */
+#define OMAP_IOMMU_ERR_TLB_MISS		(1 << 0)
+#define OMAP_IOMMU_ERR_TRANS_FAULT	(1 << 1)
+#define OMAP_IOMMU_ERR_EMU_MISS		(1 << 2)
+#define OMAP_IOMMU_ERR_TBLWALK_FAULT	(1 << 3)
+#define OMAP_IOMMU_ERR_MULTIHIT_FAULT	(1 << 4)
+
 #if defined(CONFIG_ARCH_OMAP1)
 #error "iommu for this processor not implemented yet"
 #else

@@ -161,6 +170,10 @@ extern size_t iopgtable_clear_entry(struct iommu *obj, u32 iova);
 extern int iommu_set_da_range(struct iommu *obj, u32 start, u32 end);
 extern struct iommu *iommu_get(const char *name);
 extern void iommu_put(struct iommu *obj);
+extern int iommu_set_fault_callback(const char *name,
+				    void (*fault_cb)(struct iommu *obj, u32 da,
+						     u32 errs, void *priv),
+				    void *fault_cb_priv);
 
 extern void iommu_save_ctx(struct iommu *obj);
 extern void iommu_restore_ctx(struct iommu *obj);

diff --git a/arch/arm/plat-omap/iommu.c b/arch/arm/plat-omap/iommu.c
index b1107c0..7f780ee 100644
--- a/arch/arm/plat-omap/iommu.c
+++ b/arch/arm/plat-omap/iommu.c

@@ -163,9 +163,9 @@ static u32 get_iopte_attr(struct iotlb_entry *e)
 	return arch_iommu->get_pte_attr(e);
 }
 
-static u32 iommu_report_fault(struct iommu *obj, u32 *da)
+static u32 iommu_report_fault(struct iommu *obj, u32 *da, u32 *iommu_errs)
 {
-	return arch_iommu->fault_isr(obj, da);
+	return arch_iommu->fault_isr(obj, da, iommu_errs);
 }
 
 static void iotlb_lock_get(struct iommu *obj, struct iotlb_lock *l)

@@ -780,7 +780,7 @@ static void iopgtable_clear_entry_all(struct iommu *obj)
  */
 static irqreturn_t iommu_fault_handler(int irq, void *data)
 {
-	u32 stat, da;
+	u32 stat, da, errs;
 	u32 *iopgd, *iopte;
 	int err = -EIO;
 	struct iommu *obj = data;

@@ -796,13 +796,19 @@ static irqreturn_t iommu_fault_handler(int irq, void *data)
 		return IRQ_HANDLED;
 
 	clk_enable(obj->clk);
-	stat = iommu_report_fault(obj, &da);
+	stat = iommu_report_fault(obj, &da, &errs);
 	clk_disable(obj->clk);
 	if (!stat)
 		return IRQ_HANDLED;
 
 	iommu_disable(obj);
 
+	if (obj->fault_cb) {
+		obj->fault_cb(obj, da, errs, obj->fault_cb_priv);
+		/* No need to print error message as callback is called */
+		return IRQ_NONE;
+	}
+
 	iopgd = iopgd_offset(obj, da);
 
 	if (!iopgd_is_table(*iopgd)) {

@@ -917,6 +923,33 @@ void iommu_put(struct iommu *obj)
 }
 EXPORT_SYMBOL_GPL(iommu_put);
 
+int iommu_set_fault_callback(const char *name,
+			     void (*fault_cb)(struct iommu *obj, u32 da,
+					      u32 iommu_errs, void *priv),
+			     void *fault_cb_priv)
+{
+	struct device *dev;
+	struct iommu *obj;
+
+	dev = driver_find_device(&omap_iommu_driver.driver, NULL, (void *)name,
+				 device_match_by_alias);
+	if (!dev)
+		return -ENODEV;
+
+	obj = to_iommu(dev);
+	mutex_lock(&obj->iommu_lock);
+	if (obj->refcount != 0) {
+		mutex_unlock(&obj->iommu_lock);
+		return -EBUSY;
+	}
+	obj->fault_cb = fault_cb;
+	obj->fault_cb_priv = fault_cb_priv;
+	mutex_unlock(&obj->iommu_lock);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(iommu_set_fault_callback);
+
 /*
  *	OMAP Device MMU(IOMMU) detection
  */

-- 
1.7.2.3

A missing prefix in the cover letter's subject. It's: OMAP: IOMMU:

Br,

David

On Tue, Feb 15, 2011 at 3:20 PM, David Cohen [off-list ref] wrote:

Hi,

This patch set adds fault callback support to allow IOMMU users to debug or
react when a fault happens.
IOMMU faults might be very difficult to reproduce and then to figure out
the source of the problem. Currently IOMMU driver prints not so useful
debug message and does not notice user about such issue.
With a fault callback, IOMMU user may debug much more useful information
and/or react to go back to a valid state.

Br,

David
---

David Cohen (2):
?OMAP2+: IOMMU: change OMAP2+ error message to dev_dbg()
?OMAP: IOMMU: add support to callback during fault handling

?arch/arm/mach-omap2/iommu2.c ? ? ? ? ? ?| ? 27 ++++++++++++++++----
?arch/arm/plat-omap/include/plat/iommu.h | ? 15 ++++++++++-
?arch/arm/plat-omap/iommu.c ? ? ? ? ? ? ?| ? 41 ++++++++++++++++++++++++++++---
?3 files changed, 72 insertions(+), 11 deletions(-)

--
1.7.2.3

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at ?http://vger.kernel.org/majordomo-info.html