OMAP: IOMMU fault callback support

PATCH New David Cohen 7 participants 31 messages Feb 16, 2011 – Feb 24, 2011

Subsystems (3)

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

lore

Share & tools

lore

David Cohen

author

2011-02-16 19:30 UTC #0 lore raw

v3 cover OMAP: IOMMU fault callback support

Hi,

This patch set adapts current (*isr)() to be used as fault callback. 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: don't print fault warning on specific layer
OMAP: IOMMU: add support to callback during fault handling

 arch/arm/mach-omap2/iommu2.c            |   33 +++++++++-----------
 arch/arm/plat-omap/include/plat/iommu.h |   14 ++++++++-
 arch/arm/plat-omap/iommu.c              |   52 ++++++++++++++++++++++---------
 3 files changed, 65 insertions(+), 34 deletions(-)

Signature · 1 line

1.7.2.3

David Cohen

author

2011-02-16 19:31 UTC #1 lore raw

PATCH v3 1/2 OMAP2+: IOMMU: don't print fault warning on specific layer

IOMMU upper layer and user are responsible to handle a fault and to define whether it will end up as an error or not. OMAP2+ specific layer should not print anything in such case.

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

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

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

index 14ee686..49a1e5e 100644
@@ -145,15 +145,7 @@ static void omap2_iommu_set_twl(struct iommu *obj, bool on)
 
 static u32 omap2_iommu_fault_isr(struct iommu *obj, u32 *ra)
 {
-	int i;
 	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;

@@ -163,14 +155,6 @@ 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);
-
-	for (i = 0; i < ARRAY_SIZE(err_msg); i++) {
-		if (stat & (1 << i))
-			printk("%s ", err_msg[i]);
-	}
-	printk("\n");
-
 	iommu_write_reg(obj, stat, MMU_IRQSTATUS);
 
 	return stat;

Signature · 1 line

1.7.2.3

David Cohen

author

2011-02-16 19:31 UTC #2 lore raw

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

In reply to David Cohen

Add support to register an isr for IOMMU fault situations and adapt it to allow such (*isr)() to be used as fault callback. Drivers using IOMMU module might want to be informed when errors happen in order to debug it or react.

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

---
 arch/arm/mach-omap2/iommu2.c            |   17 +++++++++-
 arch/arm/plat-omap/include/plat/iommu.h |   14 ++++++++-
 arch/arm/plat-omap/iommu.c              |   52 ++++++++++++++++++++++---------
 3 files changed, 65 insertions(+), 18 deletions(-)

arch/arm/mach-omap2/iommu2.c +15 -2

index 49a1e5e..adb083e 100644
@@ -146,18 +146,31 @@ static void omap2_iommu_set_twl(struct iommu *obj, bool on)
 static u32 omap2_iommu_fault_isr(struct iommu *obj, u32 *ra)
 {
 	u32 stat, da;
+	u32 errs = 0;
 
 	stat = iommu_read_reg(obj, MMU_IRQSTATUS);
 	stat &= MMU_IRQ_MASK;
-	if (!stat)
+	if (!stat) {
+		*ra = 0;
 		return 0;
+	}
 
 	da = iommu_read_reg(obj, MMU_FAULT_AD);
 	*ra = da;
 
+	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_write_reg(obj, stat, MMU_IRQSTATUS);
 
-	return stat;
+	return errs;
 }
 
 static void omap2_tlb_read_cr(struct iommu *obj, struct cr_regs *cr)

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

index 19cbb5e..174f1b9 100644
@@ -31,6 +31,7 @@ struct iommu {
 	struct clk	*clk;
 	void __iomem	*regbase;
 	struct device	*dev;
+	void		*isr_priv;
 
 	unsigned int	refcount;
 	struct mutex	iommu_lock;	/* global for this whole object */

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

@@ -109,6 +110,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 +169,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_isr(const char *name,
+			 int (*isr)(struct iommu *obj, u32 da, u32 iommu_errs,
+				    void *priv),
+			 void *isr_priv);
 
 extern void iommu_save_ctx(struct iommu *obj);
 extern void iommu_restore_ctx(struct iommu *obj);

arch/arm/plat-omap/iommu.c +37 ~~-15~~

83 lines not on this page · raw message

Signature · 1 line

1.7.2.3

Hiroshi DOYU

2011-02-21 08:18 UTC #3 lore raw

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

From: David Cohen <[email protected]>
Subject: [PATCH v3 2/2] OMAP: IOMMU: add support to callback during fault handling
Date: Wed, 16 Feb 2011 21:35:51 +0200

Quoted 55 lines

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

---
 arch/arm/mach-omap2/iommu2.c            |   17 +++++++++-
 arch/arm/plat-omap/include/plat/iommu.h |   14 ++++++++-
 arch/arm/plat-omap/iommu.c              |   52 ++++++++++++++++++++++---------
 3 files changed, 65 insertions(+), 18 deletions(-)

arch/arm/mach-omap2/iommu2.c +15 -2

index 49a1e5e..adb083e 100644
@@ -146,18 +146,31 @@ static void omap2_iommu_set_twl(struct iommu *obj, bool on)
 static u32 omap2_iommu_fault_isr(struct iommu *obj, u32 *ra)
 {
 	u32 stat, da;
+	u32 errs = 0;
 
 	stat = iommu_read_reg(obj, MMU_IRQSTATUS);
 	stat &= MMU_IRQ_MASK;
-	if (!stat)
+	if (!stat) {
+		*ra = 0;
 		return 0;
+	}
 
 	da = iommu_read_reg(obj, MMU_FAULT_AD);
 	*ra = da;
 
+	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_write_reg(obj, stat, MMU_IRQSTATUS);
 
-	return stat;
+	return errs;
 }
 
 static void omap2_tlb_read_cr(struct iommu *obj, struct cr_regs *cr)

arch/arm/plat-omap/include/plat/iommu.h +0 -0

index 19cbb5e..174f1b9 100644

arch/arm/plat-omap/include/plat/iommu.h struct iommu { · line 34

Quoted 4 lines

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

@@ -31,6 +31,7 @@ struct iommu {
 	struct clk	*clk;
 	void __iomem	*regbase;
 	struct device	*dev;
+	void		*isr_priv;

Ideally I'd like to avoid having "isr_priv" in iommu since it's not used for iommu but client needs the place to pass its info to its custom handler. Any better idea?

Quoted 129 lines

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

 	unsigned int	refcount;
 	struct mutex	iommu_lock;	/* global for this whole object */

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

@@ -109,6 +110,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 +169,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_isr(const char *name,
+			 int (*isr)(struct iommu *obj, u32 da, u32 iommu_errs,
+				    void *priv),
+			 void *isr_priv);
 
 extern void iommu_save_ctx(struct iommu *obj);
 extern void iommu_restore_ctx(struct iommu *obj);

arch/arm/plat-omap/iommu.c +37 ~~-15~~

index f55f458..b0e0efc 100644
@@ -780,25 +780,19 @@ static void iopgtable_clear_entry_all(struct iommu *obj)
  */
 static irqreturn_t iommu_fault_handler(int irq, void *data)
 {
-	u32 stat, da;
+	u32 da, errs;
 	u32 *iopgd, *iopte;
-	int err = -EIO;
 	struct iommu *obj = data;
 
 	if (!obj->refcount)
 		return IRQ_NONE;
 
-	/* Dynamic loading TLB or PTE */
-	if (obj->isr)
-		err = obj->isr(obj);
-
-	if (!err)
-		return IRQ_HANDLED;
-
 	clk_enable(obj->clk);
-	stat = iommu_report_fault(obj, &da);
+	errs = iommu_report_fault(obj, &da);
 	clk_disable(obj->clk);
-	if (!stat)
+
+	/* Fault callback or TLB/PTE Dynamic loading */
+	if (obj->isr && !obj->isr(obj, da, errs, obj->isr_priv))
 		return IRQ_HANDLED;
 
 	iommu_disable(obj);

@@ -806,15 +800,16 @@ 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", obj->name,
-			da, iopgd, *iopgd);
+		dev_err(obj->dev, "%s: errs:0x%08x da:0x%08x pgd:0x%p "
+			"*pgd:px%08x\n", obj->name, errs, da, iopgd, *iopgd);
 		return IRQ_NONE;
 	}
 
 	iopte = iopte_offset(iopgd, da);
 
-	dev_err(obj->dev, "%s: da:%08x pgd:%p *pgd:%08x pte:%p *pte:%08x\n",
-		obj->name, da, iopgd, *iopgd, iopte, *iopte);
+	dev_err(obj->dev, "%s: errs:0x%08x da:0x%08x pgd:0x%p *pgd:0x%08x "
+		"pte:0x%p *pte:0x%08x\n", obj->name, errs, da, iopgd, *iopgd,
+		iopte, *iopte);
 
 	return IRQ_NONE;
 }

@@ -917,6 +912,33 @@ void iommu_put(struct iommu *obj)
 }
 EXPORT_SYMBOL_GPL(iommu_put);
 
