Re: [PATCH v3] pinctrl: ocelot: Fix interrupt controller

[Michael Walle <michael@walle.cc>]

Hi Horatiu,

Am 2022-09-20 21:30, schrieb Horatiu Vultur:
> The 09/20/2022 14:06, Michael Walle wrote:
>> > When an external device generated a level based interrupt then the
>> > interrupt controller could miss the interrupt. The reason is that the
>> > interrupt controller can detect only link changes.
>> >
>> > In the following example, if there is a PHY that generates an interrupt
>> > then the following would happen. The GPIO detected that the interrupt
>> > line changed, and then the 'ocelot_irq_handler' was called. Here it
>> > detects which GPIO line saw the change and for that will call the
>> > following:
>> > 1. irq_mask
>> > 2. phy interrupt routine
>> > 3. irq_eoi
>> > 4. irq_unmask
>> >
>> > And this works fine for simple cases, but if the PHY generates many
>> > interrupts, for example when doing PTP timestamping, then the following
>> > could happen. Again the function 'ocelot_irq_handler' will be called
>> > and then from here the following could happen:
>> > 1. irq_mask
>> > 2. phy interrupt routine
>> > 3. irq_eoi
>> > 4. irq_unmask
>> >
>> > Right before step 3(irq_eoi), the PHY will generate another interrupt.
>> > Now the interrupt controller will acknowledge the change in the
>> > interrupt line. So we miss the interrupt.
>> >
>> > A solution will be to use 'handle_level_irq' instead of
>> > 'handle_fasteoi_irq', because for this will change routine order of
>> > handling the interrupt.
>> > 1. irq_mask
>> > 2. irq_ack
>> > 3. phy interrupt routine
>> > 4. irq_unmask
>> >
>> > And now if the PHY will generate a new interrupt before irq_unmask, the
>> > interrupt controller will detect this because it already acknowledge the
>> > change in interrupt line at step 2(irq_ack).
>> >
>> > But this is not the full solution because there is another issue. In
>> > case there are 2 PHYs that share the interrupt line. For example phy1
>> > generates an interrupt, then the following can happen:
>> > 1.irq_mask
>> > 2.irq_ack
>> > 3.phy0 interrupt routine
>> > 4.phy1 interrupt routine
>> > 5.irq_unmask
>> >
>> > In case phy0 will generate an interrupt while clearing the interrupt
>> > source in phy1, then the interrupt line will be kept down by phy0. So
>> > the interrupt controller will not see any changes in the interrupt line.
>> > The solution here is to update 'irq_unmask' such that it can detect if
>> > the interrupt line is still active or not. And if it is active then call
>> > again the procedure to clear the interrupts. But we don't want to do it
>> > every time, only if we know that the interrupt controller has not seen
>> > already that the interrupt line has changed.
>> >
>> > While at this, add support also for IRQ_TYPE_LEVEL_LOW.
>> 
>> Our board has a shared active low interrupt line, connected to a quad 
>> PHY
>> LAN8814 and two GPY215 PHYs. I've gave this a try but it doesn't seem 
>> to
>> work. It seems the interrupt fires multiple times. If I plug a cable 
>> in
>> one of the LAN8814 ports, I see that the interrupt count in
>> /proc/interrupts has increased by two. If I use a GPY215 port, I see 
>> about
>> 40 interrupts firing.
> 
> I really don't know why would see 40 interrupts on GPY215.

The GPY215 seems to be broken in this regard and hold the interrupt
line asserted even after its interrupt status register is read.

> But I can
> explain why you see 2 interrupts with LAN8814 ports.
> The reason is that the interrupt controller in the pinctrl detects 
> edges
> and not levels. So if we take an example: the PHY will generate an
> interrupt by pulling down the line. Then pinctrl detects this change in
> the line so will start interrupt handler rutine. It would mask, ack,
> call the PHY interrupt routine. At this point when the PHY clears the
> interrupt source, the interrupt line will be high. So the interrupt
> controller will see also this change. Then when the interrupt 
> controller
> will unmask the interrupt, it would generate a new one. And this is the
> second interrupt.
> I didn't know that is a big issue to get another interrupt. Because
> before it was possible to miss interrupts, so I thought it was a pretty
> fair trade.

Seeing 20 was definitely fishy, seeing two instead of one maybe not
so much. I guess it will create one spurious interrupt if none of
the registered handlers will care.

OTOH, the code below won't work in all cases anyway, right? It's just
best effort.

> Below I have a diff that I tried with LAN8814 PHYs and I could see that
> count in /proc/interrupts is increasing correctly.
> 
>> I've verified that there is only one low pulse on the interrupt line. 
>> I've
>> noticed though, that the number of interrupts seem to be correlating 
>> with
>> the length of the low pulse.
> ---
> diff --git a/drivers/pinctrl/pinctrl-ocelot.c 
> b/drivers/pinctrl/pinctrl-ocelot.c
> index c7df8c5fe5854..105771ff82e62 100644
> --- a/drivers/pinctrl/pinctrl-ocelot.c
> +++ b/drivers/pinctrl/pinctrl-ocelot.c
> @@ -1863,19 +1863,28 @@ static void ocelot_irq_unmask_level(struct
> irq_data *data)
>  	if (val & bit)
>  		ack = true;
> 
> +	/* Try to clear any rising edges */
> +	if (!active && ack)
> +		regmap_write_bits(info->map, REG(OCELOT_GPIO_INTR, info, gpio),
> +				  bit, bit);

Might we lose interrupts here, if the line would go active again right
after the read of the line state and before reading the "ack" bit?

> +
>  	/* Enable the interrupt now */
>  	gpiochip_enable_irq(chip, gpio);
>  	regmap_update_bits(info->map, REG(OCELOT_GPIO_INTR_ENA, info, gpio),
>  			   bit, bit);
> 
>  	/*
> -	 * In case the interrupt line is still active and the interrupt
> -	 * controller has not seen any changes in the interrupt line, then it
> -	 * means that there happen another interrupt while the line was 
> active.
> +	 * In case the interrupt line is still active then it means that
> +	 * there happen another interrupt while the line was active.
>  	 * So we missed that one, so we need to kick the interrupt again
>  	 * handler.
>  	 */
> -	if (active && !ack) {
> +	regmap_read(info->map, REG(OCELOT_GPIO_IN, info, gpio), &val);
> +	if ((!(val & bit) && trigger_level == IRQ_TYPE_LEVEL_LOW) ||
> +	      (val & bit && trigger_level == IRQ_TYPE_LEVEL_HIGH))
> +		active = true;

Why do you read the line state twice? What happens if the line state
changes right after you've read it?

> +
> +	if (active) {
>  		struct ocelot_irq_work *work;
> 
>  		work = kmalloc(sizeof(*work), GFP_ATOMIC);

So yes, maybe the trade-off that there will be two interrupts are
better than this additional patch. But it should be documented
somewhere, even if it's just a comment in this driver.

-michael