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

[Simon Horman <horms@kernel.org>]

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

...

> > [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/

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.

...

> > [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.