[PATCH 0/2] IOMMU fault callback support

[PATCH 0/2] IOMMU fault callback support

[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

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

[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 <dacohen@gmail.com>
---
 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

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

[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 <dacohen@gmail.com>
---
 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

[PATCH 0/2] IOMMU fault callback support

[David Cohen]

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 <dacohen@gmail.com> 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
>

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

[Sergei Shtylyov]

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.

> Signed-off-by: David Cohen<dacohen@gmail.com>
> ---
>   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...

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

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

[David Cohen]

On Tue, Feb 15, 2011 at 3:38 PM, Sergei Shtylyov <sshtylyov@mvista.com> wrote:
> Hello.

Hi,

>
> 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.
>
>> Signed-off-by: David Cohen<dacohen@gmail.com>
>> ---
>> ?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.

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

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

Regards,

David

>
> WBR, Sergei
>

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

[Sergei Shtylyov]

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

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

>>> Signed-off-by: David Cohen<dacohen@gmail.com>
>>> ---
>>>   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.

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

> 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

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

[Jarkko Nikula]

On Tue, 15 Feb 2011 15:44:27 +0200
David Cohen <dacohen@gmail.com> wrote:

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

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

[David Cohen]

On Tue, Feb 15, 2011 at 3:56 PM, Sergei Shtylyov <sshtylyov@mvista.com> wrote:
> On 15-02-2011 16:44, David Cohen wrote:
>
>>>> IOMMU upper layer is already printing error message. OMAP2+ specific
>>>> layer may print error message only for debug purpose.
>
>>>> Signed-off-by: David Cohen<dacohen@gmail.com>
>>>> ---
>>>> ?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.
>
>>>> ? ? ? ?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.
>
>> 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

>
>> Regards,
>
>> David
>
> WBR, Sergei
>

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

[David Cohen]

On Tue, Feb 15, 2011 at 3:59 PM, Jarkko Nikula <jhnikula@gmail.com> wrote:
> On Tue, 15 Feb 2011 15:44:27 +0200
> David Cohen <dacohen@gmail.com> wrote:
>
>> >> @@ -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
>

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

[David Cohen]

On Tue, Feb 15, 2011 at 4:08 PM, David Cohen <dacohen@gmail.com> wrote:
> On Tue, Feb 15, 2011 at 3:59 PM, Jarkko Nikula <jhnikula@gmail.com> wrote:
>> On Tue, 15 Feb 2011 15:44:27 +0200
>> David Cohen <dacohen@gmail.com> wrote:
>>
>>> >> @@ -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
>
>>
>> Just my 2 cents.
>>
>> --
>> Jarkko
>>
>

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

[Russell King - ARM Linux]

On Tue, Feb 15, 2011 at 04:38:32PM +0300, Sergei Shtylyov wrote:
>> 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...
>
>>
>>   	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.

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

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

[Jarkko Nikula]

On Tue, 15 Feb 2011 16:08:32 +0200
David Cohen <dacohen@gmail.com> wrote:

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

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

[David Cohen]

On Tue, Feb 15, 2011 at 4:29 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Tue, Feb 15, 2011 at 04:38:32PM +0300, Sergei Shtylyov wrote:
>>> 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...
>>
>>>
>>> ? ? ?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?

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

>

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

[Russell King - ARM Linux]

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.

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

[David Cohen]

On Tue, Feb 15, 2011 at 4:44 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> 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.

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

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

[Russell King - ARM Linux]

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.