+int iommu_set_isr(const char *name,
+		  int (*isr)(struct iommu *obj, u32 da, u32 iommu_errs,
+			     void *priv),
+		  void *isr_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->isr = isr;
+	obj->isr_priv = isr_priv;
+	mutex_unlock(&obj->iommu_lock);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(iommu_set_isr);
+
 /*
  *	OMAP Device MMU(IOMMU) detection
  */

Signature · 1 line

1.7.2.3

Felipe Balbi

2011-02-21 08:22 UTC #4 lore raw

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

Hi,

Quoted 53 lines · Hiroshi DOYU

From: David Cohen <[email protected]>
Subject: [PATCH v3 2/2] OMAP: IOMMU: add support to callback during fault handling
Date: Wed, 16 Feb 2011 21:35:51 +0200

Quoted 49 lines

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

---
 arch/arm/mach-omap2/iommu2.c            |   17 +++++++++-
 arch/arm/plat-omap/include/plat/iommu.h |   14 ++++++++-
 arch/arm/plat-omap/iommu.c              |   52 ++++++++++++++++++++++---------
 3 files changed, 65 insertions(+), 18 deletions(-)

arch/arm/mach-omap2/iommu2.c +15 -2

index 49a1e5e..adb083e 100644
@@ -146,18 +146,31 @@ static void omap2_iommu_set_twl(struct iommu *obj, bool on)
 static u32 omap2_iommu_fault_isr(struct iommu *obj, u32 *ra)
 {
 	u32 stat, da;
+	u32 errs = 0;
 
 	stat = iommu_read_reg(obj, MMU_IRQSTATUS);
 	stat &= MMU_IRQ_MASK;
-	if (!stat)
+	if (!stat) {
+		*ra = 0;
 		return 0;
+	}
 
 	da = iommu_read_reg(obj, MMU_FAULT_AD);
 	*ra = da;
 
+	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_write_reg(obj, stat, MMU_IRQSTATUS);
 
-	return stat;
+	return errs;
 }

Quoted 14 lines

arch/arm/plat-omap/include/plat/iommu.h struct iommu { · line 34
Quoted 10 lines
arch/arm/mach-omap2/iommu2.c +0 -0
 static void omap2_tlb_read_cr(struct iommu *obj, struct cr_regs *cr)
arch/arm/plat-omap/include/plat/iommu.h +1 -0
index 19cbb5e..174f1b9 100644
@@ -31,6 +31,7 @@ struct iommu {
 	struct clk	*clk;
 	void __iomem	*regbase;
 	struct device	*dev;
+	void		*isr_priv;
Ideally I'd like to avoid having "isr_priv" in iommu since it's not used for iommu but client needs the place to pass its info to its custom handler. Any better idea?

I'm not sure if it makes sense as I don't know the mailbox block, but maybe moving to GENIRQ ? Then IRQ subsystem would take care of the "dev_id"

Signature · 1 line

balbi

David Cohen

author

2011-02-21 08:57 UTC #5 lore raw

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

Quoted 71 lines · Felipe Balbi

Hi,
Quoted 53 lines · Hiroshi DOYU
From: David Cohen <[email protected]>
Subject: [PATCH v3 2/2] OMAP: IOMMU: add support to callback during fault handling
Date: Wed, 16 Feb 2011 21:35:51 +0200
Quoted 49 lines
Add support to register an isr for IOMMU fault situations and adapt it to allow such (*isr)() to be used as fault callback. Drivers using IOMMU module might want to be informed when errors happen in order to debug it or react.
Signed-off-by David Cohen <[email protected]>
---
?arch/arm/mach-omap2/iommu2.c ? ? ? ? ? ?| ? 17 +++++++++-
?arch/arm/plat-omap/include/plat/iommu.h | ? 14 ++++++++-
?arch/arm/plat-omap/iommu.c ? ? ? ? ? ? ?| ? 52 ++++++++++++++++++++++---------
?3 files changed, 65 insertions(+), 18 deletions(-)
arch/arm/mach-omap2/iommu2.c +15 -2
index 49a1e5e..adb083e 100644
@@ -146,18 +146,31 @@ static void omap2_iommu_set_twl(struct iommu *obj, bool on)
 ?static u32 omap2_iommu_fault_isr(struct iommu *obj, u32 *ra)
 ?{
 ? ? u32 stat, da;
+ ? u32 errs = 0;
 
 ? ? stat = iommu_read_reg(obj, MMU_IRQSTATUS);
 ? ? stat &= MMU_IRQ_MASK;
- ? if (!stat)
+ ? if (!stat) {
+ ? ? ? ? ? *ra = 0;
 ? ? ? ? ? ? return 0;
+ ? }
 
 ? ? da = iommu_read_reg(obj, MMU_FAULT_AD);
 ? ? *ra = da;
 
+ ? 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_write_reg(obj, stat, MMU_IRQSTATUS);
 
- ? return stat;
+ ? return errs;
 ?}
 
Quoted 14 lines
arch/arm/plat-omap/include/plat/iommu.h struct iommu { · line 34
Quoted 10 lines
arch/arm/mach-omap2/iommu2.c +0 -0
 ?static void omap2_tlb_read_cr(struct iommu *obj, struct cr_regs *cr)
arch/arm/plat-omap/include/plat/iommu.h +1 -0
index 19cbb5e..174f1b9 100644
@@ -31,6 +31,7 @@ struct iommu {
 ? ? struct clk ? ? ?*clk;
 ? ? void __iomem ? ?*regbase;
 ? ? struct device ? *dev;
+ ? void ? ? ? ? ? ?*isr_priv;
Ideally I'd like to avoid having "isr_priv" in iommu since it's not used for iommu but client needs the place to pass its info to its custom handler. Any better idea?

Quoted 3 lines

I'm not sure if it makes sense as I don't know the mailbox block, but maybe moving to GENIRQ ? Then IRQ subsystem would take care of the "dev_id"

Not sure if it fits in this case. It's a different module (IOMMU user) which needs to get a callback from IOMMU when a fault happens. Is there any GENIRQ usage currently in this scenario?

Br,

David

Quoted 4 lines

--
balbi

Felipe Balbi

2011-02-21 09:20 UTC #6 lore raw

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

Hi,

Quoted 11 lines · David Cohen

Quoted 7 lines
Quoted 3 lines
Ideally I'd like to avoid having "isr_priv" in iommu since it's not used for iommu but client needs the place to pass its info to its custom handler. Any better idea?
I'm not sure if it makes sense as I don't know the mailbox block, but maybe moving to GENIRQ ? Then IRQ subsystem would take care of the "dev_id"
Not sure if it fits in this case. It's a different module (IOMMU user) which needs to get a callback from IOMMU when a fault happens. Is there any GENIRQ usage currently in this scenario?

No, that's not how GENIRQ is supposed to be used. You will need a function pointer, like you added.

Signature · 1 line

balbi

David Cohen

author

2011-02-21 09:07 UTC #7 lore raw

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

In reply to Hiroshi DOYU

Quoted 53 lines · Hiroshi DOYU

From: David Cohen <[email protected]>
Subject: [PATCH v3 2/2] OMAP: IOMMU: add support to callback during fault handling
Date: Wed, 16 Feb 2011 21:35:51 +0200

Quoted 49 lines

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

---

?arch/arm/mach-omap2/iommu2.c ? ? ? ? ? ?| ? 17 +++++++++-
?arch/arm/plat-omap/include/plat/iommu.h | ? 14 ++++++++-
?arch/arm/plat-omap/iommu.c ? ? ? ? ? ? ?| ? 52 ++++++++++++++++++++++---------

?3 files changed, 65 insertions(+), 18 deletions(-)

arch/arm/mach-omap2/iommu2.c +15 -2

index 49a1e5e..adb083e 100644
@@ -146,18 +146,31 @@ static void omap2_iommu_set_twl(struct iommu *obj, bool on)
 ?static u32 omap2_iommu_fault_isr(struct iommu *obj, u32 *ra)
 ?{
 ? ? ? u32 stat, da;
+ ? ? u32 errs = 0;
 
 ? ? ? stat = iommu_read_reg(obj, MMU_IRQSTATUS);
 ? ? ? stat &= MMU_IRQ_MASK;
- ? ? if (!stat)
+ ? ? if (!stat) {
+ ? ? ? ? ? ? *ra = 0;
 ? ? ? ? ? ? ? return 0;
+ ? ? }
 
 ? ? ? da = iommu_read_reg(obj, MMU_FAULT_AD);
 ? ? ? *ra = da;
 
+ ? ? 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_write_reg(obj, stat, MMU_IRQSTATUS);
 
- ? ? return stat;
+ ? ? return errs;
 ?}

Quoted 14 lines

arch/arm/plat-omap/include/plat/iommu.h struct iommu { · line 34
Quoted 10 lines
arch/arm/mach-omap2/iommu2.c +0 -0
 ?static void omap2_tlb_read_cr(struct iommu *obj, struct cr_regs *cr)
arch/arm/plat-omap/include/plat/iommu.h +1 -0
index 19cbb5e..174f1b9 100644
@@ -31,6 +31,7 @@ struct iommu {
 ? ? ? struct clk ? ? ?*clk;
 ? ? ? void __iomem ? ?*regbase;
 ? ? ? struct device ? *dev;
+ ? ? void ? ? ? ? ? ?*isr_priv;
Ideally I'd like to avoid having "isr_priv" in iommu since it's not used for iommu but client needs the place to pass its info to its custom handler. Any better idea?

(*isr)() relies in the same situation, as it belongs to the client. Without this priv_data, it's necessary to create a global variable to store client's private data on client side. IMO, it is worse.

Br,

David

Quoted 132 lines

Quoted 131 lines

arch/arm/plat-omap/include/plat/iommu.h +0 -0

 ? ? ? unsigned int ? ?refcount;

? ? ? struct mutex ? ?iommu_lock; ? ? /* global for this whole object */

arch/arm/plat-omap/include/plat/iommu.h +0 -0

@@ -47,7 +48,7 @@ struct iommu {
 ? ? ? struct list_head ? ? ? ?mmap;

? ? ? struct mutex ? ? ? ? ? ?mmap_lock; /* protect mmap */

- ? ? int (*isr)(struct iommu *obj);
+ ? ? int (*isr)(struct iommu *obj, u32 da, u32 iommu_errs, void *priv);

? ? ? void *ctx; /* iommu context: registres saved area */ ? ? ? u32 da_start;

arch/arm/plat-omap/include/plat/iommu.h +7 -0

@@ -109,6 +110,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

arch/arm/plat-omap/include/plat/iommu.h +4 -0

@@ -161,6 +169,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_isr(const char *name,
+ ? ? ? ? ? ? ? ? ? ? ?int (*isr)(struct iommu *obj, u32 da, u32 iommu_errs,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? void *priv),
+ ? ? ? ? ? ? ? ? ? ? ?void *isr_priv);
 
 ?extern void iommu_save_ctx(struct iommu *obj);
 ?extern void iommu_restore_ctx(struct iommu *obj);

arch/arm/plat-omap/iommu.c +0 -0

index f55f458..b0e0efc 100644
@@ -780,25 +780,19 @@ static void iopgtable_clear_entry_all(struct iommu *obj)

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

? ? ? if (!obj->refcount)
? ? ? ? ? ? ? return IRQ_NONE;

- ? ? /* Dynamic loading TLB or PTE */
- ? ? if (obj->isr)
- ? ? ? ? ? ? err = obj->isr(obj);

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

-
- ? ? if (!err)
- ? ? ? ? ? ? return IRQ_HANDLED;
-

? ? ? clk_enable(obj->clk);
- ? ? stat = iommu_report_fault(obj, &da);
+ ? ? errs = iommu_report_fault(obj, &da);
? ? ? clk_disable(obj->clk);
- ? ? if (!stat)
+
+ ? ? /* Fault callback or TLB/PTE Dynamic loading */
+ ? ? if (obj->isr && !obj->isr(obj, da, errs, obj->isr_priv)) ? ? ? ? ? ? ? return IRQ_HANDLED;

? ? ? iommu_disable(obj);

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

@@ -806,15 +800,16 @@ 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", obj->name,
- ? ? ? ? ? ? ? ? ? ? da, iopgd, *iopgd);
+ ? ? ? ? ? ? dev_err(obj->dev, "%s: errs:0x%08x da:0x%08x pgd:0x%p "
+ ? ? ? ? ? ? ? ? ? ? "*pgd:px%08x\n", obj->name, errs, da, iopgd, *iopgd);
 ? ? ? ? ? ? ? return IRQ_NONE;
 ? ? ? }
 
 ? ? ? iopte = iopte_offset(iopgd, da);
 
- ? ? dev_err(obj->dev, "%s: da:%08x pgd:%p *pgd:%08x pte:%p *pte:%08x\n",
- ? ? ? ? ? ? obj->name, da, iopgd, *iopgd, iopte, *iopte);
+ ? ? dev_err(obj->dev, "%s: errs:0x%08x da:0x%08x pgd:0x%p *pgd:0x%08x "
+ ? ? ? ? ? ? "pte:0x%p *pte:0x%08x\n", obj->name, errs, da, iopgd, *iopgd,
+ ? ? ? ? ? ? iopte, *iopte);
 
 ? ? ? return IRQ_NONE;
 ?}

@@ -917,6 +912,33 @@ void iommu_put(struct iommu *obj)
 ?}
 ?EXPORT_SYMBOL_GPL(iommu_put);
 
+int iommu_set_isr(const char *name,
+ ? ? ? ? ? ? ? int (*isr)(struct iommu *obj, u32 da, u32 iommu_errs,
+ ? ? ? ? ? ? ? ? ? ? ? ? ?void *priv),
+ ? ? ? ? ? ? ? void *isr_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->isr = isr;
+ ? ? obj->isr_priv = isr_priv;
+ ? ? mutex_unlock(&obj->iommu_lock);
+
+ ? ? return 0;
+}
+EXPORT_SYMBOL_GPL(iommu_set_isr);
+

?/*
? * ? OMAP Device MMU(IOMMU) detection
? */
--
1.7.2.3

Hiroshi DOYU

2011-02-21 09:51 UTC #8 lore raw

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

From: ext David Cohen <[email protected]>
Subject: Re: [PATCH v3 2/2] OMAP: IOMMU: add support to callback during fault handling
Date: Mon, 21 Feb 2011 11:07:01 +0200

Quoted 69 lines · Hiroshi DOYU

Quoted 53 lines
From: David Cohen <[email protected]>
Subject: [PATCH v3 2/2] OMAP: IOMMU: add support to callback during fault handling
Date: Wed, 16 Feb 2011 21:35:51 +0200
Quoted 49 lines
Add support to register an isr for IOMMU fault situations and adapt it to allow such (*isr)() to be used as fault callback. Drivers using IOMMU module might want to be informed when errors happen in order to debug it or react.
Signed-off-by David Cohen <[email protected]>
---
?arch/arm/mach-omap2/iommu2.c ? ? ? ? ? ?| ? 17 +++++++++-
?arch/arm/plat-omap/include/plat/iommu.h | ? 14 ++++++++-
?arch/arm/plat-omap/iommu.c ? ? ? ? ? ? ?| ? 52 ++++++++++++++++++++++---------
?3 files changed, 65 insertions(+), 18 deletions(-)
arch/arm/mach-omap2/iommu2.c +15 -2
index 49a1e5e..adb083e 100644
@@ -146,18 +146,31 @@ static void omap2_iommu_set_twl(struct iommu *obj, bool on)
 ?static u32 omap2_iommu_fault_isr(struct iommu *obj, u32 *ra)
 ?{
 ? ? ? u32 stat, da;
+ ? ? u32 errs = 0;
 
 ? ? ? stat = iommu_read_reg(obj, MMU_IRQSTATUS);
 ? ? ? stat &= MMU_IRQ_MASK;
- ? ? if (!stat)
+ ? ? if (!stat) {
+ ? ? ? ? ? ? *ra = 0;
 ? ? ? ? ? ? ? return 0;
+ ? ? }
 
 ? ? ? da = iommu_read_reg(obj, MMU_FAULT_AD);
 ? ? ? *ra = da;
 
+ ? ? 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_write_reg(obj, stat, MMU_IRQSTATUS);
 
- ? ? return stat;
+ ? ? return errs;
 ?}
 
Quoted 14 lines
arch/arm/plat-omap/include/plat/iommu.h struct iommu { · line 34
Quoted 10 lines
arch/arm/mach-omap2/iommu2.c +0 -0
 ?static void omap2_tlb_read_cr(struct iommu *obj, struct cr_regs *cr)
arch/arm/plat-omap/include/plat/iommu.h +1 -0
index 19cbb5e..174f1b9 100644
@@ -31,6 +31,7 @@ struct iommu {
 ? ? ? struct clk ? ? ?*clk;
 ? ? ? void __iomem ? ?*regbase;
 ? ? ? struct device ? *dev;
+ ? ? void ? ? ? ? ? ?*isr_priv;
Ideally I'd like to avoid having "isr_priv" in iommu since it's not used for iommu but client needs the place to pass its info to its custom handler. Any better idea?

Quoted 3 lines

(*isr)() relies in the same situation, as it belongs to the client. Without this priv_data, it's necessary to create a global variable to store client's private data on client side. IMO, it is worse.

Ok, I see. Let's go with this. Thanks.

Ramirez Luna, Omar

2011-02-21 18:43 UTC #9 lore raw

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

In reply to David Cohen

Hi,

Quoted 4 lines · David Cohen

arch/arm/mach-omap2/iommu2.c +0 -0
index 49a1e5e..adb083e 100644

...

Quoted 4 lines

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

Quoted 10 lines

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

I still don't think this adds any value, "generic layer" and omap errors are the same thing in this case... OTOH OMAP 1710 (not supported by iommu yet) has the following bits:

3 Prefetch_err
2 Perm_fault
1 Tlb_miss
0 Trans_fault

They don't match any of your "generic layer errors" masks for reading, hence more generic errors will need to be defined, and then more OMAP# masks... I think we just need to stick with the mach specific errors, and let mach code handle its specifics when reporting.

But anyway it is just me...

Quoted 25 lines

arch/arm/plat-omap/iommu.c +0 -0
index f55f458..b0e0efc 100644
@@ -780,25 +780,19 @@ static void iopgtable_clear_entry_all(struct iommu *obj)
?*/
?static irqreturn_t iommu_fault_handler(int irq, void *data) ?{
- ? ? ? u32 stat, da;
+ ? ? ? u32 da, errs;
? ? ? ?u32 *iopgd, *iopte;
- ? ? ? int err = -EIO;
? ? ? ?struct iommu *obj = data;
? ? ? ?if (!obj->refcount)
? ? ? ? ? ? ? ?return IRQ_NONE;
- ? ? ? /* Dynamic loading TLB or PTE */
- ? ? ? if (obj->isr)
- ? ? ? ? ? ? ? err = obj->isr(obj);
arch/arm/plat-omap/iommu.c +0 -4
-
- ? ? ? if (!err)
- ? ? ? ? ? ? ? return IRQ_HANDLED;
-
? ? ? ?clk_enable(obj->clk);

Quoted 8 lines

- ? ? ? stat = iommu_report_fault(obj, &da);
+ ? ? ? errs = iommu_report_fault(obj, &da);
? ? ? ?clk_disable(obj->clk);
- ? ? ? if (!stat)
+
+ ? ? ? /* Fault callback or TLB/PTE Dynamic loading */
+ ? ? ? if (obj->isr && !obj->isr(obj, da, errs, obj->isr_priv)) ? ? ? ? ? ? ? ?return IRQ_HANDLED;

BTW, why not changing the name isr for cb, it is confusing since there is another fault_isr called by mmu, AFAIK nobody uses obj->isr

Quoted 23 lines

? ? ? ?iommu_disable(obj);

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

@@ -806,15 +800,16 @@ 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", obj->name,
- ? ? ? ? ? ? ? ? ? ? ? da, iopgd, *iopgd);
+ ? ? ? ? ? ? ? dev_err(obj->dev, "%s: errs:0x%08x da:0x%08x pgd:0x%p "
+ ? ? ? ? ? ? ? ? ? ? ? "*pgd:px%08x\n", obj->name, errs, da, iopgd, *iopgd);
 ? ? ? ? ? ? ? ?return IRQ_NONE;
 ? ? ? ?}
 
 ? ? ? ?iopte = iopte_offset(iopgd, da);
 
- ? ? ? dev_err(obj->dev, "%s: da:%08x pgd:%p *pgd:%08x pte:%p *pte:%08x\n",
- ? ? ? ? ? ? ? obj->name, da, iopgd, *iopgd, iopte, *iopte);
+ ? ? ? dev_err(obj->dev, "%s: errs:0x%08x da:0x%08x pgd:0x%p *pgd:0x%08x "
+ ? ? ? ? ? ? ? "pte:0x%p *pte:0x%08x\n", obj->name, errs, da, iopgd, *iopgd,
+ ? ? ? ? ? ? ? iopte, *iopte);
 
 ? ? ? ?return IRQ_NONE;
 ?}

arch/arm/plat-omap/iommu.c void iommu_put(struct iommu *obj) · line 918

Quoted 7 lines

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

@@ -917,6 +912,33 @@ void iommu_put(struct iommu *obj)
 ?}
 ?EXPORT_SYMBOL_GPL(iommu_put);
 
+int iommu_set_isr(const char *name,
+ ? ? ? ? ? ? ? ? int (*isr)(struct iommu *obj, u32 da, u32 iommu_errs,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ?void *priv),
+ ? ? ? ? ? ? ? ? void *isr_priv)

So I think the following will be bad practice:

        mmu = iommu_get("iva2");
        if (!IS_ERR(mmu))
                mmu->isr = mmu_fault_callback;

Shall we think anything to prevent such mis-usage?

Regards,

Omar

David Cohen

author

2011-02-21 21:12 UTC #10 lore raw

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

Quoted 1 line · Omar

Hi,

Hi,

Thanks for the comments.

Quoted 26 lines · David Cohen

Quoted 4 lines
arch/arm/mach-omap2/iommu2.c +0 -0
index 49a1e5e..adb083e 100644
...
Quoted 4 lines
? ? ? ?da = iommu_read_reg(obj, MMU_FAULT_AD);
? ? ? ?*ra = da;
Quoted 10 lines
+ ? ? ? 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;
I still don't think this adds any value, "generic layer" and omap errors are the same thing in this case... OTOH OMAP 1710 (not supported by iommu yet) has the following bits:

Quoted 6 lines

3 Prefetch_err
2 Perm_fault
1 Tlb_miss
0 Trans_fault
They don't match any of your "generic layer errors" masks for reading,

Have you noticed:
0 = OMAP_IOMMU_ERR_TRANS_FAULT
1 = OMAP_IOMMU_ERR_TLB_MISS

2 and 3 could be added.

Quoted 3 lines

hence more generic errors will need to be defined, and then more OMAP# masks... I think we just need to stick with the mach specific errors, and let mach code handle its specifics when reporting.

How many are we talking about? I don't think every new OMAP version it would completely re-invent IOMMU faults.
Generic errors codes make easier to threat possible IOMMU users which have (partially or totally) common drivers for different OMAP versions.
Unless it's really an out-of-control number of generic faults, I don't see it as a real problem.

Quoted 31 lines

But anyway it is just me...
Quoted 28 lines
arch/arm/plat-omap/iommu.c +0 -0
index f55f458..b0e0efc 100644
@@ -780,25 +780,19 @@ static void iopgtable_clear_entry_all(struct iommu *obj)
?*/
?static irqreturn_t iommu_fault_handler(int irq, void *data) ?{
- ? ? ? u32 stat, da;
+ ? ? ? u32 da, errs;
? ? ? ?u32 *iopgd, *iopte;
- ? ? ? int err = -EIO;
? ? ? ?struct iommu *obj = data;
? ? ? ?if (!obj->refcount)
? ? ? ? ? ? ? ?return IRQ_NONE;
- ? ? ? /* Dynamic loading TLB or PTE */
- ? ? ? if (obj->isr)
- ? ? ? ? ? ? ? err = obj->isr(obj);
arch/arm/plat-omap/iommu.c +0 -4
-
- ? ? ? if (!err)
- ? ? ? ? ? ? ? return IRQ_HANDLED;
-
? ? ? ?clk_enable(obj->clk);
- ? ? ? stat = iommu_report_fault(obj, &da);
+ ? ? ? errs = iommu_report_fault(obj, &da);
? ? ? ?clk_disable(obj->clk);

Quoted 8 lines

Quoted 5 lines
- ? ? ? if (!stat)
+
+ ? ? ? /* Fault callback or TLB/PTE Dynamic loading */
+ ? ? ? if (obj->isr && !obj->isr(obj, da, errs, obj->isr_priv)) ? ? ? ? ? ? ? ?return IRQ_HANDLED;
BTW, why not changing the name isr for cb, it is confusing since there is another fault_isr called by mmu, AFAIK nobody uses obj->isr

The main purpose of this function is to be an ISR, not only callback. But as you noticed, nobody is using it yet, but OMAP3 ISP should start to use it soon.

Quoted 34 lines

Quoted 24 lines

? ? ? ?iommu_disable(obj);

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

@@ -806,15 +800,16 @@ 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", obj->name,
- ? ? ? ? ? ? ? ? ? ? ? da, iopgd, *iopgd);
+ ? ? ? ? ? ? ? dev_err(obj->dev, "%s: errs:0x%08x da:0x%08x pgd:0x%p "
+ ? ? ? ? ? ? ? ? ? ? ? "*pgd:px%08x\n", obj->name, errs, da, iopgd, *iopgd);
 ? ? ? ? ? ? ? ?return IRQ_NONE;
 ? ? ? ?}
 
 ? ? ? ?iopte = iopte_offset(iopgd, da);
 
- ? ? ? dev_err(obj->dev, "%s: da:%08x pgd:%p *pgd:%08x pte:%p *pte:%08x\n",
- ? ? ? ? ? ? ? obj->name, da, iopgd, *iopgd, iopte, *iopte);
+ ? ? ? dev_err(obj->dev, "%s: errs:0x%08x da:0x%08x pgd:0x%p *pgd:0x%08x "
+ ? ? ? ? ? ? ? "pte:0x%p *pte:0x%08x\n", obj->name, errs, da, iopgd, *iopgd,
+ ? ? ? ? ? ? ? iopte, *iopte);
 
 ? ? ? ?return IRQ_NONE;
 ?}

arch/arm/plat-omap/iommu.c void iommu_put(struct iommu *obj) · line 918

Quoted 7 lines

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

@@ -917,6 +912,33 @@ void iommu_put(struct iommu *obj)
 ?}
 ?EXPORT_SYMBOL_GPL(iommu_put);
 
