[PATCH u-boot] dm: pci: Fix handling of errors when scanning device

[Pali Rohár]

On Wednesday 08 September 2021 20:57:06 Bin Meng wrote:
> On Wed, Sep 8, 2021 at 8:02 PM Marek Behún <kabel at kernel.org> wrote:
> >
> > On Wed, 8 Sep 2021 09:06:28 +0800
> > Bin Meng <bmeng.cn at gmail.com> wrote:
> >
> > > On Wed, Sep 8, 2021 at 12:07 AM Marek Behún <marek.behun at nic.cz>
> > > wrote:
> > > >
> > > > From: Pali Rohár <pali at kernel.org>
> > > >
> > > > Some PCIe controller's read_config() method support indicating error
> > > > directly via return value, but some cannot distinguish all-ones (or
> > > > all-zeros) read response from an error.
> > > >
> > > > The current code in pci_bind_bus_devices() interprets all-ones /
> > > > all-zeros in PCI_VENDOR_ID register as "nothing connected", and
> > > > continues the cycle, but an error returned via return value breaks
> > > > the cycle.
> > > >
> > > > This is wrong for the PCIe controllers which return this error via
> > > > return value.
> > > >
> > > > Handle all errors when reading PCI_VENDOR_ID the same way.
> > > >
> > > > This fixes enumeration of PCI devices for example when there is a
> > > > PCI bridge connected behind another PCI bridge and not all ports are
> > > > connected to a device, and the controller (for example Aardvark)
> > > > translates the UR error (Unsupported Request) as -EOPNOTSUPP.
> > > >
> > > > Signed-off-by: Pali Rohár <pali at kernel.org>
> > > > Signed-off-by: Marek Behún <marek.behun at nic.cz>
> > > > ---
> > > >  drivers/pci/pci-uclass.c | 9 +--------
> > > >  1 file changed, 1 insertion(+), 8 deletions(-)
> > > >
> > >
> > > Is it possible to update these PCI controller drivers to fill in
> > > vendor id to all zeros when there is an errror?
> >
> > It is, but we think it is wrong. If the PCI controller supports
> > reporting errors, they should be propagated.
> >
> > What we might want to change in those controller drivers, though, is
> > error numbers, so that they are the same for each controller. For
> > example Unsupported Request should be always translated to EOPNOTSUPP.
> 
> So if it is a real error that for some reason the controller cannot
> read the configuration space, we should abort instead of continue
> looping.

Also 0xFFFF value signals a real error that controllers cannot access
configuration space. Just it do not say what error it is. And also there
is no way to distinguish between "no error happened ans read value is
0xFFFF" and "error happened".

Anyway, I think that for multifunction device you need to continue
looping as on another function number there may be working function.

For non-multifunction devices we can abort touching configuration space.
But looking at the code, it is already implemented in this way - just it
'continue;' the loop.

> Not sure what is the best approach though.

Handle errors (independently on how they are signaled) in the same way.

We just know that card must not have PCI vendor 0xFFFF nor 0x0000 so
when reading this register we know that value 0xFFFF for sure signals
some (unknown) error.

There is a lot of stuff which could be improved, but this is the simple
step to fix usage of Bridge behind Bridge.

> FWIW,
> Reviewed-by: Bin Meng <bmeng.cn at gmail.com>