Re: [PATCH v7 4/7] PCI: pci_alloc_child_bus() return NULL if ->add_bus() returns -ENOLINK

[Jim Quinlan]

On Wed, Nov 3, 2021 at 7:30 PM Pali Rohár <pali@xxxxxxxxxx> wrote:
>
> On Wednesday 03 November 2021 14:49:34 Jim Quinlan wrote:
> > Currently, if the call to the pci_ops add_bus() method returns an error, a
> > WARNING and dev_err() occurs.  We keep this behavior for all errors except
> > -ENOLINK; for -ENOLINK we want to skip the WARNING and immediately return
> > NULL.  The argument for this case is that one does not want to continue
> > enumerating if pcie-link has not been established.  The real reason is that
> > without doing this the pcie-brcmstb.c driver panics when the dev/id is
> > read, as this controller panics on such accesses rather than returning
> > 0xffffffff.
>
> I think that this is something which should be fixed in the driver, not
> in the pci core code. Check in driver code that you can touch HW and if
> not return fabricated value 0xffffffff.
Yes --  I don't have control of the config-space data but I do have control
of the address, and I can hijack the address so that it points to an accessible
register that holds 0xffffffff.

>
> > It appears that there are only a few uses of the pci_ops add_bus() method
> > in the kernel and none of them currently return -ENOLINK so it should be
> > safe to do this.
> >
> > Signed-off-by: Jim Quinlan <jim2101024@xxxxxxxxx>
> > ---
> >  drivers/pci/probe.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > index d9fc02a71baa..fdc3f42634b7 100644
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > @@ -1122,6 +1122,9 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
> >
> >       if (child->ops->add_bus) {
> >               ret = child->ops->add_bus(child);
> > +             /* Don't return the child if w/o pcie link-up */
> > +             if (ret == -ENOLINK)
>
> In my opinion ENOLINK is not the correct errno code for signaling
> "no link-up" error. IIRC ENOLINK was defined for file/inode links. For
> network connections there is ENETDOWN errno code which is more similar
> to "no link-up" than inode link.
>
> Anyway, I still do not think if it is a good idea to have this check in
> core pci code.
This commit is no longer needed per your suggestion of having the host-bridge
driver force a return of 0xffffffff.

Thanks,
Jim

>
> (This is just my opinion... wait for Bjorn with maintainer's hat what
> will say that is the best way to handle above issue)
>
> > +                     return NULL;
> >               if (WARN_ON(ret < 0))
> >                       dev_err(&child->dev, "failed to add bus: %d\n", ret);
> >       }
> > --
> > 2.17.1
> >