Re: [PATCH v5 11/32] net/sssnic: add dev MAC ops

[Ferruh Yigit <ferruh.yigit@amd.com>]

On 9/4/2023 5:56 AM, wanry@3snic.com wrote:
> From: Renyong Wan <wanry@3snic.com>
> 
> Signed-off-by: Steven Song <steven.song@3snic.com>
> Signed-off-by: Renyong Wan <wanry@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, &ethdev->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.