[PATCH v3 0/2] OMAP: IOMMU fault callback support

[PATCH v3 0/2] OMAP: IOMMU fault callback support

[David Cohen]

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

-- 
1.7.2.3

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

[David Cohen]

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 <dacohen@gmail.com>
---
 arch/arm/mach-omap2/iommu2.c |   16 ----------------
 1 files changed, 0 insertions(+), 16 deletions(-)

diff --git a/arch/arm/mach-omap2/iommu2.c b/arch/arm/mach-omap2/iommu2.c
index 14ee686..49a1e5e 100644
--- a/arch/arm/mach-omap2/iommu2.c
+++ b/arch/arm/mach-omap2/iommu2.c
@@ -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;
-- 
1.7.2.3

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

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

diff --git a/arch/arm/mach-omap2/iommu2.c b/arch/arm/mach-omap2/iommu2.c
index 49a1e5e..adb083e 100644
--- a/arch/arm/mach-omap2/iommu2.c
+++ b/arch/arm/mach-omap2/iommu2.c
@@ -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)
diff --git a/arch/arm/plat-omap/include/plat/iommu.h b/arch/arm/plat-omap/include/plat/iommu.h
index 19cbb5e..174f1b9 100644
--- a/arch/arm/plat-omap/include/plat/iommu.h
+++ b/arch/arm/plat-omap/include/plat/iommu.h
@@ -31,6 +31,7 @@ struct iommu {
 	struct clk	*clk;
 	void __iomem	*regbase;
 	struct device	*dev;
+	void		*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);
diff --git a/arch/arm/plat-omap/iommu.c b/arch/arm/plat-omap/iommu.c
index f55f458..b0e0efc 100644
--- a/arch/arm/plat-omap/iommu.c
+++ b/arch/arm/plat-omap/iommu.c
@@ -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
  */
-- 
1.7.2.3

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

[Hiroshi DOYU]

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

> 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 <dacohen@gmail.com>
> ---
>  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(-)
> 
> diff --git a/arch/arm/mach-omap2/iommu2.c b/arch/arm/mach-omap2/iommu2.c
> index 49a1e5e..adb083e 100644
> --- a/arch/arm/mach-omap2/iommu2.c
> +++ b/arch/arm/mach-omap2/iommu2.c
> @@ -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)
> diff --git a/arch/arm/plat-omap/include/plat/iommu.h b/arch/arm/plat-omap/include/plat/iommu.h
> index 19cbb5e..174f1b9 100644
> --- a/arch/arm/plat-omap/include/plat/iommu.h
> +++ b/arch/arm/plat-omap/include/plat/iommu.h
> @@ -31,6 +31,7 @@ struct iommu {
>  	struct clk	*clk;
>  	void __iomem	*regbase;
>  	struct device	*dev;
> +	void		*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?


>  	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);
> diff --git a/arch/arm/plat-omap/iommu.c b/arch/arm/plat-omap/iommu.c
> index f55f458..b0e0efc 100644
> --- a/arch/arm/plat-omap/iommu.c
> +++ b/arch/arm/plat-omap/iommu.c
> @@ -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
>   */
> -- 
> 1.7.2.3
> 

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

[Felipe Balbi]

Hi,

