Hello Andrew,
On Fri Mar 21, 2025 at 10:06 PM CET, Andrew Lunn wrote:
On Fri, Mar 21, 2025 at 08:09:38PM +0100, Théo Lebrun wrote:
quoted
The controller does IP alignment (two bytes).
I'm a bit confused here. Is this hard coded, baked into the silicon?
It will always do IP alignment? It cannot be turned off?
Yes, the alignment is baked inside the silicon.
I looked but haven't seen any register to configure the alignment.
Sorry the commit message isn't clear, it needs improvements.
quoted
skb_reserve(skb, NET_IP_ALIGN);
Why not just replace this with
skb_reserve(skb, 2);
On arm64, NET_IP_ALIGN=0. I don't have HW to test, but the current code
is telling us that the silicon doesn't do alignment on those:
skb = netdev_alloc_skb(...);
paddr = dma_map_single(..., skb->data, ...);
macb_set_addr(..., paddr);
// arm => NET_IP_ALIGN=2 => silicon does alignment
// arm64 => NET_IP_ALIGN=0 => silicon doesn't do alignment
skb_reserve(skb, NET_IP_ALIGN);
The platform we introduce is the first one where the silicon alignment
(0 bytes) is different from the NET_IP_ALIGN value (MIPS, 2 bytes).
Does that clarify things?
quoted
The NET_IP_ALIGN value is arch-dependent and picked based on unaligned
CPU access performance. The hardware alignment value should be
compatible-specific rather than arch-specific. Offer a path forward by
adding a hw_ip_align field inside macb_config.
Values for macb_config->hw_ip_align are picked based on upstream
devicetrees:
Compatible | DTS folders | hw_ip_align
------------------------|---------------------------|----------------
cdns,at91sam9260-macb | arch/arm/ | 2
cdns,macb | arch/{arm,riscv}/ | NET_IP_ALIGN
cdns,np4-macb | NULL | NET_IP_ALIGN
cdns,pc302-gem | NULL | NET_IP_ALIGN
cdns,gem | arch/{arm,arm64}/ | NET_IP_ALIGN
cdns,sam9x60-macb | arch/arm/ | 2
atmel,sama5d2-gem | arch/arm/ | 2
atmel,sama5d29-gem | arch/arm/ | 2
atmel,sama5d3-gem | arch/arm/ | 2
atmel,sama5d3-macb | arch/arm/ | 2
atmel,sama5d4-gem | arch/arm/ | 2
cdns,at91rm9200-emac | arch/arm/ | 2
cdns,emac | arch/arm/ | 2
cdns,zynqmp-gem | *same as xlnx,zynqmp-gem* | 0
cdns,zynq-gem | *same as xlnx,zynq-gem* | 2
sifive,fu540-c000-gem | arch/riscv/ | 2
microchip,mpfs-macb | arch/riscv/ | 2
microchip,sama7g5-gem | arch/arm/ | 2
microchip,sama7g5-emac | arch/arm/ | 2
xlnx,zynqmp-gem | arch/arm64/ | 0
xlnx,zynq-gem | arch/arm/ | 2
xlnx,versal-gem | NULL | NET_IP_ALIGN
I don't remember seeing any other driver doing anything like
this. That often means it is wrong....
Good question, let's look at skb_reserve() that follow dma_map_single():
⟩ git grep -A20 dma_map_single drivers/net/ethernet/ | \
rg skb_reserve | grep -v macb_main
drivers/net/ethernet/sun/sunbmac.c: skb_reserve(copy_skb, 2);
drivers/net/ethernet/sun/sunhme.c: skb_reserve(skb, RX_OFFSET);
drivers/net/ethernet/sun/sunhme.c: skb_reserve(new_skb, RX_OFFSET);
drivers/net/ethernet/sgi/ioc3-eth.c: skb_reserve(new_skb, RX_OFFSET);
drivers/net/ethernet/chelsio/cxgb/sge.c: skb_reserve(skb, sge->rx_pkt_pad);
drivers/net/ethernet/marvell/mv643xx_eth.c: skb_reserve(skb, 2);
drivers/net/ethernet/dec/tulip/de2104x.c: skb_reserve(copy_skb, RX_OFFSET);
drivers/net/ethernet/marvell/pxa168_eth.c: skb_reserve(skb, ETH_HW_IP_ALIGN);
drivers/net/ethernet/alacritech/slicoss.c: skb_reserve(skb, offset);
drivers/net/ethernet/toshiba/tc35815.c: skb_reserve(skb, 2); /* make IP header 4byte aligned */
drivers/net/ethernet/lantiq_etop.c: skb_reserve(ch->skb[ch->dma.desc], NET_IP_ALIGN);
Out of those, two are using dynamic values:
// In drivers/net/ethernet/chelsio/cxgb/sge.c
// The value comes from [0]:
sge->rx_pkt_pad = t1_is_T1B(adapter) ? 0 : 2;
// The macro resolves to something like [1]:
adap->params.chip_version == CHBT_TERM_T1 && adap->params.chip_revision == TERM_T1B
// In drivers/net/ethernet/alacritech/slicoss.c
// In slic_refill_rx_queue() [2]
/* ensure head buffer descriptors are 256 byte aligned */
offset = 0;
misalign = paddr & ALIGN_MASK;
if (misalign) {
offset = SLIC_RX_BUFF_ALIGN - misalign;
skb_reserve(skb, offset);
}
Conclusion:
- one is HW revision dependent,
- the other knows that HW always aligns its buffer to 256.
We aren't alone, but pretty lonely.
Maybe I missed a common denominator that could be used to identify
compatibles that do or do not have hardcoded alignemnt. Without such
info, the approach taken (have alignment stored inside match data)
sounds reasonable to me.
Do you agree?
Thanks,
[0]: https://elixir.bootlin.com/linux/v6.13.7/source/drivers/net/ethernet/chelsio/cxgb/sge.c#L2106
[1]: https://elixir.bootlin.com/linux/v6.13.7/source/drivers/net/ethernet/chelsio/cxgb/common.h#L292-L299
[2]: https://elixir.bootlin.com/linux/v6.13.7/source/drivers/net/ethernet/alacritech/slicoss.c#L418-L424
--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com