Message-ID: <20250313163526.pqwp2wsfvio7avs6@skbuf>
Date: Thu, 13 Mar 2025 18:35:26 +0200
From: Vladimir Oltean <vladimir.oltean@....com>
To: Wei Fang <wei.fang@....com>
Cc: claudiu.manoil@....com, xiaoning.wang@....com, andrew+netdev@...n.ch,
	davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
	pabeni@...hat.com, christophe.leroy@...roup.eu,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
	imx@...ts.linux.dev, linuxppc-dev@...ts.ozlabs.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

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?" :)

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

> +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)?

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

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

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

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.

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

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

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?

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

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

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

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.

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.

> +}
> +
> +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?

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

Powered by blists - more mailing lists