Skip to contentSkip to conversation

drm: Added ppc64 root device getter

PATCH In review Lucas Kannebley Tavares 3 participants 7 messages Dec 13, 2012 – Dec 26, 2012

Subsystems (4)
DRM drivers DRM misc PowerPC PCI
lore
Share & tools
lore

Lucas Kannebley Tavares

author
#0 lore raw

On architectures such as ppc64, there is no root bus device (it belongs to the hypervisor). DRM attempted to get one, causing a null-pointer dereference.

Signed-off-by Lucas Kannebley Tavares <[email protected]>

--

arch/powerpc/platforms/pseries/Makefile +2 -0
index 890622b..ddfdda8 100644
@@ -1,6 +1,8 @@
 ccflags-$(CONFIG_PPC64)			:= -mno-minimal-toc
 ccflags-$(CONFIG_PPC_PSERIES_DEBUG)	+= -DDEBUG
 
+drm-y			+= drm_pci.o
+
 obj-y			:= lpar.o hvCall.o nvram.o reconfig.o \
 			   setup.o iommu.o event_sources.o ras.o \
 			   firmware.o power.o dlpar.o mobility.o
arch/powerpc/platforms/pseries/drm_pci.c new file +24 -0
new file mode 100644
index 0000000..da6675e
@@ -0,0 +1,24 @@
+/*
+ * Copyright (C) 2012 Lucas Kannebley Tavares, IBM Corporation
+ *
+ * pSeries specific routines for DRM.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307 USA
+ */
+
+inline struct pci_device *drm_get_parent_device(struct drm_device *dev) {
+	return (dev->pdev->bus->self == NULL) ? dev->pdev : dev->pdev->bus->self;
+}
+
drivers/gpu/drm/drm_pci.c +5 -1
index eb37466..5a8a4f5 100644
@@ -466,6 +466,10 @@ void drm_pci_exit(struct drm_driver *driver, struct pci_driver *pdriver)
 }
 EXPORT_SYMBOL(drm_pci_exit);
 
+inline __weak struct pci_device *drm_get_parent_device(struct drm_device *dev) {
+	return dev->pdev->bus->self;
+}
+
 int drm_pcie_get_speed_cap_mask(struct drm_device *dev, u32 *mask)
 {
 	struct pci_dev *root;

@@ -479,7 +483,7 @@ int drm_pcie_get_speed_cap_mask(struct drm_device *dev, u32 *mask)
 		return -EINVAL;
 
 	// find PCI device for capabilities
-	root = dev->pdev->bus->self;
+	root = drm_get_parent_device(dev);
 
 	// some architectures might not have host bridges as PCI devices
 	if (root == NULL)
Signature · 3 lines
Lucas Kannebley Tavares
Software Engineer
IBM Linux Technology Center
maintainer
#1 lore raw

[+cc Betty]

Quoted 3 lines · Lucas Kannebley Tavares

On architectures such as ppc64, there is no root bus device (it belongs to the hypervisor). DRM attempted to get one, causing a null-pointer dereference.

In addition to ppc64, at least ia64 and parisc have the same situation of the PCI host bridge not appearing as a PCI device itself.

Quoted 46 lines
Signed-off-by Lucas Kannebley Tavares <[email protected]>

--

arch/powerpc/platforms/pseries/Makefile +2 -0
index 890622b..ddfdda8 100644
@@ -1,6 +1,8 @@
 ccflags-$(CONFIG_PPC64)                        := -mno-minimal-toc
 ccflags-$(CONFIG_PPC_PSERIES_DEBUG)    += -DDEBUG
 
+drm-y                  += drm_pci.o
+
 obj-y                  := lpar.o hvCall.o nvram.o reconfig.o \
                           setup.o iommu.o event_sources.o ras.o \
                           firmware.o power.o dlpar.o mobility.o
arch/powerpc/platforms/pseries/drm_pci.c new file +22 -0
new file mode 100644
index 0000000..da6675e
@@ -0,0 +1,24 @@
+/*
+ * Copyright (C) 2012 Lucas Kannebley Tavares, IBM Corporation
+ *
+ * pSeries specific routines for DRM.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307 USA
+ */
+
+inline struct pci_device *drm_get_parent_device(struct drm_device *dev) {
+       return (dev->pdev->bus->self == NULL) ? dev->pdev :
Quoted 1 line

dev->pdev->bus->self;

So for DRM devices on a root bus, the parent is the DRM device itself, while for DRM devices deeper in the hierarchy, the parent is the upstream P2P bridge? That doesn't really make sense to me. If the caller operates on the DRM device in some cases and on the bridge in other cases, it's going to need to know the difference, so hiding the difference in this wrapper seems counterproductive.

Quoted 28 lines
arch/powerpc/platforms/pseries/drm_pci.c new file +2 -0
+}
+
drivers/gpu/drm/drm_pci.c +4 -0
index eb37466..5a8a4f5 100644
@@ -466,6 +466,10 @@ void drm_pci_exit(struct drm_driver *driver, struct
 pci_driver *pdriver)
 }
 EXPORT_SYMBOL(drm_pci_exit);
 
