Re: [PATCH 3/3] irqchip/irq-pruss-intc: Fix processing of IEP interrupts

[Marc Zyngier]

On Tue, 19 Sep 2023 07:19:00 +0100,
MD Danish Anwar <danishanwar@xxxxxx> wrote:
> 
> From: Suman Anna <s-anna@xxxxxx>
> 
> It was discovered that IEP capture/compare IRQs (event #7 on all SoCs
> and event #56 on K3 SoCs) are always triggered twice when PPS is
> generated and CMP hit event detected by IEP.
> 
> An example of the problem is:
>   pruss_intc_irq_handler
>    generic_handle_irq
>     handle_level_irq
>       mask_ack_irq -> IRQ 7 masked and asked in INTC,
>                       but it's not yet cleared on HW level
>       handle_irq_event()
>         <threaded on RT>
>            icss_iep_cap_cmp_handler() -> IRQ 7 is actually processed in HW
>         irq_finalize_oneshot()
>          unmask_irq()
>            pruss_intc_irq_unmask() -> IRQ 7 status is still observed as set
> 
> The solution is to actually ack these IRQs from pruss_intc_irq_unmask()
> after the IRQ source is cleared in HW.

What you don't explain is whether the interrupt is level or edge
triggered? If it is level, then the "quirk" is that the interrupt
controller is slow to recognise that the level has changed. If it is
edge, this is a guaranteed recipe to lose interrupts.

Even if it is level, what happens if you issue a mask/unmask sequence
outside of the interrupt handling and that such an interrupt becomes
pending in between? Does this spurious ack have an effect on the now
pending interrupt?

> 
> No public errata available for this yet.
> 
> Fixes: 04e2d1e06978 ("irqchip/irq-pruss-intc: Add a PRUSS irqchip driver for PRUSS interrupts")
> 
> Signed-off-by: Grygorii Strashko <grygorii.strashko@xxxxxx>

Nit: drop the empty line after Fixes:.

> Signed-off-by: Suman Anna <s-anna@xxxxxx>
> Signed-off-by: MD Danish Anwar <danishanwar@xxxxxx>
> ---
>  drivers/irqchip/irq-pruss-intc.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/irqchip/irq-pruss-intc.c b/drivers/irqchip/irq-pruss-intc.c
> index 3cf684ede564..9907847dbda8 100644
> --- a/drivers/irqchip/irq-pruss-intc.c
> +++ b/drivers/irqchip/irq-pruss-intc.c
> @@ -70,6 +70,8 @@
>  #define MAX_PRU_SYS_EVENTS 160
>  #define MAX_PRU_CHANNELS 20
>  
> +#define MAX_PRU_INT_EVENTS	64
> +
>  /**
>   * struct pruss_intc_map_record - keeps track of actual mapping state
>   * @value: The currently mapped value (channel or host)
> @@ -85,10 +87,13 @@ struct pruss_intc_map_record {
>   * @num_system_events: number of input system events handled by the PRUSS INTC
>   * @num_host_events: number of host events (which is equal to number of
>   *		     channels) supported by the PRUSS INTC
> + * @quirky_events: bitmask of events that need quirky IRQ handling (limited to
> + *		   (internal sources only for now, so 64 bits suffice)
>   */
>  struct pruss_intc_match_data {
>  	u8 num_system_events;
>  	u8 num_host_events;
> +	u64 quirky_events;

Why limit this to the first 64 interrupts, while the intc can deal
with 160? What makes you confident that this is solely limited to this
particular source? And why this source only?

	M.

-- 
Without deviation from the norm, progress is not possible.