cmd_pci: Check for VendorID earlier

From: Fabio Estevam <[email protected]>

Since commit ff3e077bd2 ("dm: pci: Add a uclass for PCI") the following error message is seen:

=> pci 0
Scanning PCI devices on bus 0
BusDevFun VendorId DeviceId Device Class Sub-Class
_____________________________________________________________
00.01.00 0x16c3 0xabcd Bridge device 0x04
Cannot read bus configuration: -1

=> pci 1
Scanning PCI devices on bus 1
BusDevFun VendorId DeviceId Device Class Sub-Class
_____________________________________________________________
01.00.00 0x8086 0x08b1 Network controller 0x80
Cannot read bus configuration: -1

When we are done scanning the PCI devices pci_read_config_word() will return -1 and VendorID will contain 0xFFFF.

The original code would exit the 'for' loop in this condition.

Keep the same original behaviour by first testing the VendorID value and then checking and propagating the pci_read_config_word() error afterwards.

Signed-off-by Fabio Estevam <[email protected]>

---
 common/cmd_pci.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

common/cmd_pci.c +2 -3

index dcecef8..92dc643 100644
@@ -77,10 +77,10 @@ void pciinfo(int BusNum, int ShortPCIListing)
 
                        ret = pci_read_config_word(dev, PCI_VENDOR_ID,
                                                   &VendorID);
-                       if (ret)
-                               goto error;
                        if ((VendorID == 0xFFFF) || (VendorID == 0x0000))
                                continue;
+                       if (ret)
+                               goto error;
 
                        if (!Function) pci_read_config_byte(dev, PCI_HEADER_TYPE, &HeaderType);
 
--

1.9.1

This seems really odd to me. Why would pci_read_config_word() return an error if there is no device there? Is that the real bug here?

Quoted 2 lines · Simon Glass

This seems really odd to me. Why would pci_read_config_word() return an error if there is no device there? Is that the real bug here?

Looks like the expected behaviour: It tried to scan all the PCI elements in the bus and it failed to read when there is no more PCI device.

I'm just surprised that it is failing when there is nothing there. I think it should succeed (and read 0xffff).

What board is this? Can you find the code that is returning this error? It may be a call to pci_set_ops() which sets read_word().

Hi Simon,

Quoted 5 lines · Simon Glass

I'm just surprised that it is failing when there is nothing there. I think it should succeed (and read 0xffff).

What board is this? Can you find the code that is returning this error? It may be a call to pci_set_ops() which sets read_word().

It is a mx6qsabresd board.

I don't have the PCI device at the moment to give it a try, but looking at the code, in pcie_imx we have:

    pci_set_ops(hose,
            pci_hose_read_config_byte_via_dword,
            pci_hose_read_config_word_via_dword,
            imx_pcie_read_config,
            pci_hose_write_config_byte_via_dword,
            pci_hose_write_config_word_via_dword,
            imx_pcie_write_config);

Then in drivers/pci/pci.c:

#define PCI_READ_VIA_DWORD_OP(size, type, off_mask) \

int pci_hose_read_config_##size##_via_dword(struct pci_controller *hose,\
                    pci_dev_t dev,            \
                    int offset, type val)        \
{                                    \
    u32 val32;                            \
                                    \
    if (pci_hose_read_config_dword(hose, dev, offset & 0xfc, &val32) < 0) {    \
        *val = -1;                        \
        return -1;                        \
    }                                \
                                    \
    *val = (val32 >> ((offset & (int)off_mask) * 8));        \
                                    \
    return 0;                            \
}

,which returns -1 (and also *val = -1) in the case of errror, which matches the values that I was reading yesterday

If you look down one more level, these end up calling imx_pcie_read_config() which calls imx_pcie_addr_valid():

static int imx_pcie_addr_valid(pci_dev_t d)

{
   if ((PCI_BUS(d) == 0) && (PCI_DEV(d) > 1))
      return -EINVAL;
   if ((PCI_BUS(d) == 1) && (PCI_DEV(d) > 0))
      return -EINVAL;
   return 0;
}

I can understand the bus check, but why return an access error if the device does not exist on the bus? That seems like a bug to me.

Quoted 12 lines · Simon Glass

If you look down one more level, these end up calling imx_pcie_read_config() which calls imx_pcie_addr_valid():

static int imx_pcie_addr_valid(pci_dev_t d)

{
   if ((PCI_BUS(d) == 0) && (PCI_DEV(d) > 1))
      return -EINVAL;
   if ((PCI_BUS(d) == 1) && (PCI_DEV(d) > 0))
      return -EINVAL;
   return 0;
}

Quoted 2 lines

I can understand the bus check, but why return an access error if the device does not exist on the bus? That seems like a bug to me.

Is your suggestion like this?

Hi Fabio,

