Re: [PATCH] iommu/arm-smmu: Clear global and context bank fault status registers

[Andreas Herrmann <andreas.herrmann-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>]

On Mon, Sep 30, 2013 at 12:06:15PM -0400, Will Deacon wrote:
> On Mon, Sep 30, 2013 at 02:56:21PM +0100, Andreas Herrmann wrote:
> > 
> > After reset these registers have unknown values.
> > This might cause problems when evaluating SMMU_GFSR and/or SMMU_CB_FSR
> > in handlers for combined interrupts.
> > 
> > Signed-off-by: Andreas Herrmann <andreas.herrmann-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
> > ---
> >  drivers/iommu/arm-smmu.c |   27 ++++++++++++++++++++-------
> >  1 file changed, 20 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> > index 579b6f8..cbbf597 100644
> > --- a/drivers/iommu/arm-smmu.c
> > +++ b/drivers/iommu/arm-smmu.c
> > @@ -631,6 +631,12 @@ static irqreturn_t arm_smmu_global_fault(int irq, void *dev)
> >  	return IRQ_HANDLED;
> >  }
> >  
> > +static void arm_smmu_clear_cb_fsr(struct arm_smmu_device *smmu, u8 cbndx)
> > +{
> > +	void __iomem *cb_base = ARM_SMMU_CB_BASE(smmu) + ARM_SMMU_CB(smmu, cbndx);
> > +	writel_relaxed(FSR_FAULT, cb_base + ARM_SMMU_CB_FSR);
> > +}
> 
> Hmm, why not just stick this in arm_smmu_init_context_bank...

Because we should clear the FSR before we call request_irq.
Otherwise we might handle interrupts although the context bank is not
enabled.

Moving request_irq after arm_smmu_init_context_bank is not optimal
either. (We should have configured the context interrupt before
translation is enabled. Otherwise it's possible to miss a fault.)

> >  static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain)
> >  {
> >  	u32 reg;
> > @@ -838,6 +844,7 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
> >  		root_cfg->irptndx = root_cfg->cbndx;
> >  	}
> >  
> > +	arm_smmu_clear_cb_fsr(smmu, root_cfg->cbndx);
> 
> ... then move that function call here instead of where it currently is?

See above.

> >  	irq = smmu->irqs[smmu->num_global_irqs + root_cfg->irptndx];
> >  	ret = request_irq(irq, arm_smmu_context_fault, IRQF_SHARED,
> >  			  "arm-smmu-context-fault", domain);
> > @@ -1564,7 +1571,11 @@ static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
> >  	void __iomem *gr0_base = ARM_SMMU_GR0(smmu);
> >  	void __iomem *sctlr_base = ARM_SMMU_CB_BASE(smmu) + ARM_SMMU_CB_SCTLR;
> >  	int i = 0;
> > -	u32 scr0 = readl_relaxed(gr0_base + ARM_SMMU_GR0_sCR0);
> > +	u32 val;
> 
> I guess we have to dilute the meaning of the variable, but perhaps `reg' is
> slightly more indicative than `val'?

Will change that.

> Do you plan to post all your patches as a new series (it's hard to keep
> track of what to pick)?

Yes.


Andreas