Message-ID:
 <PAXPR04MB851027E5F830F08F3395083888D22@PAXPR04MB8510.eurprd04.prod.outlook.com>
Date: Fri, 14 Mar 2025 13:48:21 +0000
From: Wei Fang <wei.fang@....com>
To: Vladimir Oltean <vladimir.oltean@....com>
CC: Claudiu Manoil <claudiu.manoil@....com>, Clark Wang
	<xiaoning.wang@....com>, "andrew+netdev@...n.ch" <andrew+netdev@...n.ch>,
	"davem@...emloft.net" <davem@...emloft.net>, "edumazet@...gle.com"
	<edumazet@...gle.com>, "kuba@...nel.org" <kuba@...nel.org>,
	"pabeni@...hat.com" <pabeni@...hat.com>, "christophe.leroy@...roup.eu"
	<christophe.leroy@...roup.eu>, "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "imx@...ts.linux.dev" <imx@...ts.linux.dev>,
	"linuxppc-dev@...ts.ozlabs.org" <linuxppc-dev@...ts.ozlabs.org>,
	"linux-arm-kernel@...ts.infradead.org" <linux-arm-kernel@...ts.infradead.org>
Subject: RE: [PATCH v4 net-next 01/14] net: enetc: add initial netc-lib driver
 to support NTMP



Best Regards,
Wei Fang

> -----Original Message-----
> From: Vladimir Oltean <vladimir.oltean@....com>
> 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).

I think this is a matter of personal preference. I was inspired by some
of Intel's structure definitions. I think this method allows me to quickly
understand what fields the member consists of and what bits each field
occupies.

> 
> 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

I must admit that it is my fault, so yes, NTMP_TBL_VER should be GENMASK(7, 4).
I will fix it, thanks for pointing this.

> 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.
> 

Thanks, but we have added fully NTMP support in downstream, it's a great
challenge for me to convert it to the 'packing' method. I don't think I have
too much time to do this conversion. And I also need some time to figure
out how to use it and whether it is worth doing so.

> > > 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.
> 

Sorry, I don't fully understand, for example, if we place the definition
of "struct maft_keye_data" in ntmp_private.h, how does the debugfs
get the information from "struct maft_keye_data"? Add a helper function
in the NTMP driver and export it to enetc driver? And how does
enetc4_pf_add_si_mac_exact_filter() to set the mac address to "struct
maft_keye_data", add another helper? If so, I think it is more complicated.

> > > > +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.
> 

Okay, it seems to make you feel confused, let me rename it later, how about
"ntmp_user"?

> > > 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.

Powered by blists - more mailing lists