diff mbox

[net-next,01/11] ixgbe: fix detection of SFP+ capable interfaces

Message ID 1400057663-4578-2-git-send-email-jeffrey.t.kirsher@intel.com
State Changes Requested, archived
Delegated to: David Miller
Headers show

Commit Message

Kirsher, Jeffrey T May 14, 2014, 8:54 a.m. UTC 
From: Emil Tantilov <emil.s.tantilov@intel.com>

In cases where the driver is loaded while there are no SFP+ modules in
the cage the interface was not being detected as SFP capable. To account
for this the driver called identify_sfp in ixgbe_get_settings to make
sure the data is correct. However when there is no SFP+ module in the cage
the driver waits for the I2C reads to time out which can take more than a
second and will cause issues with tools (like net-snmp) that may poll
for that information.

This patch resolves the issue by identifying 82599 based NIC with no PHY
type set as SFP capable which allows the driver to detect the SFP module
when the interface is brought up. As result of this we can also remove the
identify_sfp call from ixgbe_get_settings.

Signed-off-by: Emil Tantilov <emil.s.tantilov@intel.com>
Tested-by: Phil Schmitt <phillip.j.schmitt@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c | 7 -------
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c    | 4 ++++
 2 files changed, 4 insertions(+), 7 deletions(-)

Comments

Sergei Shtylyov May 14, 2014, 12:26 p.m. UTC | #1 
Hello.

On 14-05-2014 12:54, Jeff Kirsher wrote:

> From: Emil Tantilov <emil.s.tantilov@intel.com>

> In cases where the driver is loaded while there are no SFP+ modules in
> the cage the interface was not being detected as SFP capable. To account
> for this the driver called identify_sfp in ixgbe_get_settings to make
> sure the data is correct. However when there is no SFP+ module in the cage
> the driver waits for the I2C reads to time out which can take more than a
> second and will cause issues with tools (like net-snmp) that may poll
> for that information.

> This patch resolves the issue by identifying 82599 based NIC with no PHY
> type set as SFP capable which allows the driver to detect the SFP module
> when the interface is brought up. As result of this we can also remove the
> identify_sfp call from ixgbe_get_settings.

> Signed-off-by: Emil Tantilov <emil.s.tantilov@intel.com>
> Tested-by: Phil Schmitt <phillip.j.schmitt@intel.com>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>

[...]

> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index 8089ea9..e44c42a 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> @@ -4660,6 +4660,10 @@ static inline bool ixgbe_is_sfp(struct ixgbe_hw *hw)
>   	case ixgbe_phy_nl:
>   		if (hw->mac.type == ixgbe_mac_82598EB)
>   			return true;
> +	/* ixgbe_phy_none is set when no SFP module is present */
> +	case ixgbe_phy_none:
> +		if (hw->mac.type == ixgbe_mac_82599EB)
> +			return true;

    Shouldn't it just be combined with the previous case?

>   	default:
>   		return false;
>   	}

WBR, Sergei

