Patchwork e1000e: Fix bug for e1000e interrupt default mode select.

login
register
mail settings
Submitter Kumar Gala
Date Dec. 2, 2011, 7:11 a.m.
Message ID <1322809906-6428-1-git-send-email-galak@kernel.crashing.org>
Download mbox | patch
Permalink /patch/128809/
State Awaiting Upstream
Delegated to: David Miller
Headers show

Comments

Kumar Gala - Dec. 2, 2011, 7:11 a.m.
From: Prabhakar <prabhakar@freescale.com>

If the kernel config does not have MSI enabled (CONFIG_PCI_MSI) the driver
should not default to MSI interrupt mode but legacy interrupt mode.

Signed-off-by: Jin Qing <b24347@freescale.com>
Signed-off-by: Prabhakar <prabhakar@freescale.com>
Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
---
 drivers/net/ethernet/intel/e1000e/param.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)
Ben Hutchings - Dec. 2, 2011, 5:55 p.m.
On Fri, 2011-12-02 at 01:11 -0600, Kumar Gala wrote:
> From: Prabhakar <prabhakar@freescale.com>
> 
> If the kernel config does not have MSI enabled (CONFIG_PCI_MSI) the driver
> should not default to MSI interrupt mode but legacy interrupt mode.

It is supposed to automatically fall-back to legacy interrupt mode.
Does that not work?

Also, are there really systems with PCI Express and no MSI support?

Ben.

> Signed-off-by: Jin Qing <b24347@freescale.com>
> Signed-off-by: Prabhakar <prabhakar@freescale.com>
> Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
> ---
>  drivers/net/ethernet/intel/e1000e/param.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/e1000e/param.c b/drivers/net/ethernet/intel/e1000e/param.c
> index 20e93b0..18c35c6 100644
> --- a/drivers/net/ethernet/intel/e1000e/param.c
> +++ b/drivers/net/ethernet/intel/e1000e/param.c
> @@ -388,8 +388,13 @@ void __devinit e1000e_check_options(struct e1000_adapter *adapter)
>  		static struct e1000_option opt = {
>  			.type = range_option,
>  			.name = "Interrupt Mode",
> +#ifdef CONFIG_PCI_MSI
>  			.err  = "defaulting to 2 (MSI-X)",
>  			.def  = E1000E_INT_MODE_MSIX,
> +#else
> +			.err  = "defaulting to 0 (Legacy)",
> +			.def  = E1000E_INT_MODE_LEGACY,
> +#endif
>  			.arg  = { .r = { .min = MIN_INTMODE,
>  					 .max = MAX_INTMODE } }
>  		};
Allan, Bruce W - Dec. 2, 2011, 10:43 p.m.
>-----Original Message-----

>From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On

>Behalf Of Ben Hutchings

>Sent: Friday, December 02, 2011 9:55 AM

>To: Kumar Gala

>Cc: Kirsher, Jeffrey T; Brandeburg, Jesse; e1000-devel@lists.sourceforge.net;

>netdev@vger.kernel.org; Prabhakar

>Subject: Re: [PATCH] e1000e: Fix bug for e1000e interrupt default mode select.

>

>On Fri, 2011-12-02 at 01:11 -0600, Kumar Gala wrote:

>> From: Prabhakar <prabhakar@freescale.com>

>>

>> If the kernel config does not have MSI enabled (CONFIG_PCI_MSI) the driver

>> should not default to MSI interrupt mode but legacy interrupt mode.

>

>It is supposed to automatically fall-back to legacy interrupt mode.

>Does that not work?


It should, and that was my argument (discussed internally) when a similar
patch was previously submitted.  Different from the prior patch, this one
also changes the default message generated when the IntMode is specified
for one interface but not all.  However, it's not quite right in that only
a small subset of the devices supported by e1000e have support for MSI-X in
the hardware.  I have another patch queued up internally here at Intel that
changes the default IntMode based on not only kernel support of MSI/MSI-X
but also on feature support in hardware with additional comments for more
clarification.  My patch should be pushed upstream soon.

>

>Also, are there really systems with PCI Express and no MSI support?

>

>Ben.

>

>> Signed-off-by: Jin Qing <b24347@freescale.com>

>> Signed-off-by: Prabhakar <prabhakar@freescale.com>

>> Signed-off-by: Kumar Gala <galak@kernel.crashing.org>

>> ---

>>  drivers/net/ethernet/intel/e1000e/param.c |    5 +++++

>>  1 files changed, 5 insertions(+), 0 deletions(-)

>>

>> diff --git a/drivers/net/ethernet/intel/e1000e/param.c

>b/drivers/net/ethernet/intel/e1000e/param.c

>> index 20e93b0..18c35c6 100644

>> --- a/drivers/net/ethernet/intel/e1000e/param.c

>> +++ b/drivers/net/ethernet/intel/e1000e/param.c

>> @@ -388,8 +388,13 @@ void __devinit e1000e_check_options(struct e1000_adapter

>*adapter)

>>  		static struct e1000_option opt = {

>>  			.type = range_option,

>>  			.name = "Interrupt Mode",

>> +#ifdef CONFIG_PCI_MSI

>>  			.err  = "defaulting to 2 (MSI-X)",

>>  			.def  = E1000E_INT_MODE_MSIX,

>> +#else

>> +			.err  = "defaulting to 0 (Legacy)",

>> +			.def  = E1000E_INT_MODE_LEGACY,

>> +#endif

>>  			.arg  = { .r = { .min = MIN_INTMODE,

>>  					 .max = MAX_INTMODE } }

>>  		};

>

>--

>Ben Hutchings, Staff Engineer, Solarflare

>Not speaking for my employer; that's the marketing department's job.

>They asked us to note that Solarflare product names are trademarked.

>

>--

>To unsubscribe from this list: send the line "unsubscribe netdev" in

>the body of a message to majordomo@vger.kernel.org

>More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/net/ethernet/intel/e1000e/param.c b/drivers/net/ethernet/intel/e1000e/param.c
index 20e93b0..18c35c6 100644
--- a/drivers/net/ethernet/intel/e1000e/param.c
+++ b/drivers/net/ethernet/intel/e1000e/param.c
@@ -388,8 +388,13 @@  void __devinit e1000e_check_options(struct e1000_adapter *adapter)
 		static struct e1000_option opt = {
 			.type = range_option,
 			.name = "Interrupt Mode",
+#ifdef CONFIG_PCI_MSI
 			.err  = "defaulting to 2 (MSI-X)",
 			.def  = E1000E_INT_MODE_MSIX,
+#else
+			.err  = "defaulting to 0 (Legacy)",
+			.def  = E1000E_INT_MODE_LEGACY,
+#endif
 			.arg  = { .r = { .min = MIN_INTMODE,
 					 .max = MAX_INTMODE } }
 		};