Re: [PATCH v4 net-next 01/14] net: enetc: add initial netc-lib driver to support NTMP

[Vladimir Oltean]

On Fri, Mar 14, 2025 at 05:38:18AM +0200, Wei Fang wrote:
> > > +	__le16 update_act;
> > > +	u8 dbg_opt;
> > > +	u8 tblv_qact;
> > > +#define NTMP_QUERY_ACT		GENMASK(3, 0)
> > > +#define NTMP_TBL_VER		GENMASK(7, 0)
> > > +#define NTMP_TBLV_QACT(v, a)	(FIELD_PREP(NTMP_TBL_VER, (v)) | \
> > > +				 ((a) & NTMP_QUERY_ACT))
> > 
> > Can you please move #define macros out of structure definitions?
> 
> No, I think these macros in the structure can better reflect the specific
> meaning of these members. We can intuitively see what the bits of
> these members represent, rather than finding the definition of these
> bits in RM or elsewhere.

I mean, I was just suggesting to group the macros with the macros, and
the struct fields with the struct fields. Mixing them together looks a
bit messy to me. Even worse in the definition of "union netc_cbd" which
IMO needs to be reconsidered a bit (a hint given below).

But you mention "intuitive" definitions without having to go to the RM.
If I look just at the definitions, I see that NTMP_QUERY_ACT and NTMP_TBL_VER
overlap, and that NTMP_TBLV_QACT() just ORs them together. How does that work?
Shouldn't NTMP_TBL_VER() have been GENMASK(7, 4)? If I do open the RM
and go to the "Generic NTMP Request Data Buffer Format" section, that
table does seem to agree with me.

The "normal" way to give more meaning to struct fields is to define them
as enum values. That doesn't work with "packed" field definitions as you
have here, which I agree is a challenge. But it is one of the reasons
why I tried to develop an API together with Jacob Keller which tries to
address this, by allowing you to define the structures in an "unpacked"
format, and have a separate data structure which informs the CPU how
that structure maps over the buffer layout.

Below is an example to illustrate this for the case of NTMP request buffers.
Note that the example is incomplete and doesn't even compile, because I
haven't even converted all ntmp_fill_crd_eid() callers - I don't want to
waste too much time doing that in case you disagree, but it's still
enough to show what I'm talking about. Disclaimer: I didn't even _try_
to compile it - it may contain bugs.

-- >8 --
diff --git a/drivers/net/ethernet/freescale/enetc/Kconfig b/drivers/net/ethernet/freescale/enetc/Kconfig
index c8efbb6f2055..35d5cf21f6f4 100644
--- a/drivers/net/ethernet/freescale/enetc/Kconfig
+++ b/drivers/net/ethernet/freescale/enetc/Kconfig
@@ -17,6 +17,7 @@ config NXP_ENETC_PF_COMMON
 
 config NXP_NETC_LIB
 	tristate
+	select PACKING
 	help
 	  This module provides common functionalities for both ENETC and NETC
 	  Switch, such as NETC Table Management Protocol (NTMP) 2.0, common tc
diff --git a/drivers/net/ethernet/freescale/enetc/ntmp.c b/drivers/net/ethernet/freescale/enetc/ntmp.c
index df10f2f310c1..f96cfca92a1c 100644
--- a/drivers/net/ethernet/freescale/enetc/ntmp.c
+++ b/drivers/net/ethernet/freescale/enetc/ntmp.c
@@ -180,7 +180,7 @@ static int netc_xmit_ntmp_cmd(struct netc_cbdrs *cbdrs, union netc_cbd *cbd)
 	return err;
 }
 
