On Mon, 13 May 2019 04:15:54 +0000 Zenghui Yu <yuzeng...@huawei.com> 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? > Signed-off-by: Zenghui Yu <yuzeng...@huawei.com> > --- > 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. Cheers, Andre. > > irq_data_update_effective_affinity(d, cpumask_of(cpu)); >
