linux-kernel - Re: [PATCH net-next 07/13] net: macb: move HW IP alignment value to macb

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <967fcb66-6a64-4e97-8293-a38b0ef1bc01@lunn.ch>
Date: Mon, 24 Mar 2025 19:36:06 +0100
From: Andrew Lunn <andrew@...n.ch>
To: Théo Lebrun <theo.lebrun@...tlin.com>
Cc: Andrew Lunn <andrew+netdev@...n.ch>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Nicolas Ferre <nicolas.ferre@...rochip.com>,
	Claudiu Beznea <claudiu.beznea@...on.dev>,
	Paul Walmsley <paul.walmsley@...ive.com>,
	Palmer Dabbelt <palmer@...belt.com>,
	Albert Ou <aou@...s.berkeley.edu>, Alexandre Ghiti <alex@...ti.fr>,
	Samuel Holland <samuel.holland@...ive.com>,
	Richard Cochran <richardcochran@...il.com>,
	Russell King <linux@...linux.org.uk>,
	Thomas Bogendoerfer <tsbogend@...ha.franken.de>,
	Vladimir Kondratiev <vladimir.kondratiev@...ileye.com>,
	Gregory CLEMENT <gregory.clement@...tlin.com>,
	netdev@...r.kernel.org, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-riscv@...ts.infradead.org,
	linux-mips@...r.kernel.org,
	Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
	Tawfik Bayouk <tawfik.bayouk@...ileye.com>
Subject: Re: [PATCH net-next 07/13] net: macb: move HW IP alignment value to
 macb_config

On Mon, Mar 24, 2025 at 06:49:05PM +0100, Théo Lebrun wrote:
> 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:
> >> 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.
> 
> >> 	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:

This is part of the confusion. You say the hardware does alignment,
and then say it does not....

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

This is starting to make it clearer. So the first statement that the
controller does IP alignment (two bytes) is not the full story. I
would start there, explain the full story, otherwise readers get the
wrong idea.

> >>     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'm not sure this table is useful. What might be more interesting is

     Compatible             |  architecture |  hw_ip_align | value of NET_IP_ALIGN

We can then see if there are cases when the 3rd and 4th column differ.

	Andrew