+inline __weak struct pci_device *drm_get_parent_device(struct drm_device
 *dev) {
+       return dev->pdev->bus->self;
+}
+
 int drm_pcie_get_speed_cap_mask(struct drm_device *dev, u32 *mask)

 {
        struct pci_dev *root;
drivers/gpu/drm/drm_pci.c +1 -1
@@ -479,7 +483,7 @@ int drm_pcie_get_speed_cap_mask(struct drm_device *dev,
 u32 *mask)
                return -EINVAL;
 
        // find PCI device for capabilities
-       root = dev->pdev->bus->self;
+       root = drm_get_parent_device(dev);
 
        // some architectures might not have host bridges as PCI devices
Quoted 1 line
        if (root == NULL)

What tree does this apply to? Upstream doesn't have the "if (root == NULL)" check yet. That check looks like the sort of thing you'd need to avoid the null pointer dereference. So maybe adding that check and the associated code is enough to fix the problem, even without adding drm_get_parent_device().

With the code in the tree, it looks like you'd dereference a null pointer in pci_pcie_cap(root), so I assume that's what you tripped over.

I'm not really sure that code outside the PCI core should be looking at capabilities of upstream devices like this. It seems like the sort of thing where the core might need to provide better interfaces.

Bjorn

author
#2 lore raw
Quoted 106 lines · Bjorn Helgaas

[+cc Betty]

Quoted 3 lines · Lucas Kannebley Tavares

On architectures such as ppc64, there is no root bus device (it belongs to the hypervisor). DRM attempted to get one, causing a null-pointer dereference.

In addition to ppc64, at least ia64 and parisc have the same situation of the PCI host bridge not appearing as a PCI device itself.

Quoted 46 lines
Signed-off-by Lucas Kannebley Tavares<[email protected]>

--

arch/powerpc/platforms/pseries/Makefile +2 -0
index 890622b..ddfdda8 100644
@@ -1,6 +1,8 @@
  ccflags-$(CONFIG_PPC64)                        := -mno-minimal-toc
  ccflags-$(CONFIG_PPC_PSERIES_DEBUG)    += -DDEBUG
 
+drm-y                  += drm_pci.o
+
  obj-y                  := lpar.o hvCall.o nvram.o reconfig.o \
                            setup.o iommu.o event_sources.o ras.o \
                            firmware.o power.o dlpar.o mobility.o
arch/powerpc/platforms/pseries/drm_pci.c new file +22 -0
new file mode 100644
index 0000000..da6675e
@@ -0,0 +1,24 @@
+/*
+ * Copyright (C) 2012 Lucas Kannebley Tavares, IBM Corporation
+ *
+ * pSeries specific routines for DRM.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307 USA
+ */
+
+inline struct pci_device *drm_get_parent_device(struct drm_device *dev) {
+       return (dev->pdev->bus->self == NULL) ? dev->pdev :
Quoted 1 line

dev->pdev->bus->self;

So for DRM devices on a root bus, the parent is the DRM device itself, while for DRM devices deeper in the hierarchy, the parent is the upstream P2P bridge? That doesn't really make sense to me. If the caller operates on the DRM device in some cases and on the bridge in other cases, it's going to need to know the difference, so hiding the difference in this wrapper seems counterproductive.

Quoted 28 lines
arch/powerpc/platforms/pseries/drm_pci.c new file +2 -0
+}
+
drivers/gpu/drm/drm_pci.c +4 -0
index eb37466..5a8a4f5 100644
@@ -466,6 +466,10 @@ void drm_pci_exit(struct drm_driver *driver, struct
 pci_driver *pdriver)
  }
  EXPORT_SYMBOL(drm_pci_exit);
 
