Re: [net-next,v24,2/6] eea: introduce ring and descriptor structures

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
eea: introduce ring and descriptor structures

This commit introduces the ring and descriptor implementations for the
Alibaba Elastic Ethernet Adapter (EEA). These structures and ring APIs
are used by the RX, TX, and admin queues.

> diff --git a/drivers/net/ethernet/alibaba/eea/Makefile b/drivers/net/ethernet/alibaba/eea/Makefile
> index cf2acf1733fd..e5e4007810a6 100644
> --- a/drivers/net/ethernet/alibaba/eea/Makefile
> +++ b/drivers/net/ethernet/alibaba/eea/Makefile
> @@ -1,3 +1,4 @@
>
>  obj-$(CONFIG_EEA) += eea.o
> -eea-y := eea_pci.o
> +eea-y :=  eea_ring.o \
> +	eea_pci.o

[ ... ]

> diff --git a/drivers/net/ethernet/alibaba/eea/eea_desc.h b/drivers/net/ethernet/alibaba/eea/eea_desc.h
> new file mode 100644
> index 000000000000..541346a03375
> --- /dev/null
> +++ b/drivers/net/ethernet/alibaba/eea/eea_desc.h
> @@ -0,0 +1,156 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Driver for Alibaba Elastic Ethernet Adapter.
> + *
> + * Copyright (C) 2025 Alibaba Inc.
> + */
> +
> +#ifndef __EEA_DESC_H__
> +#define __EEA_DESC_H__
> +
> +#define EEA_DESC_TS_MASK GENMASK(47, 0)
> +#define EEA_DESC_TS(desc) (le64_to_cpu((desc)->ts) & EEA_DESC_TS_MASK)
> +
> +struct eea_aq_desc {
> +	__le16 flags;
> +	__le16 id;
> +	__le16 reserved;
> +	u8 classid;
> +	u8 command;
> +	__le64 data_addr;
> +	__le64 reply_addr;
> +	__le32 data_len;
> +	__le32 reply_len;
> +};
> +
> +struct eea_aq_cdesc {
> +	__le16 flags;
> +	__le16 id;
> +#define EEA_OK     0
> +#define EEA_ERR    0xffffffff
> +	__le32 status;
> +	__le32 reply_len;
> +	__le32 reserved1;
> +
> +	__le64 reserved2;
> +	__le64 reserved3;
> +};

[ ... ]

> +struct eea_rx_desc {
> +	__le16 flags;
> +	__le16 id;
> +	__le16 len;
> +	__le16 reserved1;
> +
> +	__le64 addr;
> +
> +	__le64 hdr_addr;
> +	__le32 reserved2;
> +	__le32 reserved3;
> +};

[ ... ]

> +struct eea_rx_cdesc {
> +#define EEA_DESC_F_DATA_VALID	BIT(6)
> +#define EEA_DESC_F_SPLIT_HDR	BIT(5)
> +	__le16 flags;
> +	__le16 id;
> +	__le16 len;
> +#define EEA_NET_PT_NONE      0
> +#define EEA_NET_PT_IPv4      1
> +#define EEA_NET_PT_TCPv4     2
> +#define EEA_NET_PT_UDPv4     3
> +#define EEA_NET_PT_IPv6      4
> +#define EEA_NET_PT_TCPv6     5
> +#define EEA_NET_PT_UDPv6     6
> +#define EEA_NET_PT_IPv6_EX   7
> +#define EEA_NET_PT_TCPv6_EX  8
> +#define EEA_NET_PT_UDPv6_EX  9
> +	/* [9:0] is packet type. */
> +	__le16 type;
> +
> +	/* hw timestamp [0:47]: ts */
> +	__le64 ts;
> +
> +	__le32 hash;
> +
> +	/* 0-9: hdr_len  split header
> +	 * 10-15: reserved1
> +	 */
> +	__le16 len_ex;
> +	__le16 reserved2;
> +
> +	__le32 reserved3;
> +	__le32 reserved4;
> +};

[ ... ]

> +struct eea_tx_desc {
> +#define EEA_DESC_F_DO_CSUM	BIT(6)
> +	__le16 flags;
> +	__le16 id;
> +	__le16 len;
> +	__le16 reserved1;
> +
> +	__le64 addr;
> +
> +	__le16 csum_start;
> +	__le16 csum_offset;
> +	u8 gso_type;
> +	u8 reserved2;
> +	__le16 gso_size;
> +	__le64 reserved3;
> +};
> +
> +struct eea_tx_cdesc {
> +	__le16 flags;
> +	__le16 id;
> +	__le16 len;
> +	__le16 reserved1;
> +
> +	/* hw timestamp [0:47]: ts */
> +	__le64 ts;
> +	__le64 reserved2;
> +	__le64 reserved3;
> +};

[ ... ]

> +struct eea_db_direct {
> +	u8 kick_flags;
> +	u8 reserved;
> +	__le16 idx;
> +
> +	__le16 tx_cq_head;
> +	__le16 rx_cq_head;
> +
> +	u8 desc[24];
> +};
> +
> +static_assert(sizeof(struct eea_rx_desc) == 32, "rx desc size does not match");
> +static_assert(sizeof(struct eea_rx_cdesc) == 32,
> +	      "rx cdesc size does not match");
> +static_assert(sizeof(struct eea_tx_desc) == 32, "tx desc size does not match");
> +static_assert(sizeof(struct eea_tx_cdesc) == 32,
> +	      "tx cdesc size does not match");
> +static_assert(sizeof(struct eea_db_direct) == 32,
> +	      "db direct size does not match");
> +#endif

This isn't a bug, but should there be similar static_assert checks for
struct eea_aq_desc and struct eea_aq_cdesc? The other DMA descriptor
structures (eea_rx_desc, eea_rx_cdesc, eea_tx_desc, eea_tx_cdesc,
eea_db_direct) all have static_assert checks to verify their 32-byte
sizes. The admin queue descriptors are also 32-byte DMA structures with
hardware-defined layouts, so adding the checks would be consistent with
the pattern used for the other descriptors.

[ ... ]