[PATCH u-boot] dm: pci: Fix handling of errors when scanning device
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,
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
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
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
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
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
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!
participants (6)
-
Bin Meng -
Marek Behún -
Marek Behún -
Pali Rohár -
Simon Glass -
Stefan Roese