netdev - Re: [PATCH iwl-next v10 09/14] libeth: move idpf_rx_csum_decoded and idpf_rx

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: <6d67f88f-8240-4564-b98c-5bcc957fc589@intel.com>
Date: Wed, 21 Aug 2024 17:03:49 +0200
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: Wojciech Drewek <wojciech.drewek@...el.com>
CC: <netdev@...r.kernel.org>, <intel-wired-lan@...ts.osuosl.org>,
	<horms@...nel.org>, <anthony.l.nguyen@...el.com>, <kuba@...nel.org>,
	<alexandr.lobakin@...el.com>
Subject: Re: [PATCH iwl-next v10 09/14] libeth: move idpf_rx_csum_decoded and
 idpf_rx_extracted

From: Wojciech Drewek <wojciech.drewek@...el.com>
Date: Wed, 21 Aug 2024 14:15:34 +0200

> From: Mateusz Polchlopek <mateusz.polchlopek@...el.com>
> 
> Structs idpf_rx_csum_decoded and idpf_rx_extracted are used both in
> idpf and iavf Intel drivers. This commit changes the prefix from

Imperative. Like "change the prefix and move mentioned structs to...".

> idpf_* to libeth_* and moves mentioned structs to libeth's rx.h header
> file.
> 
> Usage in idpf driver has been adjusted.

[...]

> diff --git a/include/net/libeth/rx.h b/include/net/libeth/rx.h
> index 43574bd6612f..03e371633f4f 100644
> --- a/include/net/libeth/rx.h
> +++ b/include/net/libeth/rx.h
> @@ -198,6 +198,48 @@ struct libeth_rx_pt {
>  	enum xdp_rss_hash_type			hash_type:16;
>  };
>  
> +/**
> + * struct libeth_rx_csum - Checksum offload bits decoded from the Rx descriptor

Lowercase after '-' to be consistent with the rest.

 * struct libeth_rx_csum - checksum offload bits...

> + * @l3l4p: detectable L3 and L4 integrity check is processed by the hardware
> + * @ipe: IP checksum error
> + * @eipe: external (outermost) IP header (only for tunels)
> + * @eudpe: external (outermost) UDP checksum error (only for tunels)
> + * @ipv6exadd: IPv6 header with extension headers
> + * @l4e: L4 integrity error
> + * @pprs: set for packets that skip checksum calculation in the HW pre parser
> + * @nat: the packet is a UDP tunneled packet
> + * @raw_csum_valid: set if raw checksum is valid
> + * @pad: padding to naturally align raw_csum field
> + * @raw_csum: raw checksum
> + */
> +struct libeth_rx_csum {
> +	u32					l3l4p:1;
> +	u32					ipe:1;
> +	u32					eipe:1;
> +	u32					eudpe:1;
> +	u32					ipv6exadd:1;
> +	u32					l4e:1;
> +	u32					pprs:1;
> +	u32					nat:1;

I'd put an empty newline here after nat.

> +	u32					raw_csum_valid:1;
> +	u32					pad:7;
> +	u32					raw_csum:16;
> +};
> +
> +/**
> + * struct libeth_rqe_info - receive queue element info
> + * @len: packet length
> + * @vlan_tag: vlan tag

Again, which tag is this? C-VLAN, S-VLAN? If it depends on the
configuration, then I'd mention it accordingly, like (note that I'd
shorten the name to just 'vlan')

 * @vlan: C-VLAN or S-VLAN tag depending on the VLAN offload configuration
("VLAN", not "vlan" BTW)

> + * @eop: end of packet

I'd expand this a bit.

 * @eop: whether it's the last fragment of the packet

> + * @rxe: MAC errors: CRC, Alignment, Oversize, Undersizes, Length error
> + */
> +struct libeth_rqe_info {
> +	u32					len;
> +	u32					vlan_tag:16;
> +	u32					eop:1;
> +	u32					rxe:1;
> +};

Ok, sorry for confusing, but now after I read the changes above I really
think it would be better to add ptype here though =\

struct libeth_rqe_info {
	u32					len;

	u32					ptype:14;
	u32					eop:1;
	u32					rxe:1;

	u32					vlan_tag:16;
}

(+ reorder and empty newlines as suggested)

> +
>  void libeth_rx_pt_gen_hash_type(struct libeth_rx_pt *pt);
>  
>  /**

Thanks,
Olek