diff mbox series

[v2,1/7] net/ixgbe: Fix ixgbe_is_sfp() to return valid result for X550EM_a devs

Message ID 20211206221922.644187-2-stephend@silicom-usa.com (mailing list archive)
State Superseded, archived
Delegated to: Qi Zhang
Headers
Series ixgbe SFP handling fixes |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Stephen Douthit Dec. 6, 2021, 10:19 p.m. UTC 
  Currently all X500EM* MAC types fallthrough to the default case and get
reported as non-SFP regardless of media type, which isn't correct.

Fixes: 0790adeb567 ("ixgbe/base: support X550em_a device")
Cc: stable@dpdk.org

Signed-off-by: Stephen Douthit <stephend@silicom-usa.com>
---
 drivers/net/ixgbe/ixgbe_ethdev.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

Comments

Wang, Haiyue Dec. 20, 2021, 7:45 a.m. UTC | #1 
> -----Original Message-----
> From: Stephen Douthit <stephend@silicom-usa.com>
> Sent: Tuesday, December 7, 2021 06:19
> To: Wang, Haiyue <haiyue.wang@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>
> Cc: dev@dpdk.org; Wen Wang <wenw@silicom-usa.com>; Stephen Douthit <stephend@silicom-usa.com>;
> stable@dpdk.org
> Subject: [PATCH v2 1/7] net/ixgbe: Fix ixgbe_is_sfp() to return valid result for X550EM_a devs
> 
> Currently all X500EM* MAC types fallthrough to the default case and get
> reported as non-SFP regardless of media type, which isn't correct.
> 
> Fixes: 0790adeb567 ("ixgbe/base: support X550em_a device")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Stephen Douthit <stephend@silicom-usa.com>
> ---
>  drivers/net/ixgbe/ixgbe_ethdev.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
> index fe61dba81d..66f7af95de 100644
> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> @@ -781,6 +781,20 @@ ixgbe_is_sfp(struct ixgbe_hw *hw)
>  	case ixgbe_phy_sfp_passive_unknown:
>  		return 1;
>  	default:
> +		/* x550em devices may be SFP, check media type */
> +		switch (hw->mac.type) {
> +		case ixgbe_mac_X550EM_x:
> +		case ixgbe_mac_X550EM_a:
> +			switch (hw->mac.ops.get_media_type(hw)) {

Use the API 'ixgbe_get_media_type' to avoid ops null ?

> +			case ixgbe_media_type_fiber:
> +			case ixgbe_media_type_fiber_qsfp:
> +				return 1;
> +			default:
> +				return 0;

Since we care 'return 1' only, then the two defaults just "break;" ?

> +			}
> +		default:
> +			return 0; 

Just 'break;'

> +		}
>  		return 0;

Then this default '0' will be used.

>  	}
>  }
> --
> 2.31.1
Stephen Douthit Dec. 20, 2021, 9:32 p.m. UTC | #2 
On 12/20/21 02:45, Wang, Haiyue wrote:
>> -----Original Message-----
>> From: Stephen Douthit <stephend@silicom-usa.com>
>> Sent: Tuesday, December 7, 2021 06:19
>> To: Wang, Haiyue <haiyue.wang@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>
>> Cc: dev@dpdk.org; Wen Wang <wenw@silicom-usa.com>; Stephen Douthit <stephend@silicom-usa.com>;
>> stable@dpdk.org
>> Subject: [PATCH v2 1/7] net/ixgbe: Fix ixgbe_is_sfp() to return valid result for X550EM_a devs
>>
>> Currently all X500EM* MAC types fallthrough to the default case and get
>> reported as non-SFP regardless of media type, which isn't correct.
>>
>> Fixes: 0790adeb567 ("ixgbe/base: support X550em_a device")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Stephen Douthit <stephend@silicom-usa.com>
>> ---
>>   drivers/net/ixgbe/ixgbe_ethdev.c | 14 ++++++++++++++
>>   1 file changed, 14 insertions(+)
>>
>> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
>> index fe61dba81d..66f7af95de 100644
>> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
>> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
>> @@ -781,6 +781,20 @@ ixgbe_is_sfp(struct ixgbe_hw *hw)
>>        case ixgbe_phy_sfp_passive_unknown:
>>                return 1;
>>        default:
>> +             /* x550em devices may be SFP, check media type */
>> +             switch (hw->mac.type) {
>> +             case ixgbe_mac_X550EM_x:
>> +             case ixgbe_mac_X550EM_a:
>> +                     switch (hw->mac.ops.get_media_type(hw)) {
> 
> Use the API 'ixgbe_get_media_type' to avoid ops null ?

Ok, I can change it.

Note that there's already a mix of calls with and without the null check
wrapper in the file already though, so maybe worth future cleanup.

$ grep get_media_type drivers/net/ixgbe/ixgbe_ethdev.c
         if (diag && (hw->mac.ops.get_media_type(hw) == ixgbe_media_type_copper)) {
         if (hw->mac.ops.get_media_type(hw) == ixgbe_media_type_copper) {
         if (hw->mac.ops.get_media_type(hw) == ixgbe_media_type_copper) {
         if (hw->mac.ops.get_media_type(hw) == ixgbe_media_type_copper) {
         if (hw->mac.ops.get_media_type(hw) == ixgbe_media_type_copper) {
         if (ixgbe_get_media_type(hw) == ixgbe_media_type_fiber) {
                 if (ixgbe_get_media_type(hw) == ixgbe_media_type_fiber) {

>> +                     case ixgbe_media_type_fiber:
>> +                     case ixgbe_media_type_fiber_qsfp:
>> +                             return 1;
>> +                     default:
>> +                             return 0;
> 
> Since we care 'return 1' only, then the two defaults just "break;" ?
> 
>> +                     }
>> +             default:
>> +                     return 0;
> 
> Just 'break;'
> 
>> +             }
>>                return 0;
> 
> Then this default '0' will be used.

Ok, will change.

Thanks,
Steve

>>        }
>>   }
>> --
>> 2.31.1
>
diff mbox series

Patch

diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
index fe61dba81d..66f7af95de 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -781,6 +781,20 @@  ixgbe_is_sfp(struct ixgbe_hw *hw)
 	case ixgbe_phy_sfp_passive_unknown:
 		return 1;
 	default:
+		/* x550em devices may be SFP, check media type */
+		switch (hw->mac.type) {
+		case ixgbe_mac_X550EM_x:
+		case ixgbe_mac_X550EM_a:
+			switch (hw->mac.ops.get_media_type(hw)) {
+			case ixgbe_media_type_fiber:
+			case ixgbe_media_type_fiber_qsfp:
+				return 1;
+			default:
+				return 0;
+			}
+		default:
+			return 0;
+		}
 		return 0;
 	}
 }