+inline __weak struct pci_device *drm_get_parent_device(struct drm_device
 *dev) {
+       return dev->pdev->bus->self;
+}
+
  int drm_pcie_get_speed_cap_mask(struct drm_device *dev, u32 *mask)

  {
         struct pci_dev *root;
drivers/gpu/drm/drm_pci.c +1 -1
@@ -479,7 +483,7 @@ int drm_pcie_get_speed_cap_mask(struct drm_device *dev,
 u32 *mask)
                 return -EINVAL;
 
         // find PCI device for capabilities
-       root = dev->pdev->bus->self;
+       root = drm_get_parent_device(dev);
 
         // some architectures might not have host bridges as PCI devices
Quoted 1 line
         if (root == NULL)

What tree does this apply to? Upstream doesn't have the "if (root == NULL)" check yet. That check looks like the sort of thing you'd need to avoid the null pointer dereference. So maybe adding that check and the associated code is enough to fix the problem, even without adding drm_get_parent_device().

With the code in the tree, it looks like you'd dereference a null pointer in pci_pcie_cap(root), so I assume that's what you tripped over.

Quoted 6 lines

I'm not really sure that code outside the PCI core should be looking at capabilities of upstream devices like this. It seems like the sort of thing where the core might need to provide better interfaces.

Bjorn

Ok Bjorn, thanks for the comments, indeed I had a dirty tree here and didn't realize it, sorry. Either way I'm then sending the "if (root == NULL)" patch as a reply to this. I'm sending it along with another independent patch (they are NOT a series) that changes pci_read_config_dword calls to pci_capability_read_dword ones on the drm driver. There were only a couple of those to start with.

Signature · 3 lines
Lucas Kannebley Tavares
Software Engineer
IBM Linux Technology Center
author
#3 lore raw

PATCH 1/1 drm: fixed access to PCI host bridges

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 <[email protected]>
---
 drivers/gpu/drm/drm_pci.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)
drivers/gpu/drm/drm_pci.c +5 -0
index 754bc96..ea41234 100644
@@ -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)
 		return -EINVAL;
Signature · 3 lines
Lucas Kannebley Tavares
Software Engineer
IBM Linux Technology Center
author
#6 lore raw

PATCH 1/1 drm: change pci_read_config_dword calls to pcie_capability_read_dword ones

In reply to Lucas Kannebley Tavares

Replacing these calls avoids compatibility problems with PCIe v1/v2 Capability structures.

Signed-off-by Lucas Kannebley Tavares <[email protected]>
---
 drivers/gpu/drm/drm_pci.c |    8 ++------
 1 files changed, 2 insertions(+), 6 deletions(-)
drivers/gpu/drm/drm_pci.c +2 -6
index ea41234..b824d4c 100644
@@ -486,17 +486,13 @@ int drm_pcie_get_speed_cap_mask(struct drm_device *dev, u32 *mask)
 	if (root == NULL)
 		root = dev->pdev;
 
-	pos = pci_pcie_cap(root);
-	if (!pos)
-		return -EINVAL;
-
 	/* 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;
 
-	pci_read_config_dword(root, pos + PCI_EXP_LNKCAP, &lnkcap);
-	pci_read_config_dword(root, pos + PCI_EXP_LNKCAP2, &lnkcap2);
+	pcie_capability_read_dword(root, PCI_EXP_LNKCAP, &lnkcap);
+	pcie_capability_read_dword(root, PCI_EXP_LNKCAP, &lnkcap2);
 
 	lnkcap &= PCI_EXP_LNKCAP_SLS;
 	lnkcap2 &= 0xfe;
Signature · 3 lines
Lucas Kannebley Tavares
Software Engineer
IBM Linux Technology Center
maintainer
#4 lore raw

Re: [PATCH] drm: fixed access to PCI host bridges

In reply to Lucas Kannebley Tavares
Quoted 14 lines · Lucas Kannebley Tavares

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 <[email protected]>
---
 drivers/gpu/drm/drm_pci.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)
drivers/gpu/drm/drm_pci.c +0 -0
index 754bc96..ea41234 100644

drivers/gpu/drm/drm_pci.c int drm_pcie_get_speed_cap_mask(struct drm_device *dev, · line 488

Quoted 10 lines
drivers/gpu/drm/drm_pci.c +4 -0
@@ -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?

Quoted 9 lines
drivers/gpu/drm/drm_pci.c +0 -1
        pos = pci_pcie_cap(root);
        if (!pos)
                return -EINVAL;
 
--

Lucas Kannebley Tavares
Software Engineer
IBM Linux Technology Center

maintainer
#5 lore raw

Re: [PATCH] drm: fixed access to PCI host bridges

Quoted 31 lines · Bjorn Helgaas
Quoted 14 lines · Lucas Kannebley Tavares

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 <[email protected]>
---
 drivers/gpu/drm/drm_pci.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)
drivers/gpu/drm/drm_pci.c +0 -0
index 754bc96..ea41234 100644

drivers/gpu/drm/drm_pci.c int drm_pcie_get_speed_cap_mask(struct drm_device *dev, · line 488

Quoted 10 lines
drivers/gpu/drm/drm_pci.c +4 -0
@@ -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.

Quoted 3 lines

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.