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

[Lorenzo Pieralisi <lpieralisi@kernel.org>]

On Thu, May 08, 2025 at 12:44:45PM +0200, Lorenzo Pieralisi wrote:

[...]

> 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);

I don't get why the settings above are written only if the irqchip
has an irq_set_type() method, maybe they should be updated in
irqdomain code (?) where:

irqd_set_trigger_type()

is executed after creating the fwspec mapping ?

Is it possible we never noticed because we have always had irqchips that
do implement irq_set_type() ?

Again, I don't know the history behind the IRQD_LEVEL flag so it is just
a question, I'd need to get this clarified though please if I remove the
PPI irq_set_type() callback.

Thanks,
Lorenzo

> 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 ?
> 
> 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.
> 
> > > 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.
> 
> > > Obviously if firmware exposes the wrong type that's a firmware bug
> > > but I was wondering whether it is better to fail the firmware-to-Linux
> > > IRQ translation if the firmware provided type is wrong rather than carry
> > > on pretending that the type is correct (I was abusing the irq_set_type()
> > > callback to do just that - namely, check that the type provided by
> > > firmware matches HW but I think that's the wrong place to put it).
> > 
> > I don't think there is anything to do. Worse case, you spit a
> > pr_warn_once() and carry on.
> 
> Thanks,
> Lorenzo