Re: [patch] PCI: modify SB700 SATA MSI quirk

Tejun Heo
2008-01-25T03:36:17Z

Shane Huang wrote:
> I did some modification to this patch and send it again, Please check
> it.
> The quirk to 0x4395 has been removed because 0x4395 only belongs to
> SB800.
>
>> SB700 SATA MSI bug will be fixed in SB700 revision A21 at
>> hardware level,
>> but the SB700 revision older than A21 will also be found in
>> the market.
>> This patch modify the original quirk commit
>> bc38b411fe696fad32b261f492cb4afbf1835256 instead of withdrawing it.

You need to merge the above two messages into one patch description.

>> Signed-off-by: Shane Huang <shane.huang@amd.com>

After S-O-B, you can put --- and between it and the patch body, you can
say things which you wanna mention but don't think should be included in
commit message.

> +static void __devinit quirk_msi_intx_disable_ati_bug(struct pci_dev
> *dev)
> +{
> + struct pci_dev *p;
> + u8 rev = 0;
> +
> + /* SB700 MSI issue will be fixed at HW level from revision A21,
> + * we need check PCI REVISION ID of SMBus controller to get
> SB700
> + * revision.
> + */
> + p = pci_get_device(PCI_VENDOR_ID_ATI,
> PCI_DEVICE_ID_ATI_SBX00_SMBUS,
> + NULL);
> + if (p) {
> + pci_read_config_byte(p, PCI_CLASS_REVISION, &rev);

Please drop unnecessary braces and you can you p->revision.

> + }
> + if ((rev < 0x3B) && (rev >= 0x30)) {
> + dev->dev_flags |= PCI_DEV_FLAGS_MSI_INTX_DISABLE_BUG;
> + }

Hmm... So, if there's no SMBUS device the quirk applies. Is this
intended? If that can't happen, just do if (!p) return;

Also, pci_get_device() requires matching pci_dev_put().

> +}
> DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_BROADCOM,
> PCI_DEVICE_ID_TIGON3_5780,
> quirk_msi_intx_disable_bug);
> @@ -1729,17 +1747,15 @@
> quirk_msi_intx_disable_bug);
>
> DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x4390,
> - quirk_msi_intx_disable_bug);
> + quirk_msi_intx_disable_ati_bug);
> DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x4391,
> - quirk_msi_intx_disable_bug);
> + quirk_msi_intx_disable_ati_bug);
> DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x4392,
> - quirk_msi_intx_disable_bug);
> + quirk_msi_intx_disable_ati_bug);
> DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x4393,
> - quirk_msi_intx_disable_bug);
> + quirk_msi_intx_disable_ati_bug);
> DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x4394,
> - quirk_msi_intx_disable_bug);
> -DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x4395,
> - quirk_msi_intx_disable_bug);
> + quirk_msi_intx_disable_ati_bug);

You tested this, right?

--
tejun