Re: [PATCH] 6700/6702PXH quirk

[Andrew Morton <akpm@osdl.org>]

Kristen Accardi <kristen.c.accardi@intel.com> 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.c	2005-07-28 15:44:44.000000000 -0700
> +++ linux-2.6.13-rc4-pxhquirk/drivers/pci/msi.c	2005-08-05 11:38:00.000000000 -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.000000000 -0700
> +++ linux-2.6.13-rc4-pxhquirk/drivers/pci/quirks.c	2005-08-05 11:54:15.000000000 -0700
> @@ -21,6 +21,10 @@
>  #include <linux/acpi.h>
>  #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");