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

[Marc Zyngier <maz@kernel.org>]

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...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel