[PATCH][BNX2]: Disable MSI on 5706 if AMD 8132 bridge is present

[PATCH][BNX2]: Disable MSI on 5706 if AMD 8132 bridge is present

[Michael Chan]

[BNX2]: Disable MSI on 5706 if AMD 8132 bridge is present.

MSI is defined to be 32-bit write.  The 5706 does 64-bit MSI writes
with byte enables disabled on the unused 32-bit word.  This is legal
but causes problems on the AMD 8132 which will eventually stop
responding after a while.

Without this patch, the MSI test done by the driver during open will
pass, but MSI will eventually stop working after a few MSIs are
written by the device.

AMD believes this incompatibility is unique to the 5706, and
prefers to locally disable MSI rather than globally disabling it
using pci_msi_quirk.

Update version to 1.4.45.

Signed-off-by: Michael Chan <mchan@broadcom.com>

diff --git a/drivers/net/bnx2.c b/drivers/net/bnx2.c
index 7fcf015..a65a697 100644
--- a/drivers/net/bnx2.c
+++ b/drivers/net/bnx2.c
@@ -56,8 +56,8 @@
 
 #define DRV_MODULE_NAME		"bnx2"
 #define PFX DRV_MODULE_NAME	": "
-#define DRV_MODULE_VERSION	"1.4.44"
-#define DRV_MODULE_RELDATE	"August 10, 2006"
+#define DRV_MODULE_VERSION	"1.4.45"
+#define DRV_MODULE_RELDATE	"September 29, 2006"
 
 #define RUN_AT(x) (jiffies + (x))
 
@@ -5805,6 +5805,34 @@ bnx2_init_board(struct pci_dev *pdev, st
 		bp->cmd_ticks_int = bp->cmd_ticks;
 	}
 
+	/* Disable MSI on 5706 if AMD 8132 bridge is found.
+	 *
+	 * MSI is defined to be 32-bit write.  The 5706 does 64-bit MSI writes
+	 * with byte enables disabled on the unused 32-bit word.  This is legal
+	 * but causes problems on the AMD 8132 which will eventually stop
+	 * responding after a while.
+	 *
+	 * AMD believes this incompatibility is unique to the 5706, and
+	 * prefers to locally disable MSI rather than globally disabling it
+	 * using pci_msi_quirk.
+	 */
+	if (CHIP_NUM(bp) == CHIP_NUM_5706 && disable_msi == 0) {
+		struct pci_dev *amd_8132 = NULL;
+
+		while ((amd_8132 = pci_get_device(PCI_VENDOR_ID_AMD,
+						  PCI_DEVICE_ID_AMD_8132_BRIDGE,
+						  amd_8132))) {
+			u8 rev;
+
+			pci_read_config_byte(amd_8132, PCI_REVISION_ID, &rev);
+			if (rev >= 0x10 && rev <= 0x13) {
+				disable_msi = 1;
+				pci_dev_put(amd_8132);
+				break;
+			}
+		}
+	}
+
 	bp->autoneg = AUTONEG_SPEED | AUTONEG_FLOW_CTRL;
 	bp->req_line_speed = 0;
 	if (bp->phy_flags & PHY_SERDES_FLAG) {
diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index 2cead40..59e52cb 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -506,6 +506,7 @@
 #define PCI_DEVICE_ID_AMD_8151_0	0x7454
 #define PCI_DEVICE_ID_AMD_8131_BRIDGE	0x7450
 #define PCI_DEVICE_ID_AMD_8131_APIC	0x7451
+#define PCI_DEVICE_ID_AMD_8132_BRIDGE	0x7458
 #define PCI_DEVICE_ID_AMD_CS5536_ISA    0x2090
 #define PCI_DEVICE_ID_AMD_CS5536_FLASH  0x2091
 #define PCI_DEVICE_ID_AMD_CS5536_AUDIO  0x2093

Re: [PATCH][BNX2]: Disable MSI on 5706 if AMD 8132 bridge is present

[Jeff Garzik]

Michael Chan wrote:
> AMD believes this incompatibility is unique to the 5706, and
> prefers to locally disable MSI rather than globally disabling it
> using pci_msi_quirk.

Why is it unique to the 5706?  Is this just a guess on AMD and 
Broadcom's part?

	Jeff

Re: [PATCH][BNX2]: Disable MSI on 5706 if AMD 8132 bridge is present

[Michael Chan]

On Fri, 2006-09-29 at 17:28 -0400, Jeff Garzik wrote:
> Michael Chan wrote:
> > AMD believes this incompatibility is unique to the 5706, and
> > prefers to locally disable MSI rather than globally disabling it
> > using pci_msi_quirk.
> 
> Why is it unique to the 5706?  Is this just a guess on AMD and 
> Broadcom's part?
> 
I just took AMD's word for it.  It doesn't matter to me whether we
disable it locally or globally.  Since this is AMD's bridge, I just
follow their recommendation.  They probably haven't seen this issue on
any other devices except ours.

Re: [PATCH][BNX2]: Disable MSI on 5706 if AMD 8132 bridge is present

[David Miller]

From: "Michael Chan" <mchan@broadcom.com>
Date: Fri, 29 Sep 2006 14:39:23 -0700

> On Fri, 2006-09-29 at 17:28 -0400, Jeff Garzik wrote:
> > Michael Chan wrote:
> > > AMD believes this incompatibility is unique to the 5706, and
> > > prefers to locally disable MSI rather than globally disabling it
> > > using pci_msi_quirk.
> > 
> > Why is it unique to the 5706?  Is this just a guess on AMD and 
> > Broadcom's part?
> > 
> I just took AMD's word for it.  It doesn't matter to me whether we
> disable it locally or globally.  Since this is AMD's bridge, I just
> follow their recommendation.  They probably haven't seen this issue on
> any other devices except ours.

I really think this is a reasonable thing to do.

It's absolutely rediculious to disable MSI for every card just
because one decided to use a masked 64-bit transaction for
what's supposed to be a 32-bit one.

Jeff, I totally understand your knee-jerk reaction to per-device MSI
validation checks, but in this case I find that knee-jerk reaction
to be totally unreasonable. :-)