+int iommu_set_isr(const char *name,
+ ? ? ? ? ? ? ? ? int (*isr)(struct iommu *obj, u32 da, u32 iommu_errs,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ?void *priv),
+ ? ? ? ? ? ? ? ? void *isr_priv)

So I think the following will be bad practice:

Quoted 5 lines

? ? ? ?mmu = iommu_get("iva2");
? ? ? ?if (!IS_ERR(mmu))
? ? ? ? ? ? ? ?mmu->isr = mmu_fault_callback;
Shall we think anything to prevent such mis-usage?

Well, the IOMMU user has access to IOMMU obj, so it can not only change the (*isr)() but to mess with a lot of other stuff. The only way to prevent it is to avoid user to have obj. But then, this fix (or issue) does not belong to this patch.

Br,

David

Quoted 5 lines

Regards,
Omar

Ramirez Luna, Omar

2011-02-21 23:25 UTC #11 lore raw

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

Hi,

Quoted 3 lines · David Cohen

Generic errors codes make easier to threat possible IOMMU users which have (partially or totally) common drivers for different OMAP versions.

Agree then.

Quoted 8 lines

Quoted 6 lines
Quoted 3 lines
+ ? ? ? /* Fault callback or TLB/PTE Dynamic loading */
+ ? ? ? if (obj->isr && !obj->isr(obj, da, errs, obj->isr_priv)) ? ? ? ? ? ? ? ?return IRQ_HANDLED;
BTW, why not changing the name isr for cb, it is confusing since there is another fault_isr called by mmu, AFAIK nobody uses obj->isr
The main purpose of this function is to be an ISR, not only callback.

