[PATCHv2,1/3] pci: added pcie_get_speed_cap_mask function

Added function to gather the speed cap for a device and return a mask to
supported speeds. The function is divided into an interface and a weak
implementation so that architecture-specific functions can be called.

This is the first step in moving function drm_pcie_get_speed_cap_mask
from the drm subsystem to the pci one.

Signed-off-by: Lucas Kannebley Tavares <lucaskt@linux.vnet.ibm.com>
---
 drivers/pci/pci.c   |   44 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pci.h |    6 ++++++
 2 files changed, 50 insertions(+), 0 deletions(-)

On Tue, Mar 19, 2013 at 11:24 PM, Lucas Kannebley Tavares
<lucaskt@linux.vnet.ibm.com> wrote:
> Added function to gather the speed cap for a device and return a mask to
> supported speeds. The function is divided into an interface and a weak
> implementation so that architecture-specific functions can be called.
>
> This is the first step in moving function drm_pcie_get_speed_cap_mask
> from the drm subsystem to the pci one.

This still doesn't feel right to me.

I'm definitely not a hardware guy, but my understanding based on the
PCIe spec r3.0, sec 6.11, is that the hardware will automatically
maintain the link at the highest speed supported by both ends of the
link, unless software sets a lower Target Link Speed.  The only users
of this function are some Radeon drivers, and it looks like they only
use it because the hardware doesn't conform to this aspect of the spec
and requires device-specific tweaking.

We already have bus->max_bus_speed, which should tell you the max
speed supported by the upstream component.  Is that enough
information?  Maybe the radeon drivers could simply do something like
this:

    speed = rdev->ddev->pdev->bus->max_bus_speed;
    if (speed == PCI_SPEED_UNKNOWN || speed < PCIE_SPEED_5_0GT)
        return;

In your original email [1], you mentioned a null pointer dereference,
presumably when reading the PCIe capability for dev->pdev->bus->self,
with "self" being NULL.  This only happens for a bus with no upstream
P2P bridge, i.e., self will be NULL only if pdev is on a root bus.
But we also know pdev is a PCIe device, and I think a PCIe device on a
root bus must be a "Root Complex Integrated Endpoint" (PCIe spec sec
1.3.2.3).  Such a device does not have a link at all, so there's no
point in fiddling with its link speed.

I don't see how a radeon device could be an integrated endpoint, but
in your hypervisor environment, maybe it isn't quite spec-compliant in
terms of its attachment.  Can you collect the output of "lspci -vv"?
Maybe that will make things clearer.

In any case, if a radeon device is on a root bus, I think the root bus
max_bus_speed will be PCI_SPEED_UNKNOWN, and if it's not on a root
bus, max_bus_speed should be set correctly based on the upstream PCIe
port.

Bjorn

[1] http://lkml.kernel.org/r/50819140.8030806@linux.vnet.ibm.com

