Re: [PATCH v4 03/10] irqchip/sun6i-r: Use a stacked irqchip driver

[Marc Zyngier]

On 2021-01-15 04:01, Samuel Holland wrote:
> Hello,
>
> On 1/14/21 3:06 PM, Marc Zyngier wrote:
> > Hi Samuel,
> >
> > On 2021-01-12 05:59, Samuel Holland wrote:
> >
> > [...]
> >
> > > +static void sun6i_r_intc_ack_nmi(void)
> > > +{
> > > +	writel(SUN6I_NMI_BIT, base + SUN6I_IRQ_PENDING(0));
> >
> > writel_relaxed()
>
> irq_chip_unmask_parent(), which calls gic_unmask_irq(), is called
> immediately after this in .irq_unmask. Since gic_unmask_irq() also uses
> writel_relaxed(), the GIC write could be ordered before the write here.

That's odd. writel() places a barrier *before* the actual write,
ensuring that this write is ordered w.r.t. previous accesses.
If you are trying to ensure ordering with what follows, you need
an explicit barrier after this access.

I guess that in the end, you may need both, as what you have orders
the access to GICC_AIR to take place before the write to this pending
register, and you also need to provide the ordering you just described.

> I was getting occasional spurious interrupts (1 out of each 20-25) when
> using a level trigger, which were resolved by switching to writel() 
> here.
>
> I mentioned this in the changelog, but it probably deserves a comment 
> in
> the code as well. Or maybe I should use an explicit barrier somewhere?

Please document it in the code. This is subtle enough to warrant a good
description.

> > > +}
> > > +
> > > +static void sun6i_r_intc_nmi_ack(struct irq_data *data)
> > > +{
> > > +	if (irqd_get_trigger_type(data) & IRQ_TYPE_EDGE_BOTH)
> > > +		sun6i_r_intc_ack_nmi();
> > > +	else
> > > +		data->chip_data = SUN6I_NMI_NEEDS_ACK;
> > > +}
> > > +
> > > +static void sun6i_r_intc_nmi_eoi(struct irq_data *data)
> > > +{
> > > +	/* For oneshot IRQs, delay the ack until the IRQ is unmasked. */
> > > +	if (data->chip_data == SUN6I_NMI_NEEDS_ACK && 
> > > !irqd_irq_masked(data))
> > > {
> > > +		sun6i_r_intc_ack_nmi();
> > > +		data->chip_data = 0;
> >
> > nit: NULL rather than 0?
>
> NULL seemed less appropriate since I'm not using the field as a 
> pointer,
> but I don't have a strong opinion about it.

chip_data *is* a pointer, which is why we conventionally use NULL rather
than an integer value. Up to you.

> > [...]
> >
> > > +static struct irq_chip sun6i_r_intc_nmi_chip = {
> > > +	.name			= "sun6i-r-intc",
> > > +	.irq_ack		= sun6i_r_intc_nmi_ack,
> > > +	.irq_mask		= irq_chip_mask_parent,
> > > +	.irq_unmask		= sun6i_r_intc_nmi_unmask,
> > > +	.irq_eoi		= sun6i_r_intc_nmi_eoi,
> > > +	.irq_set_affinity	= irq_chip_set_affinity_parent,
> > > +	.irq_set_type		= sun6i_r_intc_nmi_set_type,
> > > +	.irq_set_irqchip_state	= sun6i_r_intc_nmi_set_irqchip_state,
> >
> > You probably also want to wire irq_get_irqchip_state(), while
> > you're at it.
>
> I thought if the interrupt was pending here, it would necessarily also
> be pending at the GIC, so adding a separate layer would be redundant.
>
> irq_set_vcpu_affinity(), __irq_get_irqchip_state(), and
> irq_set_irqchip_state() [the functions, not the callbacks] have the
> interesting property that they search up the irqdomain hierarchy for 
> the
> first irqdomain with the callback. So if all the callback would do is
> defer to its parent, it doesn't need to be provided at all*.

Ah, of course... I even wrote that code!

Thanks,

         M.
--
Jazz is not dead. It just smells funny...