Re: [dpdk-dev] [PATCH v3] net/i40e: i40e get link status update from ipn3ke

["Zhang, Qi Z" <qi.z.zhang@intel.com>]

Hi Andy:

> -----Original Message-----
> From: Pei, Andy
> Sent: Thursday, July 4, 2019 2:56 PM
> To: dev@dpdk.org
> Cc: Pei, Andy <andy.pei@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>; Wu,
> Jingjing <jingjing.wu@intel.com>; Xing, Beilei <beilei.xing@intel.com>; Yigit,
> Ferruh <ferruh.yigit@intel.com>; Xu, Rosen <rosen.xu@intel.com>; Ye,
> Xiaolong <xiaolong.ye@intel.com>; Zhang, Roy Fan
> <roy.fan.zhang@intel.com>; stable@dpdk.org
> Subject: [PATCH v3] net/i40e: i40e get link status update from ipn3ke
> 
> Add switch_mode argument for i40e PF to specify the specific FPGA that i40e
> PF is connected to. i40e PF get link status update via the connected FPGA.
> Add switch_ethdev to rte_eth_dev_data to track the bind switch device.
> Try to bind i40e pf to switch device when i40e device is probed. If it fail to find
> correct switch device, bind will occur again when update i40e device link
> status.
>

......
 
>  int
>  i40e_dev_link_update(struct rte_eth_dev *dev,
>  		     int wait_to_complete)
> @@ -2786,6 +2905,8 @@ void i40e_flex_payload_reg_set_default(struct
> i40e_hw *hw)
>  	struct i40e_hw *hw =
> I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
>  	struct rte_eth_link link;
>  	bool enable_lse = dev->data->dev_conf.intr_conf.lsc ? true : false;
> +	struct rte_pci_device *pci_dev = RTE_ETH_DEV_TO_PCI(dev);
> +	struct rte_devargs *devargs;
>  	int ret;
> 
>  	memset(&link, 0, sizeof(link));
> @@ -2800,6 +2921,16 @@ void i40e_flex_payload_reg_set_default(struct
> i40e_hw *hw)
>  	else
>  		update_link_aq(hw, &link, enable_lse, wait_to_complete);
> 
> +	if (!dev->data->switch_ethdev) {
> +		devargs = pci_dev->device.devargs;
> +		if (devargs)
> +			dev->data->switch_ethdev =
> +				i40e_get_switch_ethdev_from_devargs(
> +					pci_dev->device.devargs);
> +	}

For regular mode, switch_ethdev is always null, seems above code did unnecessary devargs parsing for every link_update call
Can we add an intermediate flag (add a field in i40e_pf indicate if switch mode is required like other devargs ) so during probe this flag can be initialized, and it can be reused later.

So all switch mode related code can be braces with that flag. 
If (xxx_flag)
{
 .....
}


> +	i40e_pf_linkstatus_get_from_switch_ethdev(dev->data->switch_ethdev,
> +		&link);


Why i40e_pf_linkstatus_get_from_switch_ethdev is always be called?, should we do

If (dev->data->switch_ethdev)
	i40e_pf_linkstatus_get_from_switch_ethdev(dev->data->switch_ethdev, &link);