diff mbox

PCI: add MSI INTX_DISABLE quirks for AR8161/AR8162/AR8171/AR8172

Message ID 1362549547-2160-1-git-send-email-xiong@qca.qualcomm.com (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show

Commit Message

Huang, Xiong March 6, 2013, 5:59 a.m. UTC 
From: "Huang,Xiong" <xiong@qca.qualcomm.com>

PCI devices of AR8161/AR8162/AR8171/AR8172 which revision lower than
0x18 have this bug.

Signed-off-by: Huang,Xiong <xiong@qca.qualcomm.com>
---
 drivers/pci/quirks.c    | 35 +++++++++++++++++++++++++++++++++++
 include/linux/pci_ids.h |  4 ++++
 2 files changed, 39 insertions(+)

Comments

Huang, Xiong March 7, 2013, 3:57 p.m. UTC | #1 
> >+static void quirk_msi_intx_disable_qca_bug(struct pci_dev *dev) {
> >+	static u16 qca_eth_devid[] = {
> >+			PCI_DEVICE_ID_AR8161,
> >+			PCI_DEVICE_ID_AR8162,
> >+			PCI_DEVICE_ID_AR8171,
> >+			PCI_DEVICE_ID_AR8172};
> >+	struct pci_dev *p;
> >+	int i;
> >+
> >+	/* AR816X/AR817X MSI is fixed at HW level from revision 0x18 */
> >+	for (i = 0; i < ARRAY_SIZE(qca_eth_devid); i++) {
> >+		p = pci_get_device(PCI_VENDOR_ID_ATTANSIC,
> >+				   qca_eth_devid[i],
> 
> xiong,
> 
> The "FINAL" fixup is called just in pci_apply_final_quirks(), if I am correct.
> 
> In this function, it will go through all the pci devices which are registered in
> the system. And try to invoke the fixup hook.
> 
> Also, before invode the hook, in function pci_do_fixups() it will make sure
> the hook just apply to the devices whose VendorID and DeviceID match what
> you defined in DECLARE_PCI_FIXUP_FINAL.
> 
> So I think there is no need to iterate on all those pci device, before you want
> to change the pci_dev->dev_flags.
> 
> >+				   NULL);
> >+		if (!p)
> >+			return;
> >+
> >+		if (p->revision < 0x18)
> >+			dev->dev_flags |=
> PCI_DEV_FLAGS_MSI_INTX_DISABLE_BUG;
> >+		pci_dev_put(p);
> >+	}

Wei, thanks for you feedback, this patch is just follow the function 'quirk_msi_intx_disable_ati_bug' in quirks.c
Is it ok if I revise code as:
	static void quirk_msi_intx_disable_qca_bug(struct pci_dev *dev)
	{
		If (dev->revision < 0x18)
			dev->dev_flags |=  PCI_DEV_FLAGS_MSI_INTX_DISABLE_BUG;
	}

Please advise, thanks !

-Xiong
		

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Helgaas March 7, 2013, 5:43 p.m. UTC | #2 
On Thu, Mar 7, 2013 at 8:57 AM, Huang, Xiong <xiong@qca.qualcomm.com> wrote:
>> >+static void quirk_msi_intx_disable_qca_bug(struct pci_dev *dev) {
>> >+    static u16 qca_eth_devid[] = {
>> >+                    PCI_DEVICE_ID_AR8161,
>> >+                    PCI_DEVICE_ID_AR8162,
>> >+                    PCI_DEVICE_ID_AR8171,
>> >+                    PCI_DEVICE_ID_AR8172};
>> >+    struct pci_dev *p;
>> >+    int i;
>> >+
>> >+    /* AR816X/AR817X MSI is fixed at HW level from revision 0x18 */
>> >+    for (i = 0; i < ARRAY_SIZE(qca_eth_devid); i++) {
>> >+            p = pci_get_device(PCI_VENDOR_ID_ATTANSIC,
>> >+                               qca_eth_devid[i],
>>
>> xiong,
>>
>> The "FINAL" fixup is called just in pci_apply_final_quirks(), if I am correct.
>>
>> In this function, it will go through all the pci devices which are registered in
>> the system. And try to invoke the fixup hook.
>>
>> Also, before invode the hook, in function pci_do_fixups() it will make sure
>> the hook just apply to the devices whose VendorID and DeviceID match what
>> you defined in DECLARE_PCI_FIXUP_FINAL.
>>
>> So I think there is no need to iterate on all those pci device, before you want
>> to change the pci_dev->dev_flags.
>>
>> >+                               NULL);
>> >+            if (!p)
>> >+                    return;
>> >+
>> >+            if (p->revision < 0x18)
>> >+                    dev->dev_flags |=
>> PCI_DEV_FLAGS_MSI_INTX_DISABLE_BUG;
>> >+            pci_dev_put(p);
>> >+    }
>
> Wei, thanks for you feedback, this patch is just follow the function 'quirk_msi_intx_disable_ati_bug' in quirks.c
> Is it ok if I revise code as:
>         static void quirk_msi_intx_disable_qca_bug(struct pci_dev *dev)
>         {
>                 If (dev->revision < 0x18)
>                         dev->dev_flags |=  PCI_DEV_FLAGS_MSI_INTX_DISABLE_BUG;
>         }

Per the comment in pci_ids.h, you should not add #defines there unless
they are used in more than one place.  So in this case, you would just
use the hex constants directly in quirks.c, as we already do for the
existing ATTANSIC quirks.

As far as the loop with pci_get_device(), the revised patch is
probably what you want, but it is not functionally equivalent to the
original.  Let's say you have two devices:

  1) 8161 rev 0x18
  2) 8171 rev 0x17

The original patch will disable MSI for the 8161 because there is
*another* device (the 8171) with rev < 0x18.  I doubt that's what you
want.

I'd like to see a dev_info() saying that you're disabling MSI for this
device so that when somebody complains "my device should be using MSI
but isn't," it's easy to figure out why not.

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Huang, Xiong March 7, 2013, 6:08 p.m. UTC | #3 
> > Wei, thanks for you feedback, this patch is just follow the function
> > 'quirk_msi_intx_disable_ati_bug' in quirks.c Is it ok if I revise code as:
> >         static void quirk_msi_intx_disable_qca_bug(struct pci_dev *dev)
> >         {
> >                 If (dev->revision < 0x18)
> >                         dev->dev_flags |=  PCI_DEV_FLAGS_MSI_INTX_DISABLE_BUG;
> >         }
> 
> Per the comment in pci_ids.h, you should not add #defines there unless they
> are used in more than one place.  So in this case, you would just use the hex
> constants directly in quirks.c, as we already do for the existing ATTANSIC
> quirks.
> 
> As far as the loop with pci_get_device(), the revised patch is probably what
> you want, but it is not functionally equivalent to the original.  Let's say you
> have two devices:
> 
>   1) 8161 rev 0x18
>   2) 8171 rev 0x17
> 
> The original patch will disable MSI for the 8161 because there is
> *another* device (the 8171) with rev < 0x18.  I doubt that's what you want.
> 
> I'd like to see a dev_info() saying that you're disabling MSI for this device so
> that when somebody complains "my device should be using MSI but isn't,"
> it's easy to figure out why not.
> 

