IOMMU fault callback support

PATCH New David Cohen 4 participants 17 messages Feb 15, 2011

Subsystems (3)

Arm (32-bit) Arm sub-architectures OMAP2+ support

lore

Share & tools

lore

David Cohen

author

2011-02-15 13:15 UTC #0 lore raw

cover 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(-)

Signature · 1 line

1.7.2.3

David Cohen

author

2011-02-15 13:15 UTC #1 lore raw

PATCH 1/2 OMAP2+: IOMMU: change OMAP2+ error message to dev_dbg()

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

Signed-off-by David Cohen <[email protected]>

---
 arch/arm/mach-omap2/iommu2.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

arch/arm/mach-omap2/iommu2.c +3 -3

index 14ee686..4244a07 100644
@@ -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);

Signature · 1 line

1.7.2.3

Sergei Shtylyov

2011-02-15 13:38 UTC #2 lore raw

PATCH 1/2 OMAP2+: IOMMU: change OMAP2+ error message to dev_dbg()

Hello.

Quoted 13 lines · David Cohen

IOMMU upper layer is already printing error message. OMAP2+ specific layer may print error message only for debug purpose.
Signed-off-by David Cohen<[email protected]>
---
  arch/arm/mach-omap2/iommu2.c |    6 +++---
  1 files changed, 3 insertions(+), 3 deletions(-)
arch/arm/mach-omap2/iommu2.c +0 -0
index 14ee686..4244a07 100644

arch/arm/mach-omap2/iommu2.c static u32 omap2_iommu_fault_isr(struct iommu *obj, u32 *ra) · line 166

Quoted 5 lines

arch/arm/mach-omap2/iommu2.c +1 -1

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

arch/arm/mach-omap2/iommu2.c static u32 omap2_iommu_fault_isr(struct iommu *obj, u32 *ra) · line 170

Quoted 5 lines

arch/arm/mach-omap2/iommu2.c +1 -1

  	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.

arch/arm/mach-omap2/iommu2.c static u32 omap2_iommu_fault_isr(struct iommu *obj, u32 *ra) · line 172

Quoted 3 lines

arch/arm/mach-omap2/iommu2.c +1 -1
  	}
-	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

David Cohen

author

2011-02-15 13:44 UTC #3 lore raw

PATCH 1/2 OMAP2+: IOMMU: change OMAP2+ error message to dev_dbg()

Quoted 1 line · Sergei Shtylyov

Hello.

Hi,

Quoted 20 lines · David Cohen

Quoted 18 lines
IOMMU upper layer is already printing error message. OMAP2+ specific layer may print error message only for debug purpose.
Signed-off-by David Cohen<[email protected]>
---
?arch/arm/mach-omap2/iommu2.c | ? ?6 +++---
?1 files changed, 3 insertions(+), 3 deletions(-)
arch/arm/mach-omap2/iommu2.c +0 -0
index 14ee686..4244a07 100644
@@ -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;
 

Quoted 5 lines

