[PATCH] e1000e: test MSI interrupts

[PATCH] e1000e: test MSI interrupts

[Auke Kok]

From: Jesse Brandeburg <jesse.brandeburg@intel.com>

Test the MSI interrupt physically once before assuming that it
actually works. Several platforms have already come across that
have non-functional MSI interrupts and this code will attempt
to detect those safely. Once the test succeeds MSI interrupts
will be enabled.

Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
Signed-off-by: Auke Kok <auke-jan.h.kok@intel.com>
---

 drivers/net/e1000e/defines.h |    1 
 drivers/net/e1000e/e1000.h   |    1 
 drivers/net/e1000e/netdev.c  |  161 ++++++++++++++++++++++++++++++++++++++++--
 3 files changed, 155 insertions(+), 8 deletions(-)

diff --git a/drivers/net/e1000e/defines.h b/drivers/net/e1000e/defines.h
index a4f511f..c3593f3 100644
--- a/drivers/net/e1000e/defines.h
+++ b/drivers/net/e1000e/defines.h
@@ -385,6 +385,7 @@
 
 /* Interrupt Cause Set */
 #define E1000_ICS_LSC       E1000_ICR_LSC       /* Link Status Change */
+#define E1000_ICS_RXSEQ     E1000_ICR_RXSEQ     /* rx sequence error */
 #define E1000_ICS_RXDMT0    E1000_ICR_RXDMT0    /* rx desc min. threshold */
 
 /* Transmit Descriptor Control */