Re: [PATCH][BNX2]: Disable MSI on 5706 if AMD 8132 bridge is present

[Jeff Garzik]

David Miller wrote:
> From: "Michael Chan" <mchan@broadcom.com>
> Date: Fri, 29 Sep 2006 14:39:23 -0700
> 
>> On Fri, 2006-09-29 at 17:28 -0400, Jeff Garzik wrote:
>>> Michael Chan wrote:
>>>> AMD believes this incompatibility is unique to the 5706, and
>>>> prefers to locally disable MSI rather than globally disabling it
>>>> using pci_msi_quirk.
>>> Why is it unique to the 5706?  Is this just a guess on AMD and 
>>> Broadcom's part?
>>>
>> I just took AMD's word for it.  It doesn't matter to me whether we
>> disable it locally or globally.  Since this is AMD's bridge, I just
>> follow their recommendation.  They probably haven't seen this issue on
>> any other devices except ours.
> 
> I really think this is a reasonable thing to do.
> 
> It's absolutely rediculious to disable MSI for every card just
> because one decided to use a masked 64-bit transaction for
> what's supposed to be a 32-bit one.
> 
> Jeff, I totally understand your knee-jerk reaction to per-device MSI
> validation checks, but in this case I find that knee-jerk reaction
> to be totally unreasonable. :-)

How it is unreasonable to clarify a completely vague description of the 
problem?

The patch and description provided no information about whether or not 
it would be better to blacklist 8132 globally, as we have already done 
with the 8131.

	Jeff

Re: [PATCH][BNX2]: Disable MSI on 5706 if AMD 8132 bridge is present

[David Miller]

From: Jeff Garzik <jeff@garzik.org>
Date: Fri, 29 Sep 2006 19:00:15 -0400

> The patch and description provided no information about whether or not 
> it would be better to blacklist 8132 globally, as we have already done 
> with the 8131.

It absolutely was not vague, it gave an explicit description of what
the problem was, down to the transaction type being used by 5706 and
what the stated rules are in the PCI spec, and it also gave a clear
indication that the 5706 was in the wrong and that this was believed
to be a unique situation.

Re: [PATCH][BNX2]: Disable MSI on 5706 if AMD 8132 bridge is present

[Jeff Garzik]

David Miller wrote:
> From: Jeff Garzik <jeff@garzik.org>
> Date: Fri, 29 Sep 2006 19:00:15 -0400
> 
>> The patch and description provided no information about whether or not 
>> it would be better to blacklist 8132 globally, as we have already done 
>> with the 8131.
> 
> It absolutely was not vague, it gave an explicit description of what
> the problem was, down to the transaction type being used by 5706 and
> what the stated rules are in the PCI spec, and it also gave a clear
> indication that the 5706 was in the wrong and that this was believed
> to be a unique situation.

