Re: [RFC PATCH] irqchip/gic-v3: Correct the usage of GICD_CTLR's RWP field

[Zenghui Yu]

Hi Andre,

On 2019/5/13 16:37, Andre Przywara wrote:
> On Mon, 13 May 2019 04:15:54 +0000
> Zenghui Yu <yuzenghui@xxxxxxxxxx> wrote:
>
> Hi,
>
> > As per ARM IHI 0069D, GICD_CTLR's RWP field tracks updates to:
> >
> > 	GICD_CTLR's Group Enable bits, for transitions from 1 to 0 only
> > 	GICD_CTLR's ARE bits, E1NWF bit and DS bit (if we have)
> > 	GICD_ICENABLER<n>
> >
> > We seemed use this field in an inappropriate way, somewhere in the
> > GIC-v3 driver. For some examples:
> >
> > In gic_set_affinity(), if the interrupt was not enabled, we only need
> > to write GICD_IROUTER<n> with the appropriate mpidr value. Updates to
> > GICD_IROUTER will not be tracked by RWP field, and we can remove the
> > unnecessary RWP waiting.
>
> I am not sure this is the proper fix, see below inline.
>
> > In gic_dist_init(), we "Enable distributor with ARE, Group1" by writing
> > to GICD_CTLR, and we should use gic_dist_wait_for_rwp() here.
>
> That looks reasonable, yes.
>
> > These two are obvious cases, and there are some other cases where we don't
> > need to do RWP waiting, such as in gic_configure_irq() and gic_poke_irq().
> > But too many modifications makes me not confident. It's hard for me to say
> > whether this patch is doing the right thing and how does the RWP waiting
> > affect the system, thus RFC.
>
> So did you actually see a problem, and this patch fixes it? Or was this
> just discovered by code inspection and comparing to the spec?

The latter ;-)

> > Signed-off-by: Zenghui Yu <yuzenghui@xxxxxxxxxx>
> > ---
> >   drivers/irqchip/irq-gic-v3.c | 8 ++------
> >   1 file changed, 2 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> > index 15e55d3..8d63950 100644
> > --- a/drivers/irqchip/irq-gic-v3.c
> > +++ b/drivers/irqchip/irq-gic-v3.c
> > @@ -600,6 +600,7 @@ static void __init gic_dist_init(void)
> >   	/* Enable distributor with ARE, Group1 */
> >   	writel_relaxed(GICD_CTLR_ARE_NS | GICD_CTLR_ENABLE_G1A |
> > GICD_CTLR_ENABLE_G1, base + GICD_CTLR);
> > +	gic_dist_wait_for_rwp();
> >   
> >   	/*
> >   	 * Set all global interrupts to the boot CPU only. ARE must be
> > @@ -986,14 +987,9 @@ static int gic_set_affinity(struct irq_data *d,
> > const struct cpumask *mask_val,
> >   	gic_write_irouter(val, reg);
> >   
> > -	/*
> > -	 * If the interrupt was enabled, enabled it again. Otherwise,
> > -	 * just wait for the distributor to have digested our changes.
> > -	 */
> > +	/* If the interrupt was enabled, enabled it again. */
> >   	if (enabled)
> >   		gic_unmask_irq(d);
> > -	else
> > -		gic_dist_wait_for_rwp();
>
> I think you are right in this is not needed here.
> But I guess this call belongs further up in this function, after the
> gic_mask_irq() call, as this one writes to GICD_ICENABLER. So in case this
> IRQ was enabled, we should wait for the distributor to have properly
> disabled it, before changing its affinity.

I still think we don't need this call in gic_set_affinity().
Actually, the writes to GICD_ICENABLER happens in gic_poke_irq(). And we
already have a gic_dist_wait_for_rwp() there, named rwp_wait().


thanks,
zenghui

> Cheers,
> Andre.
>
> >   
> >   	irq_data_update_effective_affinity(d, cpumask_of(cpu));
> >   
>
>
> .