Yep, I just thought it was a bit too much of:
(1) The real isr: iommu_fault_handler, set on request_irq
(2) A plugged fault_isr for mach specific code: omap2_iommu_fault_isr, for error reporting.
(3) And then a plugged custom isr, to be set by the user.

Feel free to ignore.

Quoted 6 lines

Quoted 6 lines
So I think the following will be bad practice:
? ? ? ?mmu = iommu_get("iva2");
? ? ? ?if (!IS_ERR(mmu))
? ? ? ? ? ? ? ?mmu->isr = mmu_fault_callback;

Quoted 6 lines

Quoted 1 line
Shall we think anything to prevent such mis-usage?
Well, the IOMMU user has access to IOMMU obj, so it can not only change the (*isr)() but to mess with a lot of other stuff. The only way to prevent it is to avoid user to have obj. But then, this fix (or issue) does not belong to this patch.

Agree, just pointing out.

Regards,

Omar

David Cohen

author

2011-02-21 23:48 UTC #12 lore raw

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

Quoted 8 lines · Omar

Hi,
Quoted 3 lines · David Cohen
Generic errors codes make easier to threat possible IOMMU users which have (partially or totally) common drivers for different OMAP versions.
Agree then.

Good we agreed on that.

Quoted 10 lines

Quoted 9 lines
Quoted 7 lines
Quoted 4 lines
+ ? ? ? /* Fault callback or TLB/PTE Dynamic loading */
+ ? ? ? if (obj->isr && !obj->isr(obj, da, errs, obj->isr_priv)) ? ? ? ? ? ? ? ?return IRQ_HANDLED;
BTW, why not changing the name isr for cb, it is confusing since there is another fault_isr called by mmu, AFAIK nobody uses obj->isr
The main purpose of this function is to be an ISR, not only callback.

Quoted 7 lines

Yep, I just thought it was a bit too much of:
(1) The real isr: iommu_fault_handler, set on request_irq
(2) A plugged fault_isr for mach specific code: omap2_iommu_fault_isr, for error reporting.
(3) And then a plugged custom isr, to be set by the user.
Feel free to ignore.

(1) does nothing but to call (2) and then (3) if it exists and print error message.
(2) as you said, it's mach specific and necessary for the generic upper layer.
(3) does what the IOMMU user wants and may replace (1) if it returns 0.

They belong to different layers and should coexist.

Quoted 7 lines

Quoted 7 lines
Quoted 7 lines
So I think the following will be bad practice:
? ? ? ?mmu = iommu_get("iva2");
? ? ? ?if (!IS_ERR(mmu))
? ? ? ? ? ? ? ?mmu->isr = mmu_fault_callback;

Quoted 8 lines

Quoted 6 lines
Quoted 1 line
Shall we think anything to prevent such mis-usage?
Well, the IOMMU user has access to IOMMU obj, so it can not only change the (*isr)() but to mess with a lot of other stuff. The only way to prevent it is to avoid user to have obj. But then, this fix (or issue) does not belong to this patch.
Agree, just pointing out.

That's a valid point, but it requires an intrusive approach to prevent such mis-usage. For now we must trust the user won't mess with obj.

Br,

David

Quoted 5 lines

