| Message ID | 20211103184939.45263-5-jim2101024@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | PCI: brcmstb: have host-bridge turn on sub-device power | expand |
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. > 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@gmail.com> > --- > 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 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 >
On Wed, Nov 3, 2021 at 7:30 PM Pali Rohár <pali@kernel.org> 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@gmail.com> > > --- > > 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 > >
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) + return NULL; if (WARN_ON(ret < 0)) dev_err(&child->dev, "failed to add bus: %d\n", ret); }
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. 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@gmail.com> --- drivers/pci/probe.c | 3 +++ 1 file changed, 3 insertions(+)