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

[Samuel Holland <samuel@sholland.org>]

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.

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?

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

> [...]
> 
>> +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*.

*except in case this irqdomain has a child which calls
irq_chip_get_parent_state(), which does not look past its immediate
parent. But I did not think that case was worth worrying about.

Cheers,
Samuel

> Otherwise, looks pretty good now.
> 
> Thanks,
> 
> 	M.
>