From: Stephen Hemminger <stephen@networkplumber.org>
To: Serhii Iliushyk <sil-plv@napatech.com>
Cc: dev@dpdk.org, mko-plv@napatech.com, ckm@napatech.com
Subject: Re: [PATCH v1 00/32] add new adapter NT400D13
Date: Sat, 22 Feb 2025 13:41:57 -0800 [thread overview]
Message-ID: <20250222134157.73763253@hermes.local> (raw)
In-Reply-To: <20250220220406.3925597-1-sil-plv@napatech.com>
On Thu, 20 Feb 2025 23:03:24 +0100
Serhii Iliushyk <sil-plv@napatech.com> wrote:
> This patchset adds support for the new adapter NT400D13.
>
> Danylo Vodopianov (23):
> net/ntnic: add link agx 100g
> net/ntnic: add link state machine
> net/ntnic: add rpf and gfg init
> net/ntnic: add agx setup for port
> net/ntnic: add host loopback init
> net/ntnic: add line loopback init
> net/ntnic: add 100 gbps port init
> net/ntnic: add port post init
> net/ntnic: add nim low power API
> net/ntnic: add link handling API
> net/ntnic: add port init to the state machine
> net/ntnic: add port disable API
> net/ntnic: add nt400d13 pcm init
> net/ntnic: add HIF clock test
> net/ntnic: add nt400d13 PRM module init
> net/ntnic: add nt400d13 PRM module reset
> net/ntnic: add SPI v3 support for FPGA
> net/ntnic: add i2cm init
> net/ntnic: add pca init
> net/ntnic: add pcal init
> net/ntnic: add reset PHY init
> net/ntnic: add igam module init
> net/ntnic: init IGAM and config PLL for FPGA
>
> Serhii Iliushyk (9):
> net/ntnic: add minimal initialization new NIC NT400D13
> net/ntnic: add minimal reset FPGA
> net/ntnic: add FPGA modules and registers
> net/ntnic: add setup for fpga reset
> net/ntnic: add default reset setting for NT400D13
> net/ntnic: add DDR calibration to reset stage
> net/ntnic: add PHY ftile reset
> net/ntnic: add clock init
> net/ntnic: revert untrusted loop bound
>
> doc/guides/nics/ntnic.rst | 7 +-
> doc/guides/rel_notes/release_25_03.rst | 4 +
> drivers/net/ntnic/adapter/nt4ga_adapter.c | 9 +
> drivers/net/ntnic/include/nt4ga_link.h | 7 +
> drivers/net/ntnic/include/nthw_gfg.h | 33 +
> drivers/net/ntnic/include/ntnic_nim.h | 5 +
> .../include/ntnic_nthw_fpga_rst_nt400dxx.h | 34 +
> .../link_agx_100g/nt4ga_agx_link_100g.c | 1029 ++++++
> drivers/net/ntnic/meson.build | 16 +
> drivers/net/ntnic/nim/i2c_nim.c | 158 +-
> drivers/net/ntnic/nim/i2c_nim.h | 6 +
> ...00D13_U62_Si5332-GM2-RevD-1_V5-Registers.h | 425 +++
> .../net/ntnic/nthw/core/include/nthw_fpga.h | 10 +
> .../net/ntnic/nthw/core/include/nthw_gmf.h | 2 +
> .../net/ntnic/nthw/core/include/nthw_hif.h | 4 +
> .../net/ntnic/nthw/core/include/nthw_i2cm.h | 3 +
> .../net/ntnic/nthw/core/include/nthw_igam.h | 40 +
> .../ntnic/nthw/core/include/nthw_pca9532.h | 25 +
> .../ntnic/nthw/core/include/nthw_pcal6416a.h | 33 +
> .../nthw/core/include/nthw_pcm_nt400dxx.h | 40 +
> .../ntnic/nthw/core/include/nthw_phy_tile.h | 156 +
> .../nthw/core/include/nthw_prm_nt400dxx.h | 32 +
> .../nthw/core/include/nthw_si5332_si5156.h | 63 +
> .../net/ntnic/nthw/core/include/nthw_spi_v3.h | 107 +
> .../net/ntnic/nthw/core/include/nthw_spim.h | 58 +
> .../net/ntnic/nthw/core/include/nthw_spis.h | 63 +
> .../nthw/core/nt400dxx/nthw_fpga_nt400dxx.c | 220 ++
> .../core/nt400dxx/reset/nthw_fpga_rst9574.c | 377 ++
> .../nt400dxx/reset/nthw_fpga_rst_nt400dxx.c | 427 +++
> drivers/net/ntnic/nthw/core/nthw_fpga.c | 464 +++
> drivers/net/ntnic/nthw/core/nthw_gfg.c | 340 ++
> drivers/net/ntnic/nthw/core/nthw_gmf.c | 41 +
> drivers/net/ntnic/nthw/core/nthw_hif.c | 92 +
> drivers/net/ntnic/nthw/core/nthw_i2cm.c | 139 +
> drivers/net/ntnic/nthw/core/nthw_igam.c | 93 +
> drivers/net/ntnic/nthw/core/nthw_pca9532.c | 60 +
> drivers/net/ntnic/nthw/core/nthw_pcal6416a.c | 103 +
> .../net/ntnic/nthw/core/nthw_pcm_nt400dxx.c | 80 +
> drivers/net/ntnic/nthw/core/nthw_phy_tile.c | 1242 +++++++
> .../net/ntnic/nthw/core/nthw_prm_nt400dxx.c | 55 +
> .../net/ntnic/nthw/core/nthw_si5332_si5156.c | 142 +
> drivers/net/ntnic/nthw/core/nthw_spi_v3.c | 358 ++
> drivers/net/ntnic/nthw/core/nthw_spim.c | 113 +
> drivers/net/ntnic/nthw/core/nthw_spis.c | 121 +
> drivers/net/ntnic/nthw/nthw_drv.h | 31 +
> drivers/net/ntnic/nthw/nthw_platform.c | 3 +
> drivers/net/ntnic/nthw/nthw_platform_drv.h | 2 +
> .../supported/nthw_fpga_9574_055_049_0000.c | 3124 +++++++++++++++++
> .../nthw/supported/nthw_fpga_instances.c | 5 +-
> .../nthw/supported/nthw_fpga_instances.h | 1 +
> .../ntnic/nthw/supported/nthw_fpga_mod_defs.h | 11 +
> .../nthw/supported/nthw_fpga_mod_str_map.c | 11 +
> .../ntnic/nthw/supported/nthw_fpga_reg_defs.h | 11 +
> .../nthw/supported/nthw_fpga_reg_defs_igam.h | 32 +
> .../supported/nthw_fpga_reg_defs_pci_ta.h | 33 +
> .../nthw_fpga_reg_defs_pcm_nt400dxx.h | 29 +
> .../nthw/supported/nthw_fpga_reg_defs_pdi.h | 49 +
> .../supported/nthw_fpga_reg_defs_phy_tile.h | 213 ++
> .../nthw_fpga_reg_defs_prm_nt400dxx.h | 26 +
> .../nthw/supported/nthw_fpga_reg_defs_rfd.h | 38 +
> .../supported/nthw_fpga_reg_defs_rst9574.h | 35 +
> .../nthw/supported/nthw_fpga_reg_defs_spim.h | 76 +
> .../nthw/supported/nthw_fpga_reg_defs_spis.h | 51 +
> .../nthw/supported/nthw_fpga_reg_defs_tint.h | 28 +
> drivers/net/ntnic/ntnic_ethdev.c | 1 +
> drivers/net/ntnic/ntnic_filter/ntnic_filter.c | 2 +-
> drivers/net/ntnic/ntnic_mod_reg.c | 47 +
> drivers/net/ntnic/ntnic_mod_reg.h | 25 +
> 68 files changed, 10709 insertions(+), 11 deletions(-)
> create mode 100644 drivers/net/ntnic/include/nthw_gfg.h
> create mode 100644 drivers/net/ntnic/include/ntnic_nthw_fpga_rst_nt400dxx.h
> create mode 100644 drivers/net/ntnic/link_mgmt/link_agx_100g/nt4ga_agx_link_100g.c
> create mode 100644 drivers/net/ntnic/nthw/core/include/NT400D13_U62_Si5332-GM2-RevD-1_V5-Registers.h
> create mode 100644 drivers/net/ntnic/nthw/core/include/nthw_igam.h
> create mode 100644 drivers/net/ntnic/nthw/core/include/nthw_pca9532.h
> create mode 100644 drivers/net/ntnic/nthw/core/include/nthw_pcal6416a.h
> create mode 100644 drivers/net/ntnic/nthw/core/include/nthw_pcm_nt400dxx.h
> create mode 100644 drivers/net/ntnic/nthw/core/include/nthw_phy_tile.h
> create mode 100644 drivers/net/ntnic/nthw/core/include/nthw_prm_nt400dxx.h
> create mode 100644 drivers/net/ntnic/nthw/core/include/nthw_si5332_si5156.h
> create mode 100644 drivers/net/ntnic/nthw/core/include/nthw_spi_v3.h
> create mode 100644 drivers/net/ntnic/nthw/core/include/nthw_spim.h
> create mode 100644 drivers/net/ntnic/nthw/core/include/nthw_spis.h
> create mode 100644 drivers/net/ntnic/nthw/core/nt400dxx/nthw_fpga_nt400dxx.c
> create mode 100644 drivers/net/ntnic/nthw/core/nt400dxx/reset/nthw_fpga_rst9574.c
> create mode 100644 drivers/net/ntnic/nthw/core/nt400dxx/reset/nthw_fpga_rst_nt400dxx.c
> create mode 100644 drivers/net/ntnic/nthw/core/nthw_gfg.c
> create mode 100644 drivers/net/ntnic/nthw/core/nthw_igam.c
> create mode 100644 drivers/net/ntnic/nthw/core/nthw_pca9532.c
> create mode 100644 drivers/net/ntnic/nthw/core/nthw_pcal6416a.c
> create mode 100644 drivers/net/ntnic/nthw/core/nthw_pcm_nt400dxx.c
> create mode 100644 drivers/net/ntnic/nthw/core/nthw_phy_tile.c
> create mode 100644 drivers/net/ntnic/nthw/core/nthw_prm_nt400dxx.c
> create mode 100644 drivers/net/ntnic/nthw/core/nthw_si5332_si5156.c
> create mode 100644 drivers/net/ntnic/nthw/core/nthw_spi_v3.c
> create mode 100644 drivers/net/ntnic/nthw/core/nthw_spim.c
> create mode 100644 drivers/net/ntnic/nthw/core/nthw_spis.c
> create mode 100644 drivers/net/ntnic/nthw/supported/nthw_fpga_9574_055_049_0000.c
> create mode 100644 drivers/net/ntnic/nthw/supported/nthw_fpga_reg_defs_igam.h
> create mode 100644 drivers/net/ntnic/nthw/supported/nthw_fpga_reg_defs_pci_ta.h
> create mode 100644 drivers/net/ntnic/nthw/supported/nthw_fpga_reg_defs_pcm_nt400dxx.h
> create mode 100644 drivers/net/ntnic/nthw/supported/nthw_fpga_reg_defs_pdi.h
> create mode 100644 drivers/net/ntnic/nthw/supported/nthw_fpga_reg_defs_phy_tile.h
> create mode 100644 drivers/net/ntnic/nthw/supported/nthw_fpga_reg_defs_prm_nt400dxx.h
> create mode 100644 drivers/net/ntnic/nthw/supported/nthw_fpga_reg_defs_rfd.h
> create mode 100644 drivers/net/ntnic/nthw/supported/nthw_fpga_reg_defs_rst9574.h
> create mode 100644 drivers/net/ntnic/nthw/supported/nthw_fpga_reg_defs_spim.h
> create mode 100644 drivers/net/ntnic/nthw/supported/nthw_fpga_reg_defs_spis.h
> create mode 100644 drivers/net/ntnic/nthw/supported/nthw_fpga_reg_defs_tint.h
>
I will merge this for next-net BUT
The driver is better after this patch series, but still low quality.
Several pre-existing issues in this driver; looks like it did not get enough
review during 24.11 release when it was merged.
✔ passed
✘ Failed
Basic hygiene
✔ Look at CI results in patchwork
✔ Merge cleanly with git am
✔ Run checkpatches
✔ Run check-git-log
✔ Run check-symbol-maps.sh
✔ Run check-doc-vs-code
✔ Run check-spdk-tag
Builds
✔ Normal Gcc build; make sure driver is built!
✔ Use latest experimental Gcc 15
✔ Clang build using current version (clang-19)
✔ Doc build
o Build for 32 bit x86
o Cross build for Windows (if applicable)
✔ Debug build
✔ Enable asserts
✔ Test meson builds
Experimental builds:
✔ Enable address sanitizer
✘ Enable extra warnings (edit meson.build) for
-Wvla, -Wformat-truncation, -Waddress-of-packed-member
Let's not add more VLA's, these could be arrays with a fixed size.
[1470/3259] Compiling C object drivers...ofile_inline_flow_api_hw_db_inline.c.o
../drivers/net/ntnic/nthw/flow_api/profile_inline/flow_api_hw_db_inline.c: In function ‘hw_db_inline_alloc_prioritized_cfn’:
../drivers/net/ntnic/nthw/flow_api/profile_inline/flow_api_hw_db_inline.c:1346:9: warning: ISO C90 forbids variable length array ‘sorted_priority’ [-Wvla]
1346 | } sorted_priority[db->nb_cat];
| ^
[1479/3259] Compiling C object drivers...ile_inline_flow_api_profile_inline.c.o
../drivers/net/ntnic/nthw/flow_api/profile_inline/flow_api_profile_inline.c: In function ‘setup_db_qsl_data’:
../drivers/net/ntnic/nthw/flow_api/profile_inline/flow_api_profile_inline.c:3193:17: warning: ISO C90 forbids variable length array ‘ports’ [-Wvla]
3193 | uint32_t ports[fd->dst_num_avail];
| ^~~~~~~~
../drivers/net/ntnic/nthw/flow_api/profile_inline/flow_api_profile_inline.c:3194:17: warning: ISO C90 forbids variable length array ‘queues’ [-Wvla]
3194 | uint32_t queues[fd->dst_num_avail];
| ^~~~~~~~
This is not doing what you expect, ports is uint32_t array
uint32_t ports[fd->dst_num_avail];
uint32_t queues[fd->dst_num_avail];
memset(ports, 0, fd->dst_num_avail);
memset(queues, 0, fd->dst_num_avail);
Instead use HW_DB_INLINE_MAX_QST_PER_QSL
Look for anti-patterns:
✘ Driver must not disable warnings with compiler flags or pragma's
Using pragma pack()
✘ Driver must not use thread and signal
Using thread to monitor, should not be done by PMD specific thread.
✘ Driver should not call abort() or assert() directly
Is using assert() when should be using RTE_ASSERT()
✘ Review exposed symbol names
The driver exposes lots of global symbols (when statically linked)
that do not have a consistent prefix of nthw_...
Examples: get_rx_idle(), set_rx_idle(), dev_flow_init()
✔ Apply coccinelle scripts
✘ Review use of malloc
Several places call malloc but do not check return value
✘ Review use of memset
The code related to stats has several issues:
- function returns -1 but never checked by callers
- stats structure is already zero'd by ethdev
- if queue is greater than RTE_ETHDEV_QUEUE_STAT_CNTRS the statistics should
still be counted for that queue, just no per-queue stats
- the use of term if_index is potentially confusing; normally if_index refers to the interface
index assigned by the OS used for ioctl's etc. In this driver it appears to be the index
of the phy.
static int dpdk_stats_collect(struct pmd_internals *internals, struct rte_eth_stats *stats)
{
const struct ntnic_filter_ops *ntnic_filter_ops = get_ntnic_filter_ops();
if (ntnic_filter_ops == NULL) {
NT_LOG_DBGX(ERR, NTNIC, "ntnic_filter_ops uninitialized");
return -1;
}
...
ntnic_filter_ops->poll_statistics(internals);
memset(stats, 0, sizeof(*stats));
for (i = 0; i < RTE_ETHDEV_QUEUE_STAT_CNTRS && i < internals->nb_rx_queues; i++) {
stats->q_ipackets[i] = internals->rxq_scg[i].rx_pkts;
stats->q_ibytes[i] = internals->rxq_scg[i].rx_bytes;
rx_total += stats->q_ipackets[i];
rx_total_b += stats->q_ibytes[i];
}
for (i = 0; i < RTE_ETHDEV_QUEUE_STAT_CNTRS && i < internals->nb_tx_queues; i++) {
stats->q_opackets[i] = internals->txq_scg[i].tx_pkts;
stats->q_obytes[i] = internals->txq_scg[i].tx_bytes;
stats->q_errors[i] = internals->txq_scg[i].err_pkts;
tx_total += stats->q_opackets[i];
tx_total_b += stats->q_obytes[i];
tx_err_total += stats->q_errors[i];
}
Other:
The handling of queue start/stop in this device is odd.
Doesn't do deferred start.
When Rx is stopped most drivers do some action to stop the hardware.
Given the use of thread, driver should *not* have been merged in 24.11.
The DPDK has a set of assumptions about thread and process model, and
drivers making their own threads can cause problems in applications.
(Other drivers use alarm for this type of port monitor function).
prev parent reply other threads:[~2025-02-22 21:42 UTC|newest]
Thread overview: 37+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-02-20 22:03 Serhii Iliushyk
2025-02-20 22:03 ` [PATCH v1 01/32] net/ntnic: add link agx 100g Serhii Iliushyk
2025-02-20 22:03 ` [PATCH v1 02/32] net/ntnic: add link state machine Serhii Iliushyk
2025-02-20 22:03 ` [PATCH v1 03/32] net/ntnic: add rpf and gfg init Serhii Iliushyk
2025-02-20 22:03 ` [PATCH v1 04/32] net/ntnic: add agx setup for port Serhii Iliushyk
2025-02-20 22:03 ` [PATCH v1 05/32] net/ntnic: add host loopback init Serhii Iliushyk
2025-02-20 22:03 ` [PATCH v1 06/32] net/ntnic: add line " Serhii Iliushyk
2025-02-20 22:03 ` [PATCH v1 07/32] net/ntnic: add 100 gbps port init Serhii Iliushyk
2025-02-20 22:03 ` [PATCH v1 08/32] net/ntnic: add port post init Serhii Iliushyk
2025-02-20 22:03 ` [PATCH v1 09/32] net/ntnic: add nim low power API Serhii Iliushyk
2025-02-20 22:03 ` [PATCH v1 10/32] net/ntnic: add link handling API Serhii Iliushyk
2025-02-20 22:03 ` [PATCH v1 11/32] net/ntnic: add port init to the state machine Serhii Iliushyk
2025-02-20 22:03 ` [PATCH v1 12/32] net/ntnic: add port disable API Serhii Iliushyk
2025-02-20 22:03 ` [PATCH v1 13/32] net/ntnic: add minimal initialization new NIC NT400D13 Serhii Iliushyk
2025-02-20 22:03 ` [PATCH v1 14/32] net/ntnic: add minimal reset FPGA Serhii Iliushyk
2025-02-20 22:03 ` [PATCH v1 15/32] net/ntnic: add FPGA modules and registers Serhii Iliushyk
2025-02-20 22:03 ` [PATCH v1 16/32] net/ntnic: add setup for fpga reset Serhii Iliushyk
2025-02-20 22:03 ` [PATCH v1 17/32] net/ntnic: add default reset setting for NT400D13 Serhii Iliushyk
2025-02-20 22:03 ` [PATCH v1 18/32] net/ntnic: add DDR calibration to reset stage Serhii Iliushyk
2025-02-20 22:03 ` [PATCH v1 19/32] net/ntnic: add PHY ftile reset Serhii Iliushyk
2025-02-20 22:03 ` [PATCH v1 20/32] net/ntnic: add clock init Serhii Iliushyk
2025-02-20 22:03 ` [PATCH v1 21/32] net/ntnic: add nt400d13 pcm init Serhii Iliushyk
2025-02-20 22:03 ` [PATCH v1 22/32] net/ntnic: add HIF clock test Serhii Iliushyk
2025-02-20 22:03 ` [PATCH v1 23/32] net/ntnic: add nt400d13 PRM module init Serhii Iliushyk
2025-02-20 22:03 ` [PATCH v1 24/32] net/ntnic: add nt400d13 PRM module reset Serhii Iliushyk
2025-02-20 22:03 ` [PATCH v1 25/32] net/ntnic: add SPI v3 support for FPGA Serhii Iliushyk
2025-02-22 19:21 ` Stephen Hemminger
2025-02-20 22:03 ` [PATCH v1 26/32] net/ntnic: add i2cm init Serhii Iliushyk
2025-02-20 22:03 ` [PATCH v1 27/32] net/ntnic: add pca init Serhii Iliushyk
2025-02-20 22:03 ` [PATCH v1 28/32] net/ntnic: add pcal init Serhii Iliushyk
2025-02-20 22:03 ` [PATCH v1 29/32] net/ntnic: add reset PHY init Serhii Iliushyk
2025-02-20 22:03 ` [PATCH v1 30/32] net/ntnic: add igam module init Serhii Iliushyk
2025-02-20 22:03 ` [PATCH v1 31/32] net/ntnic: init IGAM and config PLL for FPGA Serhii Iliushyk
2025-02-20 22:03 ` [PATCH v1 32/32] net/ntnic: revert untrusted loop bound Serhii Iliushyk
2025-02-20 22:31 ` Stephen Hemminger
2025-02-20 23:49 ` [PATCH v1 00/32] add new adapter NT400D13 Stephen Hemminger
2025-02-22 21:41 ` Stephen Hemminger [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20250222134157.73763253@hermes.local \
--to=stephen@networkplumber.org \
--cc=ckm@napatech.com \
--cc=dev@dpdk.org \
--cc=mko-plv@napatech.com \
--cc=sil-plv@napatech.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).