Re: [PATCH v1 1/2] ata: libahci_platform: Get rid of dup message when IRQ can't be retrieved

On Fri, Dec 10, 2021 at 08:39:38PM +0300, Sergey Shtylyov wrote:

On 12/10/21 2:28 PM, Andy Shevchenko wrote:

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

While at it, drop redundant check for 0 as platform_get_irq() spills
out a big WARN() in such case.

   And? IRQ0 is still returned! :-(

It should not be returned in the first place.

   But it still is, despite the WARN(), right?

So, you admit that there is a code which does that?

   I admit *what*?! That platfrom_get_irq() and its ilk return IRQ0 while they
shouldn't? =)

That there is a code beneath platform_get_irq() that returns 0, yes.

   Look at the ACPI-specific GpioInt handling code (just above the out_not_found label) --
I'm not sure the check there is correct -- I'm not very familiar with ACPI, you seem to
know it much better. :-)

And what is your point here exactly?

   You're saying IRQ0 shouldn't be returned (by the ACPI code) -- from this fragment
we can see that it may be returned...

Please, provide your analysis, I really don't see how it's possible.
If you prove that, we must fix the severe bug then.

quoted

If == 0 case happens, it will be
immediately WARN() and reported (I hope)

   Well, "hope dies last"... :-)

Believe, big WARNs are quite likely to be reported if not by humans, then by
CIs and fuzzers. So, the hope is rather to word 'immediately'.

quoted

since it will mean bug in the code.

quoted

   Also, 0 can be specified via the normal IRQ resource. I know of e.g. the Alchemy MIPS SoCs
that have IRQ0 used by UART0; luckily, currently SoC IRQs are mapped starting at Linux IRQ8
(but it wasn't the case in the 2.6.1x time frame where we had issue with the serial driver)...

You mixed up HW IRQ with vIRQ.

   I didn't. Linux expects the vIRQs (I called them Linux IRQs). In the 2.6.1x time frame
those corresponded 1:1 on Alchemy. Also, there's 8259 which is always mapped at vIRQ0 (or
the legacy drivers won't work).

quoted

The former one may be 0 and it's completely valid case, while
the second one is not.

   Well, request_irq() happilly takes vIRQ0. Moreover, there are 8253 drivers in e.g. the arch/x86/
(PPC and MIPS too) which do use vIRQ0.

This is an exception which is not in the scope here. Let me remind that the
topic here is libahci_platform and platform_get_irq().

quoted

quoted

quoted

quoted

quoted

That code should be fixed first. Have you sent a patch?

   Which code?! You got me totally muddled. =)

Above mentioned.

   What needs to be fixed in this case is the interrupt controller driver.

What do you mean by that?

   You better ask Linus... ;-)

If you cite somebody you have to understand what they said, right?
Lemme repeat the question, what do you mean by that? In your own words, please.

quoted

vIRQ is handled by IRQ core, IRQ controller driver
just a mere provider of the resource. And those exceptions for vIRQ == 0
shouldn't be propagated to the platform code or so.

quoted

quoted

Quoting Linus
(imprecisely :-)), IRQ #s should be either mapped starting with #1 or IRQ0 remapped at
the end of the controller's interrupt range... I currently have no information on the
platforms requiring such kind of fixing (Alchemy don't seem to need it now)...

   Well, actually that Linus' quote predates drivers/irqchip/, so I must confess this
argument was wrong... :-)

quoted

Again, do not mix vIRQ (about which Linus ranted) and HW IRQ.

...

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

-	if (!irq)
-		return -EINVAL;

   This is prermature -- let's wait till my patch that stops returning IRQ0 from
platform_get_irq() and friends gets merged....

What patch?

   https://marc.info/?l=linux-kernel&m=163623041902285

quoted

Does it fix platform_get_irq_optional()?

   Of course! :-)

Can you share link to lore.kernel.org, please?
It will make much easier to try and comment.

   I don't know how to uise it yet, and I'm a little busy with other IRQ0 issues ATM,

quoted

   A little bit, I meant to type.

No problem. I just haven't got what other IRQ0 issues except fixing
platform_get_irq_optional() et al. could be possibly needed...

   There is other IRQ0 issue which is very old already...

Is it big secret? What is that issue?

-- 
With Best Regards,
Andy Shevchenko