* [PATCH v3 0/2] OMAP: IOMMU fault callback support
@ 2011-02-16 19:35 David Cohen
2011-02-16 19:35 ` [PATCH v3 1/2] OMAP2+: IOMMU: don't print fault warning on specific layer David Cohen
` (2 more replies)
0 siblings, 3 replies; 31+ messages in thread
From: David Cohen @ 2011-02-16 19:35 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v3 1/2] OMAP2+: IOMMU: don't print fault warning on specific layer
2011-02-16 19:35 [PATCH v3 0/2] OMAP: IOMMU fault callback support David Cohen
@ 2011-02-16 19:35 ` David Cohen
2011-02-16 19:35 ` [PATCH v3 2/2] OMAP: IOMMU: add support to callback during fault handling David Cohen
2011-02-21 9:52 ` [PATCH v3 0/2] OMAP: IOMMU fault callback support Hiroshi DOYU
2 siblings, 0 replies; 31+ messages in thread
From: David Cohen @ 2011-02-16 19:35 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v3 2/2] OMAP: IOMMU: add support to callback during fault handling
2011-02-16 19:35 [PATCH v3 0/2] OMAP: IOMMU fault callback support David Cohen
2011-02-16 19:35 ` [PATCH v3 1/2] OMAP2+: IOMMU: don't print fault warning on specific layer David Cohen
@ 2011-02-16 19:35 ` David Cohen
2011-02-21 8:18 ` Hiroshi DOYU
` (2 more replies)
2011-02-21 9:52 ` [PATCH v3 0/2] OMAP: IOMMU fault callback support Hiroshi DOYU
2 siblings, 3 replies; 31+ messages in thread
From: David Cohen @ 2011-02-16 19:35 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v3 2/2] OMAP: IOMMU: add support to callback during fault handling
2011-02-16 19:35 ` [PATCH v3 2/2] OMAP: IOMMU: add support to callback during fault handling David Cohen
@ 2011-02-21 8:18 ` Hiroshi DOYU
2011-02-21 8:22 ` Felipe Balbi
2011-02-21 9:07 ` David Cohen
2011-02-21 18:43 ` Ramirez Luna, Omar
2011-02-23 1:17 ` Guzman Lugo, Fernando
2 siblings, 2 replies; 31+ messages in thread
From: Hiroshi DOYU @ 2011-02-21 8:18 UTC (permalink / raw)
To: linux-arm-kernel
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
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v3 2/2] OMAP: IOMMU: add support to callback during fault handling
2011-02-21 8:18 ` Hiroshi DOYU
@ 2011-02-21 8:22 ` Felipe Balbi
2011-02-21 8:57 ` David Cohen
2011-02-21 9:07 ` David Cohen
1 sibling, 1 reply; 31+ messages in thread
From: Felipe Balbi @ 2011-02-21 8:22 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v3 2/2] OMAP: IOMMU: add support to callback during fault handling
2011-02-21 8:22 ` Felipe Balbi
@ 2011-02-21 8:57 ` David Cohen
2011-02-21 9:20 ` Felipe Balbi
0 siblings, 1 reply; 31+ messages in thread
From: David Cohen @ 2011-02-21 8:57 UTC (permalink / raw)
To: linux-arm-kernel
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
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v3 2/2] OMAP: IOMMU: add support to callback during fault handling
2011-02-21 8:18 ` Hiroshi DOYU
2011-02-21 8:22 ` Felipe Balbi
@ 2011-02-21 9:07 ` David Cohen
2011-02-21 9:51 ` Hiroshi DOYU
1 sibling, 1 reply; 31+ messages in thread
From: David Cohen @ 2011-02-21 9:07 UTC (permalink / raw)
To: linux-arm-kernel
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
>>
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v3 2/2] OMAP: IOMMU: add support to callback during fault handling
2011-02-21 8:57 ` David Cohen
@ 2011-02-21 9:20 ` Felipe Balbi
0 siblings, 0 replies; 31+ messages in thread
From: Felipe Balbi @ 2011-02-21 9:20 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v3 2/2] OMAP: IOMMU: add support to callback during fault handling
2011-02-21 9:07 ` David Cohen
@ 2011-02-21 9:51 ` Hiroshi DOYU
0 siblings, 0 replies; 31+ messages in thread
From: Hiroshi DOYU @ 2011-02-21 9:51 UTC (permalink / raw)
To: linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v3 0/2] OMAP: IOMMU fault callback support
2011-02-16 19:35 [PATCH v3 0/2] OMAP: IOMMU fault callback support David Cohen
2011-02-16 19:35 ` [PATCH v3 1/2] OMAP2+: IOMMU: don't print fault warning on specific layer David Cohen
2011-02-16 19:35 ` [PATCH v3 2/2] OMAP: IOMMU: add support to callback during fault handling David Cohen
@ 2011-02-21 9:52 ` Hiroshi DOYU
2 siblings, 0 replies; 31+ messages in thread
From: Hiroshi DOYU @ 2011-02-21 9:52 UTC (permalink / raw)
To: linux-arm-kernel
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
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v3 2/2] OMAP: IOMMU: add support to callback during fault handling
2011-02-16 19:35 ` [PATCH v3 2/2] OMAP: IOMMU: add support to callback during fault handling David Cohen
2011-02-21 8:18 ` Hiroshi DOYU
@ 2011-02-21 18:43 ` Ramirez Luna, Omar
2011-02-21 21:12 ` David Cohen
2011-02-23 1:17 ` Guzman Lugo, Fernando
2 siblings, 1 reply; 31+ messages in thread
From: Ramirez Luna, Omar @ 2011-02-21 18:43 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v3 2/2] OMAP: IOMMU: add support to callback during fault handling
2011-02-21 18:43 ` Ramirez Luna, Omar
@ 2011-02-21 21:12 ` David Cohen
2011-02-21 23:25 ` Ramirez Luna, Omar
0 siblings, 1 reply; 31+ messages in thread
From: David Cohen @ 2011-02-21 21:12 UTC (permalink / raw)
To: linux-arm-kernel
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
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v3 2/2] OMAP: IOMMU: add support to callback during fault handling
2011-02-21 21:12 ` David Cohen
@ 2011-02-21 23:25 ` Ramirez Luna, Omar
2011-02-21 23:48 ` David Cohen
0 siblings, 1 reply; 31+ messages in thread
From: Ramirez Luna, Omar @ 2011-02-21 23:25 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v3 2/2] OMAP: IOMMU: add support to callback during fault handling
2011-02-21 23:25 ` Ramirez Luna, Omar
@ 2011-02-21 23:48 ` David Cohen
0 siblings, 0 replies; 31+ messages in thread
From: David Cohen @ 2011-02-21 23:48 UTC (permalink / raw)
To: linux-arm-kernel
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
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v3 2/2] OMAP: IOMMU: add support to callback during fault handling
2011-02-16 19:35 ` [PATCH v3 2/2] OMAP: IOMMU: add support to callback during fault handling David Cohen
2011-02-21 8:18 ` Hiroshi DOYU
2011-02-21 18:43 ` Ramirez Luna, Omar
@ 2011-02-23 1:17 ` Guzman Lugo, Fernando
2011-02-23 9:45 ` David Cohen
2 siblings, 1 reply; 31+ messages in thread
From: Guzman Lugo, Fernando @ 2011-02-23 1:17 UTC (permalink / raw)
To: linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v3 2/2] OMAP: IOMMU: add support to callback during fault handling
2011-02-23 1:17 ` Guzman Lugo, Fernando
@ 2011-02-23 9:45 ` David Cohen
2011-02-23 13:39 ` Guzman Lugo, Fernando
0 siblings, 1 reply; 31+ messages in thread
From: David Cohen @ 2011-02-23 9:45 UTC (permalink / raw)
To: linux-arm-kernel
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.
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v3 2/2] OMAP: IOMMU: add support to callback during fault handling
2011-02-23 9:45 ` David Cohen
@ 2011-02-23 13:39 ` Guzman Lugo, Fernando
2011-02-23 19:54 ` David Cohen
2011-02-23 20:09 ` Sakari Ailus
0 siblings, 2 replies; 31+ messages in thread
From: Guzman Lugo, Fernando @ 2011-02-23 13:39 UTC (permalink / raw)
To: linux-arm-kernel
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.
>>
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v3 2/2] OMAP: IOMMU: add support to callback during fault handling
2011-02-23 13:39 ` Guzman Lugo, Fernando
@ 2011-02-23 19:54 ` David Cohen
2011-02-23 20:56 ` Sakari Ailus
2011-02-23 21:12 ` Guzman Lugo, Fernando
2011-02-23 20:09 ` Sakari Ailus
1 sibling, 2 replies; 31+ messages in thread
From: David Cohen @ 2011-02-23 19:54 UTC (permalink / raw)
To: linux-arm-kernel
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.
>>>
>>
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v3 2/2] OMAP: IOMMU: add support to callback during fault handling
2011-02-23 13:39 ` Guzman Lugo, Fernando
2011-02-23 19:54 ` David Cohen
@ 2011-02-23 20:09 ` Sakari Ailus
2011-02-23 20:21 ` David Cohen
` (2 more replies)
1 sibling, 3 replies; 31+ messages in thread
From: Sakari Ailus @ 2011-02-23 20:09 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v3 2/2] OMAP: IOMMU: add support to callback during fault handling
2011-02-23 20:09 ` Sakari Ailus
@ 2011-02-23 20:21 ` David Cohen
2011-02-23 21:30 ` Guzman Lugo, Fernando
2011-02-24 8:35 ` Felipe Balbi
2 siblings, 0 replies; 31+ messages in thread
From: David Cohen @ 2011-02-23 20:21 UTC (permalink / raw)
To: linux-arm-kernel
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
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v3 2/2] OMAP: IOMMU: add support to callback during fault handling
2011-02-23 19:54 ` David Cohen
@ 2011-02-23 20:56 ` Sakari Ailus
2011-02-23 21:48 ` Guzman Lugo, Fernando
2011-02-23 21:12 ` Guzman Lugo, Fernando
1 sibling, 1 reply; 31+ messages in thread
From: Sakari Ailus @ 2011-02-23 20:56 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v3 2/2] OMAP: IOMMU: add support to callback during fault handling
2011-02-23 19:54 ` David Cohen
2011-02-23 20:56 ` Sakari Ailus
@ 2011-02-23 21:12 ` Guzman Lugo, Fernando
1 sibling, 0 replies; 31+ messages in thread
From: Guzman Lugo, Fernando @ 2011-02-23 21:12 UTC (permalink / raw)
To: linux-arm-kernel
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.
>>>>
>>>
>>
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v3 2/2] OMAP: IOMMU: add support to callback during fault handling
2011-02-23 20:09 ` Sakari Ailus
2011-02-23 20:21 ` David Cohen
@ 2011-02-23 21:30 ` Guzman Lugo, Fernando
2011-02-24 8:35 ` Felipe Balbi
2 siblings, 0 replies; 31+ messages in thread
From: Guzman Lugo, Fernando @ 2011-02-23 21:30 UTC (permalink / raw)
To: linux-arm-kernel
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
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v3 2/2] OMAP: IOMMU: add support to callback during fault handling
2011-02-23 20:56 ` Sakari Ailus
@ 2011-02-23 21:48 ` Guzman Lugo, Fernando
2011-02-24 6:39 ` David Cohen
0 siblings, 1 reply; 31+ messages in thread
From: Guzman Lugo, Fernando @ 2011-02-23 21:48 UTC (permalink / raw)
To: linux-arm-kernel
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
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v3 2/2] OMAP: IOMMU: add support to callback during fault handling
2011-02-23 21:48 ` Guzman Lugo, Fernando
@ 2011-02-24 6:39 ` David Cohen
0 siblings, 0 replies; 31+ messages in thread
From: David Cohen @ 2011-02-24 6:39 UTC (permalink / raw)
To: linux-arm-kernel
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
>>
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v3 2/2] OMAP: IOMMU: add support to callback during fault handling
2011-02-23 20:09 ` Sakari Ailus
2011-02-23 20:21 ` David Cohen
2011-02-23 21:30 ` Guzman Lugo, Fernando
@ 2011-02-24 8:35 ` Felipe Balbi
2011-02-24 11:26 ` David Cohen
2 siblings, 1 reply; 31+ messages in thread
From: Felipe Balbi @ 2011-02-24 8:35 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v3 2/2] OMAP: IOMMU: add support to callback during fault handling
2011-02-24 8:35 ` Felipe Balbi
@ 2011-02-24 11:26 ` David Cohen
2011-02-24 11:29 ` Felipe Balbi
0 siblings, 1 reply; 31+ messages in thread
From: David Cohen @ 2011-02-24 11:26 UTC (permalink / raw)
To: linux-arm-kernel
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
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v3 2/2] OMAP: IOMMU: add support to callback during fault handling
2011-02-24 11:26 ` David Cohen
@ 2011-02-24 11:29 ` Felipe Balbi
2011-02-24 17:58 ` Guzman Lugo, Fernando
0 siblings, 1 reply; 31+ messages in thread
From: Felipe Balbi @ 2011-02-24 11:29 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v3 2/2] OMAP: IOMMU: add support to callback during fault handling
2011-02-24 11:29 ` Felipe Balbi
@ 2011-02-24 17:58 ` Guzman Lugo, Fernando
2011-02-24 20:31 ` Tony Lindgren
0 siblings, 1 reply; 31+ messages in thread
From: Guzman Lugo, Fernando @ 2011-02-24 17:58 UTC (permalink / raw)
To: linux-arm-kernel
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
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v3 2/2] OMAP: IOMMU: add support to callback during fault handling
2011-02-24 17:58 ` Guzman Lugo, Fernando
@ 2011-02-24 20:31 ` Tony Lindgren
2011-02-24 22:21 ` Tony Lindgren
0 siblings, 1 reply; 31+ messages in thread
From: Tony Lindgren @ 2011-02-24 20:31 UTC (permalink / raw)
To: linux-arm-kernel
* 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
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v3 2/2] OMAP: IOMMU: add support to callback during fault handling
2011-02-24 20:31 ` Tony Lindgren
@ 2011-02-24 22:21 ` Tony Lindgren
0 siblings, 0 replies; 31+ messages in thread
From: Tony Lindgren @ 2011-02-24 22:21 UTC (permalink / raw)
To: linux-arm-kernel
* 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
^ permalink raw reply [flat|nested] 31+ messages in thread
end of thread, other threads:[~2011-02-24 22:21 UTC | newest]
Thread overview: 31+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-02-16 19:35 [PATCH v3 0/2] OMAP: IOMMU fault callback support David Cohen
2011-02-16 19:35 ` [PATCH v3 1/2] OMAP2+: IOMMU: don't print fault warning on specific layer David Cohen
2011-02-16 19:35 ` [PATCH v3 2/2] OMAP: IOMMU: add support to callback during fault handling David Cohen
2011-02-21 8:18 ` Hiroshi DOYU
2011-02-21 8:22 ` Felipe Balbi
2011-02-21 8:57 ` David Cohen
2011-02-21 9:20 ` Felipe Balbi
2011-02-21 9:07 ` David Cohen
2011-02-21 9:51 ` Hiroshi DOYU
2011-02-21 18:43 ` Ramirez Luna, Omar
2011-02-21 21:12 ` David Cohen
2011-02-21 23:25 ` Ramirez Luna, Omar
2011-02-21 23:48 ` David Cohen
2011-02-23 1:17 ` Guzman Lugo, Fernando
2011-02-23 9:45 ` David Cohen
2011-02-23 13:39 ` Guzman Lugo, Fernando
2011-02-23 19:54 ` David Cohen
2011-02-23 20:56 ` Sakari Ailus
2011-02-23 21:48 ` Guzman Lugo, Fernando
2011-02-24 6:39 ` David Cohen
2011-02-23 21:12 ` Guzman Lugo, Fernando
2011-02-23 20:09 ` Sakari Ailus
2011-02-23 20:21 ` David Cohen
2011-02-23 21:30 ` Guzman Lugo, Fernando
2011-02-24 8:35 ` Felipe Balbi
2011-02-24 11:26 ` David Cohen
2011-02-24 11:29 ` Felipe Balbi
2011-02-24 17:58 ` Guzman Lugo, Fernando
2011-02-24 20:31 ` Tony Lindgren
2011-02-24 22:21 ` Tony Lindgren
2011-02-21 9:52 ` [PATCH v3 0/2] OMAP: IOMMU fault callback support Hiroshi DOYU
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).