--
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
Tantilov, Emil S May 14, 2014, 4:59 p.m. UTC | #2 
>-----Original Message-----
>From: Sergei Shtylyov
>[mailto:sergei.shtylyov@cogentembedded.com]
>Sent: Wednesday, May 14, 2014 5:26 AM
>To: Kirsher, Jeffrey T; davem@davemloft.net
>Cc: Tantilov, Emil S; netdev@vger.kernel.org;
>gospo@redhat.com; sassmann@redhat.com
>Subject: Re: [net-next 01/11] ixgbe: fix detection of SFP+
>capable interfaces
>
>Hello.
>
>On 14-05-2014 12:54, Jeff Kirsher wrote:
>
>> From: Emil Tantilov <emil.s.tantilov@intel.com>
>
>> In cases where the driver is loaded while there are no SFP+ modules in
>> the cage the interface was not being detected as SFP capable. To account
>> for this the driver called identify_sfp in ixgbe_get_settings to make
>> sure the data is correct. However when there is no SFP+ module in the cage
>> the driver waits for the I2C reads to time out which can take more than a
>> second and will cause issues with tools (like net-snmp) that may poll
>> for that information.
>
>> This patch resolves the issue by identifying 82599 based NIC with no PHY
>> type set as SFP capable which allows the driver to detect the SFP module
>> when the interface is brought up. As result of this we can also remove the
>> identify_sfp call from ixgbe_get_settings.
>
>> Signed-off-by: Emil Tantilov <emil.s.tantilov@intel.com>
>> Tested-by: Phil Schmitt <phillip.j.schmitt@intel.com>
>> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
>
>[...]
>
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> index 8089ea9..e44c42a 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> @@ -4660,6 +4660,10 @@ static inline bool
>ixgbe_is_sfp(struct ixgbe_hw *hw)
>>   	case ixgbe_phy_nl:
>>   		if (hw->mac.type == ixgbe_mac_82598EB)
>>   			return true;
>> +	/* ixgbe_phy_none is set when no SFP module is present
>*/
>> +	case ixgbe_phy_none:
>> +		if (hw->mac.type == ixgbe_mac_82599EB)
>> +			return true;
>
>    Shouldn't it just be combined with the previous case?

How can you combine them? The previous case is for ixgbe_phy_nl and 82598 macs this patch is for ixgbe_phy_none and 82599.

Thanks,
Emil

>
>>   	default:
>>   		return false;
>>   	}
>
>WBR, Sergei

--
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
Sergei Shtylyov May 14, 2014, 6:16 p.m. UTC | #3 
Hello.

On 05/14/2014 08:59 PM, Tantilov, Emil S wrote:

>>> From: Emil Tantilov <emil.s.tantilov@intel.com>

>>> In cases where the driver is loaded while there are no SFP+ modules in
>>> the cage the interface was not being detected as SFP capable. To account
>>> for this the driver called identify_sfp in ixgbe_get_settings to make
>>> sure the data is correct. However when there is no SFP+ module in the cage
>>> the driver waits for the I2C reads to time out which can take more than a
>>> second and will cause issues with tools (like net-snmp) that may poll
>>> for that information.

>>> This patch resolves the issue by identifying 82599 based NIC with no PHY
>>> type set as SFP capable which allows the driver to detect the SFP module
>>> when the interface is brought up. As result of this we can also remove the
>>> identify_sfp call from ixgbe_get_settings.

>>> Signed-off-by: Emil Tantilov <emil.s.tantilov@intel.com>
>>> Tested-by: Phil Schmitt <phillip.j.schmitt@intel.com>
>>> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>

>> [...]

>>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>> b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>> index 8089ea9..e44c42a 100644
>>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>> @@ -4660,6 +4660,10 @@ static inline bool ixgbe_is_sfp(struct ixgbe_hw *hw)
>>>    	case ixgbe_phy_nl:
>>>    		if (hw->mac.type == ixgbe_mac_82598EB)
>>>    			return true;
>>> +	/* ixgbe_phy_none is set when no SFP module is present */
>>> +	case ixgbe_phy_none:
>>> +		if (hw->mac.type == ixgbe_mac_82599EB)
>>> +			return true;

>>     Shouldn't it just be combined with the previous case?

> How can you combine them? The previous case is for ixgbe_phy_nl and 82598 macs this patch is for ixgbe_phy_none and 82599.

    Ah, sorry, I've misread 82598EB as 82599EB.

> Thanks,
> Emil

WBR, Sergei

