Re: [PATCH] 6700/6702PXH quirk
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?
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] 6700/6702PXH quirk
> You can hide the "complexity" of the second line behind > macros. And this is what is done in most places. oh, I agree. My only point is that if the *only* argument against bitfields is that they're inefficient (insert vague hand-waving) then people will happily decide to live with that inefficiency. I'm all for macros that are both efficient *and* abstract away the risk of getting open-coding wrong. - z - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] 6700/6702PXH quirk
From: Zach Brown <[EMAIL PROTECTED]> Date: Mon, 08 Aug 2005 10:42:37 -0700 > if (!foo->enabled) > if (!(foo->flags & FOO_FLAG_ENABLED) You can hide the "complexity" of the second line behind macros. And this is what is done in most places. Alternatively, you can use the existing bitops interfaces, and in that case you define bit numbers only then use the bitops.h interfaces to do all the work (probably the __set_bit() et al. non-atomic variants in this case). Really, I think it's worth it. I absolutely refuse to put sets of boolean states into C bitfields or even worse integer members. Just define a u32 bitmask and be done with it. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] 6700/6702PXH quirk
Jeff Garzik wrote:
>
>
> FWIW, compilers generate AWFUL code for bitfields. Bitfields are
> really tough to do optimally, whereas bit flags ["unsigned int flags &
> bitmask"] are the familiar ints and longs that the compiler is well
> tuned to optimize.
I wouldn't have chosen the micro-optimization argument against bitfields
because people who use them as booleans will be more than willing to
trade minuscule performance degredation in non-critical paths for
heaping piles of legibility:
if (!foo->enabled)
if (!(foo->flags & FOO_FLAG_ENABLED)
No, I would have chosen the maintenance risk of forgetting that they're
really 1 bit wide scalars which truncate on assignment.
struct foo {
unsigned enabled:1;
};
int some_thing_is_enabled(thing) {
return thing->whatever & some_high_bit;
}
foo->enabled = some_thing_is_enabled()
Requiring people to remember that they want !!() around assignments
seems much more dangerous. They'll get left out to "optimize" current
behaviour, leaving land mines in the tree for future maintainers.
- z
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] 6700/6702PXH quirk
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.
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;
> +
Is there any reason not to fold this into the test above it?
> +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.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] 6700/6702PXH quirk
On Saturday 06 August 2005 18:57, Jeff Garzik wrote: > On Sat, Aug 06, 2005 at 09:50:13AM +0100, Matthew Wilcox wrote: > > On Fri, Aug 05, 2005 at 11:34:55PM -0400, Jeff Garzik wrote: > > > FWIW, compilers generate AWFUL code for bitfields. Bitfields are > > > really tough to do optimally, whereas bit flags ["unsigned int flags & > > > bitmask"] are the familiar ints and longs that the compiler is well > > > tuned to optimize. > > > > I'm sure the GCC developers would appreciate a good bug report with a > > test-case that generates worse code. If you don't want to mess with their > > bug tracking system, just send me a test case and I'll add it for you. > > Its an order-of-complexity issue. No matter how hard you try, > bitfields will -always- be tougher to optimize, than machine ints. > > Bitfields are weirdly-sized, weirdly-aligned integers. A simple look at > the generated asm from gcc on ARM or MIPS demonstrates the explosion of > code that can sometimes occur, versus a simple 'and' test of a machine > int and a mask. x86 is a tiny bit better, but still more expensive to > do bitfields than machine ints. But we are talking about one-bit field here: + unsigned intno_msi:1; /* device may not use msi */ If _this_ isn't optimized nicely into ANDs, ORs, etc, then bug report is in order and gcc should be fixed. -- vda - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] 6700/6702PXH quirk
On Sat, Aug 06, 2005 at 09:50:13AM +0100, Matthew Wilcox wrote: > On Fri, Aug 05, 2005 at 11:34:55PM -0400, Jeff Garzik wrote: > > FWIW, compilers generate AWFUL code for bitfields. Bitfields are > > really tough to do optimally, whereas bit flags ["unsigned int flags & > > bitmask"] are the familiar ints and longs that the compiler is well > > tuned to optimize. > > I'm sure the GCC developers would appreciate a good bug report with a > test-case that generates worse code. If you don't want to mess with their > bug tracking system, just send me a test case and I'll add it for you. Its an order-of-complexity issue. No matter how hard you try, bitfields will -always- be tougher to optimize, than machine ints. Bitfields are weirdly-sized, weirdly-aligned integers. A simple look at the generated asm from gcc on ARM or MIPS demonstrates the explosion of code that can sometimes occur, versus a simple 'and' test of a machine int and a mask. x86 is a tiny bit better, but still more expensive to do bitfields than machine ints. Jeff - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] 6700/6702PXH quirk
On Fri, Aug 05, 2005 at 11:34:55PM -0400, Jeff Garzik wrote: > FWIW, compilers generate AWFUL code for bitfields. Bitfields are > really tough to do optimally, whereas bit flags ["unsigned int flags & > bitmask"] are the familiar ints and longs that the compiler is well > tuned to optimize. I'm sure the GCC developers would appreciate a good bug report with a test-case that generates worse code. If you don't want to mess with their bug tracking system, just send me a test case and I'll add it for you. -- "Next the statesmen will invent cheap lies, putting the blame upon the nation that is attacked, and every man will be glad of those conscience-soothing falsities, and will diligently study them, and refuse to examine any refutations of them; and thus he will by and by convince himself that the war is just, and will thank God for the better sleep he enjoys after this process of grotesque self-deception." -- Mark Twain - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] 6700/6702PXH quirk
On Fri, Aug 05, 2005 at 03:57:12PM -0700, Greg KH wrote: > Anyway, Jeff is right, add another bit field. The updated patch, which adds a new bitfield, looks OK to me. However... FWIW, compilers generate AWFUL code for bitfields. Bitfields are really tough to do optimally, whereas bit flags ["unsigned int flags & bitmask"] are the familiar ints and longs that the compiler is well tuned to optimize. Additionally, though it is not the case with struct pci_dev, bitfields cause endian headaches (see the LITTLE_ENDIAN_BITFIELD ifdefs). Bit flags are -far- superior in every case. Avoid bitfields like the plague. I wouldn't mind seeing a janitor remove all bitfields from struct pci_dev, and any other kernel structure that uses the evil constructs. Jeff - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] 6700/6702PXH quirk
On Fri, 2005-08-05 at 18:50 -0400, Jeff Garzik wrote:
>
> AFAICS we don't need a new list, simply consisting of PCI devs.
>
> Just invent, and set, a bit somewhere in struct pci_dev.
>
> Jeff
>
>
>
Great! I like that much better. How about this:
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.
Signed-off-by: Kristen Carlson Accardi <[EMAIL PROTECTED]>
diff -uprN -X linux-2.6.13-rc4/Documentation/dontdiff
linux-2.6.13-rc4/drivers/pci/msi.c linux-2.6.13-rc4-pxhquirk/drivers/pci/msi.c
--- linux-2.6.13-rc4/drivers/pci/msi.c 2005-07-28 15:44:44.0 -0700
+++ linux-2.6.13-rc4-pxhquirk/drivers/pci/msi.c 2005-08-05 16:35:16.0
-0700
@@ -453,7 +453,7 @@ static void enable_msi_mode(struct pci_d
}
}
-static void disable_msi_mode(struct pci_dev *dev, int pos, int type)
+void disable_msi_mode(struct pci_dev *dev, int pos, int type)
{
u16 control;
@@ -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;
+
temp = dev->irq;
if ((status = msi_init()) < 0)
diff -uprN -X linux-2.6.13-rc4/Documentation/dontdiff
linux-2.6.13-rc4/drivers/pci/pci.h linux-2.6.13-rc4-pxhquirk/drivers/pci/pci.h
--- linux-2.6.13-rc4/drivers/pci/pci.h 2005-07-28 15:44:44.0 -0700
+++ linux-2.6.13-rc4-pxhquirk/drivers/pci/pci.h 2005-08-05 16:37:18.0
-0700
@@ -46,7 +46,7 @@ extern int pci_msi_quirk;
#else
#define pci_msi_quirk 0
#endif
-
+void disable_msi_mode(struct pci_dev *dev, int pos, int type);
extern int pcie_mch_quirk;
extern struct device_attribute pci_dev_attrs[];
extern struct class_device_attribute class_device_attr_cpuaffinity;
diff -uprN -X linux-2.6.13-rc4/Documentation/dontdiff
linux-2.6.13-rc4/drivers/pci/quirks.c
linux-2.6.13-rc4-pxhquirk/drivers/pci/quirks.c
--- linux-2.6.13-rc4/drivers/pci/quirks.c 2005-07-28 15:44:44.0
-0700
+++ linux-2.6.13-rc4-pxhquirk/drivers/pci/quirks.c 2005-08-05
16:38:28.0 -0700
@@ -1267,6 +1267,27 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_IN
DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_E7320_MCH,
quirk_pcie_mch );
DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_E7525_MCH,
quirk_pcie_mch );
+
+/*
+ * It's possible for the MSI to get corrupted if shpc and acpi
+ * are used together on certain PXH-based systems.
+ */
+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);
+ dev->no_msi = 1;
+
+ printk(KERN_WARNING "PCI: PXH quirk detected, "
+ "disabling MSI for SHPC device\n");
+}
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_PXHD_0,
quirk_pcie_pxh);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_PXHD_1,
quirk_pcie_pxh);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_PXH_0,
quirk_pcie_pxh);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_PXH_1,
quirk_pcie_pxh);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_PXHV,
quirk_pcie_pxh);
+
+
static void __devinit quirk_netmos(struct pci_dev *dev)
{
unsigned int num_parallel = (dev->subsystem_device & 0xf0) >> 4;
diff -uprN -X linux-2.6.13-rc4/Documentation/dontdiff
linux-2.6.13-rc4/include/linux/pci.h
linux-2.6.13-rc4-pxhquirk/include/linux/pci.h
--- linux-2.6.13-rc4/include/linux/pci.h2005-07-28 15:44:44.0
-0700
+++ linux-2.6.13-rc4-pxhquirk/include/linux/pci.h 2005-08-05
16:37:08.0 -0700
@@ -556,6 +556,7 @@ struct pci_dev {
/* keep track of device state */
unsigned intis_enabled:1; /* pci_enable_device has been called */
unsigned intis_busmaster:1; /* device is busmaster */
+ unsigned intno_msi:1; /* device may not use msi */
u32 saved_config_space[16]; /* config space saved at
suspend time */
struct bin_attribute *rom_attr; /* attribute descriptor for sysfs ROM
entry */
diff -uprN -X linux-2.6.13-rc4/Documentation/dontdiff
linux-2.6.13-rc4/include/linux/pci_ids.h
linux-2.6.13-rc4-pxhquirk/include/linux/pci_ids.h
--- linux-2.6.13-rc4/include/linux/pci_ids.h2005-07-28 15:44:44.0
-0700
+++ linux-2.6.13-rc4-pxhquirk/include/linux/pci_ids.h 2005-08-02
13:58:53.0 -0700
@@ -2281,6 +2281,11 @@
#define PCI_VENDOR_ID_INTEL0x8086
#define PCI_DEVICE_ID_INTEL_EESSC 0x0008
#define PCI_DEVICE_ID_INTEL_21145 0x0039
+#define PCI_DEVICE_ID_INTEL_PXHD_0 0x0320
+#define PCI_DEVICE_ID_INTEL
Re: [PATCH] 6700/6702PXH quirk
On Fri, Aug 05, 2005 at 03:05:13PM -0700, Kristen Accardi wrote:
> +int msi_add_quirk(struct pci_dev *dev)
> +{
> + struct msi_quirk *quirk;
> +
> + quirk = (struct msi_quirk *) kmalloc(sizeof(*quirk), GFP_KERNEL);
> + if (!quirk)
> + return -ENOMEM;
> +
> + INIT_LIST_HEAD(&quirk->list);
> + quirk->dev = dev;
You just messed up the reference counting of this device :(
Anyway, Jeff is right, add another bit field.
thanks,
greg k-h
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] 6700/6702PXH quirk
Kristen Accardi <[EMAIL PROTECTED]> wrote: > > On Fri, 2005-08-05 at 15:26 -0700, Andrew Morton wrote: > > Kristen Accardi <[EMAIL PROTECTED]> wrote: > > > > + if (!quirk) > > > + return -ENOMEM; > > > + > > > + INIT_LIST_HEAD(&quirk->list); > > > + quirk->dev = dev; > > > + list_add(&quirk->list, &msi_quirk_list); > > > + return 0; > > > +} > > > > Does the list not need any locking? > > Actually, I'm glad you asked that question because I was wondering that > myself. The devices are added to the list at boot time, and after that > time, the list will never change. Does PCI enumeration happen on all > processors? I thought maybe it only happened on one. In that case we > don't need a lock I don't think. > do_basic_setup() is called after SMP is up and running. do_basic_setup() calls driver_init() and most of the initcalls. Plus there's kernel preemption. So yup, I think you need locking.. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] 6700/6702PXH quirk
AFAICS we don't need a new list, simply consisting of PCI devs. Just invent, and set, a bit somewhere in struct pci_dev. Jeff - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] 6700/6702PXH quirk
On Fri, 2005-08-05 at 15:26 -0700, Andrew Morton wrote: > Kristen Accardi <[EMAIL PROTECTED]> wrote: > > + if (!quirk) > > + return -ENOMEM; > > + > > + INIT_LIST_HEAD(&quirk->list); > > + quirk->dev = dev; > > + list_add(&quirk->list, &msi_quirk_list); > > + return 0; > > +} > > Does the list not need any locking? Actually, I'm glad you asked that question because I was wondering that myself. The devices are added to the list at boot time, and after that time, the list will never change. Does PCI enumeration happen on all processors? I thought maybe it only happened on one. In that case we don't need a lock I don't think. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] 6700/6702PXH quirk
Kristen Accardi <[EMAIL PROTECTED]> 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.
>
> I made this patch more generic than just shpc because I thought it was
> possible that other devices in the system might need to add themselves
> to the msi black list.
>
> diff -uprN -X linux-2.6.13-rc4/Documentation/dontdiff
> linux-2.6.13-rc4/drivers/pci/msi.c linux-2.6.13-rc4-pxhquirk/drivers/pci/msi.c
> --- linux-2.6.13-rc4/drivers/pci/msi.c2005-07-28 15:44:44.0
> -0700
> +++ linux-2.6.13-rc4-pxhquirk/drivers/pci/msi.c 2005-08-05
> 11:38:00.0 -0700
> @@ -38,6 +38,32 @@ int vector_irq[NR_VECTORS] = { [0 ... NR
> u8 irq_vector[NR_IRQ_VECTORS] = { FIRST_DEVICE_VECTOR , 0 };
> #endif
>
> +
> +LIST_HEAD(msi_quirk_list);
> +
Can't this have static scope?
> +struct msi_quirk
> +{
> + struct list_head list;
> + struct pci_dev *dev;
> +};
We normally do
struct msi_quirk {
> +
> +int msi_add_quirk(struct pci_dev *dev)
> +{
> + struct msi_quirk *quirk;
> +
> + quirk = (struct msi_quirk *) kmalloc(sizeof(*quirk), GFP_KERNEL);
kmalloc() returns void*, hence no typecast is needed. In fact it's
undesirable because the cast defeats all typechecking.
> + if (!quirk)
> + return -ENOMEM;
> +
> + INIT_LIST_HEAD(&quirk->list);
> + quirk->dev = dev;
> + list_add(&quirk->list, &msi_quirk_list);
> + return 0;
> +}
Does the list not need any locking?
> --- linux-2.6.13-rc4/drivers/pci/quirks.c 2005-07-28 15:44:44.0
> -0700
> +++ linux-2.6.13-rc4-pxhquirk/drivers/pci/quirks.c2005-08-05
> 11:54:15.0 -0700
> @@ -21,6 +21,10 @@
> #include
> #include "pci.h"
>
> +
> +void disable_msi_mode(struct pci_dev *dev, int pos, int type);
> +int msi_add_quirk(struct pci_dev *dev);
> +
Please put these declarations in a .h file which is visible to the
implementations and to all users.
> +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);
> + if (!msi_add_quirk(dev))
> + printk(KERN_WARNING "PCI: PXH quirk detected, disabling MSI for
> SHPC device\n");
> + else {
> + pci_msi_quirk = 1;
> + printk(KERN_WARNING "PCI: PXH quirk detected, unable to disable
> MSI for SHPC device, disabling MSI for all devices\n");
> + }
Some people use 80-column xterms. Break the strings up thusly:
printk(KERN_WARNING "PCI: PXH quirk detected, disabling "
"MSI for SHPC device\n");
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] 6700/6702PXH quirk
On Fri, 2005-08-05 at 11:35 -0700, Greg KH wrote:
> On Fri, Aug 05, 2005 at 09:27:42AM -0700, Kristen Accardi wrote:
> > @@ -1127,3 +1159,5 @@ EXPORT_SYMBOL(pci_enable_msi);
> > EXPORT_SYMBOL(pci_disable_msi);
> > EXPORT_SYMBOL(pci_enable_msix);
> > EXPORT_SYMBOL(pci_disable_msix);
> > +EXPORT_SYMBOL(disable_msi_mode);
> > +EXPORT_SYMBOL(msi_add_quirk);
>
> Why do these need to be exported? It doesn't look like you are trying
> to access these from a module, or do you have a patch that uses them
> somewhere else?
>
> thanks,
>
> greg k-h
resend with changelog info:
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.
I made this patch more generic than just shpc because I thought it was
possible that other devices in the system might need to add themselves
to the msi black list.
Signed-off-by: Kristen Carlson Accardi <[EMAIL PROTECTED]>
diff -uprN -X linux-2.6.13-rc4/Documentation/dontdiff
linux-2.6.13-rc4/drivers/pci/msi.c linux-2.6.13-rc4-pxhquirk/drivers/pci/msi.c
--- linux-2.6.13-rc4/drivers/pci/msi.c 2005-07-28 15:44:44.0 -0700
+++ linux-2.6.13-rc4-pxhquirk/drivers/pci/msi.c 2005-08-05 11:38:00.0
-0700
@@ -38,6 +38,32 @@ int vector_irq[NR_VECTORS] = { [0 ... NR
u8 irq_vector[NR_IRQ_VECTORS] = { FIRST_DEVICE_VECTOR , 0 };
#endif
+
+LIST_HEAD(msi_quirk_list);
+
+struct msi_quirk
+{
+ struct list_head list;
+ struct pci_dev *dev;
+};
+
+
+int msi_add_quirk(struct pci_dev *dev)
+{
+ struct msi_quirk *quirk;
+
+ quirk = (struct msi_quirk *) kmalloc(sizeof(*quirk), GFP_KERNEL);
+ if (!quirk)
+ return -ENOMEM;
+
+ INIT_LIST_HEAD(&quirk->list);
+ quirk->dev = dev;
+ list_add(&quirk->list, &msi_quirk_list);
+ return 0;
+}
+
+
+
static void msi_cache_ctor(void *p, kmem_cache_t *cache, unsigned long flags)
{
memset(p, 0, NR_IRQS * sizeof(struct msi_desc));
@@ -453,7 +479,7 @@ static void enable_msi_mode(struct pci_d
}
}
-static void disable_msi_mode(struct pci_dev *dev, int pos, int type)
+void disable_msi_mode(struct pci_dev *dev, int pos, int type)
{
u16 control;
@@ -695,10 +721,16 @@ int pci_enable_msi(struct pci_dev* dev)
{
int pos, temp, status = -EINVAL;
u16 control;
+ struct msi_quirk *quirk;
if (!pci_msi_enable || !dev)
return status;
+ list_for_each_entry(quirk, &msi_quirk_list, list) {
+ if (quirk->dev == dev)
+ return -EINVAL;
+ }
+
temp = dev->irq;
if ((status = msi_init()) < 0)
diff -uprN -X linux-2.6.13-rc4/Documentation/dontdiff
linux-2.6.13-rc4/drivers/pci/quirks.c
linux-2.6.13-rc4-pxhquirk/drivers/pci/quirks.c
--- linux-2.6.13-rc4/drivers/pci/quirks.c 2005-07-28 15:44:44.0
-0700
+++ linux-2.6.13-rc4-pxhquirk/drivers/pci/quirks.c 2005-08-05
11:54:15.0 -0700
@@ -21,6 +21,10 @@
#include
#include "pci.h"
+
+void disable_msi_mode(struct pci_dev *dev, int pos, int type);
+int msi_add_quirk(struct pci_dev *dev);
+
/* Deal with broken BIOS'es that neglect to enable passive release,
which can cause problems in combination with the 82441FX/PPro MTRRs */
static void __devinit quirk_passive_release(struct pci_dev *dev)
@@ -1267,6 +1271,30 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_IN
DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_E7320_MCH,
quirk_pcie_mch );
DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_E7525_MCH,
quirk_pcie_mch );
+
+/*
+ * It's possible for the MSI to get corrupted if shpc and acpi
+ * are used together on certain PXH-based systems.
+ */
+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);
+ if (!msi_add_quirk(dev))
+ printk(KERN_WARNING "PCI: PXH quirk detected, disabling MSI for
SHPC device\n");
+ else {
+ pci_msi_quirk = 1;
+ printk(KERN_WARNING "PCI: PXH quirk detected, unable to disable
MSI for SHPC device, disabling MSI for all devices\n");
+ }
+
+}
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_PXHD_0,
quirk_pcie_pxh);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_PXHD_1,
quirk_pcie_pxh);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_PXH_0,
quirk_pcie_pxh);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_PXH_1,
quirk_pcie_pxh);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_PXHV,
quirk_pcie_pxh);
+
+
static void __devinit quirk_netmos(struct pci_dev *dev)
{
unsigned int num_parallel = (dev->subsystem_device & 0xf0) >> 4;
diff -uprN -X linux-2.6.
Re: [PATCH] 6700/6702PXH quirk
On Fri, 2005-08-05 at 11:35 -0700, Greg KH wrote:
> On Fri, Aug 05, 2005 at 09:27:42AM -0700, Kristen Accardi wrote:
> > @@ -1127,3 +1159,5 @@ EXPORT_SYMBOL(pci_enable_msi);
> > EXPORT_SYMBOL(pci_disable_msi);
> > EXPORT_SYMBOL(pci_enable_msix);
> > EXPORT_SYMBOL(pci_disable_msix);
> > +EXPORT_SYMBOL(disable_msi_mode);
> > +EXPORT_SYMBOL(msi_add_quirk);
>
> Why do these need to be exported? It doesn't look like you are trying
> to access these from a module, or do you have a patch that uses them
> somewhere else?
>
> thanks,
>
> greg k-h
Oh... I thought I had to in order to access it from quirks.c. You are
right, I don't need this, and we can always export later if modules want
to add to the msi_quirks list.
Signed-off-by: Kristen Carlson Accardi <[EMAIL PROTECTED]>
diff -uprN -X linux-2.6.13-rc4/Documentation/dontdiff
linux-2.6.13-rc4/drivers/pci/msi.c linux-2.6.13-rc4-pxhquirk/drivers/pci/msi.c
--- linux-2.6.13-rc4/drivers/pci/msi.c 2005-07-28 15:44:44.0 -0700
+++ linux-2.6.13-rc4-pxhquirk/drivers/pci/msi.c 2005-08-05 11:38:00.0
-0700
@@ -38,6 +38,32 @@ int vector_irq[NR_VECTORS] = { [0 ... NR
u8 irq_vector[NR_IRQ_VECTORS] = { FIRST_DEVICE_VECTOR , 0 };
#endif
+
+LIST_HEAD(msi_quirk_list);
+
+struct msi_quirk
+{
+ struct list_head list;
+ struct pci_dev *dev;
+};
+
+
+int msi_add_quirk(struct pci_dev *dev)
+{
+ struct msi_quirk *quirk;
+
+ quirk = (struct msi_quirk *) kmalloc(sizeof(*quirk), GFP_KERNEL);
+ if (!quirk)
+ return -ENOMEM;
+
+ INIT_LIST_HEAD(&quirk->list);
+ quirk->dev = dev;
+ list_add(&quirk->list, &msi_quirk_list);
+ return 0;
+}
+
+
+
static void msi_cache_ctor(void *p, kmem_cache_t *cache, unsigned long flags)
{
memset(p, 0, NR_IRQS * sizeof(struct msi_desc));
@@ -453,7 +479,7 @@ static void enable_msi_mode(struct pci_d
}
}
-static void disable_msi_mode(struct pci_dev *dev, int pos, int type)
+void disable_msi_mode(struct pci_dev *dev, int pos, int type)
{
u16 control;
@@ -695,10 +721,16 @@ int pci_enable_msi(struct pci_dev* dev)
{
int pos, temp, status = -EINVAL;
u16 control;
+ struct msi_quirk *quirk;
if (!pci_msi_enable || !dev)
return status;
+ list_for_each_entry(quirk, &msi_quirk_list, list) {
+ if (quirk->dev == dev)
+ return -EINVAL;
+ }
+
temp = dev->irq;
if ((status = msi_init()) < 0)
diff -uprN -X linux-2.6.13-rc4/Documentation/dontdiff
linux-2.6.13-rc4/drivers/pci/quirks.c
linux-2.6.13-rc4-pxhquirk/drivers/pci/quirks.c
--- linux-2.6.13-rc4/drivers/pci/quirks.c 2005-07-28 15:44:44.0
-0700
+++ linux-2.6.13-rc4-pxhquirk/drivers/pci/quirks.c 2005-08-05
11:54:15.0 -0700
@@ -21,6 +21,10 @@
#include
#include "pci.h"
+
+void disable_msi_mode(struct pci_dev *dev, int pos, int type);
+int msi_add_quirk(struct pci_dev *dev);
+
/* Deal with broken BIOS'es that neglect to enable passive release,
which can cause problems in combination with the 82441FX/PPro MTRRs */
static void __devinit quirk_passive_release(struct pci_dev *dev)
@@ -1267,6 +1271,30 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_IN
DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_E7320_MCH,
quirk_pcie_mch );
DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_E7525_MCH,
quirk_pcie_mch );
+
+/*
+ * It's possible for the MSI to get corrupted if shpc and acpi
+ * are used together on certain PXH-based systems.
+ */
+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);
+ if (!msi_add_quirk(dev))
+ printk(KERN_WARNING "PCI: PXH quirk detected, disabling MSI for
SHPC device\n");
+ else {
+ pci_msi_quirk = 1;
+ printk(KERN_WARNING "PCI: PXH quirk detected, unable to disable
MSI for SHPC device, disabling MSI for all devices\n");
+ }
+
+}
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_PXHD_0,
quirk_pcie_pxh);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_PXHD_1,
quirk_pcie_pxh);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_PXH_0,
quirk_pcie_pxh);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_PXH_1,
quirk_pcie_pxh);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_PXHV,
quirk_pcie_pxh);
+
+
static void __devinit quirk_netmos(struct pci_dev *dev)
{
unsigned int num_parallel = (dev->subsystem_device & 0xf0) >> 4;
diff -uprN -X linux-2.6.13-rc4/Documentation/dontdiff
linux-2.6.13-rc4/include/linux/pci_ids.h
linux-2.6.13-rc4-pxhquirk/include/linux/pci_ids.h
--- linux-2.6.13-rc4/include/linux/pci_ids.h2005-07-28 15:44:44.0
-0700
Re: [PATCH] 6700/6702PXH quirk
On Fri, Aug 05, 2005 at 09:27:42AM -0700, Kristen Accardi wrote: > @@ -1127,3 +1159,5 @@ EXPORT_SYMBOL(pci_enable_msi); > EXPORT_SYMBOL(pci_disable_msi); > EXPORT_SYMBOL(pci_enable_msix); > EXPORT_SYMBOL(pci_disable_msix); > +EXPORT_SYMBOL(disable_msi_mode); > +EXPORT_SYMBOL(msi_add_quirk); Why do these need to be exported? It doesn't look like you are trying to access these from a module, or do you have a patch that uses them somewhere else? thanks, greg k-h - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] 6700/6702PXH quirk
On Friday 05 August 2005 10:27 am, 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. Can you outline the scenario that causes the corruption? This patch feels like a band-aid over a deeper problem. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] 6700/6702PXH quirk
On Fri, 2005-08-05 at 11:12 -0600, Bjorn Helgaas wrote: > On Friday 05 August 2005 10:27 am, 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. > > Can you outline the scenario that causes the corruption? This patch > feels like a band-aid over a deeper problem. This is a workaround to a hardware problem. If a MSI interrupt occurs while the ACPI driver is doing a config read of the hotplug capabilities register, the MSI interrupt will be corrupted, and the MSI interrupt will never make it to the MCH. This causes the hotplug device to not be recognized. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
