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

[Gur Stavi]

> 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@xxxxxxxxxx/
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.