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

[Lorenzo Pieralisi <lpieralisi@kernel.org>]

On Fri, May 09, 2025 at 10:07:44AM +0200, Lorenzo Pieralisi wrote:
> 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.

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/powerpc/platforms/52xx/mpc52xx_pic.c?h=v6.15-rc5#n218

There are other examples in powerpc, this does not look right to me.

Lorenzo

> 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