[PATCH v5 03/23] net/ntnic: add minimal initialization for PCI device
Ferruh Yigit
ferruh.yigit at amd.com
Wed Jul 10 16:58:14 CEST 2024
On 7/10/2024 3:30 PM, Serhii Iliushyk wrote:
> Hi Ferruh,
>
> Please find our explanation for your comment according to device
> initializationby function nthw_pci_dev_init.
>
> Our NIC handles multiple ports within the same PCI Bus Device Function
> (BDF) - hence, we need to create several ETH devices during the probe init.
> Allocation of multiple eth devices is handled inside the
> nthw_pci_dev_init() function.
> It appears to be similar to what the Octeontx driver does.
>
> We believe the current implementation fits better for our PMD, so we
> would like to keep it as is, unless it is a blocking requirement for
> upstreaming.
>
> Please let us know.
>
Hi Serhii,
Please don't top post, it makes very hard to follow a discussion this
way, instead prefered way is to response inline just below where your
comment is valid.
Like below there are bunch of comment, I am not sure which one you are
reffering.
But from context, I assume you refer to the one that suggests using
'rte_eth_dev_create()', firstly this is not hard requirement but
suggestion to help you, so it is up to you.
But initialisation is wrapped in that function, so it is helpful to use
that function to handle boilerplate code correct.
Current logic is:
```
probe()
init()
get n_phy_ports;
foreach i in n_phy_ports
// do ethdev allocation & initialization
```
I suggest it can be done as:
```
probe()
get n_phy_ports;
foreach i in n_phy_ports
rte_eth_dev_create(pci_init, init)
init()
// only ethdev initialization
// ethdev alloc and common init already done at this point
```
I may be missing some details, if above doesn't make sense or doesn't
help you, feel free to ignore it.
But please be sure basic ethdev allocation and initialization is done
properly, as far as I remember I had some questions around it, that is
why suggested to use the wrapper.
>
> *From: *Ferruh Yigit <ferruh.yigit at amd.com>
> *Date: *Friday, 5 July 2024 at 01:44
> *To: *Serhii Iliushyk <sil-plv at napatech.com>, dev at dpdk.org <dev at dpdk.org>
> *Cc: *Mykola Kostenok <mko-plv at napatech.com>, Christian Koue Muf
> <ckm at napatech.com>, andrew.rybchenko at oktetlabs.ru
> <andrew.rybchenko at oktetlabs.ru>
> *Subject: *Re: [PATCH v5 03/23] net/ntnic: add minimal initialization
> for PCI device
>
> On 6/27/2024 8:38 AM, Serhii Iliushyk wrote:
>> add implementation for probe/init and remove/deinit of the PCI device
>>
>> Signed-off-by: Serhii Iliushyk <sil-plv at napatech.com>
>> ---
>> drivers/net/ntnic/ntnic_ethdev.c | 104 ++++++++++++++++++++++++++++++-
>> 1 file changed, 103 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ntnic/ntnic_ethdev.c b/drivers/net/ntnic/ntnic_ethdev.c
>> index 3079bd98e4..e9a584877f 100644
>> --- a/drivers/net/ntnic/ntnic_ethdev.c
>> +++ b/drivers/net/ntnic/ntnic_ethdev.c
>> @@ -17,14 +17,63 @@
>> /* Global static variables: */
>>
>> static int
>> -nthw_pci_dev_init(struct rte_pci_device *pci_dev __rte_unused)
>> +nthw_pci_dev_init(struct rte_pci_device *pci_dev)
>> {
>> + uint32_t n_port_mask = -1; /* All ports enabled by default */
>> + int n_phy_ports;
>> + NT_LOG_DBGX(DEBUG, NTNIC, "Dev %s PF #%i Init : %02x:%02x:%i\n", pci_dev->name,
>> + pci_dev->addr.function, pci_dev->addr.bus, pci_dev->addr.devid,
>> + pci_dev->addr.function);
>> +
>> + n_phy_ports = 0;
>> +
>> + for (int n_intf_no = 0; n_intf_no < n_phy_ports; n_intf_no++) {
>> + struct rte_eth_dev *eth_dev = NULL;
>> + char name[32];
>> +
>> + if ((1 << n_intf_no) & ~n_port_mask)
>> + continue;
>> +
>> + snprintf(name, sizeof(name), "ntnic%d", n_intf_no);
>> +
>> + eth_dev = rte_eth_dev_allocate(name); /* TODO: name */
>>
>
> Is this TODO still valid?
>
>> +
>> + if (!eth_dev) {
>> + NT_LOG_DBGX(ERR, NTNIC, "%s: %s: error=%d\n",
>> + (pci_dev->name[0] ? pci_dev->name : "NA"), name, -1);
>> + return -1;
>> + }
>> +
>> + NT_LOG_DBGX(DEBUG, NTNIC, "eth_dev %p, port_id %u, if_index %u\n",
>> + eth_dev, eth_dev->data->port_id, n_intf_no);
>> +
>> +
>> + struct rte_eth_link pmd_link;
>> + pmd_link.link_speed = RTE_ETH_SPEED_NUM_NONE;
>> + pmd_link.link_duplex = RTE_ETH_LINK_FULL_DUPLEX;
>> + pmd_link.link_status = RTE_ETH_LINK_DOWN;
>> + pmd_link.link_autoneg = RTE_ETH_LINK_AUTONEG;
>> +
>> + eth_dev->device = &pci_dev->device;
>> + eth_dev->data->dev_link = pmd_link;
>> + eth_dev->data->numa_node = pci_dev->device.numa_node;
>>
>
> rte_eth_copy_pci_info() should be setting numa_node, no need to
> duplicate here.
>
> Please consider using 'rte_eth_dev_create()' to help these kind of
> boilerplate initialization. I did same comment below.
>
>> + eth_dev->dev_ops = NULL;
>> + eth_dev->state = RTE_ETH_DEV_ATTACHED;
>>
>
> Shouldn't need to set state directly, please call
> 'rte_eth_dev_probing_finish()' as a last thing in probe().
> This call will set the state, also will do some other required work.
>
>> +
>> + rte_eth_copy_pci_info(eth_dev, pci_dev);
>> + /* performs rte_eth_copy_pci_info() */
>> + eth_dev_pci_specific_init(eth_dev, pci_dev);
>>
>
> As comment says, 'eth_dev_pci_specific_init()' calls the
> 'rte_eth_copy_pci_info()', so why calling it twice, can clean the init
> and remove the comment.
>
>> +
>> + /* increase initialized ethernet devices - PF */
>>
>
> Is this comment valid here?
>
>> + }
>> +
>> return 0;
>> }
>>
>> static int
>> nthw_pci_dev_deinit(struct rte_eth_dev *eth_dev __rte_unused)
>> {
>> + NT_LOG_DBGX(DEBUG, NTNIC, "PCI device deinitialization\n");
>> return 0;
>> }
>>
>> @@ -33,13 +82,65 @@ nthw_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
>> struct rte_pci_device *pci_dev)
>> {
>> int res;
>> +
>> + NT_LOG_DBGX(DEBUG, NTNIC, "pcidev: name: '%s'\n", pci_dev->name);
>> + NT_LOG_DBGX(DEBUG, NTNIC, "devargs: name: '%s'\n", pci_dev->https://linkprotect.cudasvc.com/url?a=https%3a%2f%2fdevice.name&c=E,1,V5OBhPhfiNSro_oj2bitVYkKYAABsPx-MKLgmN0q8g6JbMwgnOO1gKkhj3c3IxCQvEgNbC8ofBuQUVC8VRFgpnK79cZnIRMu2iT6BvGGlWlO-BxzlAK2eTwkKk9z&typo=1);
>> +
>> + if (pci_dev->device.devargs) {
>> + NT_LOG_DBGX(DEBUG, NTNIC, "devargs: args: '%s'\n",
>> + (pci_dev->device.devargs->args ? pci_dev->device.devargs->args : "NULL"));
>> + NT_LOG_DBGX(DEBUG, NTNIC, "devargs: data: '%s'\n",
>> + (pci_dev->device.devargs->data ? pci_dev->device.devargs->data : "NULL"));
>> + }
>> +
>> + const int n_rte_has_pci = rte_eal_has_pci();
>> + NT_LOG(DBG, NTNIC, "has_pci=%d\n", n_rte_has_pci);
>> +
>> + if (n_rte_has_pci == 0) {
>> + NT_LOG(ERR, NTNIC, "has_pci=%d: this PMD needs hugepages\n", n_rte_has_pci);
>>
>
> It is checking PCI bus, but log is about hugepages.
>
>> + return -1;
>> + }
>>
>
> What is the intention here for the 'n_rte_has_pci' check?
> If pci bus is disabled, this probe call should not be called at all, in
> that manner this check is useless.
>
>> +
>> + const int n_rte_vfio_no_io_mmu_enabled = rte_vfio_noiommu_is_enabled();
>> + NT_LOG(DBG, NTNIC, "vfio_no_iommu_enabled=%d\n", n_rte_vfio_no_io_mmu_enabled);
>> +
>> + if (n_rte_vfio_no_io_mmu_enabled) {
>> + NT_LOG(ERR, NTNIC, "vfio_no_iommu_enabled=%d: this PMD needs VFIO IOMMU\n",
>> + n_rte_vfio_no_io_mmu_enabled);
>> + return -1;
>> + }
>> +
>> + const enum rte_iova_mode n_rte_io_va_mode = rte_eal_iova_mode();
>> + NT_LOG(DBG, NTNIC, "iova mode=%d\n", n_rte_io_va_mode);
>> +
>> + if (n_rte_io_va_mode != RTE_IOVA_PA) {
>> + NT_LOG(WRN, NTNIC, "iova mode (%d) should be PA for performance reasons\n",
>> + n_rte_io_va_mode);
>> + }
>>
>
> Is this comment valid?
> Won't iommu be used for address translation both IOVA_VA and IOVA_PA
> mode? How much performance improvement we are talking about?
>
>> +
>> + NT_LOG(DBG, NTNIC,
>> + "busid=" PCI_PRI_FMT
>> + " pciid=%04x:%04x_%04x:%04x locstr=%s @ numanode=%d: drv=%s drvalias=%s\n",
>> + pci_dev->addr.domain, pci_dev->addr.bus, pci_dev->addr.devid,
>> + pci_dev->addr.function, pci_dev->id.vendor_id, pci_dev->id.device_id,
>> + pci_dev->id.subsystem_vendor_id, pci_dev->id.subsystem_device_id,
>> + pci_dev->name[0] ? pci_dev->name : "NA", /* locstr */
>> + pci_dev->device.numa_node,
>> + pci_dev->driver->https://linkprotect.cudasvc.com/url?a=https%3a%2f%2fdriver.name&c=E,1,5_F-hniqJt0w3GPG-nclekQA5nc17FmonNihbX6JRyTd2j6RA7sGlIJ9gBTq_T3p01-6DsO244rVP-3PFWsjaqFJ44V77omLRmCrWio_VmtBudxV30mhcou9&typo=1 ? pci_dev->driver->https://linkprotect.cudasvc.com/url?a=https%3a%2f%2fdriver.name&c=E,1,frkLGUymGHY5ydpNKtk3f74kNLD7KupomSqM7BLD7aOyg83VIXzNCZfCJI9PDNuIjnexaqLnATwKsfczOq1DvmsNBhMbEgTT0QrJ7RKHxIw,&typo=1 : "NA",
>> + pci_dev->driver->driver.alias ? pci_dev->driver->driver.alias : "NA");
>> +
>> +
>> res = nthw_pci_dev_init(pci_dev);
>>
>
> Instead of calling 'nthw_pci_dev_init()' directly, you can use
> 'rte_eth_dev_create()' and pass 'nthw_pci_dev_init()' as paramter, this
> helps on some set of boilerplate code.
>
>> +
>> + NT_LOG_DBGX(DEBUG, NTNIC, "leave: res=%d\n", res);
>> return res;
>>
>
> Doesn't really matter but mostly 'ret' is used as short version of
> "return value", what 'res' is?
>
>
>> }
>>
>> static int
>> nthw_pci_remove(struct rte_pci_device *pci_dev)
>> {
>> + NT_LOG_DBGX(DEBUG, NTNIC);
>> +
>> return rte_eth_dev_pci_generic_remove(pci_dev, nthw_pci_dev_deinit);
>> }
>>
>> @@ -48,6 +149,7 @@ static struct rte_pci_driver rte_nthw_pmd = {
>> .name = "net_ntnic",
>> },
>>
>> + .drv_flags = RTE_PCI_DRV_NEED_MAPPING,
>> .probe = nthw_pci_probe,
>> .remove = nthw_pci_remove,
>> };
>
More information about the dev
mailing list