Hi David,
Thanks again for the review. All the points are addressed in v4; details
inline below.
> - On patch 1, prefer a simple check like:
> #define NTB_OPS_REQ(member) \
> if (hw->ntb_ops->member == NULL) { \
> NTB_LOG(ERR, "Vendor ops lacks mandatory ." # member); \
> return -EINVAL; \
> }
Done. The offsetof() table is gone; ntb_vendor_ops_check() is now just a
sequence of NTB_OPS_REQ(member) checks using your macro.
> - The change on alignment is not listed in the API changes section of
> the release notes.
> An update in the RN should highlight that an application must now
> rely on rte_pmd_ntb_get_mem_align() and not the mw_size_align field.
> Btw, why keep this field? It would be easier for a developer to
> notice a broken compilation. No strong opinion on this point.
Done, and I agree it is cleaner to drop it. The mw_size_align field is
removed from struct ntb_dev_info entirely, so stale code fails to compile
and is pointed at the new API. The API changes section of the release
notes now has an entry saying applications must use
rte_pmd_ntb_get_mem_align() instead of the removed field.
> - Even though the AMD implementation is added now, I see no reason to
> always list it after the Intel code.
> By this, I mean that you must alphabetically sort:
> - the files in meson.build,
> - when including the ntb_hw_amd.h header,
> - when adding the AMD PCI id in the driver mapping table,
> - when adding the PCI id in usertools/dpdk-devbind.py [...]
> - etc...
Done. AMD is now sorted before Intel in meson.build, the ntb_hw_*.h
includes, the pci_id_ntb_map table, the device_id switch in the probe
path, and the usertools/dpdk-devbind.py misc list.
> - The AMD topology definitions do not belong to patch 2, but patch 3.
Done. NTB_TOPO_PRI/NTB_TOPO_SEC are now added in patch 3 where they are
first used, not in the generalization patch.
> - The pmd_private field is unneeded, please remove:
> - amd_hw->self_mmio is just a copy of ntb->hw_addr,
> - all uses of amd_hw->peer_mmio involve an addition with an offset [...]
> - all uses of self_spad / peer_spad involve a check on the peer [...]
> - int_mask is an *unused* constant [...]
> - peer_status is unused,
> - ctl_status is unused,
Done. Both pmd_private (in struct ntb_hw) and the whole struct amd_ntb_hw
are removed:
- self_mmio -> ntb->hw_addr,
- peer_mmio -> AMD_PEER_MMIO(ntb) (RTE_PTR_ADD(hw_addr, AMD_PEER_OFFSET)),
- self_spad / peer_spad -> AMD_SPAD_BASE(ntb, peer), exactly as you
suggested,
- int_mask, peer_status and ctl_status were unused and are dropped.
The AMD driver no longer allocates any private data.
> - As I was reading the documentation, I noticed that you missed to
> update doc/guides/rawdevs/ntb.rst [...] Limitation [...]
> -This PMD is only supported on Intel Xeon Platforms:
> +This PMD is supported on:
> +- AMD EPYC Embedded "Turin", "Genoa" and "Siena" processors.
Done. The Limitation section now lists the supported AMD processors
alongside the Intel ones.
> - I also noticed strange indentation, so please double check the
> coding style guidelines.
> And, DPDK allows lines up to 100 characters.
Rechecked; checkpatches is now clean apart from the single
MACRO_WITH_FLOW_CONTROL warning - NTB_OPS_REQ macro body in a do { } while (0).
The series is freshly
rebased on main, each of the three commits builds on its own, and
check-git-log is clean.
Thanks,
Raghavendra
