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

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

[Marek Behún]

From: Pali Rohár <pali@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@kernel.org>
Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
 drivers/pci/pci-uclass.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c
index ce2eb5da2c..4d0e938fe5 100644
--- a/drivers/pci/pci-uclass.c
+++ b/drivers/pci/pci-uclass.c
@@ -856,10 +856,7 @@ int pci_bind_bus_devices(struct udevice *bus)
 		/* Check only the first access, we don't expect problems */
 		ret = pci_bus_read_config(bus, bdf, PCI_VENDOR_ID, &vendor,
 					  PCI_SIZE_16);
-		if (ret)
-			goto error;
-
-		if (vendor == 0xffff || vendor == 0x0000)
+		if (ret || vendor == 0xffff || vendor == 0x0000)
 			continue;
 
 		pci_bus_read_config(bus, bdf, PCI_HEADER_TYPE,
@@ -940,10 +937,6 @@ int pci_bind_bus_devices(struct udevice *bus)
 	}
 
 	return 0;
-error:
-	printf("Cannot read bus configuration: %d\n", ret);
-
-	return ret;
 }
 
 static void decode_regions(struct pci_controller *hose, ofnode parent_node,
-- 
2.32.0

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

[Bin Meng]

On Wed, Sep 8, 2021 at 12:07 AM Marek Behún <marek.behun@nic.cz> wrote:
>
> From: Pali Rohár <pali@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@kernel.org>
> Signed-off-by: Marek Behún <marek.behun@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?

Regards,
Bin

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

[Marek Behún]

On Wed, 8 Sep 2021 09:06:28 +0800
Bin Meng <bmeng.cn@gmail.com> wrote:

> On Wed, Sep 8, 2021 at 12:07 AM Marek Behún <marek.behun@nic.cz>
> wrote:
> >
> > From: Pali Rohár <pali@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@kernel.org>
> > Signed-off-by: Marek Behún <marek.behun@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.

Marek

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

[Bin Meng]

On Wed, Sep 8, 2021 at 8:02 PM Marek Behún <kabel@kernel.org> wrote:
>
> On Wed, 8 Sep 2021 09:06:28 +0800
> Bin Meng <bmeng.cn@gmail.com> wrote:
>
> > On Wed, Sep 8, 2021 at 12:07 AM Marek Behún <marek.behun@nic.cz>
> > wrote:
> > >
> > > From: Pali Rohár <pali@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@kernel.org>
> > > Signed-off-by: Marek Behún <marek.behun@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.

Not sure what is the best approach though.

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

Re: [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@kernel.org> wrote:
> >
> > On Wed, 8 Sep 2021 09:06:28 +0800
> > Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > > On Wed, Sep 8, 2021 at 12:07 AM Marek Behún <marek.behun@nic.cz>
> > > wrote:
> > > >
> > > > From: Pali Rohár <pali@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@kernel.org>
> > > > Signed-off-by: Marek Behún <marek.behun@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@gmail.com>

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

[Stefan Roese]

On 07.09.21 18:07, Marek Behún wrote:
> From: Pali Rohár <pali@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@kernel.org>
> Signed-off-by: Marek Behún <marek.behun@nic.cz>

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> ---
>   drivers/pci/pci-uclass.c | 9 +--------
>   1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c
> index ce2eb5da2c..4d0e938fe5 100644
> --- a/drivers/pci/pci-uclass.c
> +++ b/drivers/pci/pci-uclass.c
> @@ -856,10 +856,7 @@ int pci_bind_bus_devices(struct udevice *bus)
>   		/* Check only the first access, we don't expect problems */
>   		ret = pci_bus_read_config(bus, bdf, PCI_VENDOR_ID, &vendor,
>   					  PCI_SIZE_16);
> -		if (ret)
> -			goto error;
> -
> -		if (vendor == 0xffff || vendor == 0x0000)
> +		if (ret || vendor == 0xffff || vendor == 0x0000)
>   			continue;
>   
>   		pci_bus_read_config(bus, bdf, PCI_HEADER_TYPE,
> @@ -940,10 +937,6 @@ int pci_bind_bus_devices(struct udevice *bus)
>   	}
>   
>   	return 0;
> -error:
> -	printf("Cannot read bus configuration: %d\n", ret);
> -
> -	return ret;
>   }
>   
>   static void decode_regions(struct pci_controller *hose, ofnode parent_node,
> 


Viele Grüße,
Stefan

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

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

[Simon Glass]

On 07.09.21 18:07, Marek Behún wrote:
> From: Pali Rohár <pali@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@kernel.org>
> Signed-off-by: Marek Behún <marek.behun@nic.cz>

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> ---
>   drivers/pci/pci-uclass.c | 9 +--------
>   1 file changed, 1 insertion(+), 8 deletions(-)
>
Applied to u-boot-dm/next, thanks!