Re: [PATCH 19.11 1/4] net/hns3: fix residual MAC after setting default MAC

[Christian Ehrhardt <christian.ehrhardt@canonical.com>]

On Sat, Dec 25, 2021 at 11:58 AM Huisong Li <lihuisong@huawei.com> wrote:
>
> [ upstream commit 19e67d8ebced5cb12829f75c70e6c497b5925e82 ]
>

Thank you, queued for 19.11.12 (later this year)

> This problem occurs in the following scenarios:
> 1) reset is encountered when the adapter is running.
> 2) set a new default MAC address.
>
> After the above two steps, the old default MAC address should be not
> take effect. But the current behavior is contrary to that. This is due
> to the change of the "default_addr_setted" in hw->mac from 'true' to
> 'false' after the reset. As a result, the old MAC address is not removed
> when the new default MAC address is set. This variable controls whether
> to delete the old default MAC address when setting the default MAC
> address. It is only used when the mac_addr_set API is called for the
> first time. In fact, when a unicast MAC address is deleted, if the
> address isn't in the MAC address table, the driver doesn't return
> failure. So this patch remove the redundant and troublesome variables to
> resolve this problem.
>
> Fixes: 7d7f9f80bbfb ("net/hns3: support MAC address related operations")
>
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> ---
>  drivers/net/hns3/hns3_ethdev.c | 58 ++++++++++------------------------
>  drivers/net/hns3/hns3_ethdev.h |  1 -
>  2 files changed, 16 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/net/hns3/hns3_ethdev.c b/drivers/net/hns3/hns3_ethdev.c
> index 157dd34d4f..a2676f84b0 100644
> --- a/drivers/net/hns3/hns3_ethdev.c
> +++ b/drivers/net/hns3/hns3_ethdev.c
> @@ -1545,7 +1545,7 @@ hns3_remove_mc_addr_common(struct hns3_hw *hw, struct rte_ether_addr *mac_addr)
>
>  static int
>  hns3_add_mac_addr(struct rte_eth_dev *dev, struct rte_ether_addr *mac_addr,
> -                 uint32_t idx, __attribute__ ((unused)) uint32_t pool)
> +                 __rte_unused uint32_t idx, __rte_unused uint32_t pool)
>  {
>         struct hns3_hw *hw = HNS3_DEV_PRIVATE_TO_HW(dev->data->dev_private);
>         char mac_str[RTE_ETHER_ADDR_FMT_SIZE];
> @@ -1576,8 +1576,6 @@ hns3_add_mac_addr(struct rte_eth_dev *dev, struct rte_ether_addr *mac_addr,
>                 return ret;
>         }
>
> -       if (idx == 0)
> -               hw->mac.default_addr_setted = true;
>         rte_spinlock_unlock(&hw->lock);
>
>         return ret;
> @@ -1642,35 +1640,18 @@ hns3_set_default_mac_addr(struct rte_eth_dev *dev,
>         struct hns3_hw *hw = HNS3_DEV_PRIVATE_TO_HW(dev->data->dev_private);
>         struct rte_ether_addr *oaddr;
>         char mac_str[RTE_ETHER_ADDR_FMT_SIZE];
> -       bool default_addr_setted;
> -       bool rm_succes = false;
>         int ret, ret_val;
>
> -       /* check if mac addr is valid */
> -       if (!rte_is_valid_assigned_ether_addr(mac_addr)) {
> -               rte_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE,
> -                                     mac_addr);
> -               hns3_err(hw, "Failed to set mac addr, addr(%s) invalid",
> -                        mac_str);
> -               return -EINVAL;
> -       }
> -
> -       oaddr = (struct rte_ether_addr *)hw->mac.mac_addr;
> -       default_addr_setted = hw->mac.default_addr_setted;
> -       if (default_addr_setted && !!rte_is_same_ether_addr(mac_addr, oaddr))
> -               return 0;
> -
>         rte_spinlock_lock(&hw->lock);
> -       if (default_addr_setted) {
> -               ret = hns3_remove_uc_addr_common(hw, oaddr);
> -               if (ret) {
> -                       rte_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE,
> -                                             oaddr);
> -                       hns3_warn(hw, "Remove old uc mac address(%s) fail: %d",
> -                                 mac_str, ret);
> -                       rm_succes = false;
> -               } else
> -                       rm_succes = true;
> +       oaddr = (struct rte_ether_addr *)hw->mac.mac_addr;
> +       ret = hns3_remove_uc_addr_common(hw, oaddr);
> +       if (ret) {
> +               rte_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE,
> +                                       oaddr);
> +               hns3_warn(hw, "Remove old uc mac address(%s) fail: %d",
> +                               mac_str, ret);
> +               rte_spinlock_unlock(&hw->lock);
> +               return ret;
>         }
>
>         ret = hns3_add_uc_addr_common(hw, mac_addr);
> @@ -1689,7 +1670,6 @@ hns3_set_default_mac_addr(struct rte_eth_dev *dev,
>
>         rte_ether_addr_copy(mac_addr,
>                             (struct rte_ether_addr *)hw->mac.mac_addr);
> -       hw->mac.default_addr_setted = true;
>         rte_spinlock_unlock(&hw->lock);
>
>         return 0;
> @@ -1705,16 +1685,12 @@ hns3_set_default_mac_addr(struct rte_eth_dev *dev,
>         }
>
>  err_add_uc_addr:
> -       if (rm_succes) {
> -               ret_val = hns3_add_uc_addr_common(hw, oaddr);
> -               if (ret_val) {
> -                       rte_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE,
> -                                             oaddr);
> -                       hns3_warn(hw,
> -                                 "Failed to restore old uc mac addr(%s): %d",
> -                                 mac_str, ret_val);
> -                       hw->mac.default_addr_setted = false;
> -               }
> +       ret_val = hns3_add_uc_addr_common(hw, oaddr);
> +       if (ret_val) {
> +               rte_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE,
> +                                       oaddr);
> +               hns3_warn(hw, "Failed to restore old uc mac addr(%s): %d",
> +                               mac_str, ret_val);
>         }
>         rte_spinlock_unlock(&hw->lock);
>
> @@ -2880,7 +2856,6 @@ hns3_get_board_configuration(struct hns3_hw *hw)
>         hw->rss_dis_flag = false;
>         memcpy(hw->mac.mac_addr, cfg.mac_addr, RTE_ETHER_ADDR_LEN);
>         hw->mac.phy_addr = cfg.phy_addr;
> -       hw->mac.default_addr_setted = false;
>         hw->num_tx_desc = cfg.tqp_desc_num;
>         hw->num_rx_desc = cfg.tqp_desc_num;
>         hw->dcb_info.num_pg = 1;
> @@ -4699,7 +4674,6 @@ hns3_do_stop(struct hns3_adapter *hns)
>                 reset_queue = true;
>         } else
>                 reset_queue = false;
> -       hw->mac.default_addr_setted = false;
>         return hns3_stop_queues(hns, reset_queue);
>  }
>
> diff --git a/drivers/net/hns3/hns3_ethdev.h b/drivers/net/hns3/hns3_ethdev.h
> index 6c3ac6f8a9..be0fac4fd2 100644
> --- a/drivers/net/hns3/hns3_ethdev.h
> +++ b/drivers/net/hns3/hns3_ethdev.h
> @@ -144,7 +144,6 @@ enum hns3_media_type {
>
>  struct hns3_mac {
>         uint8_t mac_addr[RTE_ETHER_ADDR_LEN];
> -       bool default_addr_setted; /* whether default addr(mac_addr) is set */
>         uint8_t media_type;
>         uint8_t phy_addr;
>         uint8_t link_duplex  : 1; /* ETH_LINK_[HALF/FULL]_DUPLEX */
> --
> 2.33.0
>


-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd