From: Paul Menzel <[email protected]>
Date: Sat, 10 Jan 2026 09:35:58 +0100
> Dear Mina,
>
>
> Thank you for your patch. Some minor comments, should you resend.
[...]
>> @@ -166,6 +173,19 @@ idpf_xdp_get_qw2(struct idpf_xdp_rx_desc *desc,
>> #endif
>> }
>> +static inline void
>> +idpf_xdp_get_qw3(struct idpf_xdp_rx_desc *desc,
>> + const struct virtchnl2_rx_flex_desc_adv_nic_3 *rxd)
>> +{
>> +#ifdef __LIBETH_WORD_ACCESS
>> + desc->qw3 = ((const typeof(desc))rxd)->qw3;
>> +#else
>> + desc->qw3 = ((u64)le32_to_cpu(rxd->ts_high) << 32) |
>> + ((u64)le16_to_cpu(rxd->fmd6) << 16) |
>> + le16_to_cpu(rxd->l2tag1);
>> +#endif
>
> It’s done elsewhere in the file, but I wonder why use the preprocessor
> and not plain C code, and let the linker(?) remove the unneeded branch?
Do you mean IS_ENABLED()?
In the libeth_xdp code, several paths won't build with IS_ENABLED(),
here in idpf it might work out, but I did it the same way as in
libeth_xdp for consistency.
>
>> +}
>> +
>> void idpf_xdp_set_features(const struct idpf_vport *vport);
>> int idpf_xdp(struct net_device *dev, struct netdev_bpf *xdp);
>
>
> Kind regards,
>
> Paul
Thanks,
Olek
