netdev - Re: [PATCH net-next v2 1/5] hinic: add support to set and get pause params

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 27 Jun 2020 09:30:14 +0800
From:   "luobin (L)" <luobin9@...wei.com>
To:     Jakub Kicinski <kuba@...nel.org>
CC:     <davem@...emloft.net>, <linux-kernel@...r.kernel.org>,
        <netdev@...r.kernel.org>, <luoxianjun@...wei.com>,
        <yin.yinshi@...wei.com>, <cloud.wangxiaoyun@...wei.com>
Subject: Re: [PATCH net-next v2 1/5] hinic: add support to set and get pause
 params

On 2020/6/24 5:54, Jakub Kicinski wrote:
> On Tue, 23 Jun 2020 22:24:05 +0800 Luo bin wrote:
>> diff --git a/drivers/net/ethernet/huawei/hinic/hinic_main.c b/drivers/net/ethernet/huawei/hinic/hinic_main.c
>> index e9e6f4c9309a..e69edb01fd9b 100644
>> --- a/drivers/net/ethernet/huawei/hinic/hinic_main.c
>> +++ b/drivers/net/ethernet/huawei/hinic/hinic_main.c
>> @@ -467,6 +467,7 @@ int hinic_open(struct net_device *netdev)
>>  	if (ret)
>>  		netif_warn(nic_dev, drv, netdev,
>>  			   "Failed to revert port state\n");
>> +
> 
> Unrelated chunk, please drop.
> 
Will undo this.Thanks.
>>  err_port_state:
>>  	free_rxqs(nic_dev);
>>  	if (nic_dev->flags & HINIC_RSS_ENABLE) {
>> @@ -887,6 +888,26 @@ static void netdev_features_init(struct net_device *netdev)
>>  	netdev->features = netdev->hw_features | NETIF_F_HW_VLAN_CTAG_FILTER;
>>  }
>>  
>> +static void hinic_refresh_nic_cfg(struct hinic_dev *nic_dev)
>> +{
>> +	struct hinic_nic_cfg *nic_cfg = &nic_dev->hwdev->func_to_io.nic_cfg;
>> +	struct hinic_pause_config pause_info = {0};
>> +	struct hinic_port_cap port_cap = {0};
>> +
>> +	if (hinic_port_get_cap(nic_dev, &port_cap))
>> +		return;
>> +
>> +	mutex_lock(&nic_cfg->cfg_mutex);
>> +	if (nic_cfg->pause_set || !port_cap.autoneg_state) {
>> +		nic_cfg->auto_neg = port_cap.autoneg_state;
>> +		pause_info.auto_neg = nic_cfg->auto_neg;
>> +		pause_info.rx_pause = nic_cfg->rx_pause;
>> +		pause_info.tx_pause = nic_cfg->tx_pause;
>> +		hinic_set_hw_pause_info(nic_dev->hwdev, &pause_info);
>> +	}
>> +	mutex_unlock(&nic_cfg->cfg_mutex);
>> +}
>> +
>>  /**
>>   * link_status_event_handler - link event handler
>>   * @handle: nic device for the handler
>> @@ -918,6 +939,9 @@ static void link_status_event_handler(void *handle, void *buf_in, u16 in_size,
>>  
>>  		up(&nic_dev->mgmt_lock);
>>  
>> +		if (!HINIC_IS_VF(nic_dev->hwdev->hwif))
>> +			hinic_refresh_nic_cfg(nic_dev);
>> +
>>  		netif_info(nic_dev, drv, nic_dev->netdev, "HINIC_Link is UP\n");
>>  	} else {
>>  		down(&nic_dev->mgmt_lock);
>> @@ -950,26 +974,38 @@ static int set_features(struct hinic_dev *nic_dev,
>>  	u32 csum_en = HINIC_RX_CSUM_OFFLOAD_EN;
>>  	int err = 0;
>>  
>> -	if (changed & NETIF_F_TSO)
>> +	if (changed & NETIF_F_TSO) {
>>  		err = hinic_port_set_tso(nic_dev, (features & NETIF_F_TSO) ?
>>  					 HINIC_TSO_ENABLE : HINIC_TSO_DISABLE);
>> +		if (err)
>> +			return err;
>> +	}
>>  
>> -	if (changed & NETIF_F_RXCSUM)
>> +	if (changed & NETIF_F_RXCSUM) {
>>  		err = hinic_set_rx_csum_offload(nic_dev, csum_en);
>> +		if (err)
>> +			return err;
>> +	}
>>  
>>  	if (changed & NETIF_F_LRO) {
>>  		err = hinic_set_rx_lro_state(nic_dev,
>>  					     !!(features & NETIF_F_LRO),
>>  					     HINIC_LRO_RX_TIMER_DEFAULT,
>>  					     HINIC_LRO_MAX_WQE_NUM_DEFAULT);
>> +		if (err)
>> +			return err;
>>  	}
>>  
>> -	if (changed & NETIF_F_HW_VLAN_CTAG_RX)
>> +	if (changed & NETIF_F_HW_VLAN_CTAG_RX) {
>>  		err = hinic_set_rx_vlan_offload(nic_dev,
>>  						!!(features &
>>  						   NETIF_F_HW_VLAN_CTAG_RX));
>> +		if (err)
>> +			return err;
>> +	}
> 
> I missed this on v1, but this looks broken, multiple features may be
> changed at the same time. If user requests RXCSUM and LRO to be changed
> and LRO change fails the RXCSUM will be left in a different state than
> dev->features indicates.
>  
You're right. Will fix. Thank you.
>> -	return err;
>> +	/* enable pause and disable pfc by default */
>> +	return hinic_dcb_set_pfc(nic_dev->hwdev, 0, 0);
> 
> Why do you disable PFC every time features are changed?
>
It can be optimized. Thanks.

>> +int hinic_dcb_set_pfc(struct hinic_hwdev *hwdev, u8 pfc_en, u8 pfc_bitmap)
> 
> This is only ever called with 0, 0 as parameters.
> .
> 
This function will be called with other parameters before long to support DCB.
So I intend not to modify it.