* [PATCH 0/2] IOMMU error message cleanup
@ 2011-01-31 17:25 David Cohen
2011-01-31 17:25 ` [PATCH 1/2] OMAP: Add generic IOMMU errors code David Cohen
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: David Cohen @ 2011-01-31 17:25 UTC (permalink / raw)
To: linux-omap; +Cc: tony
From: David Cohen <david@dhcppc2.(none)>
Hi,
OMAP IOMMU prints error messages twice. These patches remove the error
message from the OMAP2,3 specific implementation and let them to be
printed on the above layer only.
Br,
David
---
David Cohen (2):
OMAP: Add generic IOMMU errors code
OMAP: Cleanup IOMMU error messages
arch/arm/mach-omap2/iommu2.c | 33 +++++++++++++--------------
arch/arm/plat-omap/include/plat/iommu.h | 9 +++++++-
arch/arm/plat-omap/iommu.c | 36 ++++++++++++++++++++----------
3 files changed, 48 insertions(+), 30 deletions(-)
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH 1/2] OMAP: Add generic IOMMU errors code 2011-01-31 17:25 [PATCH 0/2] IOMMU error message cleanup David Cohen @ 2011-01-31 17:25 ` David Cohen 2011-01-31 17:25 ` [PATCH 2/2] OMAP: Cleanup IOMMU error messages David Cohen 2011-02-02 19:54 ` [PATCH 0/2] IOMMU error message cleanup Tony Lindgren 2 siblings, 0 replies; 6+ messages in thread From: David Cohen @ 2011-01-31 17:25 UTC (permalink / raw) To: linux-omap; +Cc: tony, David Cohen Generic IOMMU errors code are necessary to handle errors on generic layer. Signed-off-by: David Cohen <david.cohen@nokia.com> --- arch/arm/plat-omap/include/plat/iommu.h | 7 +++++++ 1 files changed, 7 insertions(+), 0 deletions(-) diff --git a/arch/arm/plat-omap/include/plat/iommu.h b/arch/arm/plat-omap/include/plat/iommu.h index 69230d6..c653fd7 100644 --- a/arch/arm/plat-omap/include/plat/iommu.h +++ b/arch/arm/plat-omap/include/plat/iommu.h @@ -109,6 +109,13 @@ struct iommu_platform_data { u32 da_end; }; +/* IOMMU errors */ +#define IOMMU_ERR_TLB_MISS (1 << 0) +#define IOMMU_ERR_TRANS_FAULT (1 << 1) +#define IOMMU_ERR_EMU_MISS (1 << 2) +#define IOMMU_ERR_TBLWALK_FAULT (1 << 3) +#define IOMMU_ERR_MULTIHIT_FAULT (1 << 4) + #if defined(CONFIG_ARCH_OMAP1) #error "iommu for this processor not implemented yet" #else -- 1.7.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] OMAP: Cleanup IOMMU error messages 2011-01-31 17:25 [PATCH 0/2] IOMMU error message cleanup David Cohen 2011-01-31 17:25 ` [PATCH 1/2] OMAP: Add generic IOMMU errors code David Cohen @ 2011-01-31 17:25 ` David Cohen 2011-02-01 3:16 ` Ramirez Luna, Omar 2011-02-02 19:54 ` [PATCH 0/2] IOMMU error message cleanup Tony Lindgren 2 siblings, 1 reply; 6+ messages in thread From: David Cohen @ 2011-01-31 17:25 UTC (permalink / raw) To: linux-omap; +Cc: tony, David Cohen IOMMU error messages are duplicated. They're printed on IOMMU specific layer for OMAP2,3 and once again on the above layer. With this patch, the error message is printed on the above layer only. Signed-off-by: David Cohen <david.cohen@nokia.com> --- arch/arm/mach-omap2/iommu2.c | 33 +++++++++++++-------------- arch/arm/plat-omap/include/plat/iommu.h | 2 +- arch/arm/plat-omap/iommu.c | 36 ++++++++++++++++++++---------- 3 files changed, 41 insertions(+), 30 deletions(-) diff --git a/arch/arm/mach-omap2/iommu2.c b/arch/arm/mach-omap2/iommu2.c index 14ee686..bb3d75b 100644 --- a/arch/arm/mach-omap2/iommu2.c +++ b/arch/arm/mach-omap2/iommu2.c @@ -143,33 +143,32 @@ static void omap2_iommu_set_twl(struct iommu *obj, bool on) __iommu_set_twl(obj, false); } -static u32 omap2_iommu_fault_isr(struct iommu *obj, u32 *ra) +static u32 omap2_iommu_fault_isr(struct iommu *obj, u32 *ra, u32 *iommu_errs) { - int i; + u32 errs = 0; u32 stat, da; - const char *err_msg[] = { - "tlb miss", - "translation fault", - "emulation miss", - "table walk fault", - "multi hit fault", - }; stat = iommu_read_reg(obj, MMU_IRQSTATUS); stat &= MMU_IRQ_MASK; - if (!stat) + if (!stat) { + *iommu_errs = 0; return 0; + } da = iommu_read_reg(obj, MMU_FAULT_AD); *ra = da; - dev_err(obj->dev, "%s:\tda:%08x ", __func__, da); - - for (i = 0; i < ARRAY_SIZE(err_msg); i++) { - if (stat & (1 << i)) - printk("%s ", err_msg[i]); - } - printk("\n"); + if (stat & MMU_IRQ_TLBMISS) + errs |= IOMMU_ERR_TLB_MISS; + if (stat & MMU_IRQ_TRANSLATIONFAULT) + errs |= IOMMU_ERR_TRANS_FAULT; + if (stat & MMU_IRQ_EMUMISS) + errs |= IOMMU_ERR_EMU_MISS; + if (stat & MMU_IRQ_TABLEWALKFAULT) + errs |= IOMMU_ERR_TBLWALK_FAULT; + if (stat & MMU_IRQ_MULTIHITFAULT) + errs |= IOMMU_ERR_MULTIHIT_FAULT; + *iommu_errs = errs; iommu_write_reg(obj, stat, MMU_IRQSTATUS); diff --git a/arch/arm/plat-omap/include/plat/iommu.h b/arch/arm/plat-omap/include/plat/iommu.h index c653fd7..267c5b5 100644 --- a/arch/arm/plat-omap/include/plat/iommu.h +++ b/arch/arm/plat-omap/include/plat/iommu.h @@ -83,7 +83,7 @@ struct iommu_functions { int (*enable)(struct iommu *obj); void (*disable)(struct iommu *obj); void (*set_twl)(struct iommu *obj, bool on); - u32 (*fault_isr)(struct iommu *obj, u32 *ra); + u32 (*fault_isr)(struct iommu *obj, u32 *ra, u32 *iommu_errs); void (*tlb_read_cr)(struct iommu *obj, struct cr_regs *cr); void (*tlb_load_cr)(struct iommu *obj, struct cr_regs *cr); diff --git a/arch/arm/plat-omap/iommu.c b/arch/arm/plat-omap/iommu.c index b1107c0..c7c37a0 100644 --- a/arch/arm/plat-omap/iommu.c +++ b/arch/arm/plat-omap/iommu.c @@ -163,9 +163,9 @@ static u32 get_iopte_attr(struct iotlb_entry *e) return arch_iommu->get_pte_attr(e); } -static u32 iommu_report_fault(struct iommu *obj, u32 *da) +static u32 iommu_report_fault(struct iommu *obj, u32 *da, u32 *iommu_errs) { - return arch_iommu->fault_isr(obj, da); + return arch_iommu->fault_isr(obj, da, iommu_errs); } static void iotlb_lock_get(struct iommu *obj, struct iotlb_lock *l) @@ -780,10 +780,18 @@ static void iopgtable_clear_entry_all(struct iommu *obj) */ static irqreturn_t iommu_fault_handler(int irq, void *data) { - u32 stat, da; + int i; + u32 stat, da, errs; u32 *iopgd, *iopte; int err = -EIO; struct iommu *obj = data; + const char *err_msg[] = { + "tlb miss", + "translation fault", + "emulation miss", + "table walk fault", + "multi hit fault", + }; if (!obj->refcount) return IRQ_NONE; @@ -796,7 +804,7 @@ static irqreturn_t iommu_fault_handler(int irq, void *data) return IRQ_HANDLED; clk_enable(obj->clk); - stat = iommu_report_fault(obj, &da); + stat = iommu_report_fault(obj, &da, &errs); clk_disable(obj->clk); if (!stat) return IRQ_HANDLED; @@ -805,16 +813,20 @@ static irqreturn_t iommu_fault_handler(int irq, void *data) iopgd = iopgd_offset(obj, da); - if (!iopgd_is_table(*iopgd)) { - dev_err(obj->dev, "%s: da:%08x pgd:%p *pgd:%08x\n", __func__, - da, iopgd, *iopgd); - return IRQ_NONE; + if (iopgd_is_table(*iopgd)) { + iopte = iopte_offset(iopgd, da); + dev_err(obj->dev, "da:%08x pgd:%p *pgd:%08x pte:%p *pte:%08x :", + da, iopgd, *iopgd, iopte, *iopte); + } else { + dev_err(obj->dev, "da:%08x pgd:%p *pgd:%08x :", da, iopgd, + *iopgd); } - iopte = iopte_offset(iopgd, da); - - dev_err(obj->dev, "%s: da:%08x pgd:%p *pgd:%08x pte:%p *pte:%08x\n", - __func__, da, iopgd, *iopgd, iopte, *iopte); + for (i = 0; i < ARRAY_SIZE(err_msg); i++) { + if (errs & (1 << i)) + printk(KERN_CONT " %s", err_msg[i]); + } + printk("\n"); return IRQ_NONE; } -- 1.7.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] OMAP: Cleanup IOMMU error messages 2011-01-31 17:25 ` [PATCH 2/2] OMAP: Cleanup IOMMU error messages David Cohen @ 2011-02-01 3:16 ` Ramirez Luna, Omar 2011-02-01 8:41 ` David Cohen 0 siblings, 1 reply; 6+ messages in thread From: Ramirez Luna, Omar @ 2011-02-01 3:16 UTC (permalink / raw) To: David Cohen; +Cc: linux-omap, tony, Hiroshi DOYU Hi David, On Mon, Jan 31, 2011 at 11:25 AM, David Cohen <david.cohen@nokia.com> wrote: > IOMMU error messages are duplicated. They're printed on IOMMU specific > layer for OMAP2,3 and once again on the above layer. With this patch, > the error message is printed on the above layer only. So, you say they are duplicated, previously the type of fault was printed in the omap2,3 code and the addresses involving the error printed on plat code... with this patch both messages are printed on plat code, I don't see where the duplication/fix is about. AFAIK, your patch removes this line: - dev_err(obj->dev, "%s:\tda:%08x ", __func__, da); Which I assume is the one being printed again in plat code, right? > Signed-off-by: David Cohen <david.cohen@nokia.com> > --- > arch/arm/mach-omap2/iommu2.c | 33 +++++++++++++-------------- > arch/arm/plat-omap/include/plat/iommu.h | 2 +- > arch/arm/plat-omap/iommu.c | 36 ++++++++++++++++++++---------- > 3 files changed, 41 insertions(+), 30 deletions(-) > > diff --git a/arch/arm/mach-omap2/iommu2.c b/arch/arm/mach-omap2/iommu2.c > index 14ee686..bb3d75b 100644 > --- a/arch/arm/mach-omap2/iommu2.c > +++ b/arch/arm/mach-omap2/iommu2.c > @@ -143,33 +143,32 @@ static void omap2_iommu_set_twl(struct iommu *obj, bool on) > __iommu_set_twl(obj, false); > } > > -static u32 omap2_iommu_fault_isr(struct iommu *obj, u32 *ra) > +static u32 omap2_iommu_fault_isr(struct iommu *obj, u32 *ra, u32 *iommu_errs) > { > - int i; > + u32 errs = 0; > u32 stat, da; > - const char *err_msg[] = { > - "tlb miss", > - "translation fault", > - "emulation miss", > - "table walk fault", > - "multi hit fault", > - }; AFAIU, this code living in mach-omap2, means that this errors are common for omap2,3,4 which they are. Does moving them to plat code implies that all omap platforms expect these errors? > stat = iommu_read_reg(obj, MMU_IRQSTATUS); > stat &= MMU_IRQ_MASK; > - if (!stat) > + if (!stat) { > + *iommu_errs = 0; > return 0; > + } > > da = iommu_read_reg(obj, MMU_FAULT_AD); > *ra = da; > > - dev_err(obj->dev, "%s:\tda:%08x ", __func__, da); > - > - for (i = 0; i < ARRAY_SIZE(err_msg); i++) { > - if (stat & (1 << i)) > - printk("%s ", err_msg[i]); > - } > - printk("\n"); > + if (stat & MMU_IRQ_TLBMISS) > + errs |= IOMMU_ERR_TLB_MISS; > + if (stat & MMU_IRQ_TRANSLATIONFAULT) > + errs |= IOMMU_ERR_TRANS_FAULT; > + if (stat & MMU_IRQ_EMUMISS) > + errs |= IOMMU_ERR_EMU_MISS; > + if (stat & MMU_IRQ_TABLEWALKFAULT) > + errs |= IOMMU_ERR_TBLWALK_FAULT; > + if (stat & MMU_IRQ_MULTIHITFAULT) > + errs |= IOMMU_ERR_MULTIHIT_FAULT; > + *iommu_errs = errs; Is there any point in doing this? Basically: *iommu_errs = stat, given that the mask values and the error defines are the same for each case. Besides I haven't seen two errors trigger a single interrupt. ... > > - iopte = iopte_offset(iopgd, da); > - > - dev_err(obj->dev, "%s: da:%08x pgd:%p *pgd:%08x pte:%p *pte:%08x\n", > - __func__, da, iopgd, *iopgd, iopte, *iopte); > + for (i = 0; i < ARRAY_SIZE(err_msg); i++) { > + if (errs & (1 << i)) > + printk(KERN_CONT " %s", err_msg[i]); > + } > + printk("\n"); I think we should get rid of the "printks". Regards, Omar -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] OMAP: Cleanup IOMMU error messages 2011-02-01 3:16 ` Ramirez Luna, Omar @ 2011-02-01 8:41 ` David Cohen 0 siblings, 0 replies; 6+ messages in thread From: David Cohen @ 2011-02-01 8:41 UTC (permalink / raw) To: ext Ramirez Luna, Omar; +Cc: linux-omap, tony, Hiroshi DOYU On Mon, Jan 31, 2011 at 09:16:30PM -0600, ext Ramirez Luna, Omar wrote: > Hi David, Hi Ramirez, Thanks for the comments. > > On Mon, Jan 31, 2011 at 11:25 AM, David Cohen <david.cohen@nokia.com> wrote: > > IOMMU error messages are duplicated. They're printed on IOMMU specific > > layer for OMAP2,3 and once again on the above layer. With this patch, > > the error message is printed on the above layer only. > > So, you say they are duplicated, previously the type of fault was > printed in the omap2,3 code and the addresses involving the error > printed on plat code... with this patch both messages are printed on > plat code, I don't see where the duplication/fix is about. > A little bit of my context: I'm working on OMAP3ISP driver. Due to hw or sw issues, iommu errors (mainly iommu faults) are not so rare to happen. e.g. I have 2 hw issues being workarounded on ISP H3A submodule (one of those a bit serious) which may lead to iommu faults. With the current implementation, the error messages are a bit useless and much "heavy". For each "fault", 2 error lines are printed to show the same error. On ISP context, usually it will happen *many* times in a row, making the device not usable even to debug the issue. And the bad side is we cannot get anything out of those messages which could help to point out where is the problem. As sometimes it's difficult to reproduce it, we loose a good chance to show a bit more useful data about the triggered error. My main intentions are: - Get rid of one line. - TODO: Print something useful to let developers to *know* what is going on. - TODO: Avoid to flood console to let it able to debug. > AFAIK, your patch removes this line: > > - dev_err(obj->dev, "%s:\tda:%08x ", __func__, da); > > Which I assume is the one being printed again in plat code, right? Yes. > > > Signed-off-by: David Cohen <david.cohen@nokia.com> > > --- > > arch/arm/mach-omap2/iommu2.c | 33 +++++++++++++-------------- > > arch/arm/plat-omap/include/plat/iommu.h | 2 +- > > arch/arm/plat-omap/iommu.c | 36 ++++++++++++++++++++---------- > > 3 files changed, 41 insertions(+), 30 deletions(-) > > > > diff --git a/arch/arm/mach-omap2/iommu2.c b/arch/arm/mach-omap2/iommu2.c > > index 14ee686..bb3d75b 100644 > > --- a/arch/arm/mach-omap2/iommu2.c > > +++ b/arch/arm/mach-omap2/iommu2.c > > @@ -143,33 +143,32 @@ static void omap2_iommu_set_twl(struct iommu *obj, bool on) > > __iommu_set_twl(obj, false); > > } > > > > -static u32 omap2_iommu_fault_isr(struct iommu *obj, u32 *ra) > > +static u32 omap2_iommu_fault_isr(struct iommu *obj, u32 *ra, u32 *iommu_errs) > > { > > - int i; > > + u32 errs = 0; > > u32 stat, da; > > - const char *err_msg[] = { > > - "tlb miss", > > - "translation fault", > > - "emulation miss", > > - "table walk fault", > > - "multi hit fault", > > - }; > > AFAIU, this code living in mach-omap2, means that this errors are > common for omap2,3,4 which they are. Does moving them to plat code > implies that all omap platforms expect these errors? That's true. I can work on a different approach. > > > stat = iommu_read_reg(obj, MMU_IRQSTATUS); > > stat &= MMU_IRQ_MASK; > > - if (!stat) > > + if (!stat) { > > + *iommu_errs = 0; > > return 0; > > + } > > > > da = iommu_read_reg(obj, MMU_FAULT_AD); > > *ra = da; > > > > - dev_err(obj->dev, "%s:\tda:%08x ", __func__, da); > > - > > - for (i = 0; i < ARRAY_SIZE(err_msg); i++) { > > - if (stat & (1 << i)) > > - printk("%s ", err_msg[i]); > > - } > > - printk("\n"); > > + if (stat & MMU_IRQ_TLBMISS) > > + errs |= IOMMU_ERR_TLB_MISS; > > + if (stat & MMU_IRQ_TRANSLATIONFAULT) > > + errs |= IOMMU_ERR_TRANS_FAULT; > > + if (stat & MMU_IRQ_EMUMISS) > > + errs |= IOMMU_ERR_EMU_MISS; > > + if (stat & MMU_IRQ_TABLEWALKFAULT) > > + errs |= IOMMU_ERR_TBLWALK_FAULT; > > + if (stat & MMU_IRQ_MULTIHITFAULT) > > + errs |= IOMMU_ERR_MULTIHIT_FAULT; > > + *iommu_errs = errs; > > Is there any point in doing this? > > Basically: *iommu_errs = stat, given that the mask values and the > error defines are the same for each case. True. But can I assume a specific register value will always match to the "generic" layer? > > Besides I haven't seen two errors trigger a single interrupt. Me neither, but the current implementation assumes it can happen and my concern is to not change it here. > > ... > > > > - iopte = iopte_offset(iopgd, da); > > - > > - dev_err(obj->dev, "%s: da:%08x pgd:%p *pgd:%08x pte:%p *pte:%08x\n", > > - __func__, da, iopgd, *iopgd, iopte, *iopte); > > + for (i = 0; i < ARRAY_SIZE(err_msg); i++) { > > + if (errs & (1 << i)) > > + printk(KERN_CONT " %s", err_msg[i]); > > + } > > + printk("\n"); > > I think we should get rid of the "printks". By working on a different approach for the printed messages, yes. Regards, David > > Regards, > > Omar -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 0/2] IOMMU error message cleanup 2011-01-31 17:25 [PATCH 0/2] IOMMU error message cleanup David Cohen 2011-01-31 17:25 ` [PATCH 1/2] OMAP: Add generic IOMMU errors code David Cohen 2011-01-31 17:25 ` [PATCH 2/2] OMAP: Cleanup IOMMU error messages David Cohen @ 2011-02-02 19:54 ` Tony Lindgren 2 siblings, 0 replies; 6+ messages in thread From: Tony Lindgren @ 2011-02-02 19:54 UTC (permalink / raw) To: David Cohen; +Cc: linux-omap Hi David, * David Cohen <david.cohen@nokia.com> [110131 09:24]: > From: David Cohen <david@dhcppc2.(none)> > > Hi, > > OMAP IOMMU prints error messages twice. These patches remove the error > message from the OMAP2,3 specific implementation and let them to be > printed on the above layer only. Can you also please Cc linux-arm-kernel list when sending arch/arm/*omap*/ patches? That way I don't need to repost them before applying. Regards, Tony ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-02-02 19:54 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-01-31 17:25 [PATCH 0/2] IOMMU error message cleanup David Cohen 2011-01-31 17:25 ` [PATCH 1/2] OMAP: Add generic IOMMU errors code David Cohen 2011-01-31 17:25 ` [PATCH 2/2] OMAP: Cleanup IOMMU error messages David Cohen 2011-02-01 3:16 ` Ramirez Luna, Omar 2011-02-01 8:41 ` David Cohen 2011-02-02 19:54 ` [PATCH 0/2] IOMMU error message cleanup Tony Lindgren
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.