Re: [PATCH net-next v7 10/14] net: lan966x: add PCIe FDMA support

On Wed, Sep 23, 2026 at 10:45:38AM +0200, Daniel Machon wrote:

...

quoted

[Severity: Medium]
Is this path expected to work on a big-endian host?

Until now the descriptor producer and the FDMA engine were both on the
little-endian SoC, so the native-endian descriptor layout in
drivers/net/ethernet/microchip/fdma/fdma_api.h was fine:

  struct fdma_db {
        u64 dataptr;
        u64 status;
  };

  struct fdma_dcb {
        u64 nextptr;
        u64 info;
        struct fdma_db db[FDMA_DB_MAX];
  };

With PCIe FDMA the descriptors live in host memory and are fetched by the
endpoint, so the host and the engine can disagree on byte order. The
translated addresses written here, and the status/info words written by
__fdma_db_add()/__fdma_dcb_add(), all go out in host byte order, and the
HW-written DONE/BLOCKL fields are read back the same way in
lan966x_fdma_pci_get_next_dcb() and lan966x_fdma_pci_rx_check_frame().

CONFIG_MCHP_LAN966X_PCI in drivers/misc/Kconfig only has

  depends on PCI
  depends on OF_OVERLAY

so a big-endian host (e.g. powerpc) can select it. On such a host, would
the engine see byte-swapped nextptr/dataptr and length fields, i.e. would
RX/TX not work at all? Should the descriptor fields become explicit
little-endian types with accessors at the hardware boundary, or should the
Kconfig entry gain a little-endian dependency?

Hmm. Never tested on PowerPC, not this driver nor the DT overlay mechanism it
relies on for PCI deployment, which has only been exercised on ARM and x86 so
far (AFAICT) [1].

[1] https://lore.kernel.org/all/20240430183301.46568e35@bootlin.com/ (local)

I don't think that the baseline expectation is that drivers are tested
on big-endian systems, but rather that reasonable effort is made to ensure
that byte-order usage is correct.

In this case I suggest that will involve using __le64 instead of u64,
coupled with calling le64_to_cpu() and cpu_to_le64() as appropriate.

Sparse can aid with getting this right.

...

quoted

[Severity: Medium]
Is rx_bytes meant to count the frame as it arrived on the wire?

By the time this runs, skb->len has already been reduced twice: the
skb_trim() above drops ETH_FCS_LEN, and eth_type_trans() pulls the
MAC header out of the linear region, so skb->len is short by at least
ETH_HLEN. If lan966x_hw_offload() ends up untagging a VLAN header,
that is another four bytes gone. So every packet delivered through
lan966x_fdma_pci_rx_get_frame() undercounts rx_bytes by 14 bytes or
more, which is visible to userspace via ip -s link.

The frame length is available before any of that surgery happens --
data_len from FDMA_DCB_STATUS_BLOCKL(db->status), or skb->len right
after the skb_pull(skb, IFH_LEN_BYTES) -- so accounting could be done
there instead.

I realise this mirrors what the existing register/page path in
lan966x_fdma.c does, so if the intent is to keep the two backends
byte-for-byte consistent, please say so; otherwise it would be good
not to copy the miscount into the new file.

Not only lan966x, but sparx5 and lan969x does the exact same thing, increasing
rx_bytes after headers are pulled.  The undercount is real, but not visible to
userspace. Both implementations (platform and PCI) read hardware counters directly

I'm a little unsure, but if it's consistent then I guess that is ok.
It's an old interface anyway.