linux-kernel - Re: [PATCH net-next v09 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: <20250716183208.26b87aa8@kernel.org>
Date: Wed, 16 Jul 2025 18:32:08 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Fan Gong <gongfan1@...wei.com>
Cc: Zhu Yikai <zhuyikai1@...artners.com>, <netdev@...r.kernel.org>,
 <linux-kernel@...r.kernel.org>, "David S. Miller" <davem@...emloft.net>,
 Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>, Simon
 Horman <horms@...nel.org>, Andrew Lunn <andrew+netdev@...n.ch>,
 <linux-doc@...r.kernel.org>, Jonathan Corbet <corbet@....net>, Bjorn
 Helgaas <helgaas@...nel.org>, luosifu <luosifu@...wei.com>, Xin Guo
 <guoxin09@...wei.com>, Shen Chenyang <shenchenyang1@...ilicon.com>, Zhou
 Shuai <zhoushuai28@...wei.com>, Wu Like <wulike1@...wei.com>, Shi Jing
 <shijing34@...wei.com>, Fu Guiming <fuguiming@...artners.com>, Meny Yossefi
 <meny.yossefi@...wei.com>, Gur Stavi <gur.stavi@...wei.com>, Lee Trager
 <lee@...ger.us>, Michael Ellerman <mpe@...erman.id.au>, Vadim Fedorenko
 <vadim.fedorenko@...ux.dev>, Suman Ghosh <sumang@...vell.com>, Przemek
 Kitszel <przemyslaw.kitszel@...el.com>, Joe Damato <jdamato@...tly.com>,
 Christophe JAILLET <christophe.jaillet@...adoo.fr>
Subject: Re: [PATCH net-next v09 1/8] hinic3: Async Event Queue interfaces

On Tue, 15 Jul 2025 08:28:36 +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 is a mandatory swap regardless of the
> + * CPU endianness.

This comment makes no sense, FWIW. The device writes a byte steam 
to host memory. For what you're saying to make sense the device would
have to intentionally switch the endian based on the host CPU.
And if it could do that why wouldn't it do it in the opposite
direction, avoiding the swap ? :/

I suppose the device is always writing in be32 words, and you should 
be converting from be32.

> +	/* Ensure handler can observe our intent to unregister. */
> +	mb();

What is "our intent"? I suppose you mean the change to the cb_state
bitfield? Please document the barriers explaining what two (or more)
memory accesses are separated. Not what they are achieving at the high
level.

> +	clear_bit(HINIC3_AEQ_CB_REG, cb_state);
> +	/* Ensure handler can observe our intent to unregister. */
> +	mb();
> +	while (test_bit(HINIC3_AEQ_CB_RUNNING, cb_state))
> +		usleep_range(HINIC3_EQ_USLEEP_LOW_BOUND,
> +			     HINIC3_EQ_USLEEP_HIGH_BOUND);

Please do not try to implement locks manually using bits ops.
Use standard synchronization primitives like wait queues or normal
locks, so that lockdep can help you validate your code is correct.

> +	val = EQ_CI_SIMPLE_INDIR_SET(arm_state, ARMED);
> +	val = val |
> +		EQ_CI_SIMPLE_INDIR_SET(eq_wrap_ci, CI) |
> +		EQ_CI_SIMPLE_INDIR_SET(eq->q_id, AEQ_IDX);

Why not:

	val = EQ_CI_SIMPLE_INDIR_SET(arm_state, ARMED) |
		EQ_CI_SIMPLE_INDIR_SET(eq_wrap_ci, CI) |
		EQ_CI_SIMPLE_INDIR_SET(eq->q_id, AEQ_IDX);
-- 
pw-bot: cr