It was completely vague as to why this incompatibility was specific to 
the 5706, when -- as the description noted -- the behavior is legal.

Re-read the patch.  At no time does it say 5706 was in the wrong.

	Jeff

Re: [PATCH][BNX2]: Disable MSI on 5706 if AMD 8132 bridge is present

[Michael Chan]

On Fri, 2006-09-29 at 19:15 -0400, Jeff Garzik wrote:

> It was completely vague as to why this incompatibility was specific to 
> the 5706, when -- as the description noted -- the behavior is legal.
> 

The description is a bit vague in that one aspect that Jeff pointed out,
but otherwise very complete in my opinion.

> Re-read the patch.  At no time does it say 5706 was in the wrong.
> 

The behavior is legal but AMD believes that it is unique to the 5706.  I
cannot make this less vague since it is a recommendation from AMD.  I
just trust their opinion on this.

Re: [PATCH][BNX2]: Disable MSI on 5706 if AMD 8132 bridge is present

[David Miller]

From: Jeff Garzik <jeff@garzik.org>
Date: Fri, 29 Sep 2006 19:15:14 -0400

> It was completely vague as to why this incompatibility was specific to 
> the 5706, when -- as the description noted -- the behavior is legal.
> 
> Re-read the patch.  At no time does it say 5706 was in the wrong.

True, but it does indicate that using a masked 64-bit transaction
for MSI instead of a true 32-bit one is considered to be quite rare.

Do you wish to put a table of all devices that do this, and at PCI
quirk time disable PCI for everyone on the AMD chipset if even one
such device is found in the device?

That doesn't make any sense to me.

Re: [PATCH][BNX2]: Disable MSI on 5706 if AMD 8132 bridge is present

[Rick Jones]

> It absolutely was not vague, it gave an explicit description of what
> the problem was, down to the transaction type being used by 5706 and
> what the stated rules are in the PCI spec, and it also gave a clear
> indication that the 5706 was in the wrong and that this was believed
> to be a unique situation.

I'm not disagreeing with a per-driver check at the moment, but I thought that 
Michael told us that the masking being attempted by the 5706 was legal:

Michael Chan wrote:
> MSI is defined to be 32-bit write.  The 5706 does 64-bit MSI writes
> with byte enables disabled on the unused 32-bit word.  This is legal
> but causes problems on the AMD 8132 which will eventually stop
> responding after a while.
>
 > ...
> MSI is defined to be 32-bit write.  The 5706 does 64-bit MSI writes
> with byte enables disabled on the unused 32-bit word.  This is legal
> but causes problems on the AMD 8132 which will eventually stop
> responding after a while.
> 

rick jones

Re: [PATCH][BNX2]: Disable MSI on 5706 if AMD 8132 bridge is present

[David Miller]

From: "Michael Chan" <mchan@broadcom.com>
Date: Fri, 29 Sep 2006 14:07:32 -0700

> [BNX2]: Disable MSI on 5706 if AMD 8132 bridge is present.

Applied thanks.

We can back this out if we come up with a sceme to do this
at a generic level, or we find other faults in this AMD
chipset such that added together it warrants a full MSI
disable.

Re: [PATCH][BNX2]: Disable MSI on 5706 if AMD 8132 bridge is present

[Roland Dreier]

    Jeff> The patch and description provided no information about
    Jeff> whether or not it would be better to blacklist 8132
    Jeff> globally, as we have already done with the 8131.

8131 and 8132 are quite different: AMD 8131 simply did not implement
MSI at all, so any attempt to use MSI by a device below such a bridge
has no chance at working.  8132 at least tried to implement MSI and
I know at least some devices work with it.

It may be that the 8132 MSI implementation is too buggy to be used in
practice, but that doesn't seem to be AMD's opinion.

 - R.

Re: [PATCH][BNX2]: Disable MSI on 5706 if AMD 8132 bridge is present

[Jeff Garzik]

