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.co...@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.co...@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
