Re: [PATCH v3 20/25] irqchip/gic-v5: Add GICv5 PPI support

[Marc Zyngier <maz@kernel.org>]

On Thu, 08 May 2025 11:44:45 +0100,
Lorenzo Pieralisi <lpieralisi@kernel.org> wrote:
> 
> On Thu, May 08, 2025 at 09:42:27AM +0100, Marc Zyngier wrote:
> > On Thu, 08 May 2025 08:42:41 +0100,
> > Lorenzo Pieralisi <lpieralisi@kernel.org> wrote:
> > > 
> > > On Wed, May 07, 2025 at 04:57:07PM +0200, Thomas Gleixner wrote:
> > > > On Wed, May 07 2025 at 14:52, Marc Zyngier wrote:
> > > > > On Wed, 07 May 2025 14:42:42 +0100,
> > > > > Thomas Gleixner <tglx@linutronix.de> wrote:
> > > > >> 
> > > > >> On Wed, May 07 2025 at 10:14, Marc Zyngier wrote:
> > > > >> > On Tue, 06 May 2025 16:00:31 +0100,
> > > > >> > Thomas Gleixner <tglx@linutronix.de> wrote:
> > > > >> >> 
> > > > >> >> How does this test distinguish between LEVEL_LOW and LEVEL_HIGH? It only
> > > > >> >> tests for level, no? So the test is interesting at best ...
> > > > >> >
> > > > >> > There is no distinction between HIGH and LOW, RISING and FALLING, in
> > > > >> > any revision of the GIC architecture.
> > > > >> 
> > > > >> Then pretending that there is a set_type() functionality is pretty daft
> > > > >
> > > > > You still need to distinguish between level and edge when this is
> > > > > programmable (which is the case for a subset of the PPIs).
> > > > 
> > > > Fair enough, but can we please add a comment to this function which
> > > > explains this oddity.
> > > 
> > > Getting back to this, I would need your/Marc's input on this.
> > > 
> > > I think it is fair to remove the irq_set_type() irqchip callback for
> > > GICv5 PPIs because there is nothing to set, as I said handling mode
> > > for these IRQs is fixed. I don't think this can cause any trouble
> > > (IIUC a value within the IRQF_TRIGGER_MASK should be set on requesting
> > > an IRQ to "force" the trigger to be programmed and even then core code
> > > would not fail if the irq_set_type() irqchip callback is not
> > > implemented).
> > > 
> > > I am thinking about *existing* drivers that request GICv3 PPIs with
> > > values in IRQF_TRIGGER_MASK set (are there any ? Don't think so but you
> > > know better than I do), when we switch over to GICv5 we would have no
> > > irq_set_type() callback for PPIs but I think we are still fine, not
> > > implementing irqchip.irq_set_type() is correct IMO.
> > 
> > Nobody seems to use a hardcoded trigger (well, there is one exception,
> > but that's to paper over a firmware bug).
> 
> That's what I get if I remove the PPI irq_set_type() callback (just one
> timer, removed others because they add nothing) and enable debug for
> kernel/irq/manage.c (+additional printout):
> 
>  genirq: No set_type function for IRQ 70 (GICv5-PPI)
>   __irq_set_trigger+0x13c/0x180
>   __setup_irq+0x3d8/0x7c0
>   __request_percpu_irq+0xbc/0x114
>   arch_timer_register+0x84/0x140
>   arch_timer_of_init+0x180/0x1d0
>   timer_probe+0x74/0x124
>   time_init+0x18/0x58
>   start_kernel+0x198/0x384
>   __primary_switched+0x88/0x90
> 
>  arch_timer: check_ppi_trigger irq 70 flags 8
>  genirq: enable_percpu_irq irq 70 type 8
>  genirq: No set_type function for IRQ 70 (GICv5-PPI)
>   __irq_set_trigger+0x13c/0x180
>   enable_percpu_irq+0x100/0x140
>   arch_timer_starting_cpu+0x54/0xb8
>   cpuhp_issue_call+0x254/0x3a8
>   __cpuhp_setup_state_cpuslocked+0x208/0x2c8
>   __cpuhp_setup_state+0x50/0x74
>   arch_timer_register+0xc4/0x140
>   arch_timer_of_init+0x180/0x1d0
>   timer_probe+0x74/0x124
>   time_init+0x18/0x58
>   start_kernel+0x198/0x384
>   __primary_switched+0x88/0x90
> 
> I noticed that, if the irq_set_type() function is not implemented,
> we don't execute (in __irq_set_trigger()):
> 
> irq_settings_set_level(desc);
> irqd_set(&desc->irq_data, IRQD_LEVEL);
> 
> which in turn means that irqd_is_level_type(&desc->irq_data) is false
> for PPIs (ie arch timers, despite being level interrupts).
> 
> An immediate side effect is that they show as edge in:
> 
> /proc/interrupts
> 
> but that's just what I could notice.
> 
> Should I set them myself in PPI translate/alloc functions ?

When I say "do it in alloc", I mean "do whatever is needed to set
things up so that we can safely ignore the absence of the
.irq_set_type() callback -- this may even include some slight
modification of the core code, but that's not big deal".

> Removing the irq_set_type() for PPIs does not seem so innocuous, it is a
> bit complex to check all ramifications, please let me know if you spot
> something I have missed.

See above.

> 
> > > On the other hand, given that on GICv5 PPI handling mode is fixed,
> > > do you think that in the ppi_irq_domain_ops.translate() callback,
> > > I should check the type the firmware provided and fail the translation
> > > if it does not match the HW hardcoded value ?
> > 
> > Why? The fact that the firmware is wrong doesn't change the hardware
> > integration. It just indicates that whoever wrote the firmware didn't
> > read the documentation.
> > 
> > Even more, I wonder what the benefit of having that information in the
> > firmware tables if the only thing that matters in the immutable HW
> > view. Yes, having it in the DT/ACPI simplifies the job of the kernel
> > (only one format to parse). But it is overall useless information.
> 
> Yes, that I agree but it would force firmware bindings to special case
> PPIs to remove the type (#interrupt-cells and co.).
> 
> From what I read I understand I must ignore the PPI type provided by
> firmware.

You could warn on spotting that is inconsistent, but the HW view is
the only one that actually matters.

	M.

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