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

[Wei Fang]

> On Tue, Mar 11, 2025 at 01:38:17PM +0800, Wei Fang wrote:
> > +int ntmp_rsst_query_or_update_entry(struct netc_cbdrs *cbdrs, u32 *table,
> > +				    int count, bool query)
> > +{
> > +	struct ntmp_dma_buf data = {.dev = cbdrs->dma_dev};
> > +	struct rsst_req_update *requ;
> > +	struct ntmp_req_by_eid *req;
> > +	union netc_cbd cbd;
> > +	int err, i;
> > +	u32 len;
> > +
> > +	if (count != RSST_ENTRY_NUM)
> > +		/* HW only takes in a full 64 entry table */
> > +		return -EINVAL;
> > +
> > +	if (query)
> > +		data.size = NTMP_ENTRY_ID_SIZE + RSST_STSE_DATA_SIZE(count) +
> > +			    RSST_CFGE_DATA_SIZE(count);
> > +	else
> > +		data.size = struct_size(requ, groups, count);
> > +
> > +	err = ntmp_alloc_data_mem(&data, (void **)&req);
> > +	if (err)
> > +		return err;
> > +
> > +	/* Set the request data buffer */
> > +	if (query) {
> > +		ntmp_fill_crd_eid(req, cbdrs->tbl.rsst_ver, 0, 0, 0);
> > +		len = NTMP_LEN(sizeof(*req), data.size);
> > +		ntmp_fill_request_headr(&cbd, data.dma, len, NTMP_RSST_ID,
> > +					NTMP_CMD_QUERY, NTMP_AM_ENTRY_ID);
> 
> Please either use a commonly accepted abbreviation such as "hdr", or
> preferably,
> just spell "header" as such. This reminded me of Kevin Malone's quote
> "Why waste time say lot word when few word do trick?" :)
> 

Sure, I will fix it.

> > +	} else {
> > +		requ = (struct rsst_req_update *)req;
> > +		ntmp_fill_crd_eid(&requ->rbe, cbdrs->tbl.rsst_ver, 0,
> > +				  NTMP_GEN_UA_CFGEU | NTMP_GEN_UA_STSEU, 0);
> > +		for (i = 0; i < count; i++)
> > +			requ->groups[i] = (u8)(table[i]);
> > +
> > +		len = NTMP_LEN(data.size, 0);
> > +		ntmp_fill_request_headr(&cbd, data.dma, len, NTMP_RSST_ID,
> > +					NTMP_CMD_UPDATE, NTMP_AM_ENTRY_ID);
> > +	}
> > +
> > +	err = netc_xmit_ntmp_cmd(cbdrs, &cbd);
> > +	if (err) {
> > +		dev_err(cbdrs->dma_dev, "%s RSS table entry failed (%d)",
> > +			query ? "Query" : "Update", err);
> > +		goto end;
> > +	}
> > +
> > +	if (query) {
> > +		u8 *group = (u8 *)req;
> > +
> > +		group += NTMP_ENTRY_ID_SIZE + RSST_STSE_DATA_SIZE(count);
> > +		for (i = 0; i < count; i++)
> > +			table[i] = group[i];
> > +	}
> > +
> > +end:
> > +	ntmp_free_data_mem(&data);
> > +
> > +	return err;
> > +}
> > +EXPORT_SYMBOL_GPL(ntmp_rsst_query_or_update_entry);
> 
> Instead of exporting "query_or_update" mixed semantics, can you please
> export two separate functions, one for "query" and the other for "update"?
> For query=false, you can make the "table" argument const.
> 
> Also, from the looks of their implementation, there isn't much that is
> common anyway.
> 

Okay, accept, I will split it into two functions.