Regards,
Omar

Guzman Lugo, Fernando

2011-02-23 01:17 UTC #13 lore raw

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

In reply to David Cohen

Quoted 12 lines · David Cohen

Add support to register an isr for IOMMU fault situations and adapt it to allow such (*isr)() to be used as fault callback. Drivers using IOMMU module might want to be informed when errors happen in order to debug it or react.
Signed-off-by David Cohen <[email protected]>
---
?arch/arm/mach-omap2/iommu2.c ? ? ? ? ? ?| ? 17 +++++++++-
?arch/arm/plat-omap/include/plat/iommu.h | ? 14 ++++++++-
?arch/arm/plat-omap/iommu.c ? ? ? ? ? ? ?| ? 52 ++++++++++++++++++++++---------
?3 files changed, 65 insertions(+), 18 deletions(-)

....

Review void iommu_put(struct iommu *obj) · line 922

Quoted 12 lines

(quoted diff) +8 -0

@@ -917,6 +912,33 @@ void iommu_put(struct iommu *obj)
 ?}
 ?EXPORT_SYMBOL_GPL(iommu_put);
 
+int iommu_set_isr(const char *name,
+ ? ? ? ? ? ? ? ? int (*isr)(struct iommu *obj, u32 da, u32 iommu_errs,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ?void *priv),
+ ? ? ? ? ? ? ? ? void *isr_priv)
+{
+ ? ? ? struct device *dev;
+ ? ? ? struct iommu *obj;
+

if the driver support multiple user for the same iommu why can only one callback be registered? should it support register multiple callback function (one per user)?

Regards,
Fernando.

David Cohen

author

2011-02-23 09:45 UTC #14 lore raw

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

Quoted 19 lines · Fernando

Quoted 12 lines · David Cohen
Add support to register an isr for IOMMU fault situations and adapt it to allow such (*isr)() to be used as fault callback. Drivers using IOMMU module might want to be informed when errors happen in order to debug it or react.
Signed-off-by David Cohen <[email protected]>
---
?arch/arm/mach-omap2/iommu2.c ? ? ? ? ? ?| ? 17 +++++++++-
?arch/arm/plat-omap/include/plat/iommu.h | ? 14 ++++++++-
?arch/arm/plat-omap/iommu.c ? ? ? ? ? ? ?| ? 52 ++++++++++++++++++++++---------
?3 files changed, 65 insertions(+), 18 deletions(-)
....
Quoted 4 lines
(quoted diff) +0 -0
@@ -917,6 +912,33 @@ void iommu_put(struct iommu *obj)
 ?}
 ?EXPORT_SYMBOL_GPL(iommu_put);
 

Quoted 12 lines

Review void iommu_put(struct iommu *obj) · line 922
Quoted 8 lines
(quoted diff) +8 -0
+int iommu_set_isr(const char *name,
+ ? ? ? ? ? ? ? ? int (*isr)(struct iommu *obj, u32 da, u32 iommu_errs,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ?void *priv),
+ ? ? ? ? ? ? ? ? void *isr_priv)
+{
+ ? ? ? struct device *dev;
+ ? ? ? struct iommu *obj;
+
if the driver support multiple user for the same iommu why can only one callback be registered? should it support register multiple callback function (one per user)?

Can you define a scenario for that?
On OMAP3 ISP the multiple users are the multiple ISP submodule, but I don't think it's necessary all submodule to have a specific callback. ISP core layer should handle.

Br,

David

Quoted 4 lines

Regards,
Fernando.

Guzman Lugo, Fernando

2011-02-23 13:39 UTC #15 lore raw

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

Quoted 34 lines · David Cohen

Quoted 19 lines · Fernando
Quoted 12 lines · David Cohen
Add support to register an isr for IOMMU fault situations and adapt it to allow such (*isr)() to be used as fault callback. Drivers using IOMMU module might want to be informed when errors happen in order to debug it or react.
Signed-off-by David Cohen <[email protected]>
---
?arch/arm/mach-omap2/iommu2.c ? ? ? ? ? ?| ? 17 +++++++++-
?arch/arm/plat-omap/include/plat/iommu.h | ? 14 ++++++++-
?arch/arm/plat-omap/iommu.c ? ? ? ? ? ? ?| ? 52 ++++++++++++++++++++++---------
?3 files changed, 65 insertions(+), 18 deletions(-)
....
Quoted 4 lines
(quoted diff) +0 -0
@@ -917,6 +912,33 @@ void iommu_put(struct iommu *obj)
 ?}
 ?EXPORT_SYMBOL_GPL(iommu_put);
 
Quoted 12 lines
Review void iommu_put(struct iommu *obj) · line 922
Quoted 8 lines
(quoted diff) +8 -0
+int iommu_set_isr(const char *name,
+ ? ? ? ? ? ? ? ? int (*isr)(struct iommu *obj, u32 da, u32 iommu_errs,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ?void *priv),
+ ? ? ? ? ? ? ? ? void *isr_priv)
+{
+ ? ? ? struct device *dev;
+ ? ? ? struct iommu *obj;
+
if the driver support multiple user for the same iommu why can only one callback be registered? should it support register multiple callback function (one per user)?

Quoted 4 lines

Can you define a scenario for that?
On OMAP3 ISP the multiple users are the multiple ISP submodule, but I don't think it's necessary all submodule to have a specific callback. ISP core layer should handle.

Hi,

In OMAP4 the cortex M3 is a double core processor and as each core is running they own version of the RTOS we threat them independently. So our driver which controls the remote processor sees two processor but both use the same iommu hw. When a iommu fault happens, at this moment, it is consider as a faltal error and it is no managed to recover and continue, instead a restart of the processor is needed, if the fault happens in core0 we need to reset core1 too and vice versa. if the iommu would support several user callbacks, we can register the callback which resets core0 and also the callback which resets core1 and treat them as totally independent processors. Also we have an error event notifier driver, which is only in charge of notifying error events to userspace, so we would have multiple callbacks we could do this

iommu <---- register fault callback for error notify driver

instead of

iommu <--- register fault callback for remote processor driver <----register fault event for error notify driver.

with that, we remove one dependency of the errornotify driver.

Moreover, the iommu code support serveral users of the same hw iommu, and it does not make sense for me, that you can register only one callback, or if other user register its callback the previous one will be overwritten.

Regards,
Fernando.

Quoted 10 lines

Br,
David
Quoted 4 lines
Regards,
Fernando.

David Cohen

author

2011-02-23 19:54 UTC #16 lore raw

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

Quoted 42 lines · Fernando

Quoted 34 lines · David Cohen
Quoted 19 lines · Fernando
Quoted 12 lines · David Cohen
Add support to register an isr for IOMMU fault situations and adapt it to allow such (*isr)() to be used as fault callback. Drivers using IOMMU module might want to be informed when errors happen in order to debug it or react.
Signed-off-by David Cohen <[email protected]>
---
?arch/arm/mach-omap2/iommu2.c ? ? ? ? ? ?| ? 17 +++++++++-
?arch/arm/plat-omap/include/plat/iommu.h | ? 14 ++++++++-
?arch/arm/plat-omap/iommu.c ? ? ? ? ? ? ?| ? 52 ++++++++++++++++++++++---------
?3 files changed, 65 insertions(+), 18 deletions(-)
....
Quoted 4 lines
(quoted diff) +0 -0
@@ -917,6 +912,33 @@ void iommu_put(struct iommu *obj)
 ?}
 ?EXPORT_SYMBOL_GPL(iommu_put);
 
Quoted 12 lines
Review void iommu_put(struct iommu *obj) · line 922
Quoted 8 lines
(quoted diff) +8 -0
+int iommu_set_isr(const char *name,
+ ? ? ? ? ? ? ? ? int (*isr)(struct iommu *obj, u32 da, u32 iommu_errs,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ?void *priv),
+ ? ? ? ? ? ? ? ? void *isr_priv)
+{
+ ? ? ? struct device *dev;
+ ? ? ? struct iommu *obj;
+
if the driver support multiple user for the same iommu why can only one callback be registered? should it support register multiple callback function (one per user)?
Quoted 4 lines
Can you define a scenario for that?
On OMAP3 ISP the multiple users are the multiple ISP submodule, but I don't think it's necessary all submodule to have a specific callback. ISP core layer should handle.
Hi,

Quoted 13 lines

In OMAP4 the cortex M3 is a double core processor and as each core is running they own version of the RTOS we threat them independently. So our driver which controls the remote processor sees two processor but both use the same iommu hw. When a iommu fault happens, at this moment, it is consider as a faltal error and it is no managed to recover and continue, instead a restart of the processor is needed, if the fault happens in core0 we need to reset core1 too and vice versa. if the iommu would support several user callbacks, we can register the callback which resets core0 and also the callback which resets core1 and treat them as totally independent processors. Also we have an error event notifier driver, which is only in charge of notifying error events to userspace, so we would have multiple callbacks we could do this

I understood your point. In this case, I may not disagree about having more than one callback per obj, although it doesn't seem a nice scenario.
We can have a list of callbacks and call the entire list when a fault happens. But it's necessary to pay attention it will happen in atomic context and users should not abuse and register many callbacks. The callback should *NOT* print useless messages and must verify the error code to not execute useless steps.
In this context, callback and ISR cannot share a same pointer anymore.

Quoted 9 lines

iommu <---- register fault callback for error notify driver
instead of
iommu <--- register fault callback for remote processor driver <----register fault event for error notify driver.
with that, we remove one dependency of the errornotify driver.

I don't know very well the errornotify driver, but to bypass it seems be a good approach for me.

Quoted 5 lines

Moreover, the iommu code support serveral users of the same hw iommu, and it does not make sense for me, that you can register only one callback, or if other user register its callback the previous one will be overwritten.

It's quite complex and dangerous this situation, as one driver can crash another one. But I don't think drivers have much choice.

Hiroshi, do you have any different opinion for this subject? I can send a v4 version for this patch giving support for multiple callbacks per obj.

Br,

David

Quoted 15 lines

Regards,
Fernando.
Quoted 10 lines
Br,
David
Quoted 4 lines
Regards,
Fernando.

Sakari Ailus

2011-02-23 20:55 UTC #17 lore raw

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

Quoted 58 lines · David Cohen

Quoted 42 lines · Fernando
Quoted 34 lines · David Cohen
Quoted 19 lines · Fernando
Quoted 12 lines · David Cohen
Add support to register an isr for IOMMU fault situations and adapt it to allow such (*isr)() to be used as fault callback. Drivers using IOMMU module might want to be informed when errors happen in order to debug it or react.
Signed-off-by David Cohen <[email protected]>
---
 arch/arm/mach-omap2/iommu2.c            |   17 +++++++++-
 arch/arm/plat-omap/include/plat/iommu.h |   14 ++++++++-
 arch/arm/plat-omap/iommu.c              |   52 ++++++++++++++++++++++---------
 3 files changed, 65 insertions(+), 18 deletions(-)
....
Quoted 4 lines
arch/arm/plat-omap/iommu.c +0 -0
@@ -917,6 +912,33 @@ void iommu_put(struct iommu *obj)
 }
 EXPORT_SYMBOL_GPL(iommu_put);
 
