linux-kernel - 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] [day] [month] [year] [list]

Message-ID: <20250725152709.GE1367887@horms.kernel.org>
Date: Fri, 25 Jul 2025 16:27:09 +0100
From: Simon Horman <horms@...nel.org>
To: Fan Gong <gongfan1@...wei.com>
Cc: andrew+netdev@...n.ch, christophe.jaillet@...adoo.fr, corbet@....net,
	davem@...emloft.net, edumazet@...gle.com, fuguiming@...artners.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 24, 2025 at 09:45:51PM +0800, Fan Gong wrote:
> > > +
> > > +/* 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.
> > > + */
> >
> > This scheme may work on little endian hosts.
> > But if so it seems unlikely to work on big endian hosts.
> >
> > I expect you want be32_to_cpu_array() for data coming from hw,
> > with a source buffer as an array of __be32 while
> > the destination buffer is an array of u32.
> >
> > And cpu_to_be32_array() for data going to the hw,
> > with the types of the source and destination buffers reversed.
> >
> > If those types don't match your data, then we have
> > a framework to have that discussion.
> >
> >
> > That said, it is more usual for drivers to keep structures in the byte
> > order they are received. Stored in structures with members with types, in
> > this case it seems that would be __be32, and accessed using a combination
> > of BIT/GENMASK, FIELD_PREP/FIELD_GET, and cpu_to_be*/be*_to_cpu (in this
> > case cpu_to_be32/be32_to_cpu).
> > 
> > An advantage of this approach is that the byte order of
> > data is only changed when needed. Another is that it is clear
> > what the byte order of data is.
> 
> There is a simplified example:
> 
> Here is a 64 bit little endian that may appear in cmdq:
> __le64 x
> After the swap it will become:
> __be32 x_lo
> __be32 x_hi
> This is NOT __be64.
> __be64 looks like this:
> __be32 x_hi
> __be32 x_lo

Sure, byte swapping 64 bit entities is different to byte swapping two
consecutive 32 bit entities. I completely agree.

> 
> So the swapped data by HW is neither BE or LE. In this case, we should use
> swab32 to obtain the correct LE data because our driver currently supports LE.
> This is for compensating for bad HW decisions.

Let us assume that the host is reading data provided by HW.

If the swab32 approach works on a little endian host
to allow the host to access 32-bit values in host byte order.
Then this is because it outputs a 32-bit little endian values.

But, given the same input, it will not work on a big endian host.
This is because the same little endian output will be produced,
while the host byte order is big endian.

I think you need something based on be32_to_cpu()/cpu_to_be32().
This will effectively be swab32 on little endian hosts (no change!).
And a no-op on big endian hosts (addressing my point above).

More specifically, I think you should use be32_to_cpu_array() and
cpu_to_be32_array() instead of swab32_array().

> 
> > > +void hinic3_cmdq_buf_swab32(void *data, int len)
> > > +{
> > > +	u32 *mem = data;
> > > +	u32 i;
> > > +
> > > +	for (i = 0; i < len / sizeof(u32); i++)
> > > +		mem[i] = swab32(mem[i]);
> > > +}
> >
> > This seems to open code swab32_array().
> 
> We will use swab32_array in next patch.
> Besides, we will use LE for cmdq structs to avoid confusion and enhance
> readability.

Thanks.