Patchwork drm: fixed access to PCI host bridges

login
register
mail settings
Submitter Lucas Kannebley Tavares
Date Dec. 22, 2012, 7:01 p.m.
Message ID <50D6039D.4000001@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/207934/
State Not Applicable
Headers show

Comments

Lucas Kannebley Tavares - Dec. 22, 2012, 7:01 p.m.
During the process of obtaining the speed cap for the device, it
attempts go get the PCI Host bus. However on architectures such as PPC
or IA64, those do not appear as devices.

Signed-off-by: Lucas Kannebley Tavares <lucaskt@linux.vnet.ibm.com>
---
  drivers/gpu/drm/drm_pci.c |    5 +++++
  1 files changed, 5 insertions(+), 0 deletions(-)

  		return -EINVAL;
Bjorn Helgaas - Dec. 26, 2012, 10:40 p.m.
On Sat, Dec 22, 2012 at 12:01 PM, Lucas Kannebley Tavares
<lucaskt@linux.vnet.ibm.com> wrote:
> During the process of obtaining the speed cap for the device, it
> attempts go get the PCI Host bus. However on architectures such as PPC
> or IA64, those do not appear as devices.
>
> Signed-off-by: Lucas Kannebley Tavares <lucaskt@linux.vnet.ibm.com>
> ---
>  drivers/gpu/drm/drm_pci.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c
> index 754bc96..ea41234 100644
> --- a/drivers/gpu/drm/drm_pci.c
> +++ b/drivers/gpu/drm/drm_pci.c
> @@ -479,8 +479,13 @@ int drm_pcie_get_speed_cap_mask(struct drm_device *dev,
> u32 *mask)
>         if (!pci_is_pcie(dev->pdev))
>                 return -EINVAL;
>
> +       // find PCI device for capabilities
>         root = dev->pdev->bus->self;
>
> +       // some architectures might not have host bridges as PCI devices
> +       if (root == NULL)
> +               root = dev->pdev;

You didn't address my question about this.  Obviously this will avoid
a null pointer dereference.  But you have to also explain why this
change is correct.

If it's good enough to just look at the capabilities of the DRM device
(not the upstream bridge) on PPC and ia64, why not do that everywhere
and forget about the bridge completely?

>         pos = pci_pcie_cap(root);
>         if (!pos)
>                 return -EINVAL;
>
> --
> Lucas Kannebley Tavares
> Software Engineer
> IBM Linux Technology Center
>
--
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
Dave Airlie - Dec. 26, 2012, 10:47 p.m.
On Thu, Dec 27, 2012 at 8:40 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Sat, Dec 22, 2012 at 12:01 PM, Lucas Kannebley Tavares
> <lucaskt@linux.vnet.ibm.com> wrote:
>> During the process of obtaining the speed cap for the device, it
>> attempts go get the PCI Host bus. However on architectures such as PPC
>> or IA64, those do not appear as devices.
>>
>> Signed-off-by: Lucas Kannebley Tavares <lucaskt@linux.vnet.ibm.com>
>> ---
>>  drivers/gpu/drm/drm_pci.c |    5 +++++
>>  1 files changed, 5 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c
>> index 754bc96..ea41234 100644
>> --- a/drivers/gpu/drm/drm_pci.c
>> +++ b/drivers/gpu/drm/drm_pci.c
>> @@ -479,8 +479,13 @@ int drm_pcie_get_speed_cap_mask(struct drm_device *dev,
>> u32 *mask)
>>         if (!pci_is_pcie(dev->pdev))
>>                 return -EINVAL;
>>
>> +       // find PCI device for capabilities
>>         root = dev->pdev->bus->self;
>>
>> +       // some architectures might not have host bridges as PCI devices
>> +       if (root == NULL)
>> +               root = dev->pdev;
>
> You didn't address my question about this.  Obviously this will avoid
> a null pointer dereference.  But you have to also explain why this
> change is correct.
>
> If it's good enough to just look at the capabilities of the DRM device
> (not the upstream bridge) on PPC and ia64, why not do that everywhere
> and forget about the bridge completely?

Yeah this doesn't make sense, we need to know if the device and the
bridge are capable of doing PCIE gen2+ speeds.

At least I'm willing to accept spec pointers to why we might not need
to ask the bridge, but my current understanding is we need to know
both.

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

Patch

diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c
index 754bc96..ea41234 100644
--- a/drivers/gpu/drm/drm_pci.c
+++ b/drivers/gpu/drm/drm_pci.c
@@ -479,8 +479,13 @@  int drm_pcie_get_speed_cap_mask(struct drm_device 
*dev, u32 *mask)
  	if (!pci_is_pcie(dev->pdev))
  		return -EINVAL;

+	// find PCI device for capabilities
  	root = dev->pdev->bus->self;

+	// some architectures might not have host bridges as PCI devices
+	if (root == NULL)
+		root = dev->pdev;
+
  	pos = pci_pcie_cap(root);
  	if (!pos)