Re: [PATCH v14 net-next 03/11] net/nebula-matrix: add chip related definitions

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net/nebula-matrix: add chip related definitions

This patch adds chip-related register definitions for the Leonis
hardware and introduces nbl_write_all_regs(), which programs a large
set of P4 configuration tables at chip initialization time.

> They also should not be moved into firmware files,
> as the software functionality is tightly coupled with these
> configurations.If they were moved to firmware,users could easily end up
> with mismatched versions of the firmware and the kernel driver module,
> leading to functional inconsistencies or system malfunctions.

Is it acceptable to bypass the standard request_firmware() API by embedding
these large configuration blobs directly in the C source?

Embedding large configuration blobs bloats the kernel image. Versioning
compatibility is typically handled through embedded firmware headers and
versioned file requests rather than static inclusion.

diff --git a/drivers/net/ethernet/nebula-matrix/nbl/nbl_hw/nbl_hw_leonis/nbl_hw_leonis.h b/drivers/net/ethernet/nebula-matrix/nbl/nbl_hw/nbl_hw_leonis/nbl_hw_leonis.h
--- a/drivers/net/ethernet/nebula-matrix/nbl/nbl_hw/nbl_hw_leonis/nbl_hw_leonis.h
+++ b/drivers/net/ethernet/nebula-matrix/nbl/nbl_hw/nbl_hw_leonis/nbl_hw_leonis.h

[ ... ]

> union nbl_mailbox_qinfo_map_table_u {
> 	struct nbl_mailbox_qinfo_map_table {
> 		u32 function:3;
> 		u32 devid:5;
> 		u32 bus:8;
> 		u32 msix_idx:13;
> 		u32 msix_idx_valid:1;
> 		u32 rsv:2;
> 	} __packed info;
> 	u32 data;
> };

Can the use of C bitfields for hardware register formats cause endianness
portability regressions?

The memory layout and bit-packing direction of C bitfields are
compiler-defined and vary by CPU architecture endianness. On big-endian
systems, writing the 32-bit data representation to an MMIO register might
transmit the bits in reverse block order compared to a little-endian system.

[ ... ]

> #define NBL_FUNC_MSIX_MAP_DWLEN		3
> union nbl_function_msix_map_u {
> 	struct nbl_function_msix_map {
> 		u64 msix_map_base_addr;
> 		u32 function:3;
> 		u32 devid:5;
> 		u32 bus:8;
> 		u32 valid:1;
> 		u32 rsv0:15;
> 		u32 rsv1;
> 	} info;
> 	u32 data[NBL_FUNC_MSIX_MAP_DWLEN];
> };

Could this size mismatch lead to incorrect hardware register array strides?

The struct contains an 8-byte u64, 4 bytes of bitfields, and a 4-byte u32,
making the unpacked struct 16 bytes. However, NBL_FUNC_MSIX_MAP_DWLEN is 3,
making the u32 array 12 bytes long.

Because a union's size is determined by its largest member, the sizeof
evaluates to 16 bytes. The macro NBL_PCOMPLETER_FUNCTION_MSIX_MAP_REG_ARR(i)
uses this sizeof to calculate hardware offsets, which might lead to writing
MSI-X configuration to the wrong PCIe offsets.

diff --git a/drivers/net/ethernet/nebula-matrix/nbl/nbl_hw/nbl_hw_leonis/nbl_hw_leonis_regs.c b/drivers/net/ethernet/nebula-matrix/nbl/nbl_hw/nbl_hw_leonis/nbl_hw_leonis_regs.c
--- a/drivers/net/ethernet/nebula-matrix/nbl/nbl_hw/nbl_hw_leonis/nbl_hw_leonis_regs.c
+++ b/drivers/net/ethernet/nebula-matrix/nbl/nbl_hw/nbl_hw_leonis/nbl_hw_leonis_regs.c

[ ... ]

> #define NBL_SEC021_SIZE			1
> #define NBL_SEC021_ADDR			0x8c1f8
> #define NBL_SEC022_SIZE			256
> #define NBL_SEC022_ADDR			0x85f000
> #define NBL_SEC022_REGI(i)		(0x85f000 + NBL_BYTES_IN_REG * (i))
> #define NBL_SEC023_SIZE			128
> #define NBL_SEC023_ADDR			0x85f800

Could this NBL_SEC022_SIZE macro introduce a regression by truncating the
hardware initialization data?

The macro is defined as 256, which means only the first 256 elements of the
NBL_SEC022 block are written. However, the corresponding data array
nbl_sec022_data contains 512 elements, and the address gap between
NBL_SEC022_ADDR (0x85f000) and NBL_SEC023_ADDR (0x85f800) is exactly 0x800
bytes, which fits exactly 512 u32 registers.