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]

Message-ID: <20200623145421.163d22fd@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Tue, 23 Jun 2020 14:54:21 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Luo bin <luobin9@...wei.com>
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 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.

>  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.
  
> -	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?

> +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.