Re: [PATCH kernel] powerpc/pci: Fix broken INTx configuration via OF

On Thu, 2018-02-08 at 16:42 -0600, Bjorn Helgaas wrote:

On Fri, Feb 09, 2018 at 09:21:43AM +1100, Benjamin Herrenschmidt wrote:

quoted

On Thu, 2018-02-08 at 15:39 -0600, Bjorn Helgaas wrote:

quoted

I don't understand how this fix works.  We used to check the result of
of_irq_parse_and_map_pci() and entered the block if it was zero.

Now you enter the block if it is zero or less than zero, but:

  static int pci_read_irq_line(...)
  {
    unsigned int virq = 0;     /* unnecessarily initialized, BTW */

    virq = of_irq_parse_and_map_pci(pci_dev, 0, 0);
    if (virq <= 0) {
      ...

virq is unsigned, so "virq < 0" can never be true.  So how does this
change anything?

Yes it does:

So the unsigned thing is a second bug in the original patch that Alexey
isn't fixing, we need to fix it too.

However, the actual bug Alexey is fixing is that we lost the actual
value of virq. IE, without his fix, we test it for 0 but we don't
actually return it if it's positive.

Ah, I see, the bug is that we discarded the non-zero virq value when
we actually need it.  I'm going to wait for a new patch with a
changelog that says that and doesn't test an unsigned value for < 0.

quoted

So he fixes the normal case but there's still a bug in the error case,
we need to make virq signed.

I looked through the of_irq_parse_and_map_pci() path and I do not see
a case where it can return a negative value.  It either returns zero
or one of these:

  virq = irq_find_mapping(...)
  virq = irq_create_mapping(...)

Both of these functions return unsigned values.

Ok so the test is just wrong then. Aleey, can you respin ?

Cheers,
Ben.