-static int ntmp_alloc_data_mem(struct ntmp_dma_buf *data, void **buf_align)
+static int ntmp_alloc_data_mem(struct ntmp_dma_buf *data, ntmp_common_req_msg_data_buf *buf_align)
 {
 	void *buf;
 
@@ -221,18 +221,24 @@ static void ntmp_fill_request_headr(union netc_cbd *cbd, dma_addr_t dma,
 	cbd->req_hdr.npf = cpu_to_le32(NTMP_NPF);
 }
 
-static void ntmp_fill_crd(struct common_req_data *crd,
-			  u8 tblv, u8 qa, u16 ua)
+static const struct packed_field_u8 ntmp_common_req_fields[] = {
+	PACKED_FIELD(31, 28, struct ntmp_common_req_data, table_version),
+	PACKED_FIELD(27, 24, struct ntmp_common_req_data, query_actions),
+	PACKED_FIELD(15, 0, struct ntmp_common_req_data, update_actions),
+};
+
+static void ntmp_fill_crd(ntmp_common_req_msg_data_buf *buf,
+			  const struct common_req_data *crd)
 {
-	crd->update_act = cpu_to_le16(ua);
-	crd->tblv_qact = NTMP_TBLV_QACT(tblv, qa);
+	pack_fields(buf, sizeof(*buf), crd, ntmp_common_req_fields,
+		    QUIRK_LITTLE_ENDIAN);
 }
 
-static void ntmp_fill_crd_eid(struct ntmp_req_by_eid *rbe, u8 tblv,
-			      u8 qa, u16 ua, u32 entry_id)
+static void ntmp_fill_crd_eid(struct ntmp_common_req_msg_data_buf *buf,
+			      const struct ntmp_req_by_eid *rbe)
 {
-	ntmp_fill_crd(&rbe->crd, tblv, qa, ua);
-	rbe->entry_id = cpu_to_le32(entry_id);
+	ntmp_fill_crd(buf, &rbe->crd);
+	pack(buf + 1, rbe->entry_id, 31, 0);
 }
 
 static int ntmp_delete_entry_by_id(struct netc_cbdrs *cbdrs, int tbl_id,
@@ -240,7 +246,13 @@ static int ntmp_delete_entry_by_id(struct netc_cbdrs *cbdrs, int tbl_id,
 				   u32 resp_len)
 {
 	struct ntmp_dma_buf data = {.dev = cbdrs->dma_dev};
-	struct ntmp_req_by_eid *req;
+	struct ntmp_req_by_eid req = {
+		.crd = {
+			.table_version = tbl_ver,
+		},
+		.entry_id = entry_id,
+	};
+	ntmp_common_req_msg_data_buf buf;
 	union netc_cbd cbd;
 	u32 len;
 	int err;
@@ -255,7 +267,7 @@ static int ntmp_delete_entry_by_id(struct netc_cbdrs *cbdrs, int tbl_id,
 		req_len = sizeof(*req);
 
 	data.size = req_len >= resp_len ? req_len : resp_len;
-	err = ntmp_alloc_data_mem(&data, (void **)&req);
+	err = ntmp_alloc_data_mem(&data, &buf);
 	if (err)
 		return err;
 
diff --git a/drivers/net/ethernet/freescale/enetc/ntmp_private.h b/drivers/net/ethernet/freescale/enetc/ntmp_private.h
index 45e4d083ab0a..e68b2a060176 100644
--- a/drivers/net/ethernet/freescale/enetc/ntmp_private.h
+++ b/drivers/net/ethernet/freescale/enetc/ntmp_private.h
@@ -16,14 +16,14 @@ struct ntmp_dma_buf {
 	dma_addr_t dma;
 };
 
-struct common_req_data {
-	__le16 update_act;
-	u8 dbg_opt;
-	u8 tblv_qact;
-#define NTMP_QUERY_ACT		GENMASK(3, 0)
-#define NTMP_TBL_VER		GENMASK(7, 0)
-#define NTMP_TBLV_QACT(v, a)	(FIELD_PREP(NTMP_TBL_VER, (v)) | \
-				 ((a) & NTMP_QUERY_ACT))
+#define NTMP_COMMON_REQ_MSG_DATA_BUFSZ	4
+
+typedef struct __packed { u8 buf[NTMP_COMMON_REQ_MSG_DATA_BUFSZ]; } ntmp_common_req_msg_data_buf;
+
+struct ntmp_common_req_data {
+	u16 update_actions;
+	u8 query_actions;
+	u8 table_version;
 };
 
 struct common_resp_query {
@@ -36,8 +36,8 @@ struct common_resp_nq {
 
 /* Generic structure for request data by entry ID  */
 struct ntmp_req_by_eid {
-	struct common_req_data crd;
-	__le32 entry_id;
+	struct ntmp_common_req_data crd;
+	u32 entry_id;
 };
 
 /* MAC Address Filter Table Request Data Buffer Format of Add action */
-- >8 --

Some remarks:
 - The fact that struct ntmp_common_req_data is an unpacked structure
   now means that the layout of fields is completely decoupled from the
   layout of the data buffer. You can make each field an enum type if
   you want, which I didn't do here.
 - You don't have to access its fields with le32_to_cpu() and friends,
   it's implicitly in CPU endianness.
 - The API is tolerant of the case where the CPU is in a different
   endianness from ENETC.
 - The API protects you against overlapping PACKED_FIELD() definitions,
   and gives a compile-time error if it detects an overlap.
 - The API allows you to define buffers of any size you want, and the
   bit field ranges vary according to the buffer size you chose. Notice
   how you are grouping two fields within the "u8 tblv_qact" field, and
   their offsets are 7-4 and 3-0, respectively, within that u8, but in
   the PACKED_FIELD() definition you can make them 31-28 and 27-24, just
   like in the RM.
 - struct ntmp_req_by_eid and struct ntmp_common_req_data don't have to
   be distinct structures. You can call pack_fields() and pack only a
   subset of the fields of an unpacked structure. Then pack the rest
   with explicit pack() calls, or not pack them at all.

I just want to present this alternative because I believe it offers some
distinct advantages which may be useful here. But note that just
"QUIRK_LITTLE_ENDIAN" may be wrong for NTMP. You need to see whether you
also need QUIRK_LSW32_IS_FIRST or not. For buffers up to 32 bits,
QUIRK_LSW32_IS_FIRST makes no difference, only for buffers larger than that.

> > The question pertains to everything else that is exported to
> > include/linux/fsl/ntmp.h - what the API consumer sees. Is there a real
> > reason to export it? For many structures and macros, the answer seems no.
> > 
> > Even for cases like struct maft_keye_data which are only used by debugfs,
> > it still seems preferable to keep data encapsulation and offer a helper
> > function to retrieve a pointer to the MAC address from the MAFT entry.
> > Then, "struct maft_keye_data;" can simply be declared, without exposing
> > its full definition.
> 
> ntmp_private.h is only used for ntmp driver, I don't want it to be included
> by any enetc files. ntmp.h is used for both enetc and switch drivers, so we
> need to add full definitions of the table data.

And I agree with you, I also don't want ntmp_private.h to be exposed to
the NTMP API consumers, and I wasn't suggesting that. I just want the
NTMP API to spare consumer drivers of the gory details, like for example
the buffer layout of a MAC filtering entry, reserved fields, things like
that. What I was saying is to keep the buffer layout private to
ntmp_private.h, and expose a more abstract data structure to API
consumers.

> > > +struct ntmp_priv {
> > 
> > Would it be better to name this "struct ntmp_client"? I don't really
> > understand the way in which it is "private".
> 
> It refers to some private data of NTMP of different devices (enetc or
> switches). Since the current patch is only a small part of NTMP, many
> members have not been added to the structure. The following is the
> definition in our downstream.
> 
> struct ntmp_priv {
> 	enum netc_dev_type dev_type;
> 	struct netc_cbdrs cbdrs;
> 	u32 errata;
> 
> 	struct ntmp_caps caps;
> 	/* bitmap of table entry ID */
> 	unsigned long *ist_eid_bitmap;
> 	unsigned long *rpt_eid_bitmap;
> 	unsigned long *sgit_eid_bitmap;
> 	unsigned long *isct_eid_bitmap;
> 	unsigned long *sgclt_word_bitmap;
> 	unsigned long *ett_eid_bitmap;
> 	unsigned long *ect_eid_bitmap;
> 	u32 ett_bitmap_size;
> 	u32 ect_bitmap_size;
> 
> 	struct hlist_head flower_list;
> 	struct mutex flower_lock; /* flower_list lock */
> 
> 	u64 (*adjust_base_time)(struct ntmp_priv *priv, u64 bt, u32 ct);
> 	u32 (*get_tgst_free_words)(struct ntmp_priv *priv);
> };

Thank you for posting the downstream layout of struct ntmp_priv which I
was already aware of. What I was saying is that the word "private" means
an aspect of the implementation which is hidden from other code layers.
There's nothing "private" here if the NTMP layer has access to the
definition of this structure. I was asking whether you agree that it's
more adequate to name this structure "ntmp_client", since it represents
the data associated with a consumer of the NTMP API - a NETC SI or (in
the future) the switch. Or some other name. But the word "private" seems
misused.

> > What's the idea with the null entry ID? Why special-case it?
> > 
> 
> Some functions are configured by multiple tables. If a table is
> not needed in the current configuration, we may set its entry
> id to NTMP_NULL_ENTRY_ID, indicating that the table is bypassed.
> For the current patch, this condition can be removed.

Ok, thank you.

> > > +	return &cbdrs->ring[smp_processor_id() % cbdrs->cbdr_num];
> > 
> > I think you need to be in a "preemption disabled" / "migration disable"
> > calling context for smp_processor_id() to be reliable. Otherwise, the
> > task can migrate to another CPU as soon as this function returns.
> > 
> 
> It does not matter, we just want to select a command BD ring when all
> command BD rings are busy. So smp_processor_id() is just a parameter,
> we can also use a random number.

Ok.

> > Can you place this dma_wmb() right next to the "*cur_cbd = *cbd" line,
> > to make it obvious that updating the producer index has nothing to do
> > with it? Or is there another reason for this ordering?
> > 
> 
> No special reason for this ordering, I will move it after "*cur_cbd = *cbd".

Thanks, I appreciate it.