Hi Ferruh,
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yi...@amd.com>
> Sent: 2023年9月6日 0:56
> To: Wenbo Cao <caowe...@mucse.com>
> Cc: dev@dpdk.org; tho...@monjalon.net; andrew.rybche...@oktetlabs.ru;
> yao...@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 <caowe...@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.
