Re: [PATCH] drm/i915: fix pch pci device enumeration

[Imre Deak <imre.deak@intel.com>]

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");
> >  }
> >  
> > 
>