diff --git a/drivers/net/e1000e/e1000.h b/drivers/net/e1000e/e1000.h
index 4bf0c6c..556b258 100644
--- a/drivers/net/e1000e/e1000.h
+++ b/drivers/net/e1000e/e1000.h
@@ -305,6 +305,7 @@ struct e1000_info {
 #define FLAG_MSI_ENABLED                  (1 << 27)
 #define FLAG_RX_CSUM_ENABLED              (1 << 28)
 #define FLAG_TSO_FORCE                    (1 << 29)
+#define FLAG_MSI_TEST_FAILED              (1 << 30)
 
 #define E1000_RX_DESC_PS(R, i)	    \
 	(&(((union e1000_rx_desc_packet_split *)((R).desc))[i]))
diff --git a/drivers/net/e1000e/netdev.c b/drivers/net/e1000e/netdev.c
index f501dd5..41d57da 100644
--- a/drivers/net/e1000e/netdev.c
+++ b/drivers/net/e1000e/netdev.c
@@ -932,25 +932,30 @@ static irqreturn_t e1000_intr(int irq, void *data)
 static int e1000_request_irq(struct e1000_adapter *adapter)
 {
 	struct net_device *netdev = adapter->netdev;
-	irq_handler_t handler = e1000_intr;
 	int irq_flags = IRQF_SHARED;
 	int err;
 
-	if (!pci_enable_msi(adapter->pdev)) {
-		adapter->flags |= FLAG_MSI_ENABLED;
-		handler = e1000_intr_msi;
-		irq_flags = 0;
+	if (!(adapter->flags & FLAG_MSI_TEST_FAILED)) {
+		err = pci_enable_msi(adapter->pdev);
+		if (!err) {
+			adapter->flags |= FLAG_MSI_ENABLED;
+			irq_flags = 0;
+		}
 	}
 
-	err = request_irq(adapter->pdev->irq, handler, irq_flags, netdev->name,
-			  netdev);
+	err = request_irq(adapter->pdev->irq,
+			  (adapter->flags & FLAG_MSI_ENABLED) ?
+			  &e1000_intr_msi : &e1000_intr, irq_flags,
+			  netdev->name, netdev);
 	if (err) {
 		ndev_err(netdev,
 		       "Unable to allocate %s interrupt (return: %d)\n",
 			adapter->flags & FLAG_MSI_ENABLED ? "MSI":"INTx",
 			err);
-		if (adapter->flags & FLAG_MSI_ENABLED)
+		if (adapter->flags & FLAG_MSI_ENABLED) {
 			pci_disable_msi(adapter->pdev);
+			adapter->flags &= ~FLAG_MSI_ENABLED;
+		}
 	}
 
 	return err;
@@ -2234,6 +2239,135 @@ err:
 }
 
 /**
+ * e1000_intr_msi_test - Interrupt Handler
+ * @irq: interrupt number
+ * @data: pointer to a network interface device structure
+ **/
+static irqreturn_t e1000_intr_msi_test(int irq, void *data)
+{
+	struct net_device *netdev = data;
+	struct e1000_adapter *adapter = netdev_priv(netdev);
+	struct e1000_hw *hw = &adapter->hw;
+
+	u32 icr = er32(ICR);
+	ndev_dbg(netdev, "icr is %08X\n", icr);
+	if (icr & E1000_ICR_RXSEQ) {
+		adapter->flags &= ~FLAG_MSI_TEST_FAILED;
+		wmb();
+	}
+
+	return IRQ_HANDLED;
+}
+
+/**
+ * e1000_test_msi_interrupt - Returns 0 for successful test
+ * @adapter: board private struct
+ *
+ * code flow taken from tg3.c
+ **/
+static int e1000_test_msi_interrupt(struct e1000_adapter *adapter)
+{
+	struct net_device *netdev = adapter->netdev;
+	struct e1000_hw *hw = &adapter->hw;
+	int err;
+
+	/* poll_enable hasn't been called yet, so don't need disable */
+	/* clear any pending events */
+	er32(ICR);
+
+	/* free the real vector and request a test handler */
+	e1000_free_irq(adapter);
+
+	err = pci_enable_msi(adapter->pdev);
+	if (err)
+		goto msi_test_failed;
+
+	err = request_irq(adapter->pdev->irq, &e1000_intr_msi_test, 0,
+			  netdev->name, netdev);
+	if (err) {
+		pci_disable_msi(adapter->pdev);
+		goto msi_test_failed;
+	}
+
+	/* Assume that the test fails, if it succeeds then the test
+	 * MSI irq handler will unset this flag */
+	adapter->flags |= FLAG_MSI_TEST_FAILED;
+	wmb();
+
+	e1000_irq_enable(adapter);
+
+	/* fire an unusual interrupt on the test handler */
+	ew32(ICS, E1000_ICS_RXSEQ);
+	e1e_flush();
+	msleep(50);
+
+	e1000_irq_disable(adapter);
+
+	rmb();
+
+	if (adapter->flags & FLAG_MSI_TEST_FAILED) {
+		err = -EIO;
+		ndev_dbg(netdev, "MSI interrupt test failed!\n");
+	}
+
+	free_irq(adapter->pdev->irq, netdev);
+	pci_disable_msi(adapter->pdev);
+
+	if (err == -EIO)
+		goto msi_test_failed;
+
+	/* okay so the test worked, restore settings */
+	ndev_dbg(netdev, "MSI interrupt test succeeded!\n");
+msi_test_failed:
+	/* restore the original vector, even if it failed */
+	e1000_request_irq(adapter);
+	return err;
+}
+
+/**
+ * e1000_test_msi - Returns 0 if MSI test succeeds or INTx mode is restored
+ * @adapter: board private struct
+ *
+ * code flow taken from tg3.c, called with e1000 interrupts disabled.
+ **/
+static int e1000_test_msi(struct e1000_adapter *adapter)
+{
+	int err;
+	u16 pci_cmd;
+
+	if (!(adapter->flags & FLAG_MSI_ENABLED))
+		return 0;
+
+	/* disable SERR in case the MSI write causes a master abort */
+	pci_read_config_word(adapter->pdev, PCI_COMMAND, &pci_cmd);
+	pci_write_config_word(adapter->pdev, PCI_COMMAND,
+			      pci_cmd & ~PCI_COMMAND_SERR);
+
+	err = e1000_test_msi_interrupt(adapter);
+
+	/* restore previous setting of command word */
+	pci_write_config_word(adapter->pdev, PCI_COMMAND, pci_cmd);
+
+	/* success ! */
+	if (!err)
+		return 0;
+
+	/* EIO means MSI test failed */
+	if (err != -EIO)
+		return err;
+
+	/* back to INTx mode */
+	ndev_warn(adapter->netdev, "MSI interrupt test failed, using legacy "
+		  "interrupt.\n");
+
+	e1000_free_irq(adapter);
+
+	err = e1000_request_irq(adapter);
+
+	return err;
+}
+
+/**
  * e1000_open - Called when a network interface is made active
  * @netdev: network interface device structure
  *
@@ -2288,6 +2422,17 @@ static int e1000_open(struct net_device *netdev)
 	if (err)
 		goto err_req_irq;
 
+	/*
+	 * Work around PCIe errata with MSI interrupts causing some chipsets to
+	 * ignore e1000e MSI messages, which means we need to test our MSI
+	 * interrupt now
+	 */
+	err = e1000_test_msi(adapter);
+	if (err) {
+		ndev_err(netdev, "Interrupt allocation failed\n");
+		goto err_req_irq;
+	}
+
 	/* From here on the code is the same as e1000e_up() */
 	clear_bit(__E1000_DOWN, &adapter->state);
 


-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace

Re: [PATCH] e1000e: test MSI interrupts

[Jeff Garzik]

Auke Kok wrote:
> From: Jesse Brandeburg <jesse.brandeburg@intel.com>
> 
> Test the MSI interrupt physically once before assuming that it
> actually works. Several platforms have already come across that
> have non-functional MSI interrupts and this code will attempt
> to detect those safely. Once the test succeeds MSI interrupts
> will be enabled.
> 
> Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
> Signed-off-by: Auke Kok <auke-jan.h.kok@intel.com>
> ---
> 
>  drivers/net/e1000e/defines.h |    1 
>  drivers/net/e1000e/e1000.h   |    1 
>  drivers/net/e1000e/netdev.c  |  161 ++++++++++++++++++++++++++++++++++++++++--
>  3 files changed, 155 insertions(+), 8 deletions(-)

Ah, the perennial add-same-test-to-every-driver conundrum.

I think we are far enough along with MSI to _not_ do this anymore in 
drivers.

The platforms with MSI problems should be discovered, made public, and 
worked around.

Otherwise you hide the same problem, just for someone else to discover 
with another component of the machine.

	Jeff

Re: [E1000-devel] [PATCH] e1000e: test MSI interrupts

[Kok, Auke]

Jeff Garzik wrote:
> Auke Kok wrote:
>> From: Jesse Brandeburg <jesse.brandeburg@intel.com>
>>
>> Test the MSI interrupt physically once before assuming that it
>> actually works. Several platforms have already come across that
>> have non-functional MSI interrupts and this code will attempt
>> to detect those safely. Once the test succeeds MSI interrupts
>> will be enabled.
>>
>> Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
>> Signed-off-by: Auke Kok <auke-jan.h.kok@intel.com>
>> ---
>>
>>  drivers/net/e1000e/defines.h |    1 
>>  drivers/net/e1000e/e1000.h   |    1 
>>  drivers/net/e1000e/netdev.c  |  161 ++++++++++++++++++++++++++++++++++++++++--
>>  3 files changed, 155 insertions(+), 8 deletions(-)
> 
> Ah, the perennial add-same-test-to-every-driver conundrum.
> 
> I think we are far enough along with MSI to _not_ do this anymore in 
> drivers.
> 
> The platforms with MSI problems should be discovered, made public, and 
> worked around.
> 
> Otherwise you hide the same problem, just for someone else to discover 
> with another component of the machine.

ok, that's reasonable as I have not recently seen any "new" reports against the
current trees.

We have to keep this patch in our out-of-tree version around though, since a lot
of distros still ship kernels without all the new MSI quirks. But that's not for
this forum.

Thanks,

Auke

RE: [PATCH] e1000e: test MSI interrupts

[Brandeburg, Jesse]

Kok, Auke wrote:
> Jeff Garzik wrote:
>> Auke Kok wrote:
>>> From: Jesse Brandeburg <jesse.brandeburg@intel.com>
>>> 
>>> Test the MSI interrupt physically once before assuming that it
>>> actually works. Several platforms have already come across that
>>> have non-functional MSI interrupts and this code will attempt
>>> to detect those safely. Once the test succeeds MSI interrupts
>>> will be enabled.
>>> 
>>> Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
>>> Signed-off-by: Auke Kok <auke-jan.h.kok@intel.com>

>> Ah, the perennial add-same-test-to-every-driver conundrum.
>> 
>> I think we are far enough along with MSI to _not_ do this anymore in
>> drivers. 

Actually, I'm hoping you'll allow this Jeff, we have a production system
(see below) we know about that doesn't like the way 82571 formats MSI
interrupt messages.  All other systems seem to be okay with this format
of MSI messages, but this system implemented a stricter interpretation
of the spec, and so even though that system doesn't need a quirk for MSI
because MSI works in general, we still MUST test the MSI vector to make
sure it works *for us*  In this case it comes down to being an errata
workaround.

Since there is no way to "test" generation of an interrupt from any
specific hardware device without internal knowledge of said device,
there isn't a way for us to help the kernel by writing a generic "test
MSI" routine.

I would prefer this "generic test" code be in the driver rather than
having to identify all the chipsets that fail and have the driver do
*specific chipset* detection ala bnx2.c's 8132 bridge workaround.

>> The platforms with MSI problems should be discovered, made public,
>> and worked around. 

This is our workaround, it is our fault, the incompatible chipset is in
an x3850 IBM system.

>> Otherwise you hide the same problem, just for someone else to
>> discover with another component of the machine.

We had avoided this test in the past based on your recommendation but in
this case I don't see a way around it, do you?  I'm open to other
suggestions but we need to solve the problem somehow in an *automated*
way.  IBM requested we do it this way.
 
Jesse

Re: [PATCH] e1000e: test MSI interrupts

[Jeff Garzik]

Brandeburg, Jesse wrote:
> Kok, Auke wrote:
>> Jeff Garzik wrote:
>>> Auke Kok wrote:
>>>> From: Jesse Brandeburg <jesse.brandeburg@intel.com>
>>>>
>>>> Test the MSI interrupt physically once before assuming that it
>>>> actually works. Several platforms have already come across that
>>>> have non-functional MSI interrupts and this code will attempt
>>>> to detect those safely. Once the test succeeds MSI interrupts
>>>> will be enabled.
>>>>
>>>> Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
>>>> Signed-off-by: Auke Kok <auke-jan.h.kok@intel.com>
> 
>>> Ah, the perennial add-same-test-to-every-driver conundrum.
>>>
>>> I think we are far enough along with MSI to _not_ do this anymore in
>>> drivers. 
> 
> Actually, I'm hoping you'll allow this Jeff, we have a production system
> (see below) we know about that doesn't like the way 82571 formats MSI
> interrupt messages.  All other systems seem to be okay with this format
> of MSI messages, but this system implemented a stricter interpretation
> of the spec, and so even though that system doesn't need a quirk for MSI
> because MSI works in general, we still MUST test the MSI vector to make
> sure it works *for us*  In this case it comes down to being an errata
> workaround.
> 
> Since there is no way to "test" generation of an interrupt from any
> specific hardware device without internal knowledge of said device,
> there isn't a way for us to help the kernel by writing a generic "test
> MSI" routine.
> 
> I would prefer this "generic test" code be in the driver rather than
> having to identify all the chipsets that fail and have the driver do
> *specific chipset* detection ala bnx2.c's 8132 bridge workaround.

Well if it's a problem with the networking chip rather than the 
platform, then absolutely stick it in the driver (unless its so severe 
it needs to be in pci/quirks.c before the driver even loads).

But that seems like a quick id test, with no need for all that generic 
MSI test code.

Certainly the work is scoped based on where the problem lies, either 
platform, device, or platform+device.

	Jeff




-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace

Re: [PATCH] e1000e: test MSI interrupts

[David Miller]

From: Jeff Garzik <jeff@garzik.org>
Date: Thu, 27 Mar 2008 15:43:42 -0400

> But that seems like a quick id test, with no need for all that generic 
> MSI test code.

Yes, that is the way to do it.

-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace

Re: [PATCH] e1000e: test MSI interrupts

[Brandeburg, Jesse]

David Miller wrote:
> From: Jeff Garzik <jeff@garzik.org>
> Date: Thu, 27 Mar 2008 15:43:42 -0400
> 
>> But that seems like a quick id test, with no need for all that
>> generic MSI test code.
> 
> Yes, that is the way to do it.

I get your point, but this seems a maintainance problem due to not being
able to "future proof" the solution.  I know what is (IDs) available
now, but I don't know how many systems in the future IBM will release
with a similar bridge but a different device ID that causes the same
issue.  Should we take on the maintenance of continually having to add
every new bridge device that has this issue to our driver?  Users just
want this stuff to work when they plug it in.

tg3.c has exactly this kind of test and workaround (in fact its where I
got the code) to work around the same kind of issues.

-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace

Re: [PATCH] e1000e: test MSI interrupts

[David Miller]

From: "Brandeburg, Jesse" <jesse.brandeburg@intel.com>
Date: Thu, 27 Mar 2008 15:55:34 -0700

> David Miller wrote:
> > From: Jeff Garzik <jeff@garzik.org>
> > Date: Thu, 27 Mar 2008 15:43:42 -0400
> > 
> >> But that seems like a quick id test, with no need for all that
> >> generic MSI test code.
> > 
> > Yes, that is the way to do it.
> 
> I get your point, but this seems a maintainance problem due to not being
> able to "future proof" the solution.  I know what is (IDs) available
> now, but I don't know how many systems in the future IBM will release
> with a similar bridge but a different device ID that causes the same
> issue.  Should we take on the maintenance of continually having to add
> every new bridge device that has this issue to our driver?  Users just
> want this stuff to work when they plug it in.

This is exactly what the PCI quirks list is and it works just fine.

I know the truth is that Intel as a vendor frowns upon putting into
it's driver a list of another vendor's PCI IDs as errata items because
it looks bad.

So just be honest about that.

It is, however, the correct way to address this problem.

> tg3.c has exactly this kind of test and workaround (in fact its where I
> got the code) to work around the same kind of issues.

That is from an era when the situation was much different.

-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace

Re: [PATCH] e1000e: test MSI interrupts

[Jeff Garzik]

Brandeburg, Jesse wrote:
> I get your point, but this seems a maintainance problem due to not being
> able to "future proof" the solution.  I know what is (IDs) available
> now, but I don't know how many systems in the future IBM will release
> with a similar bridge but a different device ID that causes the same
> issue.

Future-proofing in that way is a pipe dream.

You hope to predict what _errata_, what out-of-spec behavior future 
hardware will have.  Trying to code for such N^M possible futures will 
lead to code bloat, depression, and eventually madness.


> Should we take on the maintenance of continually having to add
> every new bridge device that has this issue to our driver?  Users just
> want this stuff to work when they plug it in.

As David noted, we touch quirks.c all the time for various platform 
eccentricities.  Adding a new id is easy and takes two seconds.  The 
same ease of change applies to any driver-local list of ids, too.

Anyway, I think a better question to ask is:  should we bloat up every 
driver testing for platform quirks found on a minority of platforms?

Moreover, "it doesn't work" type errata is typically fixed in future 
chip generations -- making any such generic test /less/ valuable, 
because of the lower likelihood that IBM will continue to release this 
buggy hardware for decades.

We have an existing "this bridge and MSI don't get along" list.  Adding 
an id is a one-line patch.

	Jeff




-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace

Re: [PATCH] e1000e: test MSI interrupts

[Kok, Auke]

Jeff Garzik wrote:
> Brandeburg, Jesse wrote:
>> I get your point, but this seems a maintainance problem due to not being
>> able to "future proof" the solution.  I know what is (IDs) available
>> now, but I don't know how many systems in the future IBM will release
>> with a similar bridge but a different device ID that causes the same
>> issue.
> 
> Future-proofing in that way is a pipe dream.
> 
> You hope to predict what _errata_, what out-of-spec behavior future
> hardware will have.  Trying to code for such N^M possible futures will
> lead to code bloat, depression, and eventually madness.
> 
> 
>> Should we take on the maintenance of continually having to add
>> every new bridge device that has this issue to our driver?  Users just
>> want this stuff to work when they plug it in.
> 
> As David noted, we touch quirks.c all the time for various platform
> eccentricities.  Adding a new id is easy and takes two seconds.  The
> same ease of change applies to any driver-local list of ids, too.
> 
> Anyway, I think a better question to ask is:  should we bloat up every
> driver testing for platform quirks found on a minority of platforms?
> 
> Moreover, "it doesn't work" type errata is typically fixed in future
> chip generations -- making any such generic test /less/ valuable,
> because of the lower likelihood that IBM will continue to release this
> buggy hardware for decades.
> 
> We have an existing "this bridge and MSI don't get along" list.  Adding
> an id is a one-line patch.

well it's just slightly more complex than just that... the bridge works OK with
MSI for every device except 82571

so 82572, 82573 e1000's work fine as well as all the newer pci-e e1000's, or any
of the non-e1000 hardware that uses MSI interrupts.

so disabling MSI entirely for that chipset by quirking it is not what we want to
do in the first place.

hence the test :)

the only alternative I see is that we have a quirk in the e1000 driver that acts
when the combination of an 82571 and this particular chipset is found, but I have
no information ATM on the chipset id's yet and on how to identify this configuration.

Auke

Re: [PATCH] e1000e: test MSI interrupts

[David Miller]

From: "Kok, Auke" <auke-jan.h.kok@intel.com>
Date: Thu, 27 Mar 2008 16:53:55 -0700

> the only alternative I see is that we have a quirk in the e1000
> driver that acts when the combination of an 82571 and this
> particular chipset is found, but I have no information ATM on the
> chipset id's yet and on how to identify this configuration.

And this is how you will fix this bug, once you know these
IDs.

-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace

Re: [PATCH] e1000e: test MSI interrupts

[David Miller]

From: "Brandeburg, Jesse" <jesse.brandeburg@intel.com>
Date: Thu, 27 Mar 2008 10:53:43 -0700

> >> The platforms with MSI problems should be discovered, made public,
> >> and worked around. 
> 
> This is our workaround, it is our fault, the incompatible chipset is in
> an x3850 IBM system.

Then you look for that system chipset variant in the e1000e driver and
not use MSI when it is found.

A generic test really is not warranted.

Re: [PATCH] e1000e: test MSI interrupts

[Andy Gospodarek]

On Thu, Mar 27, 2008 at 10:53:43AM -0700, Brandeburg, Jesse wrote:
> Kok, Auke wrote:
> > Jeff Garzik wrote:
> >> Auke Kok wrote:
> >>> From: Jesse Brandeburg <jesse.brandeburg@intel.com>
> >>> 
> >>> Test the MSI interrupt physically once before assuming that it
> >>> actually works. Several platforms have already come across that
> >>> have non-functional MSI interrupts and this code will attempt
> >>> to detect those safely. Once the test succeeds MSI interrupts
> >>> will be enabled.
> >>> 
> >>> Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
> >>> Signed-off-by: Auke Kok <auke-jan.h.kok@intel.com>
> 
> >> Ah, the perennial add-same-test-to-every-driver conundrum.
> >> 
> >> I think we are far enough along with MSI to _not_ do this anymore in
> >> drivers. 
> 
> Actually, I'm hoping you'll allow this Jeff, we have a production system
> (see below) we know about that doesn't like the way 82571 formats MSI
> interrupt messages.  All other systems seem to be okay with this format
> of MSI messages, but this system implemented a stricter interpretation
> of the spec, and so even though that system doesn't need a quirk for MSI
> because MSI works in general, we still MUST test the MSI vector to make
> sure it works *for us*  In this case it comes down to being an errata
> workaround.
> 
> Since there is no way to "test" generation of an interrupt from any
> specific hardware device without internal knowledge of said device,
> there isn't a way for us to help the kernel by writing a generic "test
> MSI" routine.
> 
> I would prefer this "generic test" code be in the driver rather than
> having to identify all the chipsets that fail and have the driver do
> *specific chipset* detection ala bnx2.c's 8132 bridge workaround.
> 
> >> The platforms with MSI problems should be discovered, made public,
> >> and worked around. 
> 
> This is our workaround, it is our fault, the incompatible chipset is in
> an x3850 IBM system.
> 

I've seem similar problems on other systems, so this would be a nice bit
of code to have somewhere.  I can see Jeff's argument for having this
outside of drivers, but to make my life easier I'd like to see this in
e1000e (and e1000!).  :-)