arch/arm/mach-omap2/iommu2.c static u32 omap2_iommu_fault_isr(struct iommu *obj, · line 167
Quoted 2 lines
arch/arm/mach-omap2/iommu2.c +1 -1
- ? ? ? 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 10 lines

arch/arm/mach-omap2/iommu2.c static u32 omap2_iommu_fault_isr(struct iommu *obj, · line 170
Quoted 6 lines
arch/arm/mach-omap2/iommu2.c +1 -1
 ? ? ? ?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 6 lines

arch/arm/mach-omap2/iommu2.c static u32 omap2_iommu_fault_isr(struct iommu *obj, · line 172
Quoted 3 lines
arch/arm/mach-omap2/iommu2.c +1 -1
 ? ? ? ?}
- ? ? ? 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

Quoted 3 lines

WBR, Sergei

Sergei Shtylyov

2011-02-15 13:56 UTC #4 lore raw

PATCH 1/2 OMAP2+: IOMMU: change OMAP2+ error message to dev_dbg()

Quoted 33 lines · David Cohen

Quoted 17 lines
Quoted 17 lines
IOMMU upper layer is already printing error message. OMAP2+ specific layer may print error message only for debug purpose.
Signed-off-by David Cohen<[email protected]>
---
  arch/arm/mach-omap2/iommu2.c |    6 +++---
  1 files changed, 3 insertions(+), 3 deletions(-)
arch/arm/mach-omap2/iommu2.c +0 -0
index 14ee686..4244a07 100644
@@ -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;
 
Quoted 5 lines
arch/arm/mach-omap2/iommu2.c static u32 omap2_iommu_fault_isr(struct iommu *obj, u32 *ra) · line 166
Quoted 2 lines
arch/arm/mach-omap2/iommu2.c +1 -1
-       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 7 lines
arch/arm/mach-omap2/iommu2.c static u32 omap2_iommu_fault_isr(struct iommu *obj, u32 *ra) · line 170
Quoted 4 lines
arch/arm/mach-omap2/iommu2.c +1 -1
        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 8 lines

Quoted 6 lines
arch/arm/mach-omap2/iommu2.c static u32 omap2_iommu_fault_isr(struct iommu *obj, u32 *ra) · line 172
Quoted 3 lines
arch/arm/mach-omap2/iommu2.c +1 -1
        }
-       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.

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.

Quoted 3 lines

Regards,
David

WBR, Sergei

David Cohen

author

2011-02-15 14:01 UTC #5 lore raw

PATCH 1/2 OMAP2+: IOMMU: change OMAP2+ error message to dev_dbg()

Quoted 43 lines · Sergei Shtylyov

Quoted 40 lines · David Cohen
Quoted 18 lines
Quoted 18 lines
IOMMU upper layer is already printing error message. OMAP2+ specific layer may print error message only for debug purpose.
Signed-off-by David Cohen<[email protected]>
---
?arch/arm/mach-omap2/iommu2.c | ? ?6 +++---
?1 files changed, 3 insertions(+), 3 deletions(-)
arch/arm/mach-omap2/iommu2.c +0 -0
index 14ee686..4244a07 100644
@@ -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;
 
Quoted 5 lines
arch/arm/mach-omap2/iommu2.c static u32 omap2_iommu_fault_isr(struct iommu · line 168
Quoted 2 lines
arch/arm/mach-omap2/iommu2.c +1 -1
- ? ? ? 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 14 lines
arch/arm/mach-omap2/iommu2.c static u32 omap2_iommu_fault_isr(struct iommu · line 171
Quoted 4 lines
arch/arm/mach-omap2/iommu2.c +1 -1
 ? ? ? ?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.
arch/arm/mach-omap2/iommu2.c static u32 omap2_iommu_fault_isr(struct iommu · line 173
Quoted 3 lines
arch/arm/mach-omap2/iommu2.c +1 -1
 ? ? ? ?}
- ? ? ? 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.

Quoted 6 lines

Quoted 1 line
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 7 lines

Quoted 4 lines
Regards,
David
WBR, Sergei

Jarkko Nikula

2011-02-15 13:59 UTC #6 lore raw

PATCH 1/2 OMAP2+: IOMMU: change OMAP2+ error message to dev_dbg()

In reply to David Cohen

Quoted 5 lines · On Tue, 15 Feb 2011 15:44:27 +0200 David Cohen <[email protected]> wrote:

Quoted 5 lines

arch/arm/mach-omap2/iommu2.c +0 -0

@@ -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;

Quoted 8 lines

Quoted 5 lines
arch/arm/mach-omap2/iommu2.c static u32 omap2_iommu_fault_isr(struct iommu *obj, u32 *ra) · line 166
Quoted 2 lines
arch/arm/mach-omap2/iommu2.c +1 -1
-       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.

Signature · 5 lines

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

David Cohen

author

2011-02-15 14:08 UTC #7 lore raw

PATCH 1/2 OMAP2+: IOMMU: change OMAP2+ error message to dev_dbg()

Quoted 14 lines · Jarkko Nikula

Quoted 11 lines · On Tue, 15 Feb 2011 15:44:27 +0200 David Cohen <[email protected]> wrote:
Quoted 10 lines
Review static u32 omap2_iommu_fault_isr(struct iommu *obj, · line 167
Quoted 7 lines
(quoted diff) +1 -1
@@ -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 5 lines

Quoted 2 lines
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

Quoted 6 lines

Just my 2 cents.
--
Jarkko

David Cohen

author

2011-02-15 14:21 UTC #8 lore raw

PATCH 1/2 OMAP2+: IOMMU: change OMAP2+ error message to dev_dbg()

Quoted 30 lines · David Cohen

Quoted 14 lines · Jarkko Nikula
Quoted 11 lines · On Tue, 15 Feb 2011 15:44:27 +0200 David Cohen <[email protected]> wrote:
Quoted 10 lines
Review static u32 omap2_iommu_fault_isr(struct iommu *obj, · line 167
Quoted 7 lines
(quoted diff) +1 -1
@@ -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 5 lines
Quoted 2 lines
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.

Quoted 2 lines

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

Quoted 12 lines

Regards,
David
Quoted 6 lines
Just my 2 cents.
--
Jarkko

Jarkko Nikula

2011-02-15 14:30 UTC #9 lore raw

PATCH 1/2 OMAP2+: IOMMU: change OMAP2+ error message to dev_dbg()

In reply to David Cohen

Quoted 13 lines · On Tue, 15 Feb 2011 16:08:32 +0200 David Cohen <[email protected]> wrote:

Quoted 3 lines
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.

Quoted 3 lines

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.

Signature · 1 line

Jarkko

Russell King - ARM Linux

2011-02-15 14:29 UTC #10 lore raw

PATCH 1/2 OMAP2+: IOMMU: change OMAP2+ error message to dev_dbg()

In reply to Sergei Shtylyov

Quoted 15 lines · Sergei Shtylyov

arch/arm/mach-omap2/iommu2.c static u32 omap2_iommu_fault_isr(struct iommu *obj, u32 *ra) · line 166
Quoted 10 lines
arch/arm/mach-omap2/iommu2.c +1 -1
index 14ee686..4244a07 100644
@@ -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 1 line

Quoted 6 lines

arch/arm/mach-omap2/iommu2.c static u32 omap2_iommu_fault_isr(struct iommu *obj, u32 *ra) · line 170
Quoted 4 lines
  	for (i = 0; i<  ARRAY_SIZE(err_msg); i++) {
  		if (stat & (1<<  i))
arch/arm/mach-omap2/iommu2.c +1 -1
-			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 6 lines

arch/arm/mach-omap2/iommu2.c static u32 omap2_iommu_fault_isr(struct iommu *obj, u32 *ra) · line 172
Quoted 3 lines
  	}
arch/arm/mach-omap2/iommu2.c +1 -1
-	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.

David Cohen

author

2011-02-15 14:36 UTC #11 lore raw

PATCH 1/2 OMAP2+: IOMMU: change OMAP2+ error message to dev_dbg()

Quoted 16 lines · Russell King - ARM Linux

Quoted 15 lines · Sergei Shtylyov
arch/arm/mach-omap2/iommu2.c static u32 omap2_iommu_fault_isr(struct iommu *obj, u32 *ra) · line 166
Quoted 10 lines
arch/arm/mach-omap2/iommu2.c +1 -1
index 14ee686..4244a07 100644
@@ -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 1 line

Quoted 8 lines

Quoted 6 lines
Quoted 4 lines
? ? ?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 10 lines

Quoted 7 lines
Quoted 4 lines
? ? ?}
- ? ?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

Quoted 1 line

Russell King - ARM Linux

2011-02-15 14:44 UTC #12 lore raw

PATCH 1/2 OMAP2+: IOMMU: change OMAP2+ error message to dev_dbg()

Quoted 1 line · David Cohen

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.

David Cohen

author

2011-02-15 14:50 UTC #13 lore raw

PATCH 1/2 OMAP2+: IOMMU: change OMAP2+ error message to dev_dbg()

Quoted 13 lines · Russell King - ARM Linux

Quoted 1 line · David Cohen
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]);

Quoted 8 lines

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

Russell King - ARM Linux

2011-02-15 15:51 UTC #14 lore raw

PATCH 1/2 OMAP2+: IOMMU: change OMAP2+ error message to dev_dbg()

Quoted 3 lines · David Cohen

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.

David Cohen

author

2011-02-15 13:16 UTC #15 lore raw

PATCH 2/2 OMAP: IOMMU: add support to callback during fault handling

In reply to David Cohen

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 <[email protected]>

---
 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(-)

arch/arm/mach-omap2/iommu2.c +18 -3

index 4244a07..559a066 100644
@@ -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;

arch/arm/plat-omap/include/plat/iommu.h +14 -1

index 19cbb5e..5a2475f 100644
@@ -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);

arch/arm/plat-omap/iommu.c +37 -4

72 lines not on this page · raw message

Signature · 1 line

1.7.2.3

David Cohen

author

2011-02-15 13:32 UTC #16 lore raw

cover IOMMU fault callback support

In reply to David Cohen

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

Br,

David

Quoted 32 lines · David Cohen

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