> Signed-off-by: Lucas Kannebley Tavares <lucaskt@linux.vnet.ibm.com>
> ---
>  drivers/pci/pci.c   |   44 ++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/pci.h |    6 ++++++
>  2 files changed, 50 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index b099e00..d94ab79 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -3931,6 +3931,50 @@ static int __init pci_setup(char *str)
>  }
>  early_param("pci", pci_setup);
>
> +int __weak pcibios_get_speed_cap_mask(struct pci_dev *dev, u32 *mask)
> +{
> +       struct pci_dev *root;
> +       u32 lnkcap, lnkcap2;
> +
> +       *mask = 0;
> +       if (!dev)
> +               return -EINVAL;
> +
> +       root = dev->bus->self;
> +
> +       /* we've been informed via and serverworks don't make the cut */
> +       if (root->vendor == PCI_VENDOR_ID_VIA ||
> +           root->vendor == PCI_VENDOR_ID_SERVERWORKS)
> +               return -EINVAL;
> +
> +       pcie_capability_read_dword(root, PCI_EXP_LNKCAP, &lnkcap);
> +       pcie_capability_read_dword(root, PCI_EXP_LNKCAP2, &lnkcap2);
> +
> +       if (lnkcap2) {  /* PCIe r3.0-compliant */
> +               if (lnkcap2 & PCI_EXP_LNKCAP2_SLS_2_5GB)
> +                       *mask |= PCIE_SPEED_25;
> +               if (lnkcap2 & PCI_EXP_LNKCAP2_SLS_5_0GB)
> +                       *mask |= PCIE_SPEED_50;
> +               if (lnkcap2 & PCI_EXP_LNKCAP2_SLS_8_0GB)
> +                       *mask |= PCIE_SPEED_80;
> +       } else {        /* pre-r3.0 */
> +               if (lnkcap & PCI_EXP_LNKCAP_SLS_2_5GB)
> +                       *mask |= PCIE_SPEED_25;
> +               if (lnkcap & PCI_EXP_LNKCAP_SLS_5_0GB)
> +                       *mask |= (PCIE_SPEED_25 | PCIE_SPEED_50);
> +       }
> +
> +       dev_info(&dev->dev, "probing gen 2 caps for device %x:%x = %x/%x\n",
> +               root->vendor, root->device, lnkcap, lnkcap2);
> +       return 0;
> +}
> +
> +int pcie_get_speed_cap_mask(struct pci_dev *dev, u32 *mask)
> +{
> +       return pcibios_get_speed_cap_mask(dev, mask);
> +}
> +EXPORT_SYMBOL(pcie_get_speed_cap_mask);
> +
>  EXPORT_SYMBOL(pci_reenable_device);
>  EXPORT_SYMBOL(pci_enable_device_io);
>  EXPORT_SYMBOL(pci_enable_device_mem);
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 2461033a..24a2f63 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1861,4 +1861,10 @@ static inline struct eeh_dev *pci_dev_to_eeh_dev(struct pci_dev *pdev)
>   */
>  struct pci_dev *pci_find_upstream_pcie_bridge(struct pci_dev *pdev);
>
> +#define PCIE_SPEED_25 1
> +#define PCIE_SPEED_50 2
> +#define PCIE_SPEED_80 4
> +
> +extern int pcie_get_speed_cap_mask(struct pci_dev *dev, u32 *speed_mask);
> +
>  #endif /* LINUX_PCI_H */
> --
> 1.7.4.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Tue, 2013-03-26 at 12:39 -0600, Bjorn Helgaas wrote:
> But we also know pdev is a PCIe device, and I think a PCIe device on a
> root bus must be a "Root Complex Integrated Endpoint" (PCIe spec sec
> 1.3.2.3).  Such a device does not have a link at all, so there's no
> point in fiddling with its link speed.

This is where our IBM hypervisor makes things murky. It doesn't expose
the PCIe parents (basically somewhat makes PCIe look like PCI except we
still have the PCIe caps on the child devices, just no access to the
parent device).

It's garbage but can't be fixed (would break AIX :-)

However we might be able to populate the bus->max_bus_speed from some
architecture specific quirk and have radeon use that.

Cheers,
Ben.

On Wed, Mar 27, 2013 at 9:25 AM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Tue, 2013-03-26 at 12:39 -0600, Bjorn Helgaas wrote:
>> But we also know pdev is a PCIe device, and I think a PCIe device on a
>> root bus must be a "Root Complex Integrated Endpoint" (PCIe spec sec
>> 1.3.2.3).  Such a device does not have a link at all, so there's no
>> point in fiddling with its link speed.
>
> This is where our IBM hypervisor makes things murky. It doesn't expose
> the PCIe parents (basically somewhat makes PCIe look like PCI except we
> still have the PCIe caps on the child devices, just no access to the
> parent device).

Interesting.  I wonder if we'll trip over this anywhere else, e.g.,
ASPM or other link-related things.  I guess we'll just have to see if
anything else breaks.

> It's garbage but can't be fixed (would break AIX :-)
>
> However we might be able to populate the bus->max_bus_speed from some
> architecture specific quirk and have radeon use that.

That sounds like a good solution to me.  It seems like it's really an
arch-specific deviation from the spec, so it'd be nice to have an
arch-specific solution for it.

Bjorn