David Miller wrote:
> From: Jeff Garzik <jeff@garzik.org>
> Date: Fri, 29 Sep 2006 19:15:14 -0400
> 
>> It was completely vague as to why this incompatibility was specific to 
>> the 5706, when -- as the description noted -- the behavior is legal.
>>
>> Re-read the patch.  At no time does it say 5706 was in the wrong.
> 
> True, but it does indicate that using a masked 64-bit transaction
> for MSI instead of a true 32-bit one is considered to be quite rare.
> 
> Do you wish to put a table of all devices that do this, and at PCI
> quirk time disable PCI for everyone on the AMD chipset if even one
> such device is found in the device?
> 
> That doesn't make any sense to me.

David, rejoin reality.  You are either arguing with a fictionalized Jeff 
Garzik in your head, or constructing a classical strawman.

Let me say it for the cheap seats:  AT NO TIME DID I PROPOSE ACTION OF 
ANY KIND.  I sought clarification.  Information.

So, please, quit making stupid and incorrect assumptions about my 
intentions.  If this is indeed a rare behavior and most other MSI cases 
work with the 8132, then the patch is quite reasonable.  I see no reason 
to NAK the patch.

	Jeff

Re: [PATCH][BNX2]: Disable MSI on 5706 if AMD 8132 bridge is present

[David Miller]

From: Jeff Garzik <jeff@garzik.org>
Date: Fri, 29 Sep 2006 20:38:07 -0400

> David, rejoin reality.  You are either arguing with a fictionalized Jeff 
> Garzik in your head, or constructing a classical strawman.
> 
> Let me say it for the cheap seats:  AT NO TIME DID I PROPOSE ACTION OF 
> ANY KIND.  I sought clarification.  Information.

Ok, my bad, and my apologies.

> So, please, quit making stupid and incorrect assumptions about my 
> intentions.  If this is indeed a rare behavior and most other MSI cases 
> work with the 8132, then the patch is quite reasonable.  I see no reason 
> to NAK the patch.

Awesome.

Re: [PATCH][BNX2]: Disable MSI on 5706 if AMD 8132 bridge is present

[Brice Goglin]

Michael Chan a écrit :
> AMD believes this incompatibility is unique to the 5706, and
> prefers to locally disable MSI rather than globally disabling it
> using pci_msi_quirk.
>   

FYI, pci_msi_quirk is the extreme solution, there is something in the
middle :) It is possible to disable MSI for only devices that are behind
this bridge (by setting the NO_MSI flag in its bus flag). We just merged
a couple patchs related to this NO_MSI flag and pci_msi_quirk (there are
very very few cases where MSI must be disabled globally, most of the
time a subset behind a bridge is enough) so I am very glad that you
didn't use it :)
Anyway, disabling locally is better here.

> +	if (CHIP_NUM(bp) == CHIP_NUM_5706 && disable_msi == 0) {
> +		struct pci_dev *amd_8132 = NULL;
> +
> +		while ((amd_8132 = pci_get_device(PCI_VENDOR_ID_AMD,
> +						  PCI_DEVICE_ID_AMD_8132_BRIDGE,
> +						  amd_8132))) {
>   

What if the machine has such a bridge and board, but the board is not
actually located somewhere behind the bridge? I would rather walk the
PCI hierarchy from the board to the top and check whether we find a
AMD8132. Probably something like:

	struct pci_dev * bridge = <the bnx2 pci_dev>;
	while (bridge->bus && bridge->bus->self)
		bridge = bridge->bus->self;
		if (bridge->vendor == PCI_VENDOR_ID_AMD
		    && bridge->device == PCI_VENDOR_ID_AMD_8132_BRIDGE)
			<do your stuff to disable MSI on your board>


Brice

Re: [PATCH][BNX2]: Disable MSI on 5706 if AMD 8132 bridge is present

[Michael Chan]

On Sat, 2006-09-30 at 12:13 +0200, Brice Goglin wrote:

> What if the machine has such a bridge and board, but the board is not
> actually located somewhere behind the bridge? I would rather walk the
> PCI hierarchy from the board to the top and check whether we find a
> AMD8132. Probably something like:
> 

I have considered that.  I have PCI walking code like in tg3 to
determine if certain tg3 devices are behind some ICH or ServerWorks EPB
bridges.  The workaround needed in those cases have big impact on
performance and therefore it is important to determine exactly when to
apply those workarounds.

Here in this case, since the difference in performance between MSI and
INTA is very minor and almost negligible in a lot of cases, I decided to
keep it simple and just check for the presence of the 8132.