[U-Boot] [PATCH] cmd_pci: Check for VendorID earlier

[Simon Glass]

Hi Fabio,

On 9 October 2015 at 14:36, Fabio Estevam <festevam at gmail.com> wrote:
> On Fri, Oct 9, 2015 at 10:31 AM, Simon Glass <sjg at chromium.org> wrote:
>
>> 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.
>
> Is your suggestion like this?
>
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -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.

Regards,
Simon