* [PATCH] drm/i915: fix pch pci device enumeration
@ 2014-02-14 17:23 Imre Deak
2014-02-14 17:35 ` Chris Wilson
2014-02-14 18:23 ` [PATCH v2] " Imre Deak
0 siblings, 2 replies; 8+ messages in thread
From: Imre Deak @ 2014-02-14 17:23 UTC (permalink / raw)
To: intel-gfx
pci_get_class(class, from) drops the refcount for 'from', so the
extra pci_dev_put we do on it will result in a use after free bug
sometime later starting with the WARN below.
Regression introduced in
commit 6a9c4b35e6696a63805b6da5e4889c6986e9ee1b
Author: Rui Guo <firemeteor@users.sourceforge.net>
Date: Wed Jun 19 21:10:23 2013 +0800
drm/i915: Fix PCH detect with multiple ISA bridges in VM
[ 164.338460] WARNING: CPU: 1 PID: 2094 at include/linux/kref.h:47 klist_next+0xae/0x110()
[ 164.347731] CPU: 1 PID: 2094 Comm: modprobe Tainted: G O 3.13.0-imre+ #354
[ 164.356468] Hardware name: Intel Corp. VALLEYVIEW B0 PLATFORM/NOTEBOOK, BIOS BYTICRB1.X64.0062.R70.1310112051 10/11/2013
[ 164.368796] Call Trace:
[ 164.371609] [<ffffffff816a32a6>] dump_stack+0x4e/0x7a
[ 164.377447] [<ffffffff8104f75d>] warn_slowpath_common+0x7d/0xa0
[ 164.384238] [<ffffffff8104f83a>] warn_slowpath_null+0x1a/0x20
[ 164.390851] [<ffffffff8169aeae>] klist_next+0xae/0x110
[ 164.396777] [<ffffffff8130a110>] ? pci_do_find_bus+0x70/0x70
[ 164.403286] [<ffffffff813cb4a9>] bus_find_device+0x89/0xc0
[ 164.409719] [<ffffffff8130a373>] pci_get_dev_by_id+0x63/0xa0
[ 164.416238] [<ffffffff8130a4e4>] pci_get_class+0x44/0x50
[ 164.422433] [<ffffffffa034821f>] intel_dsm_detect+0x16f/0x1f0 [i915]
[ 164.429801] [<ffffffffa03482ae>] intel_register_dsm_handler+0xe/0x10 [i915]
[ 164.437831] [<ffffffffa02d30fe>] i915_driver_load+0xafe/0xf30 [i915]
[ 164.445126] [<ffffffff8158a150>] ? intel_alloc_coherent+0x110/0x110
[ 164.452340] [<ffffffffa0148c07>] drm_dev_register+0xc7/0x150 [drm]
[ 164.459462] [<ffffffffa014b23f>] drm_get_pci_dev+0x11f/0x1f0 [drm]
[ 164.466554] [<ffffffff816abb81>] ? _raw_spin_unlock_irqrestore+0x51/0x70
[ 164.474287] [<ffffffffa02cf7a6>] i915_pci_probe+0x56/0x60 [i915]
[ 164.481185] [<ffffffff8130a028>] pci_device_probe+0x78/0xf0
[ 164.487603] [<ffffffff813cd495>] driver_probe_device+0x155/0x350
[ 164.494505] [<ffffffff813cd74e>] __driver_attach+0x6e/0xa0
[ 164.500826] [<ffffffff813cd6e0>] ? __device_attach+0x50/0x50
[ 164.507333] [<ffffffff813cb2be>] bus_for_each_dev+0x6e/0xc0
[ 164.513752] [<ffffffff813ccefe>] driver_attach+0x1e/0x20
[ 164.519870] [<ffffffff813cc958>] bus_add_driver+0x138/0x260
[ 164.526289] [<ffffffffa0188000>] ? 0xffffffffa0187fff
[ 164.532116] [<ffffffff813cde78>] driver_register+0x98/0xe0
[ 164.538558] [<ffffffffa0188000>] ? 0xffffffffa0187fff
[ 164.544389] [<ffffffff813087b0>] __pci_register_driver+0x60/0x70
[ 164.551336] [<ffffffffa014b37d>] drm_pci_init+0x6d/0x120 [drm]
[ 164.558040] [<ffffffffa0188000>] ? 0xffffffffa0187fff
[ 164.563928] [<ffffffffa018806a>] i915_init+0x6a/0x6c [i915]
[ 164.570363] [<ffffffff810002da>] do_one_initcall+0xaa/0x160
[ 164.576783] [<ffffffff8103b140>] ? set_memory_nx+0x40/0x50
[ 164.583100] [<ffffffff810ce7f5>] load_module+0x1fb5/0x2550
[ 164.589410] [<ffffffff810caab0>] ? store_uevent+0x40/0x40
[ 164.595628] [<ffffffff810cee7d>] SyS_init_module+0xed/0x100
[ 164.602048] [<ffffffff816b3c52>] system_call_fastpath+0x16/0x1b
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
drivers/gpu/drm/i915/i915_drv.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 2d05d7c..4e4fc0c 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -347,7 +347,6 @@ void intel_detect_pch(struct drm_device *dev)
*/
pch = pci_get_class(PCI_CLASS_BRIDGE_ISA << 8, NULL);
while (pch) {
- struct pci_dev *curr = pch;
if (pch->vendor == PCI_VENDOR_ID_INTEL) {
unsigned short id;
id = pch->device & INTEL_PCH_DEVICE_ID_MASK;
@@ -385,15 +384,15 @@ void intel_detect_pch(struct drm_device *dev)
} else {
goto check_next;
}
- pci_dev_put(pch);
break;
}
check_next:
- pch = pci_get_class(PCI_CLASS_BRIDGE_ISA << 8, curr);
- pci_dev_put(curr);
+ pch = pci_get_class(PCI_CLASS_BRIDGE_ISA << 8, pch);
}
if (!pch)
DRM_DEBUG_KMS("No PCH found?\n");
+ else
+ pci_dev_put(pch);
}
bool i915_semaphore_is_enabled(struct drm_device *dev)
--
1.8.4
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/i915: fix pch pci device enumeration
2014-02-14 17:23 [PATCH] drm/i915: fix pch pci device enumeration Imre Deak
@ 2014-02-14 17:35 ` Chris Wilson
2014-02-14 17:48 ` Imre Deak
2014-02-14 18:23 ` [PATCH v2] " Imre Deak
1 sibling, 1 reply; 8+ messages in thread
From: Chris Wilson @ 2014-02-14 17:35 UTC (permalink / raw)
To: Imre Deak; +Cc: intel-gfx
On Fri, Feb 14, 2014 at 07:23:32PM +0200, Imre Deak wrote:
> pci_get_class(class, from) drops the refcount for 'from', so the
> extra pci_dev_put we do on it will result in a use after free bug
> sometime later starting with the WARN below.
That's a very nice find.
But you can tidy this loop up even more...
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 2d05d7c..4e4fc0c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -347,7 +347,6 @@ void intel_detect_pch(struct drm_device *dev)
> */
> pch = pci_get_class(PCI_CLASS_BRIDGE_ISA << 8, NULL);
> while (pch) {
...by noting that what you want here is
while ((pch = pci_get_class(ISA<<8, pch)) {
> - struct pci_dev *curr = pch;
> if (pch->vendor == PCI_VENDOR_ID_INTEL) {
> unsigned short id;
> id = pch->device & INTEL_PCH_DEVICE_ID_MASK;
> @@ -385,15 +384,15 @@ void intel_detect_pch(struct drm_device *dev)
> } else {
> goto check_next;
> }
> - pci_dev_put(pch);
> break;
> }
> check_next:
> - pch = pci_get_class(PCI_CLASS_BRIDGE_ISA << 8, curr);
> - pci_dev_put(curr);
> + pch = pci_get_class(PCI_CLASS_BRIDGE_ISA << 8, pch);
> }
> if (!pch)
> DRM_DEBUG_KMS("No PCH found?\n");
> + else
> + pci_dev_put(pch);
> }
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 2d05d7c..9962aef 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -324,7 +324,7 @@ MODULE_DEVICE_TABLE(pci, pciidlist);
void intel_detect_pch(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev->dev_private;
- struct pci_dev *pch;
+ struct pci_dev *pch = NULL;
/* In all current cases, num_pipes is equivalent to the PCH_NOP setting
* (which really amounts to a PCH but no South Display).
@@ -345,12 +345,9 @@ void intel_detect_pch(struct drm_device *dev)
* all the ISA bridge devices and check for the first match, instead
* of only checking the first one.
*/
- pch = pci_get_class(PCI_CLASS_BRIDGE_ISA << 8, NULL);
- while (pch) {
- struct pci_dev *curr = pch;
+ while ((pch = pci_get_class(PCI_CLASS_BRIDGE_ISA << 8, pch))) {
if (pch->vendor == PCI_VENDOR_ID_INTEL) {
- unsigned short id;
- id = pch->device & INTEL_PCH_DEVICE_ID_MASK;
+ unsigned short id = pch->device & INTEL_PCH_DEVICE_ID_MASK;
dev_priv->pch_id = id;
if (id == INTEL_PCH_IBX_DEVICE_ID_TYPE) {
@@ -382,18 +379,15 @@ void intel_detect_pch(struct drm_device *dev)
DRM_DEBUG_KMS("Found LynxPoint LP PCH\n");
WARN_ON(!IS_HASWELL(dev));
WARN_ON(!IS_ULT(dev));
- } else {
- goto check_next;
- }
+ } else
+ continue;
+
pci_dev_put(pch);
break;
}
-check_next:
- pch = pci_get_class(PCI_CLASS_BRIDGE_ISA << 8, curr);
- pci_dev_put(curr);
}
if (!pch)
- DRM_DEBUG_KMS("No PCH found?\n");
+ DRM_DEBUG_KMS("No PCH found.\n");
}
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/i915: fix pch pci device enumeration
2014-02-14 17:35 ` Chris Wilson
@ 2014-02-14 17:48 ` Imre Deak
2014-02-14 17:54 ` Chris Wilson
2014-02-14 17:56 ` Imre Deak
0 siblings, 2 replies; 8+ messages in thread
From: Imre Deak @ 2014-02-14 17:48 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
On Fri, 2014-02-14 at 17:35 +0000, Chris Wilson wrote:
> On Fri, Feb 14, 2014 at 07:23:32PM +0200, Imre Deak wrote:
> > pci_get_class(class, from) drops the refcount for 'from', so the
> > extra pci_dev_put we do on it will result in a use after free bug
> > sometime later starting with the WARN below.
>
> That's a very nice find.
>
> But you can tidy this loop up even more...
>
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index 2d05d7c..4e4fc0c 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -347,7 +347,6 @@ void intel_detect_pch(struct drm_device *dev)
> > */
> > pch = pci_get_class(PCI_CLASS_BRIDGE_ISA << 8, NULL);
> > while (pch) {
>
> ...by noting that what you want here is
> while ((pch = pci_get_class(ISA<<8, pch)) {
>
> > - struct pci_dev *curr = pch;
> > if (pch->vendor == PCI_VENDOR_ID_INTEL) {
> > unsigned short id;
> > id = pch->device & INTEL_PCH_DEVICE_ID_MASK;
> > @@ -385,15 +384,15 @@ void intel_detect_pch(struct drm_device *dev)
> > } else {
> > goto check_next;
> > }
> > - pci_dev_put(pch);
> > break;
> > }
> > check_next:
> > - pch = pci_get_class(PCI_CLASS_BRIDGE_ISA << 8, curr);
> > - pci_dev_put(curr);
> > + pch = pci_get_class(PCI_CLASS_BRIDGE_ISA << 8, pch);
> > }
> > if (!pch)
> > DRM_DEBUG_KMS("No PCH found?\n");
> > + else
> > + pci_dev_put(pch);
> > }
>
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 2d05d7c..9962aef 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -324,7 +324,7 @@ MODULE_DEVICE_TABLE(pci, pciidlist);
> void intel_detect_pch(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> - struct pci_dev *pch;
> + struct pci_dev *pch = NULL;
>
> /* In all current cases, num_pipes is equivalent to the PCH_NOP setting
> * (which really amounts to a PCH but no South Display).
> @@ -345,12 +345,9 @@ void intel_detect_pch(struct drm_device *dev)
> * all the ISA bridge devices and check for the first match, instead
> * of only checking the first one.
> */
> - pch = pci_get_class(PCI_CLASS_BRIDGE_ISA << 8, NULL);
> - while (pch) {
> - struct pci_dev *curr = pch;
> + while ((pch = pci_get_class(PCI_CLASS_BRIDGE_ISA << 8, pch))) {
> if (pch->vendor == PCI_VENDOR_ID_INTEL) {
> - unsigned short id;
> - id = pch->device & INTEL_PCH_DEVICE_ID_MASK;
> + unsigned short id = pch->device & INTEL_PCH_DEVICE_ID_MASK;
> dev_priv->pch_id = id;
>
> if (id == INTEL_PCH_IBX_DEVICE_ID_TYPE) {
> @@ -382,18 +379,15 @@ void intel_detect_pch(struct drm_device *dev)
> DRM_DEBUG_KMS("Found LynxPoint LP PCH\n");
> WARN_ON(!IS_HASWELL(dev));
> WARN_ON(!IS_ULT(dev));
> - } else {
> - goto check_next;
> - }
> + } else
> + continue;
> +
> pci_dev_put(pch);
> break;
Yep, looks better. I would also move the pci_dev_put out of the loop and
remove the above continue. But it's fine for me either way.
--Imre
> }
> -check_next:
> - pch = pci_get_class(PCI_CLASS_BRIDGE_ISA << 8, curr);
> - pci_dev_put(curr);
> }
> if (!pch)
> - DRM_DEBUG_KMS("No PCH found?\n");
> + DRM_DEBUG_KMS("No PCH found.\n");
> }
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/i915: fix pch pci device enumeration
2014-02-14 17:48 ` Imre Deak
@ 2014-02-14 17:54 ` Chris Wilson
2014-02-14 17:56 ` Imre Deak
1 sibling, 0 replies; 8+ messages in thread
From: Chris Wilson @ 2014-02-14 17:54 UTC (permalink / raw)
To: Imre Deak; +Cc: intel-gfx
On Fri, Feb 14, 2014 at 07:48:18PM +0200, Imre Deak wrote:
> On Fri, 2014-02-14 at 17:35 +0000, Chris Wilson wrote:
> > @@ -382,18 +379,15 @@ void intel_detect_pch(struct drm_device *dev)
> > DRM_DEBUG_KMS("Found LynxPoint LP PCH\n");
> > WARN_ON(!IS_HASWELL(dev));
> > WARN_ON(!IS_ULT(dev));
> > - } else {
> > - goto check_next;
> > - }
> > + } else
> > + continue;
> > +
> > pci_dev_put(pch);
> > break;
>
> Yep, looks better. I would also move the pci_dev_put out of the loop and
> remove the above continue. But it's fine for me either way.
I don't think you can drop the continue without adding breaks to every
branch; one continue won vs remembering to add a break every time.
pci_dev_put(NULL) is safe so you could put it outside the loop, I left
it inside as I felt it was cleaner to leave the debug msg by itself.
-Chris
> > }
> > -check_next:
> > - pch = pci_get_class(PCI_CLASS_BRIDGE_ISA << 8, curr);
> > - pci_dev_put(curr);
> > }
> > if (!pch)
> > - DRM_DEBUG_KMS("No PCH found?\n");
> > + DRM_DEBUG_KMS("No PCH found.\n");
> > }
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/i915: fix pch pci device enumeration
2014-02-14 17:48 ` Imre Deak
2014-02-14 17:54 ` Chris Wilson
@ 2014-02-14 17:56 ` Imre Deak
1 sibling, 0 replies; 8+ messages in thread
From: Imre Deak @ 2014-02-14 17:56 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
On Fri, 2014-02-14 at 19:48 +0200, Imre Deak wrote:
> On Fri, 2014-02-14 at 17:35 +0000, Chris Wilson wrote:
> > On Fri, Feb 14, 2014 at 07:23:32PM +0200, Imre Deak wrote:
> > > pci_get_class(class, from) drops the refcount for 'from', so the
> > > extra pci_dev_put we do on it will result in a use after free bug
> > > sometime later starting with the WARN below.
> >
> > That's a very nice find.
> >
> > But you can tidy this loop up even more...
> >
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > > index 2d05d7c..4e4fc0c 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > @@ -347,7 +347,6 @@ void intel_detect_pch(struct drm_device *dev)
> > > */
> > > pch = pci_get_class(PCI_CLASS_BRIDGE_ISA << 8, NULL);
> > > while (pch) {
> >
> > ...by noting that what you want here is
> > while ((pch = pci_get_class(ISA<<8, pch)) {
> >
> > > - struct pci_dev *curr = pch;
> > > if (pch->vendor == PCI_VENDOR_ID_INTEL) {
> > > unsigned short id;
> > > id = pch->device & INTEL_PCH_DEVICE_ID_MASK;
> > > @@ -385,15 +384,15 @@ void intel_detect_pch(struct drm_device *dev)
> > > } else {
> > > goto check_next;
> > > }
> > > - pci_dev_put(pch);
> > > break;
> > > }
> > > check_next:
> > > - pch = pci_get_class(PCI_CLASS_BRIDGE_ISA << 8, curr);
> > > - pci_dev_put(curr);
> > > + pch = pci_get_class(PCI_CLASS_BRIDGE_ISA << 8, pch);
> > > }
> > > if (!pch)
> > > DRM_DEBUG_KMS("No PCH found?\n");
> > > + else
> > > + pci_dev_put(pch);
> > > }
> >
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index 2d05d7c..9962aef 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -324,7 +324,7 @@ MODULE_DEVICE_TABLE(pci, pciidlist);
> > void intel_detect_pch(struct drm_device *dev)
> > {
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > - struct pci_dev *pch;
> > + struct pci_dev *pch = NULL;
> >
> > /* In all current cases, num_pipes is equivalent to the PCH_NOP setting
> > * (which really amounts to a PCH but no South Display).
> > @@ -345,12 +345,9 @@ void intel_detect_pch(struct drm_device *dev)
> > * all the ISA bridge devices and check for the first match, instead
> > * of only checking the first one.
> > */
> > - pch = pci_get_class(PCI_CLASS_BRIDGE_ISA << 8, NULL);
> > - while (pch) {
> > - struct pci_dev *curr = pch;
> > + while ((pch = pci_get_class(PCI_CLASS_BRIDGE_ISA << 8, pch))) {
> > if (pch->vendor == PCI_VENDOR_ID_INTEL) {
> > - unsigned short id;
> > - id = pch->device & INTEL_PCH_DEVICE_ID_MASK;
> > + unsigned short id = pch->device & INTEL_PCH_DEVICE_ID_MASK;
> > dev_priv->pch_id = id;
> >
> > if (id == INTEL_PCH_IBX_DEVICE_ID_TYPE) {
> > @@ -382,18 +379,15 @@ void intel_detect_pch(struct drm_device *dev)
> > DRM_DEBUG_KMS("Found LynxPoint LP PCH\n");
> > WARN_ON(!IS_HASWELL(dev));
> > WARN_ON(!IS_ULT(dev));
> > - } else {
> > - goto check_next;
> > - }
> > + } else
> > + continue;
> > +
> > pci_dev_put(pch);
> > break;
>
> Yep, looks better. I would also move the pci_dev_put out of the loop and
> remove the above continue. But it's fine for me either way.
Ah sorry, the continue is still needed. But for clarity I'd still move
the pci_dev_put out.
--Imre
>
> --Imre
>
> > }
> > -check_next:
> > - pch = pci_get_class(PCI_CLASS_BRIDGE_ISA << 8, curr);
> > - pci_dev_put(curr);
> > }
> > if (!pch)
> > - DRM_DEBUG_KMS("No PCH found?\n");
> > + DRM_DEBUG_KMS("No PCH found.\n");
> > }
> >
> >
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2] drm/i915: fix pch pci device enumeration
2014-02-14 17:23 [PATCH] drm/i915: fix pch pci device enumeration Imre Deak
2014-02-14 17:35 ` Chris Wilson
@ 2014-02-14 18:23 ` Imre Deak
2014-02-14 18:36 ` Chris Wilson
1 sibling, 1 reply; 8+ messages in thread
From: Imre Deak @ 2014-02-14 18:23 UTC (permalink / raw)
To: intel-gfx
pci_get_class(class, from) drops the refcount for 'from', so the
extra pci_dev_put we do on it will result in a use after free bug
starting with the WARN below.
Regression introduced in
commit 6a9c4b35e6696a63805b6da5e4889c6986e9ee1b
Author: Rui Guo <firemeteor@users.sourceforge.net>
Date: Wed Jun 19 21:10:23 2013 +0800
drm/i915: Fix PCH detect with multiple ISA bridges in VM
[ 164.338460] WARNING: CPU: 1 PID: 2094 at include/linux/kref.h:47 klist_next+0xae/0x110()
[ 164.347731] CPU: 1 PID: 2094 Comm: modprobe Tainted: G O 3.13.0-imre+ #354
[ 164.356468] Hardware name: Intel Corp. VALLEYVIEW B0 PLATFORM/NOTEBOOK, BIOS BYTICRB1.X64.0062.R70.1310112051 10/11/2013
[ 164.368796] Call Trace:
[ 164.371609] [<ffffffff816a32a6>] dump_stack+0x4e/0x7a
[ 164.377447] [<ffffffff8104f75d>] warn_slowpath_common+0x7d/0xa0
[ 164.384238] [<ffffffff8104f83a>] warn_slowpath_null+0x1a/0x20
[ 164.390851] [<ffffffff8169aeae>] klist_next+0xae/0x110
[ 164.396777] [<ffffffff8130a110>] ? pci_do_find_bus+0x70/0x70
[ 164.403286] [<ffffffff813cb4a9>] bus_find_device+0x89/0xc0
[ 164.409719] [<ffffffff8130a373>] pci_get_dev_by_id+0x63/0xa0
[ 164.416238] [<ffffffff8130a4e4>] pci_get_class+0x44/0x50
[ 164.422433] [<ffffffffa034821f>] intel_dsm_detect+0x16f/0x1f0 [i915]
[ 164.429801] [<ffffffffa03482ae>] intel_register_dsm_handler+0xe/0x10 [i915]
[ 164.437831] [<ffffffffa02d30fe>] i915_driver_load+0xafe/0xf30 [i915]
[ 164.445126] [<ffffffff8158a150>] ? intel_alloc_coherent+0x110/0x110
[ 164.452340] [<ffffffffa0148c07>] drm_dev_register+0xc7/0x150 [drm]
[ 164.459462] [<ffffffffa014b23f>] drm_get_pci_dev+0x11f/0x1f0 [drm]
[ 164.466554] [<ffffffff816abb81>] ? _raw_spin_unlock_irqrestore+0x51/0x70
[ 164.474287] [<ffffffffa02cf7a6>] i915_pci_probe+0x56/0x60 [i915]
[ 164.481185] [<ffffffff8130a028>] pci_device_probe+0x78/0xf0
[ 164.487603] [<ffffffff813cd495>] driver_probe_device+0x155/0x350
[ 164.494505] [<ffffffff813cd74e>] __driver_attach+0x6e/0xa0
[ 164.500826] [<ffffffff813cd6e0>] ? __device_attach+0x50/0x50
[ 164.507333] [<ffffffff813cb2be>] bus_for_each_dev+0x6e/0xc0
[ 164.513752] [<ffffffff813ccefe>] driver_attach+0x1e/0x20
[ 164.519870] [<ffffffff813cc958>] bus_add_driver+0x138/0x260
[ 164.526289] [<ffffffffa0188000>] ? 0xffffffffa0187fff
[ 164.532116] [<ffffffff813cde78>] driver_register+0x98/0xe0
[ 164.538558] [<ffffffffa0188000>] ? 0xffffffffa0187fff
[ 164.544389] [<ffffffff813087b0>] __pci_register_driver+0x60/0x70
[ 164.551336] [<ffffffffa014b37d>] drm_pci_init+0x6d/0x120 [drm]
[ 164.558040] [<ffffffffa0188000>] ? 0xffffffffa0187fff
[ 164.563928] [<ffffffffa018806a>] i915_init+0x6a/0x6c [i915]
[ 164.570363] [<ffffffff810002da>] do_one_initcall+0xaa/0x160
[ 164.576783] [<ffffffff8103b140>] ? set_memory_nx+0x40/0x50
[ 164.583100] [<ffffffff810ce7f5>] load_module+0x1fb5/0x2550
[ 164.589410] [<ffffffff810caab0>] ? store_uevent+0x40/0x40
[ 164.595628] [<ffffffff810cee7d>] SyS_init_module+0xed/0x100
[ 164.602048] [<ffffffff816b3c52>] system_call_fastpath+0x16/0x1b
v2: simplify the loop further (Chris)
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
drivers/gpu/drm/i915/i915_drv.c | 23 +++++++++--------------
1 file changed, 9 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 2d05d7c..08052f3d 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -324,7 +324,7 @@ MODULE_DEVICE_TABLE(pci, pciidlist);
void intel_detect_pch(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev->dev_private;
- struct pci_dev *pch;
+ struct pci_dev *pch = NULL;
/* In all current cases, num_pipes is equivalent to the PCH_NOP setting
* (which really amounts to a PCH but no South Display).
@@ -345,12 +345,9 @@ void intel_detect_pch(struct drm_device *dev)
* all the ISA bridge devices and check for the first match, instead
* of only checking the first one.
*/
- pch = pci_get_class(PCI_CLASS_BRIDGE_ISA << 8, NULL);
- while (pch) {
- struct pci_dev *curr = pch;
+ while ((pch = pci_get_class(PCI_CLASS_BRIDGE_ISA << 8, pch))) {
if (pch->vendor == PCI_VENDOR_ID_INTEL) {
- unsigned short id;
- id = pch->device & INTEL_PCH_DEVICE_ID_MASK;
+ unsigned short id = pch->device & INTEL_PCH_DEVICE_ID_MASK;
dev_priv->pch_id = id;
if (id == INTEL_PCH_IBX_DEVICE_ID_TYPE) {
@@ -382,18 +379,16 @@ void intel_detect_pch(struct drm_device *dev)
DRM_DEBUG_KMS("Found LynxPoint LP PCH\n");
WARN_ON(!IS_HASWELL(dev));
WARN_ON(!IS_ULT(dev));
- } else {
- goto check_next;
- }
- pci_dev_put(pch);
+ } else
+ continue;
+
break;
}
-check_next:
- pch = pci_get_class(PCI_CLASS_BRIDGE_ISA << 8, curr);
- pci_dev_put(curr);
}
if (!pch)
- DRM_DEBUG_KMS("No PCH found?\n");
+ DRM_DEBUG_KMS("No PCH found.\n");
+
+ pci_dev_put(pch);
}
bool i915_semaphore_is_enabled(struct drm_device *dev)
--
1.8.4
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2] drm/i915: fix pch pci device enumeration
2014-02-14 18:23 ` [PATCH v2] " Imre Deak
@ 2014-02-14 18:36 ` Chris Wilson
2014-02-28 14:12 ` Jani Nikula
0 siblings, 1 reply; 8+ messages in thread
From: Chris Wilson @ 2014-02-14 18:36 UTC (permalink / raw)
To: Imre Deak; +Cc: intel-gfx
On Fri, Feb 14, 2014 at 08:23:54PM +0200, Imre Deak wrote:
> pci_get_class(class, from) drops the refcount for 'from', so the
> extra pci_dev_put we do on it will result in a use after free bug
> starting with the WARN below.
>
> Regression introduced in
>
> commit 6a9c4b35e6696a63805b6da5e4889c6986e9ee1b
> Author: Rui Guo <firemeteor@users.sourceforge.net>
> Date: Wed Jun 19 21:10:23 2013 +0800
>
> drm/i915: Fix PCH detect with multiple ISA bridges in VM
>
> [ 164.338460] WARNING: CPU: 1 PID: 2094 at include/linux/kref.h:47 klist_next+0xae/0x110()
> [ 164.347731] CPU: 1 PID: 2094 Comm: modprobe Tainted: G O 3.13.0-imre+ #354
> [ 164.356468] Hardware name: Intel Corp. VALLEYVIEW B0 PLATFORM/NOTEBOOK, BIOS BYTICRB1.X64.0062.R70.1310112051 10/11/2013
> [ 164.368796] Call Trace:
> [ 164.371609] [<ffffffff816a32a6>] dump_stack+0x4e/0x7a
> [ 164.377447] [<ffffffff8104f75d>] warn_slowpath_common+0x7d/0xa0
> [ 164.384238] [<ffffffff8104f83a>] warn_slowpath_null+0x1a/0x20
> [ 164.390851] [<ffffffff8169aeae>] klist_next+0xae/0x110
> [ 164.396777] [<ffffffff8130a110>] ? pci_do_find_bus+0x70/0x70
> [ 164.403286] [<ffffffff813cb4a9>] bus_find_device+0x89/0xc0
> [ 164.409719] [<ffffffff8130a373>] pci_get_dev_by_id+0x63/0xa0
> [ 164.416238] [<ffffffff8130a4e4>] pci_get_class+0x44/0x50
> [ 164.422433] [<ffffffffa034821f>] intel_dsm_detect+0x16f/0x1f0 [i915]
> [ 164.429801] [<ffffffffa03482ae>] intel_register_dsm_handler+0xe/0x10 [i915]
> [ 164.437831] [<ffffffffa02d30fe>] i915_driver_load+0xafe/0xf30 [i915]
> [ 164.445126] [<ffffffff8158a150>] ? intel_alloc_coherent+0x110/0x110
> [ 164.452340] [<ffffffffa0148c07>] drm_dev_register+0xc7/0x150 [drm]
> [ 164.459462] [<ffffffffa014b23f>] drm_get_pci_dev+0x11f/0x1f0 [drm]
> [ 164.466554] [<ffffffff816abb81>] ? _raw_spin_unlock_irqrestore+0x51/0x70
> [ 164.474287] [<ffffffffa02cf7a6>] i915_pci_probe+0x56/0x60 [i915]
> [ 164.481185] [<ffffffff8130a028>] pci_device_probe+0x78/0xf0
> [ 164.487603] [<ffffffff813cd495>] driver_probe_device+0x155/0x350
> [ 164.494505] [<ffffffff813cd74e>] __driver_attach+0x6e/0xa0
> [ 164.500826] [<ffffffff813cd6e0>] ? __device_attach+0x50/0x50
> [ 164.507333] [<ffffffff813cb2be>] bus_for_each_dev+0x6e/0xc0
> [ 164.513752] [<ffffffff813ccefe>] driver_attach+0x1e/0x20
> [ 164.519870] [<ffffffff813cc958>] bus_add_driver+0x138/0x260
> [ 164.526289] [<ffffffffa0188000>] ? 0xffffffffa0187fff
> [ 164.532116] [<ffffffff813cde78>] driver_register+0x98/0xe0
> [ 164.538558] [<ffffffffa0188000>] ? 0xffffffffa0187fff
> [ 164.544389] [<ffffffff813087b0>] __pci_register_driver+0x60/0x70
> [ 164.551336] [<ffffffffa014b37d>] drm_pci_init+0x6d/0x120 [drm]
> [ 164.558040] [<ffffffffa0188000>] ? 0xffffffffa0187fff
> [ 164.563928] [<ffffffffa018806a>] i915_init+0x6a/0x6c [i915]
> [ 164.570363] [<ffffffff810002da>] do_one_initcall+0xaa/0x160
> [ 164.576783] [<ffffffff8103b140>] ? set_memory_nx+0x40/0x50
> [ 164.583100] [<ffffffff810ce7f5>] load_module+0x1fb5/0x2550
> [ 164.589410] [<ffffffff810caab0>] ? store_uevent+0x40/0x40
> [ 164.595628] [<ffffffff810cee7d>] SyS_init_module+0xed/0x100
> [ 164.602048] [<ffffffff816b3c52>] system_call_fastpath+0x16/0x1b
>
> v2: simplify the loop further (Chris)
>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
Reported-by: Jesse Barnes <jbarnes@virtuousgeek.org>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=65652
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=74161
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: stable@vger.kernel.org
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] drm/i915: fix pch pci device enumeration
2014-02-14 18:36 ` Chris Wilson
@ 2014-02-28 14:12 ` Jani Nikula
0 siblings, 0 replies; 8+ messages in thread
From: Jani Nikula @ 2014-02-28 14:12 UTC (permalink / raw)
To: Chris Wilson, Imre Deak; +Cc: intel-gfx
On Fri, 14 Feb 2014, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Fri, Feb 14, 2014 at 08:23:54PM +0200, Imre Deak wrote:
>> pci_get_class(class, from) drops the refcount for 'from', so the
>> extra pci_dev_put we do on it will result in a use after free bug
>> starting with the WARN below.
>>
>> Regression introduced in
>>
>> commit 6a9c4b35e6696a63805b6da5e4889c6986e9ee1b
>> Author: Rui Guo <firemeteor@users.sourceforge.net>
>> Date: Wed Jun 19 21:10:23 2013 +0800
>>
>> drm/i915: Fix PCH detect with multiple ISA bridges in VM
>>
>> [ 164.338460] WARNING: CPU: 1 PID: 2094 at include/linux/kref.h:47 klist_next+0xae/0x110()
>> [ 164.347731] CPU: 1 PID: 2094 Comm: modprobe Tainted: G O 3.13.0-imre+ #354
>> [ 164.356468] Hardware name: Intel Corp. VALLEYVIEW B0 PLATFORM/NOTEBOOK, BIOS BYTICRB1.X64.0062.R70.1310112051 10/11/2013
>> [ 164.368796] Call Trace:
>> [ 164.371609] [<ffffffff816a32a6>] dump_stack+0x4e/0x7a
>> [ 164.377447] [<ffffffff8104f75d>] warn_slowpath_common+0x7d/0xa0
>> [ 164.384238] [<ffffffff8104f83a>] warn_slowpath_null+0x1a/0x20
>> [ 164.390851] [<ffffffff8169aeae>] klist_next+0xae/0x110
>> [ 164.396777] [<ffffffff8130a110>] ? pci_do_find_bus+0x70/0x70
>> [ 164.403286] [<ffffffff813cb4a9>] bus_find_device+0x89/0xc0
>> [ 164.409719] [<ffffffff8130a373>] pci_get_dev_by_id+0x63/0xa0
>> [ 164.416238] [<ffffffff8130a4e4>] pci_get_class+0x44/0x50
>> [ 164.422433] [<ffffffffa034821f>] intel_dsm_detect+0x16f/0x1f0 [i915]
>> [ 164.429801] [<ffffffffa03482ae>] intel_register_dsm_handler+0xe/0x10 [i915]
>> [ 164.437831] [<ffffffffa02d30fe>] i915_driver_load+0xafe/0xf30 [i915]
>> [ 164.445126] [<ffffffff8158a150>] ? intel_alloc_coherent+0x110/0x110
>> [ 164.452340] [<ffffffffa0148c07>] drm_dev_register+0xc7/0x150 [drm]
>> [ 164.459462] [<ffffffffa014b23f>] drm_get_pci_dev+0x11f/0x1f0 [drm]
>> [ 164.466554] [<ffffffff816abb81>] ? _raw_spin_unlock_irqrestore+0x51/0x70
>> [ 164.474287] [<ffffffffa02cf7a6>] i915_pci_probe+0x56/0x60 [i915]
>> [ 164.481185] [<ffffffff8130a028>] pci_device_probe+0x78/0xf0
>> [ 164.487603] [<ffffffff813cd495>] driver_probe_device+0x155/0x350
>> [ 164.494505] [<ffffffff813cd74e>] __driver_attach+0x6e/0xa0
>> [ 164.500826] [<ffffffff813cd6e0>] ? __device_attach+0x50/0x50
>> [ 164.507333] [<ffffffff813cb2be>] bus_for_each_dev+0x6e/0xc0
>> [ 164.513752] [<ffffffff813ccefe>] driver_attach+0x1e/0x20
>> [ 164.519870] [<ffffffff813cc958>] bus_add_driver+0x138/0x260
>> [ 164.526289] [<ffffffffa0188000>] ? 0xffffffffa0187fff
>> [ 164.532116] [<ffffffff813cde78>] driver_register+0x98/0xe0
>> [ 164.538558] [<ffffffffa0188000>] ? 0xffffffffa0187fff
>> [ 164.544389] [<ffffffff813087b0>] __pci_register_driver+0x60/0x70
>> [ 164.551336] [<ffffffffa014b37d>] drm_pci_init+0x6d/0x120 [drm]
>> [ 164.558040] [<ffffffffa0188000>] ? 0xffffffffa0187fff
>> [ 164.563928] [<ffffffffa018806a>] i915_init+0x6a/0x6c [i915]
>> [ 164.570363] [<ffffffff810002da>] do_one_initcall+0xaa/0x160
>> [ 164.576783] [<ffffffff8103b140>] ? set_memory_nx+0x40/0x50
>> [ 164.583100] [<ffffffff810ce7f5>] load_module+0x1fb5/0x2550
>> [ 164.589410] [<ffffffff810caab0>] ? store_uevent+0x40/0x40
>> [ 164.595628] [<ffffffff810cee7d>] SyS_init_module+0xed/0x100
>> [ 164.602048] [<ffffffff816b3c52>] system_call_fastpath+0x16/0x1b
>>
>> v2: simplify the loop further (Chris)
>>
>> Signed-off-by: Imre Deak <imre.deak@intel.com>
> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> Reported-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=65652
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=74161
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: stable@vger.kernel.org
Pushed to -fixes, thanks for the patch and review.
BR,
Jani.
>
> --
> Chris Wilson, Intel Open Source Technology Centre
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Jani Nikula, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2014-02-28 14:13 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-02-14 17:23 [PATCH] drm/i915: fix pch pci device enumeration Imre Deak
2014-02-14 17:35 ` Chris Wilson
2014-02-14 17:48 ` Imre Deak
2014-02-14 17:54 ` Chris Wilson
2014-02-14 17:56 ` Imre Deak
2014-02-14 18:23 ` [PATCH v2] " Imre Deak
2014-02-14 18:36 ` Chris Wilson
2014-02-28 14:12 ` Jani Nikula
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.