Re: [PATCH] 6700/6702PXH quirk

[Kristen Accardi <kristen.c.accardi@intel.com>]

On Mon, 2005-08-08 at 10:36 -0600, Bjorn Helgaas wrote:
> On Friday 05 August 2005 5:51 pm, Kristen Accardi wrote:
> > On the 6700/6702 PXH part, a MSI may get corrupted if an ACPI hotplug
> > driver and SHPC driver in MSI mode are used together.  This patch will
> > prevent MSI from being enabled for the SHPC as part of an early pci
> > quirk, as well as on any pci device which sets the no_msi bit.  
> 
> For mailing list archaeology, I assume this is erratum 15 in the
> 6700/6702 PXH spec update:
> 	http://download.intel.com/design/chipsets/specupdt/30270609.pdf
> 
> which says
> 
> 	An MSI is generated by the standard hot-plug controller may
> 	get corrupted in presence of another ACPI hot-plug driver.
> 	The ACPI driver performs configuration reads of DWSEL/DWORD
> 	register in order to determine the hot-plug capability of all
> 	the ACPI devices.  If the MSI is generated by the Standard
> 	Hot-Plug Controller (SHPC) in this time period, there is a
> 	possibility of the MSI getting corrupted.  As a result the
> 	MSI may not get issued upstream to the MCH.  The above is a
> 	result of interaction of separate events that are unpredict-
> 	able.

That's correct.

> 
> So what still bugs me about this (and I'm probably just showing my
> ignorance here), is that we seem to have two drivers (SHPC and ACPI)
> poking at the same hardware.  Why is this?
> 
> And where exactly is the ACPI code that is involved?  I see shpc_init()
> doing config reads of DWORD_DATA, but I don't see how ACPI is involved.
> Is there some AML that's doing the config accesses?  Why would there
> be AML if we're using SHPC?
> 
> > @@ -699,6 +699,9 @@ int pci_enable_msi(struct pci_dev* dev)
> >  	if (!pci_msi_enable || !dev)
> >   		return status;
> >  
> > +	if (dev->no_msi)
> > +		return status;
> > +
> 

I am just learning this stuff as well, so hopefully someone will correct
me if I'm wrong.  This seems like a poorly worded erratum.  The acpiphp
driver does not actual do any config reads - it just asks the acpi core
to read the acpi namespace to determine the hotplug capabilities.  I
will try to find out more about the test case that they used to discover
this problem and get someone to explain it to me in english.


> Is there any reason not to fold this into the test above it?
> 

No, it seems that patches done at 4:45 on Friday don't turn out
optimally :).

> > +static void __devinit quirk_pcie_pxh(struct pci_dev *dev)
> > +{
> > +	disable_msi_mode(dev, pci_find_capability(dev, PCI_CAP_ID_MSI),
> > +					PCI_CAP_ID_MSI);
> 
> Is this even needed?  You're doing early fixups, which happen before
> any drivers touch the device, so you should only need to disable MSI
> if the BIOS can leave it enabled.  But I would have thought MSI would
> be disabled until a driver explicitly enables it.

This was me being paranoid.  I was concerned that some BIOS might decide
to enable by default, so I was just trying to make really really sure
that MSI would be turned off.  Think that's overkill?