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

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

[Fabio Estevam]

From: Fabio Estevam <fabio.estevam@freescale.com>

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 <fabio.estevam@freescale.com>
---
 common/cmd_pci.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/common/cmd_pci.c b/common/cmd_pci.c
index dcecef8..92dc643 100644
--- a/common/cmd_pci.c
+++ b/common/cmd_pci.c
@@ -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

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

[Simon Glass]

Hi Fabio,

On 8 October 2015 at 00:37, Fabio Estevam <festevam@gmail.com> wrote:
> From: Fabio Estevam <fabio.estevam@freescale.com>
>
> 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 <fabio.estevam@freescale.com>
> ---
>  common/cmd_pci.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/common/cmd_pci.c b/common/cmd_pci.c
> index dcecef8..92dc643 100644
> --- a/common/cmd_pci.c
> +++ b/common/cmd_pci.c
> @@ -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?

Regards,
Simon

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

[Fabio Estevam]

On Fri, Oct 9, 2015 at 6:36 AM, Simon Glass <sjg@chromium.org> wrote:

> 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.

Regards,

Fabio Estevam

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

[Simon Glass]

Hi Fabio,

On 9 October 2015 at 13:52, Fabio Estevam <festevam@gmail.com> wrote:
> On Fri, Oct 9, 2015 at 6:36 AM, Simon Glass <sjg@chromium.org> wrote:
>
>> 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().

Regards,
Simon

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

[Fabio Estevam]

Hi Simon,

On Fri, Oct 9, 2015 at 10:01 AM, Simon Glass <sjg@chromium.org> wrote:

> 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

Regards,

Fabio Estevam

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

[Simon Glass]

Hi Fabio,

On 9 October 2015 at 14:22, Fabio Estevam <festevam@gmail.com> wrote:
> Hi Simon,
>
> On Fri, Oct 9, 2015 at 10:01 AM, Simon Glass <sjg@chromium.org> wrote:
>
>> 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.

Regards,
Simon

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

[Fabio Estevam]

On Fri, Oct 9, 2015 at 10:31 AM, Simon Glass <sjg@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));

Regards,

Fabio Estevam

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

[Simon Glass]

Hi Fabio,

On 9 October 2015 at 14:36, Fabio Estevam <festevam@gmail.com> wrote:
> On Fri, Oct 9, 2015 at 10:31 AM, Simon Glass <sjg@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

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

[Bin Meng]

+Alison, York,

Hi,

On Fri, Oct 9, 2015 at 9:44 PM, Simon Glass <sjg@chromium.org> wrote:
> Hi Fabio,
>
> On 9 October 2015 at 14:36, Fabio Estevam <festevam@gmail.com> wrote:
>> On Fri, Oct 9, 2015 at 10:31 AM, Simon Glass <sjg@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.
>

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.

BTW: it looks to me that both imx and layerscape PCIe IPs are derived
from Synopsis Designware. Why didn't we create a single driver for
that IP?

Regards,
Bin

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

[Fabio Estevam]

Hi Bin,

On Thu, Dec 31, 2015 at 7:32 AM, Bin Meng <bmeng.cn@gmail.com> wrote:

> 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?

--- a/drivers/pci/pcie_imx.c
+++ b/drivers/pci/pcie_imx.c
@@ -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:

--- a/drivers/pci/pcie_layerscape.c
+++ b/drivers/pci/pcie_layerscape.c
@@ -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;
        }

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

Regards,

Fabio Estevam

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

[Bin Meng]

Hi Fabio,

On Thu, Dec 31, 2015 at 11:20 PM, Fabio Estevam <festevam@gmail.com> wrote:
> Hi Bin,
>
> On Thu, Dec 31, 2015 at 7:32 AM, Bin Meng <bmeng.cn@gmail.com> wrote:
>
>> 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

> --- a/drivers/pci/pcie_imx.c
> +++ b/drivers/pci/pcie_imx.c
> @@ -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:
>

Yes

> --- a/drivers/pci/pcie_layerscape.c
> +++ b/drivers/pci/pcie_layerscape.c
> @@ -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;
>         }
>
>         if (PCI_BUS(d) == hose->first_busno)
>

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

Regards,
Bin

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

[Tom Rini]

On Mon, Jan 04, 2016 at 11:11:18AM +0800, Bin Meng wrote:
> Hi Fabio,
> 
> On Thu, Dec 31, 2015 at 11:20 PM, Fabio Estevam <festevam@gmail.com> wrote:
> > Hi Bin,
> >
> > On Thu, Dec 31, 2015 at 7:32 AM, Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> >> 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
> 
> > --- a/drivers/pci/pcie_imx.c
> > +++ b/drivers/pci/pcie_imx.c
> > @@ -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:
> >
> 
> Yes
> 
> > --- a/drivers/pci/pcie_layerscape.c
> > +++ b/drivers/pci/pcie_layerscape.c
> > @@ -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;
> >         }
> >
> >         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!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160104/2a680133/attachment.sig>

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

[Fabio Estevam]

On Mon, Jan 4, 2016 at 2:22 PM, Tom Rini <trini@konsulko.com> wrote:

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

Agreed.

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

Yes, will send it today.

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

[Fabio Estevam]

Hi Bin,

On Thu, Dec 31, 2015 at 1:20 PM, Fabio Estevam <festevam@gmail.com> wrote:

> Do you mean like this for imx?
>
> --- a/drivers/pci/pcie_imx.c
> +++ b/drivers/pci/pcie_imx.c
> @@ -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 :-)

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

[Fabio Estevam]

On Tue, Jan 5, 2016 at 9:58 PM, Fabio Estevam <festevam@gmail.com> wrote:
> Hi Bin,
>
> On Thu, Dec 31, 2015 at 1:20 PM, Fabio Estevam <festevam@gmail.com> wrote:
>
>> Do you mean like this for imx?
>>
>> --- a/drivers/pci/pcie_imx.c
>> +++ b/drivers/pci/pcie_imx.c
>> @@ -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 :-)

Ok, looked at the kernel PCI designware driver and will send a patch
series that fixes this issue by using the approach done in the kernel.