Quoted 12 lines
arch/arm/plat-omap/iommu.c void iommu_put(struct iommu *obj) · line 922
Quoted 8 lines
arch/arm/plat-omap/iommu.c +8 -0
+int iommu_set_isr(const char *name,
+                 int (*isr)(struct iommu *obj, u32 da, u32 iommu_errs,
+                            void *priv),
+                 void *isr_priv)
+{
+       struct device *dev;
+       struct iommu *obj;
+
if the driver support multiple user for the same iommu why can only one callback be registered? should it support register multiple callback function (one per user)?
Quoted 4 lines
Can you define a scenario for that?
On OMAP3 ISP the multiple users are the multiple ISP submodule, but I don't think it's necessary all submodule to have a specific callback. ISP core layer should handle.
Hi,
Quoted 13 lines
In OMAP4 the cortex M3 is a double core processor and as each core is running they own version of the RTOS we threat them independently. So our driver which controls the remote processor sees two processor but both use the same iommu hw. When a iommu fault happens, at this moment, it is consider as a faltal error and it is no managed to recover and continue, instead a restart of the processor is needed, if the fault happens in core0 we need to reset core1 too and vice versa. if the iommu would support several user callbacks, we can register the callback which resets core0 and also the callback which resets core1 and treat them as totally independent processors. Also we have an error event notifier driver, which is only in charge of notifying error events to userspace, so we would have multiple callbacks we could do this

Quoted 9 lines

I understood your point. In this case, I may not disagree about having more than one callback per obj, although it doesn't seem a nice scenario.
We can have a list of callbacks and call the entire list when a fault happens. But it's necessary to pay attention it will happen in atomic context and users should not abuse and register many callbacks. The callback should *NOT* print useless messages and must verify the error code to not execute useless steps.
In this context, callback and ISR cannot share a same pointer anymore.

I think this is outside of the scope of the patch but...

To efficiently debug iommu faults (with a driver using iommu page walking), besides the actual fault address the list of existing mappings and the information which driver created them and for which purpose is useful.

The list of mappings is already available in the iommu structure. It'd be nice if there was a function a driver could call to print them.

I can only think of ugly ways to implement the other.

Just my 5 cents (as we have no 2 cent coins here).

Regards,

Signature · 2 lines

Sakari Ailus
[email protected]

Guzman Lugo, Fernando

2011-02-23 21:48 UTC #18 lore raw

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

Quoted 69 lines · Sakari Ailus

Quoted 58 lines · David Cohen
Quoted 42 lines · Fernando
Quoted 34 lines · David Cohen
Quoted 19 lines · Fernando
Quoted 12 lines · David Cohen
Add support to register an isr for IOMMU fault situations and adapt it to allow such (*isr)() to be used as fault callback. Drivers using IOMMU module might want to be informed when errors happen in order to debug it or react.
Signed-off-by David Cohen <[email protected]>
---
?arch/arm/mach-omap2/iommu2.c ? ? ? ? ? ?| ? 17 +++++++++-
?arch/arm/plat-omap/include/plat/iommu.h | ? 14 ++++++++-
?arch/arm/plat-omap/iommu.c ? ? ? ? ? ? ?| ? 52 ++++++++++++++++++++++---------
?3 files changed, 65 insertions(+), 18 deletions(-)
....
Quoted 4 lines
(quoted diff) +0 -0
@@ -917,6 +912,33 @@ void iommu_put(struct iommu *obj)
 ?}
 ?EXPORT_SYMBOL_GPL(iommu_put);
 
Quoted 12 lines
Review void iommu_put(struct iommu *obj) · line 922
Quoted 8 lines
(quoted diff) +8 -0
+int iommu_set_isr(const char *name,
+ ? ? ? ? ? ? ? ? int (*isr)(struct iommu *obj, u32 da, u32 iommu_errs,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ?void *priv),
+ ? ? ? ? ? ? ? ? void *isr_priv)
+{
+ ? ? ? struct device *dev;
+ ? ? ? struct iommu *obj;
+
if the driver support multiple user for the same iommu why can only one callback be registered? should it support register multiple callback function (one per user)?
Quoted 4 lines
Can you define a scenario for that?
On OMAP3 ISP the multiple users are the multiple ISP submodule, but I don't think it's necessary all submodule to have a specific callback. ISP core layer should handle.
Hi,
Quoted 13 lines
In OMAP4 the cortex M3 is a double core processor and as each core is running they own version of the RTOS we threat them independently. So our driver which controls the remote processor sees two processor but both use the same iommu hw. When a iommu fault happens, at this moment, it is consider as a faltal error and it is no managed to recover and continue, instead a restart of the processor is needed, if the fault happens in core0 we need to reset core1 too and vice versa. if the iommu would support several user callbacks, we can register the callback which resets core0 and also the callback which resets core1 and treat them as totally independent processors. Also we have an error event notifier driver, which is only in charge of notifying error events to userspace, so we would have multiple callbacks we could do this
Quoted 9 lines
I understood your point. In this case, I may not disagree about having more than one callback per obj, although it doesn't seem a nice scenario.
We can have a list of callbacks and call the entire list when a fault happens. But it's necessary to pay attention it will happen in atomic context and users should not abuse and register many callbacks. The callback should *NOT* print useless messages and must verify the error code to not execute useless steps.
In this context, callback and ISR cannot share a same pointer anymore.

Quoted 1 line

I think this is outside of the scope of the patch but...

yes, the same behaviour was before the patches, but as the patches are changing the isr, I think it is a good time to modify, not in patch 2, but in a new patch to be added to the serie between patch 1 and 2, so that we dont need to change ISR part again after this set of patches.

Quoted 20 lines

To efficiently debug iommu faults (with a driver using iommu page walking), besides the actual fault address the list of existing mappings and the information which driver created them and for which purpose is useful.
The list of mappings is already available in the iommu structure. It'd be nice if there was a function a driver could call to print them.
I can only think of ugly ways to implement the other.
Just my 5 cents (as we have no 2 cent coins here).
Regards,
--
Sakari Ailus
sakari.ailus at maxwell.research.nokia.com

David Cohen

author

2011-02-24 06:39 UTC #19 lore raw

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

Quoted 71 lines · Fernando

Quoted 69 lines · Sakari Ailus
Quoted 67 lines · David Cohen
Quoted 42 lines · Fernando
Quoted 34 lines · David Cohen
Quoted 19 lines · Fernando
Quoted 12 lines · David Cohen
Add support to register an isr for IOMMU fault situations and adapt it to allow such (*isr)() to be used as fault callback. Drivers using IOMMU module might want to be informed when errors happen in order to debug it or react.
Signed-off-by David Cohen <[email protected]>
---
?arch/arm/mach-omap2/iommu2.c ? ? ? ? ? ?| ? 17 +++++++++-
?arch/arm/plat-omap/include/plat/iommu.h | ? 14 ++++++++-
?arch/arm/plat-omap/iommu.c ? ? ? ? ? ? ?| ? 52 ++++++++++++++++++++++---------
?3 files changed, 65 insertions(+), 18 deletions(-)
....
Quoted 4 lines
(quoted diff) +0 -0
@@ -917,6 +912,33 @@ void iommu_put(struct iommu *obj)
 ?}
 ?EXPORT_SYMBOL_GPL(iommu_put);
 
Quoted 12 lines
Review void iommu_put(struct iommu *obj) · line 922
Quoted 8 lines
(quoted diff) +8 -0
+int iommu_set_isr(const char *name,
+ ? ? ? ? ? ? ? ? int (*isr)(struct iommu *obj, u32 da, u32 iommu_errs,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ?void *priv),
+ ? ? ? ? ? ? ? ? void *isr_priv)
+{
+ ? ? ? struct device *dev;
+ ? ? ? struct iommu *obj;
+
if the driver support multiple user for the same iommu why can only one callback be registered? should it support register multiple callback function (one per user)?
Quoted 4 lines
Can you define a scenario for that?
On OMAP3 ISP the multiple users are the multiple ISP submodule, but I don't think it's necessary all submodule to have a specific callback. ISP core layer should handle.
Hi,
Quoted 13 lines
In OMAP4 the cortex M3 is a double core processor and as each core is running they own version of the RTOS we threat them independently. So our driver which controls the remote processor sees two processor but both use the same iommu hw. When a iommu fault happens, at this moment, it is consider as a faltal error and it is no managed to recover and continue, instead a restart of the processor is needed, if the fault happens in core0 we need to reset core1 too and vice versa. if the iommu would support several user callbacks, we can register the callback which resets core0 and also the callback which resets core1 and treat them as totally independent processors. Also we have an error event notifier driver, which is only in charge of notifying error events to userspace, so we would have multiple callbacks we could do this
I understood your point. In this case, I may not disagree about having more than one callback per obj, although it doesn't seem a nice scenario.
We can have a list of callbacks and call the entire list when a fault happens. But it's necessary to pay attention it will happen in atomic context and users should not abuse and register many callbacks. The callback should *NOT* print useless messages and must verify the error code to not execute useless steps.
In this context, callback and ISR cannot share a same pointer anymore.

Quoted 6 lines

Quoted 1 line
I think this is outside of the scope of the patch but...
yes, the same behaviour was before the patches, but as the patches are changing the isr, I think it is a good time to modify, not in patch 2, but in a new patch to be added to the serie between patch 1 and 2, so that we dont need to change ISR part again after this set of patches.

Let's wait for Hiroshi's opinion and decide if I change or not the patches.

Br,

David

Quoted 22 lines

Quoted 21 lines
To efficiently debug iommu faults (with a driver using iommu page walking), besides the actual fault address the list of existing mappings and the information which driver created them and for which purpose is useful.
The list of mappings is already available in the iommu structure. It'd be nice if there was a function a driver could call to print them.
I can only think of ugly ways to implement the other.
Just my 5 cents (as we have no 2 cent coins here).
Regards,
--
Sakari Ailus
sakari.ailus at maxwell.research.nokia.com

Guzman Lugo, Fernando

2011-02-23 21:12 UTC #20 lore raw

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

In reply to David Cohen

Quoted 58 lines · David Cohen

Quoted 42 lines · Fernando
Quoted 34 lines · David Cohen
Quoted 19 lines · Fernando
Quoted 12 lines · David Cohen
Add support to register an isr for IOMMU fault situations and adapt it to allow such (*isr)() to be used as fault callback. Drivers using IOMMU module might want to be informed when errors happen in order to debug it or react.
Signed-off-by David Cohen <[email protected]>
---
?arch/arm/mach-omap2/iommu2.c ? ? ? ? ? ?| ? 17 +++++++++-
?arch/arm/plat-omap/include/plat/iommu.h | ? 14 ++++++++-
?arch/arm/plat-omap/iommu.c ? ? ? ? ? ? ?| ? 52 ++++++++++++++++++++++---------
?3 files changed, 65 insertions(+), 18 deletions(-)
....
Quoted 4 lines
(quoted diff) +0 -0
@@ -917,6 +912,33 @@ void iommu_put(struct iommu *obj)
 ?}
 ?EXPORT_SYMBOL_GPL(iommu_put);
 