> > +static int ntmp_alloc_data_mem(struct ntmp_dma_buf *data, void
> **buf_align)
> > +{
> > +	void *buf;
> > +
> > +	buf = dma_alloc_coherent(data->dev, data->size +
> NTMP_DATA_ADDR_ALIGN,
> > +				 &data->dma, GFP_ATOMIC);
> 
> Is there any call site that can't use sleeping allocations (GFP_KERNEL)?
> 

The initial reason was that we called ntmp_maft_add_entry() directly when
implementing ndo_set_rx_mode(), so we changed it to GFP_ATOMIC. Later,
we implemented ndo_set_rx_mode() using workqueue, so for the current
patch, we can change it back to GFP_KERNEL.

> > +	if (!buf)
> > +		return -ENOMEM;
> > +
> > +	data->buf = buf;
> > +	*buf_align = PTR_ALIGN(buf, NTMP_DATA_ADDR_ALIGN);
> > +
> > +	return 0;
> > +}
> > +
> > diff --git a/drivers/net/ethernet/freescale/enetc/ntmp_private.h
> b/drivers/net/ethernet/freescale/enetc/ntmp_private.h
> > new file mode 100644
> > index 000000000000..45e4d083ab0a
> > --- /dev/null
> > +++ b/drivers/net/ethernet/freescale/enetc/ntmp_private.h
> > @@ -0,0 +1,63 @@
> > +/* SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause) */
> > +/*
> > + * NTMP table request and response data buffer formats
> > + * Copyright 2025 NXP
> > + */
> > +
> > +#ifndef __NTMP_PRIVATE_H
> > +#define __NTMP_PRIVATE_H
> > +
> > +#include <linux/fsl/ntmp.h>
> > +
> > +struct ntmp_dma_buf {
> > +	struct device *dev;
> > +	size_t size;
> > +	void *buf;
> > +	dma_addr_t dma;
> > +};
> > +
> > +struct common_req_data {
> 
> Some maintainers prefer to avoid definitions which "sound" generic, but truly
> are driver-specific, and instead recommend to prefix their names with
> some kind of driver specific indication
> (example:
> https://lore.kernel.org/netdev/20190413205311.GC2268@nanopsycho.orion/).
> 
> So, maybe something like "ntmp_common_req_data",
> "ntmp_common_resp_query", ...
> would make that more clear?
> 

I will rename these structs, thanks.

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

> 
> > +};
> > +
> > +struct common_resp_query {
> > +	__le32 entry_id;
> > +};
> > +
> > +struct common_resp_nq {
> > +	__le32 status;
> > +};
> > +
> > +/* Generic structure for request data by entry ID  */
> > +struct ntmp_req_by_eid {
> > +	struct common_req_data crd;
> > +	__le32 entry_id;
> > +};
> > +
> > +/* MAC Address Filter Table Request Data Buffer Format of Add action */
> > +struct maft_req_add {
> > +	struct ntmp_req_by_eid rbe;
> > +	struct maft_keye_data keye;
> > +	struct maft_cfge_data cfge;
> > +};
> > +
> > +/* MAC Address Filter Table Response Data Buffer Format of Query action */
> > +struct maft_resp_query {
> > +	__le32 entry_id;
> > +	struct maft_keye_data keye;
> > +	struct maft_cfge_data cfge;
> > +};
> > +
> > +/* RSS Table Request Data Buffer Format of Update action */
> > +struct rsst_req_update {
> > +	struct ntmp_req_by_eid rbe;
> > +	u8 groups[];
> > +};
> > +
> > +#endif
> > diff --git a/include/linux/fsl/ntmp.h b/include/linux/fsl/ntmp.h
> > new file mode 100644
> > index 000000000000..fe15e394c4a4
> > --- /dev/null
> > +++ b/include/linux/fsl/ntmp.h
> > @@ -0,0 +1,174 @@
> > +/* SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause) */
> > +/* Copyright 2025 NXP */
> > +#ifndef __NETC_NTMP_H
> > +#define __NETC_NTMP_H
> > +
> > +#include <linux/bitops.h>
> > +#include <linux/if_ether.h>
> > +
> > +#define NTMP_NULL_ENTRY_ID		0xffffffffU
> > +#define NETC_CBDR_BD_NUM		256
> > +
> > +union netc_cbd {
> 
> Do you seriously need to export the netc_cbd definition outside of
> drivers/net/ethernet/freescale/enetc/ntmp.c? I would say even if you do
> (which this patch set doesn't appear to need), the NTMP library exports
> an API which doesn't do a great job abstracting the information.

Sorry, I was not aware of the netc_cbd could be moved to ntmp_private.h
as well.

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

> 
> > +	struct {
> > +		__le64 addr;
> > +		__le32 len;
> > +#define NTMP_RESP_LEN		GENMASK(19, 0)
> > +#define NTMP_REQ_LEN		GENMASK(31, 20)
> > +#define NTMP_LEN(req, resp)	(FIELD_PREP(NTMP_REQ_LEN, (req)) | \
> > +				((resp) & NTMP_RESP_LEN))
> > +		u8 cmd;
> > +#define NTMP_CMD_DELETE		BIT(0)
> > +#define NTMP_CMD_UPDATE		BIT(1)
> > +#define NTMP_CMD_QUERY		BIT(2)
> > +#define NTMP_CMD_ADD		BIT(3)
> > +#define NTMP_CMD_QD		(NTMP_CMD_QUERY |
> NTMP_CMD_DELETE)
> > +#define NTMP_CMD_QU		(NTMP_CMD_QUERY |
> NTMP_CMD_UPDATE)
> > +#define NTMP_CMD_AU		(NTMP_CMD_ADD | NTMP_CMD_UPDATE)
> > +#define NTMP_CMD_AQ		(NTMP_CMD_ADD | NTMP_CMD_QUERY)
> > +#define NTMP_CMD_AQU		(NTMP_CMD_AQ | NTMP_CMD_UPDATE)
> > +		u8 access_method;
> > +#define NTMP_ACCESS_METHOD	GENMASK(7, 4)
> > +#define NTMP_AM_ENTRY_ID	0
> > +#define NTMP_AM_EXACT_KEY	1
> > +#define NTMP_AM_SEARCH		2
> > +#define NTMP_AM_TERNARY_KEY	3
> > +		u8 table_id;
> > +		u8 ver_cci_rr;
> > +#define NTMP_HDR_VERSION	GENMASK(5, 0)
> > +#define NTMP_HDR_VER2		2
> > +#define NTMP_CCI		BIT(6)
> > +#define NTMP_RR			BIT(7)
> > +		__le32 resv[3];
> > +		__le32 npf;
> > +#define NTMP_NPF		BIT(15)
> > +	} req_hdr;	/* NTMP Request Message Header Format */
> > +
> > +	struct {
> > +		__le32 resv0[3];
> > +		__le16 num_matched;
> > +		__le16 error_rr;
> > +#define NTMP_RESP_ERROR		GENMASK(11, 0)
> > +#define NTMP_RESP_RR		BIT(15)
> > +		__le32 resv1[4];
> > +	} resp_hdr; /* NTMP Response Message Header Format */
> > +};
> > +
> > +struct maft_keye_data {
> > +	u8 mac_addr[ETH_ALEN];
> > +	__le16 resv;
> > +};
> > +
> > +struct maft_cfge_data {
> > +	__le16 si_bitmap;
> > +	__le16 resv;
> > +};
> > +
> > +struct netc_cbdr_regs {
> > +	void __iomem *pir;
> > +	void __iomem *cir;
> > +	void __iomem *mr;
> > +
> > +	void __iomem *bar0;
> > +	void __iomem *bar1;
> > +	void __iomem *lenr;
> > +};
> > +
> > +struct netc_tbl_vers {
> > +	u8 maft_ver;
> > +	u8 rsst_ver;
> > +};
> > +
> > +struct netc_cbdr {
> > +	struct netc_cbdr_regs regs;
> > +
> > +	int bd_num;
> > +	int next_to_use;
> > +	int next_to_clean;
> > +
> > +	int dma_size;
> > +	void *addr_base;
> > +	void *addr_base_align;
> > +	dma_addr_t dma_base;
> > +	dma_addr_t dma_base_align;
> > +
> > +	spinlock_t ring_lock; /* Avoid race condition */
> 
> Can this description be more specific? This type of comment is as
> useful as not having it. Make the reader understand what is serialized
> with what, to prevent concurrent, non-atomic access to what resources.
> 
> > +};
> > +
> > +struct netc_cbdrs {
> > +	int cbdr_num;	/* number of control BD ring */
> > +	int cbdr_size;	/* number of BDs per control BD ring */
> > +	struct device *dma_dev;
> > +	struct netc_cbdr *ring;
> > +	struct netc_tbl_vers tbl;
> > +};
> > +
> > +enum netc_dev_type {
> > +	NETC_DEV_ENETC,
> > +	NETC_DEV_SWITCH
> > +};
> 
> Can you delay the introduction of this distinction until when the
> "dev_type" will actually be used for something, and it's clear to
> reviewers what is the intention behind it? Currently the switch driver
> does not exist, and this has no purpose.
> 

