Re: [PATCH v6 6/8] net/rnp add port info resource init

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

On 9/1/2023 3:30 AM, Wenbo Cao wrote:
> Add Api For FW Mac Info, Port Resoucre info init Code
> For Different Shape Of Nic
> 
> Signed-off-by: Wenbo Cao <caowenbo@mucse.com>

<...>

> @@ -47,11 +104,53 @@ rnp_init_port_resource(struct rnp_eth_adapter *adapter,
>  		       uint8_t p_id)
>  {
>  	struct rnp_eth_port *port = RNP_DEV_TO_PORT(dev);
> +	struct rte_pci_device *pci_dev = adapter->pdev;
> +	struct rnp_hw *hw = &adapter->hw;
>  
> +	port->adapt = adapter;
> +	port->s_mode = adapter->s_mode;
> +	port->port_stopped = 1;
> +	port->hw = hw;
>  	port->eth_dev = dev;
> -	adapter->ports[p_id] = port;
> +
> +	dev->device = &pci_dev->device;
> +	rte_eth_copy_pci_info(dev, pci_dev);
>  	dev->dev_ops = &rnp_eth_dev_ops;
> -	RTE_SET_USED(name);
> +	dev->rx_queue_count       = rnp_dev_rx_queue_count;
> +	dev->rx_descriptor_status = rnp_dev_rx_descriptor_status;
> +	dev->tx_descriptor_status = rnp_dev_tx_descriptor_status;
> +	dev->rx_pkt_burst = rnp_recv_pkts;
> +	dev->tx_pkt_burst = rnp_xmit_pkts;
> +	dev->tx_pkt_prepare = rnp_prep_pkts;
> +
> +	rnp_setup_port_attr(port, dev, adapter->num_ports, p_id);
> +	rnp_init_filter_setup(port, adapter->num_ports);
> +	rnp_get_mac_addr(dev, port->mac_addr);
> +	dev->data->mac_addrs = rte_zmalloc(name, sizeof(struct rte_ether_addr) *
> +			port->attr.max_mac_addrs, 0);
> +	if (!dev->data->mac_addrs) {
> +		RNP_PMD_DRV_LOG(ERR, "Memory allocation "
> +				"for MAC failed! Exiting.\n");
> +		return -ENOMEM;
> +	}
> +	/* Allocate memory for storing hash filter MAC addresses */
> +	dev->data->hash_mac_addrs = rte_zmalloc(name,
> +			RTE_ETHER_ADDR_LEN * port->attr.max_uc_mac_hash, 0);
> +	if (dev->data->hash_mac_addrs == NULL) {
> +		RNP_PMD_INIT_LOG(ERR, "Failed to allocate %d bytes "
> +				"needed to store MAC addresses",
> +				RTE_ETHER_ADDR_LEN * port->attr.max_uc_mac_hash);
> +		return -ENOMEM;

Should free 'dev->data->mac_addrs' here, or even better can be to
implement 'rnp_dev_close()' to free device resources.


> +	}
> +
> +	rnp_set_default_mac(dev, port->mac_addr);

I guess the 'port->mac_addr' got from device via 'rnp_get_mac_addr()'
above, but if 'rnp_get_mac_addr()' fails what will be the value. Should
there be check and set a random mac if required?

> +	rte_ether_addr_copy((const struct rte_ether_addr *)port->mac_addr,
> +			dev->data->mac_addrs);
> +	/* MTU */
> +	dev->data->mtu = RTE_ETHER_MAX_LEN -
> +		RTE_ETHER_HDR_LEN - RTE_ETHER_CRC_LEN;
> +	adapter->ports[p_id] = port;
> +	rte_eth_dev_probing_finish(dev);
>  

rte_eth_dev_probing_finish() is not required, as
'rte_eth_dev_pci_generic_probe()' calls it if dev_init() returns success.

<...>

> +static int32_t rnp_get_mac_addr_pf(struct rnp_eth_port *port,
> +				   uint8_t lane,
> +				   uint8_t *macaddr)
> +{
> +	struct rnp_hw *hw = RNP_DEV_TO_HW(port->eth_dev);
> +
> +	return rnp_fw_get_macaddr(port->eth_dev, hw->pf_vf_num, macaddr, lane);
> +}
> +

These are mac_ops functions, normally the reason to have mac_ops is to
support different HW that behaves slightly different and this difference
managed by different function pointers per device. Is this your usecase?

And since these are mac_ops, defined in the base folder header, it suits
better to have separate .c file for them which is under base file, what
do you think? Or am I getting these mac_ops wrong?

> +static int32_t
> +rnp_set_default_mac_pf(struct rnp_eth_port *port,
> +		       uint8_t *mac)
> +{
> +	struct rnp_eth_adapter *adap = RNP_PORT_TO_ADAPTER(port);
> +	uint16_t max_vfs;
> +
> +	if (port->s_mode == RNP_SHARE_INDEPENDENT)
> +		return rnp_set_rafb(port->eth_dev, (uint8_t *)mac,
> +				UINT8_MAX, 0);
> +
> +	max_vfs = adap->max_vfs;
> +
> +	return rnp_set_rafb(port->eth_dev, mac, max_vfs, 0);
> +}
> +
>  const struct rnp_mac_api rnp_mac_ops = {
>  	.reset_hw	= rnp_reset_hw_pf,
> -	.init_hw	= rnp_init_hw_pf
> +	.init_hw	= rnp_init_hw_pf,
> +	.get_mac_addr	= rnp_get_mac_addr_pf,
> +	.set_default_mac = rnp_set_default_mac_pf,
> +	.set_rafb	= rnp_set_mac_addr_pf,
> +	.clear_rafb	= rnp_clear_mac_addr_pf
>  };
>  
>  static void
> @@ -228,7 +434,11 @@ rnp_common_ops_init(struct rnp_eth_adapter *adapter)
>  static int
>  rnp_special_ops_init(struct rte_eth_dev *eth_dev)
>  {
> -	RTE_SET_USED(eth_dev);
> +	struct rnp_eth_adapter *adapter = RNP_DEV_TO_ADAPTER(eth_dev);
> +	struct rnp_share_ops *share_priv;
> +
> +	share_priv = adapter->share_priv;
> +	share_priv->mac_api = &rnp_mac_ops;
>  

Can you please describe why this 'rnp_special_ops_init()' is for (its
difference from rnp_common_ops_init()) ?