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.
