[PATCH 0/2] IOMMU error message cleanup

[PATCH 0/2] IOMMU error message cleanup

[David Cohen]

From: David Cohen <david@dhcppc2.(none)>

Hi,

OMAP IOMMU prints error messages twice. These patches remove the error
message from the OMAP2,3 specific implementation and let them to be
printed on the above layer only.

Br,

David
---

David Cohen (2):
  OMAP: Add generic IOMMU errors code
  OMAP: Cleanup IOMMU error messages

 arch/arm/mach-omap2/iommu2.c            |   33 +++++++++++++--------------
 arch/arm/plat-omap/include/plat/iommu.h |    9 +++++++-
 arch/arm/plat-omap/iommu.c              |   36 ++++++++++++++++++++----------
 3 files changed, 48 insertions(+), 30 deletions(-)

[PATCH 1/2] OMAP: Add generic IOMMU errors code

[David Cohen]

Generic IOMMU errors code are necessary to handle errors on generic
layer.

Signed-off-by: David Cohen <david.cohen@nokia.com>
---
 arch/arm/plat-omap/include/plat/iommu.h |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/arch/arm/plat-omap/include/plat/iommu.h b/arch/arm/plat-omap/include/plat/iommu.h
index 69230d6..c653fd7 100644
--- a/arch/arm/plat-omap/include/plat/iommu.h
+++ b/arch/arm/plat-omap/include/plat/iommu.h
@@ -109,6 +109,13 @@ struct iommu_platform_data {
 	u32 da_end;
 };
 
+/* IOMMU errors */
+#define IOMMU_ERR_TLB_MISS		(1 << 0)
+#define IOMMU_ERR_TRANS_FAULT		(1 << 1)
+#define IOMMU_ERR_EMU_MISS		(1 << 2)
+#define IOMMU_ERR_TBLWALK_FAULT		(1 << 3)
+#define IOMMU_ERR_MULTIHIT_FAULT	(1 << 4)
+
 #if defined(CONFIG_ARCH_OMAP1)
 #error "iommu for this processor not implemented yet"
 #else
-- 
1.7.1

[PATCH 2/2] OMAP: Cleanup IOMMU error messages

[David Cohen]

IOMMU error messages are duplicated. They're printed on IOMMU specific
layer for OMAP2,3 and once again on the above layer. With this patch,
the error message is printed on the above layer only.

Signed-off-by: David Cohen <david.cohen@nokia.com>
---
 arch/arm/mach-omap2/iommu2.c            |   33 +++++++++++++--------------
 arch/arm/plat-omap/include/plat/iommu.h |    2 +-
 arch/arm/plat-omap/iommu.c              |   36 ++++++++++++++++++++----------
 3 files changed, 41 insertions(+), 30 deletions(-)

diff --git a/arch/arm/mach-omap2/iommu2.c b/arch/arm/mach-omap2/iommu2.c
index 14ee686..bb3d75b 100644
--- a/arch/arm/mach-omap2/iommu2.c
+++ b/arch/arm/mach-omap2/iommu2.c
@@ -143,33 +143,32 @@ 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 errs = 0;
 	u32 stat, da;
-	const char *err_msg[] =	{
-		"tlb miss",
-		"translation fault",
-		"emulation miss",
-		"table walk fault",
-		"multi hit fault",
-	};
 
 	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;
 
