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

[王志克 <wangzhike@jd.com>]

Hi,

Thanks for comment. I sent out v3.

BTW, I do not use --subject-prefix='PATCH branch.x' since it seems do not take effect (and my git does not show such option)

Br,
Zhike Wang 
JDCloud, Product Development, IaaS   
------------------------------------------------------------------------------------------------
Mobile／+86 13466719566
E- mail／wangzhike@jd.com
Address／5F Building A,North-Star Century Center,8 Beichen West Street,Chaoyang District Beijing
Https://JDCloud.com
------------------------------------------------------------------------------------------------


-----Original Message-----
From: Li, Xiaoyun [mailto:xiaoyun.li@intel.com] 
Sent: Tuesday, March 17, 2020 9:35 AM
To: Kevin Traynor; 王志克; dev@dpdk.org
Cc: Xing, Beilei; Zhang, Qi Z; stable@dpdk.org
Subject: RE: [dpdk-dev] [dpdk-stable] [PATCH] net/i40e: fix X722 judgement when disable adminq operation




> -----Original Message-----
> From: Kevin Traynor <ktraynor@redhat.com>
> Sent: Monday, March 16, 2020 18: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 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?
I've checked the base code update in 20.02. When the mac type is X722, this flag is not set. So I think it should be correct.
The following is the related base code.
	switch (hw->mac.type) {
	case I40E_MAC_XL710:
		if (aq->api_maj_ver > 1 ||
		    (aq->api_maj_ver == 1 &&
		     aq->api_min_ver >= I40E_MINOR_VER_GET_LINK_INFO_XL710)) {
			hw->flags |= I40E_HW_FLAG_AQ_PHY_ACCESS_CAPABLE;
			hw->flags |= I40E_HW_FLAG_FW_LLDP_STOPPABLE;
			/* The ability to RX (not drop) 802.1ad frames */
			hw->flags |= I40E_HW_FLAG_802_1AD_CAPABLE;
		}
		break;
	case I40E_MAC_X722:
		hw->flags |= I40E_HW_FLAG_AQ_SRCTL_ACCESS_ENABLE |
			     I40E_HW_FLAG_NVM_READ_REQUIRES_LOCK;

		if (aq->api_maj_ver > 1 ||
		    (aq->api_maj_ver == 1 &&
		     aq->api_min_ver >= I40E_MINOR_VER_FW_LLDP_STOPPABLE_X722))
			hw->flags |= I40E_HW_FLAG_FW_LLDP_STOPPABLE;

		if (aq->api_maj_ver > 1 ||
		    (aq->api_maj_ver == 1 &&
		     aq->api_min_ver >= I40E_MINOR_VER_GET_LINK_INFO_X722))
			hw->flags |= I40E_HW_FLAG_AQ_PHY_ACCESS_CAPABLE;
		/* fall through */
	default:
		break;
	}

> 
> >>
> >>>
> >>>
> >>>> 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-patc
> >> hes-
> >> 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
> >>>
> >>>>
> >>>
> >