On 9/4/2023 5:56 AM, wa...@3snic.com wrote:
> From: Renyong Wan <wa...@3snic.com>
>
> Signed-off-by: Steven Song <steven.s...@3snic.com>
> Signed-off-by: Renyong Wan <wa...@3snic.com>
>
<...>
> +int
> +sssnic_mac_addr_update(struct sssnic_hw *hw, uint8_t *new, uint8_t *old)
> +{
> + int ret;
> + struct sssnic_mac_addr_update_cmd cmd;
> + struct sssnic_msg msg;
> + uint32_t cmd_len;
> + uint16_t func;
> +
> + if (hw == NULL || new == NULL || old == NULL)
> + return -EINVAL;
> +
> + if (SSSNIC_FUNC_TYPE(hw) == SSSNIC_FUNC_TYPE_VF)
> + func = SSSNIC_PF_FUNC_IDX(hw);
> + else
> + func = SSSNIC_MPU_FUNC_IDX;
> +
> + memset(&cmd, 0, sizeof(cmd));
> + cmd_len = sizeof(cmd);
> + cmd.function = SSSNIC_FUNC_IDX(hw);
> + rte_memcpy(cmd.new_addr, new, 6);
> + rte_memcpy(cmd.old_addr, old, 6);>
Can use 'RTE_ETHER_ADDR_LEN' instead of hardcoded '6', same for a few
other locations,
<...>
> +static int
> +sssnic_ethdev_mac_addrs_init(struct rte_eth_dev *ethdev)
> +{
> + int ret;
> + struct sssnic_netdev *netdev;
> + struct sssnic_hw *hw;
> + struct rte_ether_addr default_addr;
> + char mac_str[RTE_ETHER_ADDR_FMT_SIZE];
> +
> + PMD_INIT_FUNC_TRACE();
> +
> + netdev = SSSNIC_ETHDEV_PRIVATE(ethdev);
> + hw = SSSNIC_NETDEV_TO_HW(netdev);
> +
> + ethdev->data->mac_addrs = rte_zmalloc(NULL,
> + SSSNIC_ETHDEV_MAX_NUM_UC_MAC * sizeof(struct rte_ether_addr),
> + 0);
> + if (ethdev->data->mac_addrs == NULL) {
> + PMD_DRV_LOG(ERR,
> + "Failed to allocate memory to store %u mac addresses",
> + SSSNIC_ETHDEV_MAX_NUM_UC_MAC);
> + return -ENOMEM;
> + }
> +
> + netdev->mcast_addrs = rte_zmalloc(NULL,
> + SSSNIC_ETHDEV_MAX_NUM_MC_MAC * sizeof(struct rte_ether_addr),
> + 0);
> + if (netdev->mcast_addrs == NULL) {
> + PMD_DRV_LOG(ERR,
> + "Failed to allocate memory to store %u mcast addresses",
> + SSSNIC_ETHDEV_MAX_NUM_MC_MAC);
> + ret = -ENOMEM;
> + goto alloc_mcast_addr_fail;
> + }
> +
> + /* initialize default MAC address */
> + memset(&default_addr, 0, sizeof(default_addr));
> + ret = sssnic_mac_addr_get(hw, default_addr.addr_bytes);
> + if (ret != 0)
> + PMD_DRV_LOG(NOTICE,
> + "Could not get default MAC address, will use random
> address");
> +
> + if (rte_is_zero_ether_addr(&default_addr))
> + rte_eth_random_addr(default_addr.addr_bytes);
> +
> + rte_ether_format_addr(mac_str, RTE_ETHER_ADDR_FMT_SIZE, &default_addr);
> +
> + ret = sssnic_mac_addr_add(hw, default_addr.addr_bytes);
> + if (ret != 0) {
> + PMD_DRV_LOG(ERR, "Failed to add default MAC address: %s",
> + mac_str);
> + goto add_ether_addr_fail;
> + }
> +
> + rte_ether_addr_copy(&default_addr, ðdev->data->mac_addrs[0]);
> + rte_ether_addr_copy(&default_addr, &netdev->default_addr);
> +
>
Is there a reason to keep 'default_addr' in the device private data,
when it is already stored in 'ethdev->data->mac_addrs[0]'?
<...>
> @@ -80,6 +333,7 @@ sssnic_ethdev_release(struct rte_eth_dev *ethdev)
> {
> struct sssnic_hw *hw = SSSNIC_ETHDEV_TO_HW(ethdev);
>
> + sssnic_ethdev_mac_addrs_clean(ethdev);
>
Should 'netdev->mcast_addrs' freed in this context?
As far as I can see it is not freed anywhere.
<...>
> @@ -140,7 +408,7 @@ sssnic_ethdev_uninit(struct rte_eth_dev *ethdev)
>
> sssnic_ethdev_release(ethdev);
>
> - return -EINVAL;
> + return 0;
>
This should be in previous patch, 4/32, which updates uninit function.
