Re: [dpdk-stable] [dpdk-dev] [PATCH] net/i40e: fix X722 judgement when disable adminq operation

[Kevin Traynor <ktraynor@redhat.com>]

On 16/03/2020 02:02, Li, Xiaoyun wrote:
> 
> 
>> -----Original Message-----
>> From: Kevin Traynor [mailto:ktraynor@redhat.com]
>> Sent: Saturday, March 14, 2020 03:16
>> To: Li, Xiaoyun <xiaoyun.li@intel.com>; Zhike Wang <wangzhike@jd.com>;
>> dev@dpdk.org
>> Cc: Xing, Beilei <beilei.xing@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>;
>> stable@dpdk.org
>> Subject: Re: [dpdk-dev] [dpdk-stable] [PATCH] net/i40e: fix X722 judgement
>> when disable adminq operation
>>
>> On 13/03/2020 04:40, Li, Xiaoyun wrote:
>>> Hi
>>>
>>> This issue is already fixed in 20.02 with base code update of the following
>> commit.
>>>
>>>
>>>
>>> commit 37b091c75b13d2f26359be9b77adbc33c55a7581
>>>
>>> Author: Xiaolong Ye <xiaolong.ye@intel.com>
>>>
>>> Date:   Mon Jan 13 10:39:31 2020 +0800
>>>
>>>
>>>
>>>     net/i40e/base: extend PHY access AQ command
>>>
>>>
>>>
>>>     Currently FW use MDIO I/F number corresponded with current PF for PHY
>>>
>>>     access. This code allow to specify used MDIO I/F number.
>>>
>>>
>>>
>>>     Signed-off-by: Piotr Azarewicz <piotr.azarewicz@intel.com>
>>>
>>>     Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
>>>
>>>     Acked-by: Qi Zhang <qi.z.zhang@intel.com>
>>>
>>> Acked-by: Beilei Xing beilei.xing@intel.com<mailto:beilei.xing@intel.com>
>>>
>>>
>>>
>>> But it makes sense to back port this to stable release so it can work with dpdk
>> version <=19.11.
>>>
>>> Still some comments inline.
>>>
>>>
>>>
>>>> -----Original Message-----
>>>
>>>> From: stable [mailto:stable-bounces@dpdk.org] On Behalf Of Zhike Wang
>>>
>>>> Sent: Thursday, January 16, 2020 09:20
>>>
>>>> To: dev@dpdk.org
>>>
>>>> Cc: Xing, Beilei <beilei.xing@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>;
>>>
>>>> stable@dpdk.org; Zhike Wang <wangzhike@jd.com>
>>>
>>>> Subject: [dpdk-stable] [PATCH] net/i40e: fix X722 judgement when disable
>>>
>>>> adminq operation
>>>
>>>>
>>>
>>>> X722 SFPs have different flavors, eg I40E_DEV_ID_SFP_X722
>>>
>>>> /I40E_DEV_ID_SFP_I_X722. So instead we use mac.type to judge whether it is
>>>
>>>> X722 or not.
>>>
>>>>
>>>
>>> Please be careful about your commit log. If you use mac.type == X722, then
>> that's not just X722 SFP but all X722 devices.
>>>
>>> You can just say something like X722 doesn't support adminq operation so
>> disable it.
>>>
>>
>> You say commit log, but isn't that what is done in the code?
> 
> His commit log only says about X722 SFPs but mac.type == X722 means all X722 devices. SFPs are only a part of X722.
> So I prefer his commit log can explain the root cause.
> 

Yes, my comment/question is similar about the code.

"if (hw->mac.type == I40E_MAC_X722)"

is this correct?

>>
>>>
>>>
>>>> Fixes: 9efa8d28b ("net/i40e: fix SFP X722 with FW4.16")
>>>
>>> Please follow the DPDK community code style. Fix line needs commit id of 12
>> bit.
>>>
>>> In this case, should be: Fixes: 9efa8d28b4da ("net/i40e: fix SFP X722 with
>> FW4.16")
>>>
>>> And since this fix makes sense for <=19.11, please add this line: Cc:
>> stable@dpdk.org
>>>
>>
>> Please add the stable branches requested in the usual format.
>>
>> http://doc.dpdk.org/guides/contributing/patches.html#backporting-patches-
>> for-stable-releases
>>
>>>
>>>
>>>> Signed-off-by: Zhike Wang <wangzhike@jd.com<mailto:wangzhike@jd.com>>
>>>
>>>> ---
>>>
>>>>  drivers/net/i40e/i40e_ethdev.c | 2 +-
>>>
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>>>
>>>
>>>> diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
>>>
>>>> index 5999c96..85ccb76 100644
>>>
>>>> --- a/drivers/net/i40e/i40e_ethdev.c
>>>
>>>> +++ b/drivers/net/i40e/i40e_ethdev.c
>>>
>>>> @@ -1443,7 +1443,7 @@ static inline void i40e_config_automask(struct
>>>
>>>> i40e_pf *pf)
>>>
>>>>                            return -EIO;
>>>
>>>>             }
>>>
>>>>             /* Firmware of SFP x722 does not support adminq option */
>>>
>>>> -          if (hw->device_id == I40E_DEV_ID_SFP_X722)
>>>
>>>> +          if (hw->mac.type == I40E_MAC_X722)
>>>
>>>>                            hw->flags &= ~I40E_HW_FLAG_802_1AD_CAPABLE;
>>>
>>>>
>>>
>>>>             PMD_INIT_LOG(INFO, "FW %d.%d API %d.%d NVM %02d.%02d.%02d
>>>
>>>> eetrack %04x",
>>>
>>>> --
>>>
>>>> 1.8.3.1
>>>
>>>>
>>>
>