I suppose an more general alternative would be to make this code run as
an ethtool test (current or new one) and then have a module option to
disable/enable MSI so all could be done in userspace, but something
automatic like this sure would be great.


-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace

Re: [PATCH] e1000e: test MSI interrupts

[Kok, Auke]

Andy Gospodarek wrote:
> On Thu, Mar 27, 2008 at 10:53:43AM -0700, Brandeburg, Jesse wrote:
>> Kok, Auke wrote:
>>> Jeff Garzik wrote:
>>>> Auke Kok wrote:
>>>>> From: Jesse Brandeburg <jesse.brandeburg@intel.com>
>>>>>
>>>>> Test the MSI interrupt physically once before assuming that it
>>>>> actually works. Several platforms have already come across that
>>>>> have non-functional MSI interrupts and this code will attempt
>>>>> to detect those safely. Once the test succeeds MSI interrupts
>>>>> will be enabled.
>>>>>
>>>>> Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
>>>>> Signed-off-by: Auke Kok <auke-jan.h.kok@intel.com>
>>>> Ah, the perennial add-same-test-to-every-driver conundrum.
>>>>
>>>> I think we are far enough along with MSI to _not_ do this anymore in
>>>> drivers. 
>> Actually, I'm hoping you'll allow this Jeff, we have a production system
>> (see below) we know about that doesn't like the way 82571 formats MSI
>> interrupt messages.  All other systems seem to be okay with this format
>> of MSI messages, but this system implemented a stricter interpretation
>> of the spec, and so even though that system doesn't need a quirk for MSI
>> because MSI works in general, we still MUST test the MSI vector to make
>> sure it works *for us*  In this case it comes down to being an errata
>> workaround.
>>
>> Since there is no way to "test" generation of an interrupt from any
>> specific hardware device without internal knowledge of said device,
>> there isn't a way for us to help the kernel by writing a generic "test
>> MSI" routine.
>>
>> I would prefer this "generic test" code be in the driver rather than
>> having to identify all the chipsets that fail and have the driver do
>> *specific chipset* detection ala bnx2.c's 8132 bridge workaround.
>>
>>>> The platforms with MSI problems should be discovered, made public,
>>>> and worked around. 
>> This is our workaround, it is our fault, the incompatible chipset is in
>> an x3850 IBM system.
>>
> 
> I've seem similar problems on other systems, so this would be a nice bit
> of code to have somewhere.  I can see Jeff's argument for having this
> outside of drivers, but to make my life easier I'd like to see this in
> e1000e (and e1000!).  :-)