Bjorn, your example is very clear for me. I made a bug for the original patch :)

BTW. The flag of PCI_DEV_FLAGS_MSI_INTX_DISABLE_BUG isn't disable MSI on such devices,
But not enable PCI INT_DISABLE bit for PCI_COMMAND register when enable MSI function.
Some devices like AR8161 with revision lower than 0x18 will not issue MSI interrupt if INT_DISABLE bit is set.

Thanks
Xiong
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Huang, Xiong March 9, 2013, 4:44 a.m. UTC | #4 
> >Wei, thanks for you feedback, this patch is just follow the function
> >'quirk_msi_intx_disable_ati_bug' in quirks.c Is it ok if I revise code as:
> >	static void quirk_msi_intx_disable_qca_bug(struct pci_dev *dev)
> >	{
> >		If (dev->revision < 0x18)
> >			dev->dev_flags |=
> PCI_DEV_FLAGS_MSI_INTX_DISABLE_BUG;
> >	}
> 
> I think it is ok. Have you tested it?
> 
> If your test passed, I think you can send out another version.
> 

Wei, 
     I have tested it. It's ok.  thanks for your help.
     I sent out another version patch yesterday.

-Xiong
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 0369fb6..bf31d72 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -2594,6 +2594,29 @@  static void quirk_msi_intx_disable_ati_bug(struct pci_dev *dev)
 		dev->dev_flags |= PCI_DEV_FLAGS_MSI_INTX_DISABLE_BUG;
 	pci_dev_put(p);
 }
+static void quirk_msi_intx_disable_qca_bug(struct pci_dev *dev)
+{
+	static u16 qca_eth_devid[] = {
+			PCI_DEVICE_ID_AR8161,
+			PCI_DEVICE_ID_AR8162,
+			PCI_DEVICE_ID_AR8171,
+			PCI_DEVICE_ID_AR8172};
+	struct pci_dev *p;
+	int i;
+
+	/* AR816X/AR817X MSI is fixed at HW level from revision 0x18 */
+	for (i = 0; i < ARRAY_SIZE(qca_eth_devid); i++) {
+		p = pci_get_device(PCI_VENDOR_ID_ATTANSIC,
+				   qca_eth_devid[i],
+				   NULL);
+		if (!p)
+			return;
+
+		if (p->revision < 0x18)
+			dev->dev_flags |= PCI_DEV_FLAGS_MSI_INTX_DISABLE_BUG;
+		pci_dev_put(p);
+	}
+}
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_BROADCOM,
 			PCI_DEVICE_ID_TIGON3_5780,
 			quirk_msi_intx_disable_bug);
@@ -2643,6 +2666,18 @@  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATTANSIC, 0x1073,
 			quirk_msi_intx_disable_bug);
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATTANSIC, 0x1083,
 			quirk_msi_intx_disable_bug);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATTANSIC,
+			PCI_DEVICE_ID_AR8161,
+			quirk_msi_intx_disable_qca_bug);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATTANSIC,
+			PCI_DEVICE_ID_AR8162,
+			quirk_msi_intx_disable_qca_bug);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATTANSIC,
+			PCI_DEVICE_ID_AR8171,
+			quirk_msi_intx_disable_qca_bug);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATTANSIC,
+			PCI_DEVICE_ID_AR8172,
+			quirk_msi_intx_disable_qca_bug);
 #endif /* CONFIG_PCI_MSI */
 
 /* Allow manual resource allocation for PCI hotplug bridges
diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index f11c1c2..7e171fb 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -2438,6 +2438,10 @@ 
 #define PCI_VENDOR_ID_ATTANSIC		0x1969
 #define PCI_DEVICE_ID_ATTANSIC_L1	0x1048
 #define PCI_DEVICE_ID_ATTANSIC_L2	0x2048
+#define PCI_DEVICE_ID_AR8161		0x1091
+#define PCI_DEVICE_ID_AR8162		0x1090
+#define PCI_DEVICE_ID_AR8171		0x10A1
+#define PCI_DEVICE_ID_AR8172		0x10A0
 
 #define PCI_VENDOR_ID_JMICRON		0x197B
 #define PCI_DEVICE_ID_JMICRON_JMB360	0x2360