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