Quoted 12 lines
Review void iommu_put(struct iommu *obj) · line 922
Quoted 8 lines
(quoted diff) +8 -0
+int iommu_set_isr(const char *name,
+ ? ? ? ? ? ? ? ? int (*isr)(struct iommu *obj, u32 da, u32 iommu_errs,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ?void *priv),
+ ? ? ? ? ? ? ? ? void *isr_priv)
+{
+ ? ? ? struct device *dev;
+ ? ? ? struct iommu *obj;
+
if the driver support multiple user for the same iommu why can only one callback be registered? should it support register multiple callback function (one per user)?
Quoted 4 lines
Can you define a scenario for that?
On OMAP3 ISP the multiple users are the multiple ISP submodule, but I don't think it's necessary all submodule to have a specific callback. ISP core layer should handle.
Hi,
Quoted 13 lines
In OMAP4 the cortex M3 is a double core processor and as each core is running they own version of the RTOS we threat them independently. So our driver which controls the remote processor sees two processor but both use the same iommu hw. When a iommu fault happens, at this moment, it is consider as a faltal error and it is no managed to recover and continue, instead a restart of the processor is needed, if the fault happens in core0 we need to reset core1 too and vice versa. if the iommu would support several user callbacks, we can register the callback which resets core0 and also the callback which resets core1 and treat them as totally independent processors. Also we have an error event notifier driver, which is only in charge of notifying error events to userspace, so we would have multiple callbacks we could do this

Quoted 4 lines

I understood your point. In this case, I may not disagree about having more than one callback per obj, although it doesn't seem a nice scenario.
We can have a list of callbacks and call the entire list when a fault

check kernel/notifier.c module and how it is manage in mailbox, so you can implement something similar.

Quoted 4 lines

happens. But it's necessary to pay attention it will happen in atomic context and users should not abuse and register many callbacks. The callback should *NOT* print useless messages and must verify the error code to not execute useless steps.

Sure, users needs to consider the callback as a ISR and if they need to do something heavy then schedule a task to do that.

Quoted 7 lines

In this context, callback and ISR cannot share a same pointer anymore.
Quoted 5 lines
iommu <---- register fault callback for error notify driver
instead of

Quoted 7 lines

Quoted 4 lines
iommu <--- register fault callback for remote processor driver <----register fault event for error notify driver.
with that, we remove one dependency of the errornotify driver.
I don't know very well the errornotify driver, but to bypass it seems be a good approach for me.

that driver is not upstream yet, the intention is to have a driver which can handle many errors, at this moments it is not handle the errors, but notifying them to userspace. it is thought to be a generic error handler with the posibility of notify many errors, among them the iommu fault, so if the error handler driver will notify iommy fault errors it makes more sense to register with iommu module instead of doing with other driver which uses iommu module, and that can be done having multiple callbacks.

Quoted 9 lines

Quoted 6 lines
Moreover, the iommu code support serveral users of the same hw iommu, and it does not make sense for me, that you can register only one callback, or if other user register its callback the previous one will be overwritten.
It's quite complex and dangerous this situation, as one driver can crash another one.

Yes that can happen, and if you don't have multiple callbacks the other driver wont even notice it crashed and will try to work normally. That is not good.

Quoted 26 lines

But I don't think drivers have much choice.
Hiroshi, do you have any different opinion for this subject? I can send a v4 version for this patch giving support for multiple callbacks per obj.
Br,
David
Quoted 15 lines
Regards,
Fernando.
Quoted 10 lines
Br,
David
Quoted 4 lines
Regards,
Fernando.

Sakari Ailus

2011-02-23 20:07 UTC #21 lore raw

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

In reply to Guzman Lugo, Fernando

Quoted 1 line · Fernando

Hi,

Hi Fernando,

Quoted 5 lines

In OMAP4 the cortex M3 is a double core processor and as each core is running they own version of the RTOS we threat them independently. So our driver which controls the remote processor sees two processor but both use the same iommu hw. When a iommu fault happens, at this moment, it is consider as a faltal error and it is no managed to

Quoted 8 lines

recover and continue, instead a restart of the processor is needed, if the fault happens in core0 we need to reset core1 too and vice versa. if the iommu would support several user callbacks, we can register the callback which resets core0 and also the callback which resets core1 and treat them as totally independent processors. Also we have an error event notifier driver, which is only in charge of notifying error events to userspace, so we would have multiple callbacks we could do this

The original purpose of the patch, as far as I understand, is to allow getting useful information for debugging purposes should an iommu fault happen.

Also, I'm not sure it's necessarily a good idea to just go and reset the M3 cores in case an iommu fault happens --- this is very probably a grave bug in the software running on those M3s. It should be fixed instead of just hiding it. There will be consequences to host side as well, won't there?

Quoted 8 lines

iommu <---- register fault callback for error notify driver
instead of
iommu <--- register fault callback for remote processor driver <----register fault event for error notify driver.
with that, we remove one dependency of the errornotify driver.

I suppose this is not in mainline?

Regards,

Signature · 2 lines

Sakari Ailus
[email protected]

David Cohen

author

2011-02-23 20:21 UTC #22 lore raw

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

Quoted 23 lines · Sakari Ailus

Quoted 1 line · Fernando
Hi,
Hi Fernando,
Quoted 5 lines
In OMAP4 the cortex M3 is a double core processor and as each core is running they own version of the RTOS we threat them independently. So our driver which controls the remote processor sees two processor but both use the same iommu hw. When a iommu fault happens, at this moment, it is consider as a faltal error and it is no managed to
Quoted 8 lines
recover and continue, instead a restart of the processor is needed, if the fault happens in core0 we need to reset core1 too and vice versa. if the iommu would support several user callbacks, we can register the callback which resets core0 and also the callback which resets core1 and treat them as totally independent processors. Also we have an error event notifier driver, which is only in charge of notifying error events to userspace, so we would have multiple callbacks we could do this
The original purpose of the patch, as far as I understand, is to allow getting useful information for debugging purposes should an iommu fault happen.

Quoted 5 lines

Also, I'm not sure it's necessarily a good idea to just go and reset the M3 cores in case an iommu fault happens --- this is very probably a grave bug in the software running on those M3s. It should be fixed instead of just hiding it. There will be consequences to host side as well, won't there?

That's really a good point. callbacks in this situation are mostly to debug purpose. Drivers shouldn't rely on that to work properly.

David

Quoted 22 lines

Quoted 9 lines
iommu <---- register fault callback for error notify driver
instead of
iommu <--- register fault callback for remote processor driver <----register fault event for error notify driver.
with that, we remove one dependency of the errornotify driver.
I suppose this is not in mainline?
Regards,
--
Sakari Ailus
sakari.ailus at maxwell.research.nokia.com
--
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

Guzman Lugo, Fernando

2011-02-23 21:30 UTC #23 lore raw

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

In reply to Sakari Ailus

Quoted 23 lines · Sakari Ailus

Quoted 1 line · Fernando
Hi,
Hi Fernando,
Quoted 5 lines
In OMAP4 the cortex M3 is a double core processor and as each core is running they own version of the RTOS we threat them independently. So our driver which controls the remote processor sees two processor but both use the same iommu hw. When a iommu fault happens, at this moment, it is consider as a faltal error and it is no managed to
Quoted 8 lines
recover and continue, instead a restart of the processor is needed, if the fault happens in core0 we need to reset core1 too and vice versa. if the iommu would support several user callbacks, we can register the callback which resets core0 and also the callback which resets core1 and treat them as totally independent processors. Also we have an error event notifier driver, which is only in charge of notifying error events to userspace, so we would have multiple callbacks we could do this
The original purpose of the patch, as far as I understand, is to allow getting useful information for debugging purposes should an iommu fault happen.

Quoted 5 lines

Also, I'm not sure it's necessarily a good idea to just go and reset the M3 cores in case an iommu fault happens --- this is very probably a grave bug in the software running on those M3s. It should be fixed instead of just hiding it. There will be consequences to host side as well, won't there?

the code running in the M3 side is a RTOS, and it does not have something like kernel and userspace as linux, so if some app crashes it crash the whole system (like a crash in a driver) it can be improved later. And the issues in the host side are taking into account, host apps are notified about the issue and they release and start the communication with the remote cores.

But event if we were able to fixed the issue, think is this example

you have two independent OS running on each core of the cortex M3 (core0 and core1). You make your own mapping in core0 and your own mapping in core1 and each core has a callback for mmu fault:

case 1: mmufault in core1:

1.- iommu fault isr in iommu module is triggered and it calls to all callbacks.

2.- it calls callback for core0 (cb_c0)

3.- cb_c0 tries to fix the problem but, as it does not have information about that fault address (core1 does)it can not fixed the issue and return an error to say it could not manage the issue.

4.- iommu fault isr check the value returned by cb_c0 and it sees cb_c0 did not fix the issue, so it call to the next callback (cb_c1)

5.- cb_c1 is executed and fixes the issue and they continue working.

case 2: mmufault in core0:

1.- iommu fault isr in iommu module is triggered and it calls to all callbacks.

2.- it calls callback for core0 (cb_c0)

3.- cb_c0 fixes the problem and returns a success value.

4.- iommu fault isr check the value returned by cb_c0 and it sees cb_c0 it fixed the issue, so it does not call any other callback (cb_c1)

So, multiple callbacks looks really nice for me.

Regards,
Fernando.

Quoted 18 lines

Quoted 9 lines
iommu <---- register fault callback for error notify driver
instead of
iommu <--- register fault callback for remote processor driver <----register fault event for error notify driver.
with that, we remove one dependency of the errornotify driver.
I suppose this is not in mainline?
Regards,
--
Sakari Ailus
sakari.ailus at maxwell.research.nokia.com

Felipe Balbi

2011-02-24 08:35 UTC #24 lore raw

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

In reply to Sakari Ailus

Hi,

Quoted 14 lines · Sakari Ailus

Quoted 5 lines
In OMAP4 the cortex M3 is a double core processor and as each core is running they own version of the RTOS we threat them independently. So our driver which controls the remote processor sees two processor but both use the same iommu hw. When a iommu fault happens, at this moment, it is consider as a faltal error and it is no managed to
Quoted 8 lines
recover and continue, instead a restart of the processor is needed, if the fault happens in core0 we need to reset core1 too and vice versa. if the iommu would support several user callbacks, we can register the callback which resets core0 and also the callback which resets core1 and treat them as totally independent processors. Also we have an error event notifier driver, which is only in charge of notifying error events to userspace, so we would have multiple callbacks we could do this

Quoted 8 lines

The original purpose of the patch, as far as I understand, is to allow getting useful information for debugging purposes should an iommu fault happen.
Also, I'm not sure it's necessarily a good idea to just go and reset the M3 cores in case an iommu fault happens --- this is very probably a grave bug in the software running on those M3s. It should be fixed instead of just hiding it. There will be consequences to host side as

I have to agree here. Besides the fact that multiple callbacks is outside the scope of this patch.

Signature · 1 line

balbi

David Cohen

author

2011-02-24 11:26 UTC #25 lore raw

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

Quoted 21 lines · Felipe Balbi

Hi,
Quoted 18 lines · Sakari Ailus
Quoted 5 lines
In OMAP4 the cortex M3 is a double core processor and as each core is running they own version of the RTOS we threat them independently. So our driver which controls the remote processor sees two processor but both use the same iommu hw. When a iommu fault happens, at this moment, it is consider as a faltal error and it is no managed to
Quoted 8 lines
recover and continue, instead a restart of the processor is needed, if the fault happens in core0 we need to reset core1 too and vice versa. if the iommu would support several user callbacks, we can register the callback which resets core0 and also the callback which resets core1 and treat them as totally independent processors. Also we have an error event notifier driver, which is only in charge of notifying error events to userspace, so we would have multiple callbacks we could do this
The original purpose of the patch, as far as I understand, is to allow getting useful information for debugging purposes should an iommu fault happen.

Quoted 7 lines

Quoted 4 lines
Also, I'm not sure it's necessarily a good idea to just go and reset the M3 cores in case an iommu fault happens --- this is very probably a grave bug in the software running on those M3s. It should be fixed instead of just hiding it. There will be consequences to host side as
I have to agree here. Besides the fact that multiple callbacks is outside the scope of this patch.

This patch is already acked. What about leave it as it is and discuss multiple callbacks before release a new patch to support it?

Br,

David

Quoted 4 lines

--
balbi

Felipe Balbi

2011-02-24 11:29 UTC #26 lore raw

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

Quoted 27 lines · David Cohen

Quoted 26 lines · Felipe Balbi
Hi,
Quoted 22 lines · Sakari Ailus
Quoted 5 lines
In OMAP4 the cortex M3 is a double core processor and as each core is running they own version of the RTOS we threat them independently. So our driver which controls the remote processor sees two processor but both use the same iommu hw. When a iommu fault happens, at this moment, it is consider as a faltal error and it is no managed to
Quoted 8 lines
recover and continue, instead a restart of the processor is needed, if the fault happens in core0 we need to reset core1 too and vice versa. if the iommu would support several user callbacks, we can register the callback which resets core0 and also the callback which resets core1 and treat them as totally independent processors. Also we have an error event notifier driver, which is only in charge of notifying error events to userspace, so we would have multiple callbacks we could do this
The original purpose of the patch, as far as I understand, is to allow getting useful information for debugging purposes should an iommu fault happen.
Also, I'm not sure it's necessarily a good idea to just go and reset the M3 cores in case an iommu fault happens --- this is very probably a grave bug in the software running on those M3s. It should be fixed instead of just hiding it. There will be consequences to host side as

Quoted 5 lines

Quoted 2 lines
I have to agree here. Besides the fact that multiple callbacks is outside the scope of this patch.
This patch is already acked. What about leave it as it is and discuss multiple callbacks before release a new patch to support it?

fine by me ;-)