-	dev_err(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("\n");
+	if (stat & MMU_IRQ_TLBMISS)
+		errs |= IOMMU_ERR_TLB_MISS;
+	if (stat & MMU_IRQ_TRANSLATIONFAULT)
+		errs |= IOMMU_ERR_TRANS_FAULT;
+	if (stat & MMU_IRQ_EMUMISS)
+		errs |= IOMMU_ERR_EMU_MISS;
+	if (stat & MMU_IRQ_TABLEWALKFAULT)
+		errs |= IOMMU_ERR_TBLWALK_FAULT;
+	if (stat & MMU_IRQ_MULTIHITFAULT)
+		errs |= IOMMU_ERR_MULTIHIT_FAULT;
+	*iommu_errs = errs;
 
 	iommu_write_reg(obj, stat, MMU_IRQSTATUS);
 
diff --git a/arch/arm/plat-omap/include/plat/iommu.h b/arch/arm/plat-omap/include/plat/iommu.h
index c653fd7..267c5b5 100644
--- a/arch/arm/plat-omap/include/plat/iommu.h
+++ b/arch/arm/plat-omap/include/plat/iommu.h
@@ -83,7 +83,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);
diff --git a/arch/arm/plat-omap/iommu.c b/arch/arm/plat-omap/iommu.c
index b1107c0..c7c37a0 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,10 +780,18 @@ static void iopgtable_clear_entry_all(struct iommu *obj)
  */
 static irqreturn_t iommu_fault_handler(int irq, void *data)
 {
-	u32 stat, da;
+	int i;
+	u32 stat, da, errs;
 	u32 *iopgd, *iopte;
 	int err = -EIO;
 	struct iommu *obj = data;
+	const char *err_msg[] =	{
+		"tlb miss",
+		"translation fault",
+		"emulation miss",
+		"table walk fault",
+		"multi hit fault",
+	};
 
 	if (!obj->refcount)
 		return IRQ_NONE;
@@ -796,7 +804,7 @@ 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;
@@ -805,16 +813,20 @@ static irqreturn_t iommu_fault_handler(int irq, void *data)
 
 	iopgd = iopgd_offset(obj, da);
 
-	if (!iopgd_is_table(*iopgd)) {
-		dev_err(obj->dev, "%s: da:%08x pgd:%p *pgd:%08x\n", __func__,
-			da, iopgd, *iopgd);
-		return IRQ_NONE;
+	if (iopgd_is_table(*iopgd)) {
+		iopte = iopte_offset(iopgd, da);
+		dev_err(obj->dev, "da:%08x pgd:%p *pgd:%08x pte:%p *pte:%08x :",
+			da, iopgd, *iopgd, iopte, *iopte);
+	} else {
+		dev_err(obj->dev, "da:%08x pgd:%p *pgd:%08x :", da, iopgd,
+			*iopgd);
 	}
 
-	iopte = iopte_offset(iopgd, da);
-
-	dev_err(obj->dev, "%s: da:%08x pgd:%p *pgd:%08x pte:%p *pte:%08x\n",
-		__func__, da, iopgd, *iopgd, iopte, *iopte);
+	for (i = 0; i < ARRAY_SIZE(err_msg); i++) {
+		if (errs & (1 << i))
+			printk(KERN_CONT " %s", err_msg[i]);
+	}
+	printk("\n");
 
 	return IRQ_NONE;
 }
-- 
1.7.1

Re: [PATCH 2/2] OMAP: Cleanup IOMMU error messages

[Ramirez Luna, Omar]

Hi David,

On Mon, Jan 31, 2011 at 11:25 AM, David Cohen <david.cohen@nokia.com> wrote:
> IOMMU error messages are duplicated. They're printed on IOMMU specific
> layer for OMAP2,3 and once again on the above layer. With this patch,
> the error message is printed on the above layer only.

So, you say they are duplicated, previously the type of fault was
printed in the omap2,3 code and the addresses involving the error
printed on plat code... with this patch both messages are printed on
plat code, I don't see where the duplication/fix is about.

AFAIK, your patch removes this line:

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

Which I assume is the one being printed again in plat code, right?

> Signed-off-by: David Cohen <david.cohen@nokia.com>
> ---
>  arch/arm/mach-omap2/iommu2.c            |   33 +++++++++++++--------------
>  arch/arm/plat-omap/include/plat/iommu.h |    2 +-
>  arch/arm/plat-omap/iommu.c              |   36 ++++++++++++++++++++----------
>  3 files changed, 41 insertions(+), 30 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/iommu2.c b/arch/arm/mach-omap2/iommu2.c
> index 14ee686..bb3d75b 100644
> --- a/arch/arm/mach-omap2/iommu2.c
> +++ b/arch/arm/mach-omap2/iommu2.c
> @@ -143,33 +143,32 @@ 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 errs = 0;
>        u32 stat, da;
> -       const char *err_msg[] = {
> -               "tlb miss",
> -               "translation fault",
> -               "emulation miss",
> -               "table walk fault",
> -               "multi hit fault",
> -       };

AFAIU, this code living in mach-omap2, means that this errors are
common for omap2,3,4 which they are. Does moving them to plat code
implies that all omap platforms expect these errors?

>        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;
>
> -       dev_err(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("\n");
> +       if (stat & MMU_IRQ_TLBMISS)
> +               errs |= IOMMU_ERR_TLB_MISS;
> +       if (stat & MMU_IRQ_TRANSLATIONFAULT)
> +               errs |= IOMMU_ERR_TRANS_FAULT;
> +       if (stat & MMU_IRQ_EMUMISS)
> +               errs |= IOMMU_ERR_EMU_MISS;
> +       if (stat & MMU_IRQ_TABLEWALKFAULT)
> +               errs |= IOMMU_ERR_TBLWALK_FAULT;
> +       if (stat & MMU_IRQ_MULTIHITFAULT)
> +               errs |= IOMMU_ERR_MULTIHIT_FAULT;
> +       *iommu_errs = errs;

Is there any point in doing this?

Basically: *iommu_errs = stat, given that the mask values and the
error defines are the same for each case.

Besides I haven't seen two errors trigger a single interrupt.

...
>
> -       iopte = iopte_offset(iopgd, da);
> -
> -       dev_err(obj->dev, "%s: da:%08x pgd:%p *pgd:%08x pte:%p *pte:%08x\n",
> -               __func__, da, iopgd, *iopgd, iopte, *iopte);
> +       for (i = 0; i < ARRAY_SIZE(err_msg); i++) {
> +               if (errs & (1 << i))
> +                       printk(KERN_CONT " %s", err_msg[i]);
> +       }
> +       printk("\n");

I think we should get rid of the "printks".

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

Re: [PATCH 2/2] OMAP: Cleanup IOMMU error messages

[David Cohen]

On Mon, Jan 31, 2011 at 09:16:30PM -0600, ext Ramirez Luna, Omar wrote:
> Hi David,

Hi Ramirez,

Thanks for the comments.

> 
> On Mon, Jan 31, 2011 at 11:25 AM, David Cohen <david.cohen@nokia.com> wrote:
> > IOMMU error messages are duplicated. They're printed on IOMMU specific
> > layer for OMAP2,3 and once again on the above layer. With this patch,
> > the error message is printed on the above layer only.
> 
> So, you say they are duplicated, previously the type of fault was
> printed in the omap2,3 code and the addresses involving the error
> printed on plat code... with this patch both messages are printed on
> plat code, I don't see where the duplication/fix is about.
> 

A little bit of my context:

I'm working on OMAP3ISP driver. Due to hw or sw issues, iommu errors
(mainly iommu faults) are not so rare to happen. e.g. I have 2 hw issues
being workarounded on ISP H3A submodule (one of those a bit serious)
which may lead to iommu faults.
With the current implementation, the error messages are a bit useless
and much "heavy". For each "fault", 2 error lines are printed to show the
same error. On ISP context, usually it will happen *many* times in a
row, making the device not usable even to debug the issue. And the bad
side is we cannot get anything out of those messages which could help to
point out where is the problem. As sometimes it's difficult to reproduce it,
we loose a good chance to show a bit more useful data about the triggered
error.

My main intentions are:
 - Get rid of one line.
 - TODO: Print something useful to let developers to *know* what is
   going on.
 - TODO: Avoid to flood console to let it able to debug.

> AFAIK, your patch removes this line:
> 
>  -       dev_err(obj->dev, "%s:\tda:%08x ", __func__, da);
> 
> Which I assume is the one being printed again in plat code, right?

Yes.

> 
> > Signed-off-by: David Cohen <david.cohen@nokia.com>
> > ---
> >  arch/arm/mach-omap2/iommu2.c            |   33 +++++++++++++--------------
> >  arch/arm/plat-omap/include/plat/iommu.h |    2 +-
> >  arch/arm/plat-omap/iommu.c              |   36 ++++++++++++++++++++----------
> >  3 files changed, 41 insertions(+), 30 deletions(-)
> >
> > diff --git a/arch/arm/mach-omap2/iommu2.c b/arch/arm/mach-omap2/iommu2.c
> > index 14ee686..bb3d75b 100644
> > --- a/arch/arm/mach-omap2/iommu2.c
> > +++ b/arch/arm/mach-omap2/iommu2.c
> > @@ -143,33 +143,32 @@ 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 errs = 0;
> >        u32 stat, da;
> > -       const char *err_msg[] = {
> > -               "tlb miss",
> > -               "translation fault",
> > -               "emulation miss",
> > -               "table walk fault",
> > -               "multi hit fault",
> > -       };
> 
> AFAIU, this code living in mach-omap2, means that this errors are
> common for omap2,3,4 which they are. Does moving them to plat code
> implies that all omap platforms expect these errors?

That's true. I can work on a different approach.

> 
> >        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;
> >
> > -       dev_err(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("\n");
> > +       if (stat & MMU_IRQ_TLBMISS)
> > +               errs |= IOMMU_ERR_TLB_MISS;
> > +       if (stat & MMU_IRQ_TRANSLATIONFAULT)
> > +               errs |= IOMMU_ERR_TRANS_FAULT;
> > +       if (stat & MMU_IRQ_EMUMISS)
> > +               errs |= IOMMU_ERR_EMU_MISS;
> > +       if (stat & MMU_IRQ_TABLEWALKFAULT)
> > +               errs |= IOMMU_ERR_TBLWALK_FAULT;
> > +       if (stat & MMU_IRQ_MULTIHITFAULT)
> > +               errs |= IOMMU_ERR_MULTIHIT_FAULT;
> > +       *iommu_errs = errs;
> 
> Is there any point in doing this?
> 
> Basically: *iommu_errs = stat, given that the mask values and the
> error defines are the same for each case.

True. But can I assume a specific register value will always match to
the "generic" layer?

> 
> Besides I haven't seen two errors trigger a single interrupt.

Me neither, but the current implementation assumes it can happen and my
concern is to not change it here.

> 
> ...
> >
> > -       iopte = iopte_offset(iopgd, da);
> > -
> > -       dev_err(obj->dev, "%s: da:%08x pgd:%p *pgd:%08x pte:%p *pte:%08x\n",
> > -               __func__, da, iopgd, *iopgd, iopte, *iopte);
> > +       for (i = 0; i < ARRAY_SIZE(err_msg); i++) {
> > +               if (errs & (1 << i))
> > +                       printk(KERN_CONT " %s", err_msg[i]);
> > +       }
> > +       printk("\n");
> 
> I think we should get rid of the "printks".

By working on a different approach for the printed messages, yes.

Regards,

David

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

Re: [PATCH 0/2] IOMMU error message cleanup

[Tony Lindgren]

Hi David,

* David Cohen <david.cohen@nokia.com> [110131 09:24]:
> From: David Cohen <david@dhcppc2.(none)>
> 
> Hi,
> 
> OMAP IOMMU prints error messages twice. These patches remove the error
> message from the OMAP2,3 specific implementation and let them to be
> printed on the above layer only.

Can you also please Cc linux-arm-kernel list when sending arch/arm/*omap*/
patches? That way I don't need to repost them before applying.

Regards,

Tony