netdev - Re: [PATCH net-next v7 6/9] ice: use <linux/packing.h> for Tx and Rx queue context data

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: <dfdf794b-01f2-4a61-a208-0907e410b9c1@intel.com>
Date: Tue, 3 Dec 2024 10:01:04 -0800
From: Jacob Keller <jacob.e.keller@...el.com>
To: Vladimir Oltean <olteanv@...il.com>
CC: Andrew Morton <akpm@...ux-foundation.org>, Eric Dumazet
	<edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni
	<pabeni@...hat.com>, Tony Nguyen <anthony.l.nguyen@...el.com>, "Przemek
 Kitszel" <przemyslaw.kitszel@...el.com>, Masahiro Yamada
	<masahiroy@...nel.org>, netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next v7 6/9] ice: use <linux/packing.h> for Tx and Rx
 queue context data



On 12/3/2024 5:43 AM, Vladimir Oltean wrote:
> On Mon, Dec 02, 2024 at 04:26:29PM -0800, Jacob Keller wrote:
>> +/**
>> + * ice_pack_rxq_ctx - Pack Rx queue context into a HW buffer
>> + * @ctx: the Rx queue context to pack
>> + * @buf: the HW buffer to pack into
>> + *
>> + * Pack the Rx queue context from the CPU-friendly unpacked buffer into its
>> + * bit-packed HW layout.
>> + */
>> +static void ice_pack_rxq_ctx(const struct ice_rlan_ctx *ctx,
>> +			     ice_rxq_ctx_buf_t *buf)
>> +{
>> +	pack_fields(buf, sizeof(*buf), ctx, ice_rlan_ctx_fields,
>> +		    QUIRK_LITTLE_ENDIAN | QUIRK_LSW32_IS_FIRST);
> 
> An alternative pack_fields() design would enforce that the pbuf argument
> has a sizeof() which reveals the packed buffer size. Pro: one macro
> argument less. Con: too intrusive in forcing authors to write code in a
> certain way maybe?
> 

Yea, I had it that way in an earlier version, but I ultimately felt like
the resulting macro was less friendly, as existing code may not have
structured types with the appropriate sizes.

We still have to enforce that the size value is a constant (to get the
compile time checks) which I guess has its own downsides over forcing it
to be sizeof().

I think I'd prefer to keep the simplicity of the 5 argument style that
does not require forcing your buffers to be structures.

>> +}