e1000 will soon no longer support PCI Express devices, so this is unneeded.

> I suppose an more general alternative would be to make this code run as
> an ethtool test (current or new one) and then have a module option to
> disable/enable MSI so all could be done in userspace, but something
> automatic like this sure would be great.

a lot of work ...

Re: [PATCH] e1000e: test MSI interrupts

[Andy Gospodarek]

On Thu, Mar 27, 2008 at 03:16:41PM -0700, Kok, Auke wrote:
> Andy Gospodarek wrote:
> > On Thu, Mar 27, 2008 at 10:53:43AM -0700, Brandeburg, Jesse wrote:
> >> Kok, Auke wrote:
> >>> Jeff Garzik wrote:
> >>>> Auke Kok wrote:
> >>>>> From: Jesse Brandeburg <jesse.brandeburg@intel.com>
> >>>>>
> >>>>> Test the MSI interrupt physically once before assuming that it
> >>>>> actually works. Several platforms have already come across that
> >>>>> have non-functional MSI interrupts and this code will attempt
> >>>>> to detect those safely. Once the test succeeds MSI interrupts
> >>>>> will be enabled.
> >>>>>
> >>>>> Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
> >>>>> Signed-off-by: Auke Kok <auke-jan.h.kok@intel.com>
> >>>> Ah, the perennial add-same-test-to-every-driver conundrum.
> >>>>
> >>>> I think we are far enough along with MSI to _not_ do this anymore in
> >>>> drivers. 
> >> Actually, I'm hoping you'll allow this Jeff, we have a production system
> >> (see below) we know about that doesn't like the way 82571 formats MSI
> >> interrupt messages.  All other systems seem to be okay with this format
> >> of MSI messages, but this system implemented a stricter interpretation
> >> of the spec, and so even though that system doesn't need a quirk for MSI
> >> because MSI works in general, we still MUST test the MSI vector to make
> >> sure it works *for us*  In this case it comes down to being an errata
> >> workaround.
> >>
> >> Since there is no way to "test" generation of an interrupt from any
> >> specific hardware device without internal knowledge of said device,
> >> there isn't a way for us to help the kernel by writing a generic "test
> >> MSI" routine.
> >>
> >> I would prefer this "generic test" code be in the driver rather than
> >> having to identify all the chipsets that fail and have the driver do
> >> *specific chipset* detection ala bnx2.c's 8132 bridge workaround.
> >>
> >>>> The platforms with MSI problems should be discovered, made public,
> >>>> and worked around. 
> >> This is our workaround, it is our fault, the incompatible chipset is in
> >> an x3850 IBM system.
> >>
> > 
> > I've seem similar problems on other systems, so this would be a nice bit
> > of code to have somewhere.  I can see Jeff's argument for having this
> > outside of drivers, but to make my life easier I'd like to see this in
> > e1000e (and e1000!).  :-)
> 
> e1000 will soon no longer support PCI Express devices, so this is unneeded.
> 

