netdev - Re: [PATCH net-next v10 1/8] hinic3: Async Event Queue interfaces

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: <20250731183420.1138336-1-gur.stavi@huawei.com>
Date: Thu, 31 Jul 2025 21:34:20 +0300
From: Gur Stavi <gur.stavi@...wei.com>
To: <horms@...nel.org>
CC: <andrew+netdev@...n.ch>, <christophe.jaillet@...adoo.fr>,
	<corbet@....net>, <davem@...emloft.net>, <edumazet@...gle.com>,
	<fuguiming@...artners.com>, <gongfan1@...wei.com>, <guoxin09@...wei.com>,
	<gur.stavi@...wei.com>, <helgaas@...nel.org>, <jdamato@...tly.com>,
	<kuba@...nel.org>, <lee@...ger.us>, <linux-doc@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, <luosifu@...wei.com>,
	<meny.yossefi@...wei.com>, <mpe@...erman.id.au>, <netdev@...r.kernel.org>,
	<pabeni@...hat.com>, <przemyslaw.kitszel@...el.com>,
	<shenchenyang1@...ilicon.com>, <shijing34@...wei.com>, <sumang@...vell.com>,
	<vadim.fedorenko@...ux.dev>, <wulike1@...wei.com>, <zhoushuai28@...wei.com>,
	<zhuyikai1@...artners.com>
Subject: Re: [PATCH net-next v10 1/8] hinic3: Async Event Queue interfaces

> On Thu, Jul 31, 2025 at 03:58:39PM +0300, Gur Stavi wrote:
> >
> > Lets define a "coherent struct" as a structure made of fields that makes sense
> > to human beings. Every field endianity is defined and fields are arranged in
> > order that "makes sense". Fields can be of any integer size 8,16,32,64 and not
> > necessarily naturally aligned.
> >
> > swab32_array transforms a coherent struct into "byte jumble". Small fields are
> > reordered and larger (misaligned) fields may be split into 2 (or even 3) parts.
> > swab32_array is reversible so a 2nd call with byte jumble as input will produce
> > the original coherent struct.
> >
> > hinic3 dma has "swab32_array" built in.
> > On send-to-device it expects a byte jubmle so the DMA engine will transform it
> > into a coherent struct.
> > On receive-from-device it provides a byte jumble so the driver needs
> > to call swab32_array to transform it into a coherent struct.
> >
> > The hinic3_cmdq_buf_swab32 function will work correctly, producing byte jumble,
> > on little endian and big endian hosts.
> >
> > The code that runs prior to hinic3_cmdq_buf_swab32 that initializes a coherent
> > struct is endianity sensitive. It needs to initialize fields based on their
> > coherent endianity with or without byte swap. Practically use cpu_to_le or
> > cpu_to_be based on the coherent definition.
> >
> > Specifically, cmdq "coherent structs" in hinic3 use little endian and since
> > Kconfig currently declares that big endian hosts are not supported then
> > coherent structs are initialized without explicit cpu_to_le macros.
> >
> > And this is what the comment says:
> >
> > /* Data provided to/by cmdq is arranged in structs with little endian fields but
> >  * every dword (32bits) should be swapped since HW swaps it again when it
> >  * copies it from/to host memory.
> >  */
> >
>
> Thanks, I think I am closer to understanding things now.
>
> Let me try and express things in my own words:
>
> 1. On the hardware side, things are stored in a way that may be represented
>    as structures with little-endian values. The members of the structures may
>    have different sizes: 8-bit, 16-bit, 32-bit, ...
>
> 2. The hardware runs the equivalent of swab32_array() over this data
>    when writing it to (or reading it from) the host. So we get a
>    "byte jumble".
>
> 3. In this patch, the hinic3_cmdq_buf_swab32 reverses this jumbling
>    by running he equivalent of swab32_array() over this data again.
>
>    As 3 exactly reverses 2, what is left are structures exactly as in 1.
>

Yes. Your understanding matches mine.

> If so, I agree this makes sense and I am sorry for missing this before.
>
> And if so, is the intention for the cmdq "coherent structs" in the driver
> to look something like this.
>
>    struct {
> 	u8 a;
> 	u8 b;
> 	__le16 c;
> 	__le32 d;
>    };
>
> If so, this seems sensible to me.
>
> But I think it would be best so include some code in this patchset
> that makes use of such structures - sorry if it is there, I couldn't find
> it just now.
>
> And, although there is no intention for the driver to run on big endian
> systems, the __le* fields should be accessed using cpu_to_le*/le*_to_cpu
> helpers.

There was a long and somewhat heated debate about this issue.
https://lore.kernel.org/netdev/20241230192326.384fd21d@kernel.org/
I agree that having __le in the code is better coding practice.
But flooding the code with cpu_to_le and le_to_cpu does hurt readability.
And there are precedences of drivers that avoid it.

However, our dev team (I am mostly an advisor) decided to give it a try anyway.
I hope they manage to survive it.