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

[Jacob Keller <jacob.e.keller@intel.com>]

On 10/29/2024 7:50 AM, Daniel Machon wrote:
> Hi Jacob,

>> +/**
>> + * 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)
>> +{
>> +       CHECK_PACKED_FIELDS_20(ice_rlan_ctx_fields, ICE_RXQ_CTX_SZ);
>> +       BUILD_BUG_ON(sizeof(*buf) != ICE_RXQ_CTX_SZ);
>> +
>> +       pack_fields(buf, sizeof(*buf), ctx, ice_rlan_ctx_fields,
>> +                   QUIRK_LITTLE_ENDIAN | QUIRK_LSW32_IS_FIRST);
>> +}
>> +
> 
> FWIW, I noticed that smatch bails out checking all the CHECK_PACKED_FIELDS_*
> variants >= 20, with the warning:
> 
> ice_common.c:1486 ice_pack_txq_ctx() parse error: OOM: 3000148Kb sm_state_count = 413556
> ice_common.c:1486 ice_pack_txq_ctx() warn: Function too hairy.  No more merges.
> ice_common.c:1486 ice_pack_txq_ctx() parse error: Function too hairy.  Giving up. 43 second
> 

We might need to wrap these checks to become no-ops when running under
such a checker. It looks like the parser doesn't like the size of the
macros?

> Maybe this can just be ignored .. not sure :-)
> 

I would prefer if we found a way to at least silence it, rather than
straight up ignore it.

I am not that familiar with smatch. Let me see if there's an obvious way
we can handle this.

>> +/**
>> + * ice_pack_txq_ctx - Pack Tx queue context into a HW buffer
>> + * @ctx: the Tx queue context to pack
>> + * @buf: the HW buffer to pack into
>> + *
>> + * Pack the Tx queue context from the CPU-friendly unpacked buffer into its
>> + * bit-packed HW layout.
>> + */
>> +void ice_pack_txq_ctx(const struct ice_tlan_ctx *ctx, ice_txq_ctx_buf_t *buf)
>> +{
>> +       CHECK_PACKED_FIELDS_27(ice_tlan_ctx_fields, ICE_TXQ_CTX_SZ);
>> +       BUILD_BUG_ON(sizeof(*buf) != ICE_TXQ_CTX_SZ);
>> +
>> +       pack_fields(buf, sizeof(*buf), ctx, ice_tlan_ctx_fields,
>> +                   QUIRK_LITTLE_ENDIAN | QUIRK_LSW32_IS_FIRST);
>> +}
>> +
> 
> Same here with the 27 variant.
>

Yea, same problem of course. The generated checks are too large to be
parsed by smatch.


<snip removed code>
> 
> Some nice cleanup!
> 

Yea. I got motivated with this because I was looking at introducing the
inverse 'unpacking' variants for supporting VF live migration, and
realized how ugly the existing code was and how much worse adding yet
more code to unpack would be.

> 
> /Daniel

Thanks for the careful review!