On Mon, Feb 21, 2011 at 10:18:56AM +0200, Hiroshi DOYU wrote:
> From: David Cohen <dacohen@gmail.com>
> Subject: [PATCH v3 2/2] OMAP: IOMMU: add support to callback during fault handling
> Date: Wed, 16 Feb 2011 21:35:51 +0200
> 
> > 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 <dacohen@gmail.com>
> > ---
> >  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(-)
> > 
> > diff --git a/arch/arm/mach-omap2/iommu2.c b/arch/arm/mach-omap2/iommu2.c
> > index 49a1e5e..adb083e 100644
> > --- a/arch/arm/mach-omap2/iommu2.c
> > +++ b/arch/arm/mach-omap2/iommu2.c
> > @@ -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)
> > diff --git a/arch/arm/plat-omap/include/plat/iommu.h b/arch/arm/plat-omap/include/plat/iommu.h
> > index 19cbb5e..174f1b9 100644
> > --- a/arch/arm/plat-omap/include/plat/iommu.h
> > +++ b/arch/arm/plat-omap/include/plat/iommu.h
> > @@ -31,6 +31,7 @@ struct iommu {
> >  	struct clk	*clk;
> >  	void __iomem	*regbase;
> >  	struct device	*dev;
> > +	void		*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"

-- 
balbi

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

[David Cohen]

On Mon, Feb 21, 2011 at 10:22 AM, Felipe Balbi <balbi@ti.com> wrote:
> Hi,
>
> On Mon, Feb 21, 2011 at 10:18:56AM +0200, Hiroshi DOYU wrote:
>> From: David Cohen <dacohen@gmail.com>
>> Subject: [PATCH v3 2/2] OMAP: IOMMU: add support to callback during fault handling
>> Date: Wed, 16 Feb 2011 21:35:51 +0200
>>
>> > 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 <dacohen@gmail.com>
>> > ---
>> > ?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(-)
>> >
>> > diff --git a/arch/arm/mach-omap2/iommu2.c b/arch/arm/mach-omap2/iommu2.c
>> > index 49a1e5e..adb083e 100644
>> > --- a/arch/arm/mach-omap2/iommu2.c
>> > +++ b/arch/arm/mach-omap2/iommu2.c
>> > @@ -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)
>> > diff --git a/arch/arm/plat-omap/include/plat/iommu.h b/arch/arm/plat-omap/include/plat/iommu.h
>> > index 19cbb5e..174f1b9 100644
>> > --- a/arch/arm/plat-omap/include/plat/iommu.h
>> > +++ b/arch/arm/plat-omap/include/plat/iommu.h
>> > @@ -31,6 +31,7 @@ struct iommu {
>> > ? ? struct clk ? ? ?*clk;
>> > ? ? void __iomem ? ?*regbase;
>> > ? ? struct device ? *dev;
>> > + ? void ? ? ? ? ? ?*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"

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

>
> --
> balbi
>

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

[Felipe Balbi]

Hi,

On Mon, Feb 21, 2011 at 10:57:45AM +0200, David Cohen wrote:
> >> 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.

-- 
balbi

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

[David Cohen]

On Mon, Feb 21, 2011 at 10:18 AM, Hiroshi DOYU <Hiroshi.DOYU@nokia.com> wrote:
> From: David Cohen <dacohen@gmail.com>
> Subject: [PATCH v3 2/2] OMAP: IOMMU: add support to callback during fault handling
> Date: Wed, 16 Feb 2011 21:35:51 +0200
>
>> 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 <dacohen@gmail.com>
>> ---
>> ?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(-)
>>
>> diff --git a/arch/arm/mach-omap2/iommu2.c b/arch/arm/mach-omap2/iommu2.c
>> index 49a1e5e..adb083e 100644
>> --- a/arch/arm/mach-omap2/iommu2.c
>> +++ b/arch/arm/mach-omap2/iommu2.c
>> @@ -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)
>> diff --git a/arch/arm/plat-omap/include/plat/iommu.h b/arch/arm/plat-omap/include/plat/iommu.h
>> index 19cbb5e..174f1b9 100644
>> --- a/arch/arm/plat-omap/include/plat/iommu.h
>> +++ b/arch/arm/plat-omap/include/plat/iommu.h
>> @@ -31,6 +31,7 @@ struct iommu {
>> ? ? ? struct clk ? ? ?*clk;
>> ? ? ? void __iomem ? ?*regbase;
>> ? ? ? struct device ? *dev;
>> + ? ? void ? ? ? ? ? ?*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

>
>
>> ? ? ? 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);
>> diff --git a/arch/arm/plat-omap/iommu.c b/arch/arm/plat-omap/iommu.c
>> index f55f458..b0e0efc 100644
>> --- a/arch/arm/plat-omap/iommu.c
>> +++ b/arch/arm/plat-omap/iommu.c
>> @@ -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
>> ? */
>> --
>> 1.7.2.3
>>
>

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

[Hiroshi DOYU]

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