Okay, I will remove it.

> > +
> > +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);
};

> 
> I'm looking at this from an API perspective, and I don't really
> understand which one is the "top-level" object for an NTMP consumer
> driver. Is it ntmp_priv or netc_cbdrs? Logically, ntmp_priv encapsulates
> netc_cbdrs, but I see that all functions take the smaller netc_cbdrs,
> which I find unintuitive. Could you just perhaps squash them into a
> single structure, if they in fact serve the same purpose?
> 
> > +	enum netc_dev_type dev_type;
> > +	struct netc_cbdrs cbdrs;
> > +};
> > +
> > +struct maft_entry_data {
> > +	struct maft_keye_data keye;
> > +	struct maft_cfge_data cfge;
> > +};
> 
> > +static int ntmp_delete_entry_by_id(struct netc_cbdrs *cbdrs, int tbl_id,
> > +				   u8 tbl_ver, u32 entry_id, u32 req_len,
> > +				   u32 resp_len)
> > +{
> > +	struct ntmp_dma_buf data = {.dev = cbdrs->dma_dev};
> > +	struct ntmp_req_by_eid *req;
> > +	union netc_cbd cbd;
> > +	u32 len;
> > +	int err;
> > +
> > +	if (entry_id == NTMP_NULL_ENTRY_ID)
> > +		return 0;
> 
> 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.

> > +
> > +	/* If the req_len is 0, indicates the requested length is the
> > +	 * standard length.
> > +	 */
> > +	if (!req_len)
> > +		req_len = sizeof(*req);
> 
> Objection: as submitted in this patch set, the req_len argument is _only_
> passed as zero (the only caller is ntmp_maft_delete_entry()). I don't
> know about downstream, but let's only add complexity that we need, when
> we need it.
> 

Okay, I will remove it.

> > +
> > +	data.size = req_len >= resp_len ? req_len : resp_len;
> > +	err = ntmp_alloc_data_mem(&data, (void **)&req);
> > +	if (err)
> > +		return err;
> > +
> > +	ntmp_fill_crd_eid(req, tbl_ver, 0, 0, entry_id);
> > +	len = NTMP_LEN(req_len, resp_len);
> > +	ntmp_fill_request_headr(&cbd, data.dma, len, tbl_id,
> > +				NTMP_CMD_DELETE, NTMP_AM_ENTRY_ID);
> > +
> > +	err = netc_xmit_ntmp_cmd(cbdrs, &cbd);
> > +	if (err)
> > +		dev_err(cbdrs->dma_dev, "Delete table (id: %d) entry failed: %d",
> > +			tbl_id, err);
> > +
> > +	ntmp_free_data_mem(&data);
> > +
> > +	return err;
> > +}
> > +
> > +static int ntmp_query_entry_by_id(struct netc_cbdrs *cbdrs, int tbl_id,
> > +				  u32 len, struct ntmp_req_by_eid *req,
> > +				  dma_addr_t dma, bool compare_eid)
> > +{
> > +	struct device *dev = cbdrs->dma_dev;
> > +	struct common_resp_query *resp;
> > +	int cmd = NTMP_CMD_QUERY;
> > +	union netc_cbd cbd;
> > +	u32 entry_id;
> > +	int err;
> > +
> > +	entry_id = le32_to_cpu(req->entry_id);
> > +	if (le16_to_cpu(req->crd.update_act))
> > +		cmd = NTMP_CMD_QU;
> > +
> > +	/* Request header */
> > +	ntmp_fill_request_headr(&cbd, dma, len, tbl_id,
> > +				cmd, NTMP_AM_ENTRY_ID);
> > +
> > +	err = netc_xmit_ntmp_cmd(cbdrs, &cbd);
> > +	if (err) {
> > +		dev_err(dev, "Query table (id: %d) entry failed: %d\n",
> > +			tbl_id, err);
> > +		return err;
> > +	}
> > +
> > +	/* For a few tables, the first field of its response data is not
> 
> s/its/their/
> 
> > +	 * entry_id, so directly return success.
> > +	 */
> > +	if (!compare_eid)
> > +		return 0;
> > +
> > +	resp = (struct common_resp_query *)req;
> > +	if (unlikely(le32_to_cpu(resp->entry_id) != entry_id)) {
> > +		dev_err(dev, "Table (id: %d) query EID:0x%0x, response EID:0x%x\n",
> 
> Can you please put some spaces between ":" and "0".

Yes, sure.

> 
> > +			tbl_id, entry_id, le32_to_cpu(resp->entry_id));
> > +		return -EIO;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +int ntmp_maft_add_entry(struct netc_cbdrs *cbdrs, u32 entry_id,
> > +			struct maft_entry_data *maft)
> > +{
> > +	struct ntmp_dma_buf data = {.dev = cbdrs->dma_dev};
> > +	struct maft_req_add *req;
> > +	union netc_cbd cbd;
> > +	int err;
> > +
> > +	data.size = sizeof(*req);
> > +	err = ntmp_alloc_data_mem(&data, (void **)&req);
> > +	if (err)
> > +		return err;
> > +
> > +	/* Set mac address filter table request data buffer */
> > +	ntmp_fill_crd_eid(&req->rbe, cbdrs->tbl.maft_ver, 0, 0, entry_id);
> > +	req->keye = maft->keye;
> > +	req->cfge = maft->cfge;
> > +
> > +	ntmp_fill_request_headr(&cbd, data.dma, NTMP_LEN(data.size, 0),
> > +				NTMP_MAFT_ID, NTMP_CMD_ADD,
> > +				NTMP_AM_ENTRY_ID);
> > +	err = netc_xmit_ntmp_cmd(cbdrs, &cbd);
> > +	if (err)
> > +		dev_err(cbdrs->dma_dev, "Add MAFT entry failed (%d)", err);
> 
> Can you use symbolic error names? "Adding MAFT entry failed: %pe\n",
> ERR_PTR(err).
> Also notice the missing \n in the error message..

Okay, thanks.

> 
> Same comment for the error message in:
> - ntmp_delete_entry_by_id()
> - ntmp_rsst_query_or_update_entry() - which as per review feedback here
>   should become 2 functions
> 
> > +
> > +	ntmp_free_data_mem(&data);
> > +
> > +	return err;
> > +}
> > +EXPORT_SYMBOL_GPL(ntmp_maft_add_entry);
> > +
> > +int ntmp_maft_query_entry(struct netc_cbdrs *cbdrs, u32 entry_id,
> > +			  struct maft_entry_data *maft)
> > +{
> > +	struct ntmp_dma_buf data = {.dev = cbdrs->dma_dev};
> > +	struct maft_resp_query *resp;
> > +	struct ntmp_req_by_eid *req;
> > +	u32 req_len = sizeof(*req);
> > +	int err;
> > +
> > +	if (entry_id == NTMP_NULL_ENTRY_ID)
> > +		return -EINVAL;
> > +
> > +	data.size = sizeof(*resp);
> > +	err = ntmp_alloc_data_mem(&data, (void **)&req);
> > +	if (err)
> > +		return err;
> > +
> > +	ntmp_fill_crd_eid(req, cbdrs->tbl.maft_ver, 0, 0, entry_id);
> > +	err = ntmp_query_entry_by_id(cbdrs, NTMP_MAFT_ID,
> > +				     NTMP_LEN(req_len, data.size),
> > +				     req, data.dma, true);
> > +	if (err)
> > +		goto end;
> > +
> > +	resp = (struct maft_resp_query *)req;
> > +	maft->keye = resp->keye;
> > +	maft->cfge = resp->cfge;
> > +
> > +end:
> > +	ntmp_free_data_mem(&data);
> > +
> > +	return err;
> > +}
> > +EXPORT_SYMBOL_GPL(ntmp_maft_query_entry);
> > +
> > +int ntmp_maft_delete_entry(struct netc_cbdrs *cbdrs, u32 entry_id)
> > +{
> > +	return ntmp_delete_entry_by_id(cbdrs, NTMP_MAFT_ID,
> > +				       cbdrs->tbl.maft_ver,
> > +				       entry_id, 0, 0);
> > +}
> > +EXPORT_SYMBOL_GPL(ntmp_maft_delete_entry);
> 
> > +static void netc_clean_cbdr(struct netc_cbdr *cbdr)
> > +{
> > +	union netc_cbd *cbd;
> > +	int i;
> > +
> > +	i = cbdr->next_to_clean;
> > +	while (netc_read(cbdr->regs.cir) != i) {
> > +		cbd = netc_get_cbd(cbdr, i);
> > +		memset(cbd, 0, sizeof(*cbd));
> > +		i = (i + 1) % cbdr->bd_num;
> > +	}
> > +
> > +	cbdr->next_to_clean = i;
> > +}
> > +
> > +static struct netc_cbdr *netc_select_cbdr(struct netc_cbdrs *cbdrs)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < cbdrs->cbdr_num; i++) {
> > +		if (spin_is_locked(&cbdrs->ring[i].ring_lock))
> > +			continue;
> > +
> > +		return &cbdrs->ring[i];
> > +	}
> > +
> > +	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.

> Anyway, much can be said about this, but currently it is useless
> complexity, because the only user, enetc4_setup_cbdr(), sets
> "cbdrs->cbdr_num = 1", which side-steps the entire netc_select_cbdr()
> logic.
> 
> Please strip all unnecessary logic and only add it when the need
> presents itself, so we can all assess whether the solution is
> appropriate for that particular need.
> 

Okay, agree

> > +}
> > +
> > +static int netc_xmit_ntmp_cmd(struct netc_cbdrs *cbdrs, union netc_cbd
> *cbd)
> > +{
> > +	union netc_cbd *cur_cbd;
> > +	struct netc_cbdr *cbdr;
> > +	int i, err;
> > +	u16 status;
> > +	u32 val;
> > +
> > +	if (cbdrs->cbdr_num == 1)
> > +		cbdr = &cbdrs->ring[0];
> > +	else
> > +		cbdr = netc_select_cbdr(cbdrs);
> > +
> > +	spin_lock_bh(&cbdr->ring_lock);
> > +
> > +	if (unlikely(!netc_get_free_cbd_num(cbdr)))
> > +		netc_clean_cbdr(cbdr);
> > +
> > +	i = cbdr->next_to_use;
> > +	cur_cbd = netc_get_cbd(cbdr, i);
> > +	*cur_cbd = *cbd;
> > +
> > +	/* Update producer index of both software and hardware */
> > +	i = (i + 1) % cbdr->bd_num;
> > +	cbdr->next_to_use = i;
> > +	dma_wmb();
> 
> 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".

> > +	netc_write(cbdr->regs.pir, i);
> > +
> > +	err = read_poll_timeout_atomic(netc_read, val, val == i,
> > +				       10, NETC_CBDR_TIMEOUT, true,
> > +				       cbdr->regs.cir);
> > +	if (unlikely(err))
> > +		goto cbdr_unlock;
> > +
> > +	dma_rmb();
> > +	/* Get the writeback command BD, because the caller may need
> > +	 * to check some other fields of the response header.
> > +	 */
> > +	*cbd = *cur_cbd;
> > +
> > +	/* Check the writeback error status */
> > +	status = le16_to_cpu(cbd->resp_hdr.error_rr) & NTMP_RESP_ERROR;
> > +	if (unlikely(status)) {
> > +		err = -EIO;
> > +		dev_err(cbdrs->dma_dev, "Command BD error: 0x%04x\n", status);
> > +	}
> > +
> > +	netc_clean_cbdr(cbdr);
> > +	dma_wmb();
> > +
> > +cbdr_unlock:
> > +	spin_unlock_bh(&cbdr->ring_lock);
> > +
> > +	return err;
> > +}