Re: [PATCH 2/3] platform/x86: intel: punit_ipc: Propagate errors from optional IRQ lookup

[Bui Duc Phuc]

Hi Andy,

Thank you for your review .

>
> >         irq = platform_get_irq_optional(pdev, 0);
> >         if (irq < 0) {
> > +               if (irq != -ENXIO)
> > +                       return irq;
> >                 dev_warn(&pdev->dev, "Invalid IRQ, using polling mode\n");
>
> I can admit that the warning message is not comprehensive, but after
> this patch it obviously makes a regression. A previously working
> driver for even some Linux failures (besides deferred probe) now
> becomes unusable. Have you studied the possible error codes returned
> by platform_get_irq_optional()? What are they? Perhaps you need to
> improve the documentation of that API?
>

So I understand you'd prefer to keep the current behavior of the function,
to make sure existing systems keep working fine?

However, I think if this IRQs is truly meant to be optional, then any error
other than -ENXIO should be captured and returned
so that developers can actually investigate and fix the underlying issue.
Silently swallowing an error and falling back to polling doesn't
really guarantee
the system is working correctly or safely either, does it?

That said, if you'd rather keep the current implementation as-is,
then I think it would make more sense to switch to platform_get_irq() instead.
That would more accurately reflect that this driver doesn't actually
treat the IRQ
as optional, rather than using an "optional" lookup function while implementing
it as if it weren't optional.

https://elixir.bootlin.com/linux/v7.2-rc6/source/drivers/base/platform.c#L301

Best regards,
Phuc