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

["11" <caowenbo@mucse.com>]

Hi Ferruh,

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@amd.com>
> Sent: 2023年9月6日 0:56
> To: Wenbo Cao <caowenbo@mucse.com>
> Cc: dev@dpdk.org; thomas@monjalon.net; andrew.rybchenko@oktetlabs.ru;
> yaojun@mucse.com
> Subject: Re: [PATCH v6 6/8] net/rnp add port info resource init
> 
> 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.
> 
> 
> > +	dev->data->mac_addrs = rte_zmalloc(name, sizeof(struct
Yes, for the failed branch, need to free the last alloc success argument mac_addrs
> > +	}
> > +
> > +	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?
> 
Yes, you are right :) ,  it the rnp_get_mac_addr failed, this may have fireware issue thing about 
Communicate.This need to be check and warning.
> > +	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.
> 
> <...>
> 
For N10  chip is just have two pcie-bdf, the one pcie-bdf may have multiple port(max 4)
'rte_eth_dev_pci_generic_probe' probe the pcie-pdf port(master port port 0)
the other port alloc by 'rnp_alloc_eth_port', not set 'rte_eth_dev_probing_finish()'
so the 1,2,3 port will set 'rte_eth_dev_probing_finish()'.
> > +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?
> 
This is because for one pcie-bdf one port and one pcie-bdf multiple port usecase
For hareware design limit, we max can support eight port but just have two pcie-bdf.
We must use two set of api to resolve one pcie-bdf one port and
one pcie-bdf multiple port for the port Control independence feature. 
Such as vlan-filter, mac unicast/Multicast filter, promise, Multicast promise, Broadcast promise.
So that some API is common used for one-port-pcie-bdf and  multiple-port-one-pcie-bdf :(.
And some mac-api must change when known the nic is multiple-port mode.
> 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?
> 
You are right, 😊 mac_ops api need to depart from rnp.c  to a separate.c .
> > +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()) ?
> 
> 
In 'rnp_common_ops_init' both one pcie-bdf one port and one pcie-bdf multiple port,
will  use mailbox to communicase with fireware.
'rnp_special_ops_init() is design to change the mac api when the nic is  multiple-port
mode.
For now, the code is just for one-pcie-one-port mode.