--
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
Tantilov, Emil S May 14, 2014, 6:22 p.m. UTC | #4 
>-----Original Message-----
>From: Sergei Shtylyov
>[mailto:sergei.shtylyov@cogentembedded.com]
>Sent: Wednesday, May 14, 2014 11:16 AM
>To: Tantilov, Emil S; Kirsher, Jeffrey T;
>davem@davemloft.net
>Cc: netdev@vger.kernel.org; gospo@redhat.com;
>sassmann@redhat.com
>Subject: Re: [net-next 01/11] ixgbe: fix detection of SFP+
>capable interfaces
>
>Hello.
>
>On 05/14/2014 08:59 PM, Tantilov, Emil S wrote:
>
>>>> From: Emil Tantilov <emil.s.tantilov@intel.com>
>
>>>> In cases where the driver is loaded while there are no
>SFP+ modules in
>>>> the cage the interface was not being detected as SFP
>capable. To account
>>>> for this the driver called identify_sfp in
>ixgbe_get_settings to make
>>>> sure the data is correct. However when there is no SFP+
>module in the cage
>>>> the driver waits for the I2C reads to time out which can
>take more than a
>>>> second and will cause issues with tools (like net-snmp)
>that may poll
>>>> for that information.
>
>>>> This patch resolves the issue by identifying 82599 based
>NIC with no PHY
>>>> type set as SFP capable which allows the driver to
>detect the SFP module
>>>> when the interface is brought up. As result of this we
>can also remove the
>>>> identify_sfp call from ixgbe_get_settings.
>
>>>> Signed-off-by: Emil Tantilov <emil.s.tantilov@intel.com>
>>>> Tested-by: Phil Schmitt <phillip.j.schmitt@intel.com>
>>>> Signed-off-by: Jeff Kirsher
><jeffrey.t.kirsher@intel.com>
>
>>> [...]
>
>>>> diff --git
>a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>>> b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>>> index 8089ea9..e44c42a 100644
>>>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>>> @@ -4660,6 +4660,10 @@ static inline bool
>ixgbe_is_sfp(struct ixgbe_hw *hw)
>>>>    	case ixgbe_phy_nl:
>>>>    		if (hw->mac.type == ixgbe_mac_82598EB)
>>>>    			return true;
>>>> +	/* ixgbe_phy_none is set when no SFP module is
>present */
>>>> +	case ixgbe_phy_none:
>>>> +		if (hw->mac.type == ixgbe_mac_82599EB)
>>>> +			return true;
>
>>>     Shouldn't it just be combined with the previous case?
>
>> How can you combine them? The previous case is for
>ixgbe_phy_nl and 82598 macs this patch is for ixgbe_phy_none
>and 82599.
>
>    Ah, sorry, I've misread 82598EB as 82599EB.

Actually there is a problem with this patch due to this and the previous case falling through which can theoretically lead to a wrong return value. 

Please do not apply this patch. I will submit ver 2.

Thanks for reviewing!
Emil
--
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
David Miller May 14, 2014, 7 p.m. UTC | #5 
From: "Tantilov, Emil S" <emil.s.tantilov@intel.com>
Date: Wed, 14 May 2014 18:22:20 +0000

> Please do not apply this patch. I will submit ver 2.

Jeff, please resubmit this series once these issues have been resolved,
thanks.
--
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
diff mbox

Patch

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
index 31d7268..1e701d3 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
@@ -161,13 +161,6 @@  static int ixgbe_get_settings(struct net_device *netdev,
 	bool autoneg = false;
 	bool link_up;
 
-	/* SFP type is needed for get_link_capabilities */
-	if (hw->phy.media_type & (ixgbe_media_type_fiber |
-				  ixgbe_media_type_fiber_qsfp)) {
-		if (hw->phy.sfp_type == ixgbe_sfp_type_not_present)
-				hw->phy.ops.identify_sfp(hw);
-	}
-
 	hw->mac.ops.get_link_capabilities(hw, &supported_link, &autoneg);
 
 	/* set the supported link speeds */
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 8089ea9..e44c42a 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -4660,6 +4660,10 @@  static inline bool ixgbe_is_sfp(struct ixgbe_hw *hw)
 	case ixgbe_phy_nl:
 		if (hw->mac.type == ixgbe_mac_82598EB)
 			return true;
+	/* ixgbe_phy_none is set when no SFP module is present */
+	case ixgbe_phy_none:
+		if (hw->mac.type == ixgbe_mac_82599EB)
+			return true;
 	default:
 		return false;
 	}