So you plan to drop the bits around the device additions that are
related to e1000e config sometime soon?

Re: [PATCH] e1000e: test MSI interrupts

[Kok, Auke]

Andy Gospodarek wrote:
> On Thu, Mar 27, 2008 at 03:16:41PM -0700, Kok, Auke wrote:
>> Andy Gospodarek wrote:
>>> On Thu, Mar 27, 2008 at 10:53:43AM -0700, Brandeburg, Jesse wrote:
>>>> Kok, Auke wrote:
>>>>> Jeff Garzik wrote:
>>>>>> Auke Kok wrote:
>>>>>>> From: Jesse Brandeburg <jesse.brandeburg@intel.com>
>>>>>>>
>>>>>>> Test the MSI interrupt physically once before assuming that it
>>>>>>> actually works. Several platforms have already come across that
>>>>>>> have non-functional MSI interrupts and this code will attempt
>>>>>>> to detect those safely. Once the test succeeds MSI interrupts
>>>>>>> will be enabled.
>>>>>>>
>>>>>>> Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
>>>>>>> Signed-off-by: Auke Kok <auke-jan.h.kok@intel.com>
>>>>>> Ah, the perennial add-same-test-to-every-driver conundrum.
>>>>>>
>>>>>> I think we are far enough along with MSI to _not_ do this anymore in
>>>>>> drivers. 
>>>> Actually, I'm hoping you'll allow this Jeff, we have a production system
>>>> (see below) we know about that doesn't like the way 82571 formats MSI
>>>> interrupt messages.  All other systems seem to be okay with this format
>>>> of MSI messages, but this system implemented a stricter interpretation
>>>> of the spec, and so even though that system doesn't need a quirk for MSI
>>>> because MSI works in general, we still MUST test the MSI vector to make
>>>> sure it works *for us*  In this case it comes down to being an errata
>>>> workaround.
>>>>
>>>> Since there is no way to "test" generation of an interrupt from any
>>>> specific hardware device without internal knowledge of said device,
>>>> there isn't a way for us to help the kernel by writing a generic "test
>>>> MSI" routine.
>>>>
>>>> I would prefer this "generic test" code be in the driver rather than
>>>> having to identify all the chipsets that fail and have the driver do
>>>> *specific chipset* detection ala bnx2.c's 8132 bridge workaround.
>>>>
>>>>>> The platforms with MSI problems should be discovered, made public,
>>>>>> and worked around. 
>>>> This is our workaround, it is our fault, the incompatible chipset is in
>>>> an x3850 IBM system.
>>>>
>>> I've seem similar problems on other systems, so this would be a nice bit
>>> of code to have somewhere.  I can see Jeff's argument for having this
>>> outside of drivers, but to make my life easier I'd like to see this in
>>> e1000e (and e1000!).  :-)
>> e1000 will soon no longer support PCI Express devices, so this is unneeded.
>>
> 
> So you plan to drop the bits around the device additions that are
> related to e1000e config sometime soon?

yes I was hoping to do this for 2.6.26 already, if not 2.6.27.

I definately don't want to have that linger.

Auke


-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace