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:
> > On Thu, 2018-02-08 at 15:39 -0600, Bjorn Helgaas wrote:
> > > 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.
>
> > 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.