Quoted 22 lines · Fabio Estevam

Quoted 12 lines · Simon Glass

If you look down one more level, these end up calling imx_pcie_read_config() which calls imx_pcie_addr_valid():

static int imx_pcie_addr_valid(pci_dev_t d)

{
   if ((PCI_BUS(d) == 0) && (PCI_DEV(d) > 1))
      return -EINVAL;
   if ((PCI_BUS(d) == 1) && (PCI_DEV(d) > 0))
      return -EINVAL;
   return 0;
}

Quoted 2 lines

I can understand the bus check, but why return an access error if the device does not exist on the bus? That seems like a bug to me.

Is your suggestion like this?

drivers/pci/pci.c int pci_hose_read_config_##size##_via_dword(struct pci_control · line 73

Quoted 8 lines

drivers/pci/pci.c +1 -1

@@ -67,7 +67,7 @@ int pci_hose_read_config_##size##_via_dword(struct pci_control
                                                                        \
        if (pci_hose_read_config_dword(hose, dev, offset & 0xfc, &val32) < 0) {
                *val = -1;                                              \
-               return -1;                                              \
+               return 0;                                               \
        }                                                               \
                                                                        \
        *val = (val32 >> ((offset & (int)off_mask) * 8));

Not really.

I don't understand the hardware so I don't know what exactly is wrong.

To me, imx_pcie_addr_valid() should ideally not fail when the device number is in range (0..31) but references a missing device. It should be possible for its caller (imx_pcie_read_config()) to read from that address and get 0xffff as expected.

If that works, then I'd suggest changing to imx_pcie_addr_valid() to ignore PCI_DEV(f).

If not, then I'd suggest changing imx_pcie_read_config() to return 0 even when imx_pcie_addr_valid() does not, and add a comment as to why the error is being squashed.

I also see this behavior on ls1021atwr board. I agree with Simon, the correct fix should fix the PCIe driver to return 0 instead of -EINVAL.

Hi Bin,

Quoted 2 lines · Bin Meng

I also see this behavior on ls1021atwr board. I agree with Simon, the correct fix should fix the PCIe driver to return 0 instead of -EINVAL.

Do you mean like this for imx?

drivers/pci/pcie_imx.c +1 -1

@@ -381,7 +381,7 @@ static int imx_pcie_read_config(struct pci_controller *hose,
        ret = imx_pcie_addr_valid(d);
        if (ret) {
                *val = 0xffffffff;
-               return ret;
+               return 0;
        }
 
        va_address = get_bus_address(d, where);

and like this for layerscape:

Hi Fabio,

Quoted 9 lines · Fabio Estevam

Hi Bin,

Quoted 2 lines · Bin Meng

I also see this behavior on ls1021atwr board. I agree with Simon, the correct fix should fix the PCIe driver to return 0 instead of -EINVAL.

Do you mean like this for imx?

Yes

Quoted 12 lines

drivers/pci/pcie_imx.c +1 -1

@@ -381,7 +381,7 @@ static int imx_pcie_read_config(struct pci_controller *hose,
        ret = imx_pcie_addr_valid(d);
        if (ret) {
                *val = 0xffffffff;
-               return ret;
+               return 0;
        }
 
        va_address = get_bus_address(d, where);

Quoted 2 lines

and like this for layerscape:

Yes

Quoted 10 lines

drivers/pci/pcie_layerscape.c +1 -1

@@ -314,7 +314,7 @@ static int ls_pcie_read_config(struct pci_controller *hose,
 
        if (ls_pcie_addr_valid(hose, d)) {
                *val = 0xffffffff;
-               return -EINVAL;
+               return 0;
        }
 

drivers/pci/pcie_layerscape.c static int ls_pcie_read_config(struct pci_controller *hose, · line 320

Quoted 2 lines

drivers/pci/pcie_layerscape.c +0 -0

        if (PCI_BUS(d) == hose->first_busno)

Again, I was wondering why we created two drivers for the same (or similar) PCIe IPs.

Something to consolidate for the next release it sounds like. However

we need this fixed this release yes? Can I get a v2 of this patch with a proper commit message? Thanks!

Hi Bin,

drivers/pci/pcie_imx.c static int imx_pcie_read_config(struct pci_controller *hose, · line 384

Quoted 10 lines · Fabio Estevam

Do you mean like this for imx?

drivers/pci/pcie_imx.c +1 -1

@@ -381,7 +381,7 @@ static int imx_pcie_read_config(struct pci_controller *hose,
        ret = imx_pcie_addr_valid(d);
        if (ret) {
                *val = 0xffffffff;
-               return ret;
+               return 0;

Thinking a bit more about this: shouldn't we fix imx_pcie_addr_valid() instead?

I am not sure if the above change is the correct fix. At least I am not able to prepare a convincing commit log for it :-)