Signature · 1 line

balbi

Guzman Lugo, Fernando

2011-02-24 17:58 UTC #27 lore raw

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

Quoted 28 lines · Felipe Balbi

Quoted 27 lines · David Cohen
Quoted 26 lines · Felipe Balbi
Hi,
Quoted 22 lines · Sakari Ailus
Quoted 5 lines
In OMAP4 the cortex M3 is a double core processor and as each core is running they own version of the RTOS we threat them independently. So our driver which controls the remote processor sees two processor but both use the same iommu hw. When a iommu fault happens, at this moment, it is consider as a faltal error and it is no managed to
Quoted 8 lines
recover and continue, instead a restart of the processor is needed, if the fault happens in core0 we need to reset core1 too and vice versa. if the iommu would support several user callbacks, we can register the callback which resets core0 and also the callback which resets core1 and treat them as totally independent processors. Also we have an error event notifier driver, which is only in charge of notifying error events to userspace, so we would have multiple callbacks we could do this
The original purpose of the patch, as far as I understand, is to allow getting useful information for debugging purposes should an iommu fault happen.
Also, I'm not sure it's necessarily a good idea to just go and reset the M3 cores in case an iommu fault happens --- this is very probably a grave bug in the software running on those M3s. It should be fixed instead of just hiding it. There will be consequences to host side as

Quoted 7 lines

Quoted 5 lines
Quoted 2 lines
I have to agree here. Besides the fact that multiple callbacks is outside the scope of this patch.
This patch is already acked. What about leave it as it is and discuss multiple callbacks before release a new patch to support it?
fine by me ;-)

Ok, maybe it was too late to change it, due to it is already acked, I just wanted to avoid change isr here and then change it on other patch. it is ok then.

Regards,
Fernando.

Quoted 4 lines

--
balbi

Tony Lindgren

maintainer

2011-02-24 20:31 UTC #28 lore raw

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

Quoted 4 lines · Guzman Lugo, Fernando

Quoted 3 lines · Felipe Balbi
Quoted 2 lines · David Cohen
On Thu, Feb 24, 2011 at 10:35 AM, Felipe Balbi <[email protected]> wrote:

Quoted 8 lines

Quoted 4 lines
Quoted 2 lines
This patch is already acked. What about leave it as it is and discuss multiple callbacks before release a new patch to support it?
fine by me ;-)
Ok, maybe it was too late to change it, due to it is already acked, I just wanted to avoid change isr here and then change it on other patch. it is ok then.

Hiroshi any comments?

Would like to see a proper Acked-by before I apply, now there's just the earlier comment "Ok, I see. Let's go with this. Thanks." :)

Tony

Tony Lindgren

maintainer

2011-02-24 22:21 UTC #29 lore raw

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

Quoted 10 lines · Tony Lindgren

Quoted 9 lines · Guzman Lugo, Fernando
Quoted 7 lines · Felipe Balbi
Quoted 4 lines · David Cohen
On Thu, Feb 24, 2011 at 10:35 AM, Felipe Balbi <[email protected]> wrote:
This patch is already acked. What about leave it as it is and discuss multiple callbacks before release a new patch to support it?
fine by me ;-)

Quoted 8 lines

Quoted 3 lines
Ok, maybe it was too late to change it, due to it is already acked, I just wanted to avoid change isr here and then change it on other patch. it is ok then.
Hiroshi any comments?
Would like to see a proper Acked-by before I apply, now there's just the earlier comment "Ok, I see. Let's go with this. Thanks." :)

Ah I see an ack in another mail. So applying these.

Tony

Hiroshi DOYU

2011-02-21 09:52 UTC #30 lore raw

v3 cover OMAP: IOMMU fault callback support

In reply to David Cohen

From: David Cohen <[email protected]>
Subject: [PATCH v3 0/2] OMAP: IOMMU fault callback support
Date: Wed, 16 Feb 2011 21:35:49 +0200

Quoted 8 lines

Hi,
This patch set adapts current (*isr)() to be used as fault callback. 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.

Tony, please put them in your queue too. Thanks.

Quoted 8 lines

Br,
David
---
David Cohen (2):
OMAP2+: IOMMU: don't print fault warning on specific layer
OMAP: IOMMU: add support to callback during fault handling

Acked-by Hiroshi DOYU <[email protected]>

Quoted 9 lines

 arch/arm/mach-omap2/iommu2.c            |   33 +++++++++-----------
 arch/arm/plat-omap/include/plat/iommu.h |   14 ++++++++-
 arch/arm/plat-omap/iommu.c              |   52 ++++++++++++++++++++++---------
 3 files changed, 65 insertions(+), 34 deletions(-)

Signature · 1 line

1.7.2.3