> On Mon, Feb 21, 2011 at 10:18 AM, Hiroshi DOYU <Hiroshi.DOYU@nokia.com> wrote:
>> From: David Cohen <dacohen@gmail.com>
>> Subject: [PATCH v3 2/2] OMAP: IOMMU: add support to callback during fault handling
>> Date: Wed, 16 Feb 2011 21:35:51 +0200
>>
>>> 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 <dacohen@gmail.com>
>>> ---
>>> ?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(-)
>>>
>>> diff --git a/arch/arm/mach-omap2/iommu2.c b/arch/arm/mach-omap2/iommu2.c
>>> index 49a1e5e..adb083e 100644
>>> --- a/arch/arm/mach-omap2/iommu2.c
>>> +++ b/arch/arm/mach-omap2/iommu2.c
>>> @@ -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)
>>> diff --git a/arch/arm/plat-omap/include/plat/iommu.h b/arch/arm/plat-omap/include/plat/iommu.h
>>> index 19cbb5e..174f1b9 100644
>>> --- a/arch/arm/plat-omap/include/plat/iommu.h
>>> +++ b/arch/arm/plat-omap/include/plat/iommu.h
>>> @@ -31,6 +31,7 @@ struct iommu {
>>> ? ? ? struct clk ? ? ?*clk;
>>> ? ? ? void __iomem ? ?*regbase;
>>> ? ? ? struct device ? *dev;
>>> + ? ? void ? ? ? ? ? ?*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.

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

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

[Ramirez Luna, Omar]

Hi,

On Wed, Feb 16, 2011 at 1:35 PM, David Cohen <dacohen@gmail.com> wrote:
> diff --git a/arch/arm/mach-omap2/iommu2.c b/arch/arm/mach-omap2/iommu2.c
> index 49a1e5e..adb083e 100644
> --- a/arch/arm/mach-omap2/iommu2.c
> +++ b/arch/arm/mach-omap2/iommu2.c
...
>
> ? ? ? ?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;

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

> diff --git a/arch/arm/plat-omap/iommu.c b/arch/arm/plat-omap/iommu.c
> index f55f458..b0e0efc 100644
> --- a/arch/arm/plat-omap/iommu.c
> +++ b/arch/arm/plat-omap/iommu.c
> @@ -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;

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

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

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

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

[David Cohen]

On Mon, Feb 21, 2011 at 8:43 PM, Ramirez Luna, Omar <omar.ramirez@ti.com> wrote:
> Hi,

Hi,

Thanks for the comments.

>
> On Wed, Feb 16, 2011 at 1:35 PM, David Cohen <dacohen@gmail.com> wrote:
>> diff --git a/arch/arm/mach-omap2/iommu2.c b/arch/arm/mach-omap2/iommu2.c
>> index 49a1e5e..adb083e 100644
>> --- a/arch/arm/mach-omap2/iommu2.c
>> +++ b/arch/arm/mach-omap2/iommu2.c
> ...
>>
>> ? ? ? ?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;
>
> 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,

Have you noticed:
 0 = OMAP_IOMMU_ERR_TRANS_FAULT
 1 = OMAP_IOMMU_ERR_TLB_MISS

2 and 3 could be added.

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

>
> But anyway it is just me...
>
>> diff --git a/arch/arm/plat-omap/iommu.c b/arch/arm/plat-omap/iommu.c
>> index f55f458..b0e0efc 100644
>> --- a/arch/arm/plat-omap/iommu.c
>> +++ b/arch/arm/plat-omap/iommu.c
>> @@ -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;
>
> 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.

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

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

>
> Regards,
>
> Omar
>

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

[Ramirez Luna, Omar]

Hi,

On Mon, Feb 21, 2011 at 3:12 PM, David Cohen <dacohen@gmail.com> wrote:
> Generic errors codes make easier to threat possible IOMMU users which
> have (partially or totally) common drivers for different OMAP
> versions.

Agree then.

>>> + ? ? ? /* 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.

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

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

[David Cohen]

On Tue, Feb 22, 2011 at 1:25 AM, Ramirez Luna, Omar <omar.ramirez@ti.com> wrote:
> Hi,
>
> On Mon, Feb 21, 2011 at 3:12 PM, David Cohen <dacohen@gmail.com> wrote:
>> 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.

>
>>>> + ? ? ? /* 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.

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

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

>
> Regards,
>
> Omar
>

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

[Guzman Lugo, Fernando]

On Wed, Feb 16, 2011 at 1:35 PM, David Cohen <dacohen@gmail.com> wrote:
> 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 <dacohen@gmail.com>
> ---
> ?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(-)
>
....

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

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

[David Cohen]

On Wed, Feb 23, 2011 at 3:17 AM, Guzman Lugo, Fernando
<fernando.lugo@ti.com> wrote:
> On Wed, Feb 16, 2011 at 1:35 PM, David Cohen <dacohen@gmail.com> wrote:
>> 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 <dacohen@gmail.com>
>> ---
>> ?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(-)
>>
> ....
>
>> @@ -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)?

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

>
> Regards,
> Fernando.
>

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

[Guzman Lugo, Fernando]

On Wed, Feb 23, 2011 at 3:45 AM, David Cohen <dacohen@gmail.com> wrote:
> On Wed, Feb 23, 2011 at 3:17 AM, Guzman Lugo, Fernando
> <fernando.lugo@ti.com> wrote:
>> On Wed, Feb 16, 2011 at 1:35 PM, David Cohen <dacohen@gmail.com> wrote:
>>> 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 <dacohen@gmail.com>
>>> ---
>>> ?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(-)
>>>
>> ....
>>
>>> @@ -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)?
>
> 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.

>
> Br,
>
> David
>
>>
>> Regards,
>> Fernando.
>>
>

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

[David Cohen]

On Wed, Feb 23, 2011 at 3:39 PM, Guzman Lugo, Fernando
<fernando.lugo@ti.com> wrote:
> On Wed, Feb 23, 2011 at 3:45 AM, David Cohen <dacohen@gmail.com> wrote:
>> On Wed, Feb 23, 2011 at 3:17 AM, Guzman Lugo, Fernando
>> <fernando.lugo@ti.com> wrote:
>>> On Wed, Feb 16, 2011 at 1:35 PM, David Cohen <dacohen@gmail.com> wrote:
>>>> 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 <dacohen@gmail.com>
>>>> ---
>>>> ?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(-)
>>>>
>>> ....
>>>
>>>> @@ -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)?
>>
>> 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

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.

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

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

>
> Regards,
> Fernando.
>
>>
>> Br,
>>
>> David
>>
>>>
>>> Regards,
>>> Fernando.
>>>
>>
>

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

[Sakari Ailus]

David Cohen wrote:
> On Wed, Feb 23, 2011 at 3:39 PM, Guzman Lugo, Fernando
> <fernando.lugo@ti.com> wrote:
>> On Wed, Feb 23, 2011 at 3:45 AM, David Cohen <dacohen@gmail.com> wrote:
>>> On Wed, Feb 23, 2011 at 3:17 AM, Guzman Lugo, Fernando
>>> <fernando.lugo@ti.com> wrote:
>>>> On Wed, Feb 16, 2011 at 1:35 PM, David Cohen <dacohen@gmail.com> wrote:
>>>>> 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 <dacohen@gmail.com>
>>>>> ---
>>>>>  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(-)
>>>>>
>>>> ....
>>>>
>>>>> @@ -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)?
>>>
>>> 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
> 
> 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,

-- 
Sakari Ailus
sakari.ailus at maxwell.research.nokia.com

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

[Guzman Lugo, Fernando]

On Wed, Feb 23, 2011 at 2:56 PM, Sakari Ailus
<sakari.ailus@maxwell.research.nokia.com> wrote:
> David Cohen wrote:
>> On Wed, Feb 23, 2011 at 3:39 PM, Guzman Lugo, Fernando
>> <fernando.lugo@ti.com> wrote:
>>> On Wed, Feb 23, 2011 at 3:45 AM, David Cohen <dacohen@gmail.com> wrote:
>>>> On Wed, Feb 23, 2011 at 3:17 AM, Guzman Lugo, Fernando
>>>> <fernando.lugo@ti.com> wrote:
>>>>> On Wed, Feb 16, 2011 at 1:35 PM, David Cohen <dacohen@gmail.com> wrote:
>>>>>> 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 <dacohen@gmail.com>
>>>>>> ---
>>>>>> ?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(-)
>>>>>>
>>>>> ....
>>>>>
>>>>>> @@ -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)?
>>>>
>>>> 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
>>
>> 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...

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.

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

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

[David Cohen]

On Wed, Feb 23, 2011 at 11:48 PM, Guzman Lugo, Fernando
<fernando.lugo@ti.com> wrote:
> On Wed, Feb 23, 2011 at 2:56 PM, Sakari Ailus
> <sakari.ailus@maxwell.research.nokia.com> wrote:
>> David Cohen wrote:
>>> On Wed, Feb 23, 2011 at 3:39 PM, Guzman Lugo, Fernando
>>> <fernando.lugo@ti.com> wrote:
>>>> On Wed, Feb 23, 2011 at 3:45 AM, David Cohen <dacohen@gmail.com> wrote:
>>>>> On Wed, Feb 23, 2011 at 3:17 AM, Guzman Lugo, Fernando
>>>>> <fernando.lugo@ti.com> wrote:
>>>>>> On Wed, Feb 16, 2011 at 1:35 PM, David Cohen <dacohen@gmail.com> wrote:
>>>>>>> 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 <dacohen@gmail.com>
>>>>>>> ---
>>>>>>> ?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(-)
>>>>>>>
>>>>>> ....
>>>>>>
>>>>>>> @@ -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)?
>>>>>
>>>>> 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
>>>
>>> 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...
>
> 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

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

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

[Guzman Lugo, Fernando]

On Wed, Feb 23, 2011 at 1:54 PM, David Cohen <dacohen@gmail.com> wrote:
> On Wed, Feb 23, 2011 at 3:39 PM, Guzman Lugo, Fernando
> <fernando.lugo@ti.com> wrote:
>> On Wed, Feb 23, 2011 at 3:45 AM, David Cohen <dacohen@gmail.com> wrote:
>>> On Wed, Feb 23, 2011 at 3:17 AM, Guzman Lugo, Fernando
>>> <fernando.lugo@ti.com> wrote:
>>>> On Wed, Feb 16, 2011 at 1:35 PM, David Cohen <dacohen@gmail.com> wrote:
>>>>> 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 <dacohen@gmail.com>
>>>>> ---
>>>>> ?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(-)
>>>>>
>>>> ....
>>>>
>>>>> @@ -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)?
>>>
>>> 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
>
> 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.

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

> In this context, callback and ISR cannot share a same pointer anymore.
>
>>
>> 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.

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.

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

> 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
>
>>
>> Regards,
>> Fernando.
>>
>>>
>>> Br,
>>>
>>> David
>>>
>>>>
>>>> Regards,
>>>> Fernando.
>>>>
>>>
>>
>

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

[Sakari Ailus]

Guzman Lugo, Fernando wrote:
> Hi,

Hi Fernando,

> 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

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?

> 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

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

[David Cohen]

On Wed, Feb 23, 2011 at 10:09 PM, Sakari Ailus
<sakari.ailus@maxwell.research.nokia.com> wrote:
> Guzman Lugo, Fernando wrote:
>> Hi,
>
> Hi Fernando,
>
>> 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
>
> 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?

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

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

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

[Guzman Lugo, Fernando]

On Wed, Feb 23, 2011 at 2:09 PM, Sakari Ailus
<sakari.ailus@maxwell.research.nokia.com> wrote:
> Guzman Lugo, Fernando wrote:
>> Hi,
>
> Hi Fernando,
>
>> 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
>
> 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?

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.

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

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

[Felipe Balbi]

Hi,

On Wed, Feb 23, 2011 at 10:09:05PM +0200, Sakari Ailus wrote:
> > 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
> 
> 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.

-- 
balbi

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

[David Cohen]

On Thu, Feb 24, 2011 at 10:35 AM, Felipe Balbi <balbi@ti.com> wrote:
> Hi,
>
> On Wed, Feb 23, 2011 at 10:09:05PM +0200, Sakari Ailus wrote:
>> > 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
>>
>> 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.

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

>
> --
> balbi
>

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

[Felipe Balbi]

On Thu, Feb 24, 2011 at 01:26:05PM +0200, David Cohen wrote:
> On Thu, Feb 24, 2011 at 10:35 AM, Felipe Balbi <balbi@ti.com> wrote:
> > Hi,
> >
> > On Wed, Feb 23, 2011 at 10:09:05PM +0200, Sakari Ailus wrote:
> >> > 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
> >>
> >> 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.
> 
> 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 ;-)

-- 
balbi

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

[Guzman Lugo, Fernando]

On Thu, Feb 24, 2011 at 5:29 AM, Felipe Balbi <balbi@ti.com> wrote:
> On Thu, Feb 24, 2011 at 01:26:05PM +0200, David Cohen wrote:
>> On Thu, Feb 24, 2011 at 10:35 AM, Felipe Balbi <balbi@ti.com> wrote:
>> > Hi,
>> >
>> > On Wed, Feb 23, 2011 at 10:09:05PM +0200, Sakari Ailus wrote:
>> >> > 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
>> >>
>> >> 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.
>>
>> 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.

>
> --
> balbi
>

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

[Tony Lindgren]

* Guzman Lugo, Fernando <fernando.lugo@ti.com> [110224 09:56]:
> On Thu, Feb 24, 2011 at 5:29 AM, Felipe Balbi <balbi@ti.com> wrote:
> > On Thu, Feb 24, 2011 at 01:26:05PM +0200, David Cohen wrote:
> >> On Thu, Feb 24, 2011 at 10:35 AM, Felipe Balbi <balbi@ti.com> 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 ;-)
> 
> 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

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

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [110224 12:29]:
> * Guzman Lugo, Fernando <fernando.lugo@ti.com> [110224 09:56]:
> > On Thu, Feb 24, 2011 at 5:29 AM, Felipe Balbi <balbi@ti.com> wrote:
> > > On Thu, Feb 24, 2011 at 01:26:05PM +0200, David Cohen wrote:
> > >> On Thu, Feb 24, 2011 at 10:35 AM, Felipe Balbi <balbi@ti.com> 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 ;-)
> > 
> > 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

[PATCH v3 0/2] OMAP: IOMMU fault callback support

[Hiroshi DOYU]

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

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

> 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 <Hiroshi.DOYU@nokia.com>

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