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

[Vladimir Oltean <olteanv@gmail.com>]

I see this is "Changes requested", so here are some more nitpicks from me.

On Fri, Apr 11, 2025 at 05:57:39PM +0800, Wei Fang wrote:
> Some NETC functionality is controlled using control messages sent to the
> hardware using BD ring interface with 32B descriptor similar to transmit
> BD ring used on ENETC. This BD ring interface is referred to as command
> BD ring. It is used to configure functionality where the underlying
> resources may be shared between different entities or being too large to
> configure using direct registers. Therefore, a messaging protocol called
> NETC Table Management Protocol (NTMP) is provided for exchanging
> configuration and management information between the software and the
> hardware using the command BD ring interface.
> 
> For i.MX95, NTMP has been upgraded to version 2.0, which is incompatible
> with LS1028A, because the message formats have been changed.

Can you please add one more sentence clarifying that the LS1028A
management protocol has been retroactively named NTMP 1.0 and its
implementation is in enetc_cbdr.c and enetc_tsn.c? The driver, like new
NETC documentation, refers to NTMP 2.0 as simply "NTMP".

> Therefore, add the netc-lib driver to support NTMP 2.0 to operate various tables.
> Note that, only MAC address filter table and RSS table are supported at
> the moment. More tables will be supported in subsequent patches.
> 
> It is worth mentioning that the purpose of the netc-lib driver is to
> provide some NTMP-based generic interfaces for ENETC and NETC Switch
> drivers. Currently, it only supports the configurations of some tables.
> Interfaces such as tc flower and debugfs will be added in the future.
> 
> Signed-off-by: Wei Fang <wei.fang@nxp.com>
> ---
> +static int netc_xmit_ntmp_cmd(struct ntmp_user *user, union netc_cbd *cbd)
> +{
> +	union netc_cbd *cur_cbd;
> +	struct netc_cbdr *cbdr;
> +	int i, err;
> +	u16 status;
> +	u32 val;
> +
> +	/* Currently only i.MX95 ENETC is supported, and it only has one
> +	 * command BD ring
> +	 */
> +	cbdr = &user->ring[0];
> +
> +	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;
> +	dma_wmb();
> +
> +	/* Update producer index of both software and hardware */
> +	i = (i + 1) % cbdr->bd_num;
> +	cbdr->next_to_use = i;
> +	netc_write(cbdr->regs.pir, i);
> +
> +	err = read_poll_timeout_atomic(netc_read, val, val == i,
> +				       10, NETC_CBDR_TIMEOUT, true,

Please create a #define for NETC_CBDR_SLEEP_US too.

> +				       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(user->dev, "Command BD error: 0x%04x\n", status);
> +	}
> +
> +	netc_clean_cbdr(cbdr);
> +	dma_wmb();
> +
> +cbdr_unlock:
> +	spin_unlock_bh(&cbdr->ring_lock);
> +
> +	return err;
> +}
> +
> +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_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	data->buf = buf;
> +	*buf_align = PTR_ALIGN(buf, NTMP_DATA_ADDR_ALIGN);
> +
> +	return 0;
> +}
> +
> +static void ntmp_free_data_mem(struct ntmp_dma_buf *data)
> +{
> +	dma_free_coherent(data->dev, data->size + NTMP_DATA_ADDR_ALIGN,
> +			  data->buf, data->dma);
> +}
> +
> +static void ntmp_fill_request_hdr(union netc_cbd *cbd, dma_addr_t dma,
> +				  int len, int table_id, int cmd,
> +				  int access_method)
> +{
> +	dma_addr_t dma_align;
> +
> +	memset(cbd, 0, sizeof(*cbd));
> +	dma_align = ALIGN(dma, NTMP_DATA_ADDR_ALIGN);
> +	cbd->req_hdr.addr = cpu_to_le64(dma_align);
> +	cbd->req_hdr.len = cpu_to_le32(len);
> +	cbd->req_hdr.cmd = cmd;
> +	cbd->req_hdr.access_method = FIELD_PREP(NTMP_ACCESS_METHOD,
> +						access_method);
> +	cbd->req_hdr.table_id = table_id;
> +	cbd->req_hdr.ver_cci_rr = FIELD_PREP(NTMP_HDR_VERSION,
> +					     NTMP_HDR_VER2);
> +	/* For NTMP version 2.0 or later version */
> +	cbd->req_hdr.npf = cpu_to_le32(NTMP_NPF);
> +}
> +
> +static void ntmp_fill_crd(struct ntmp_cmn_req_data *crd, u8 tblv,
> +			  u8 qa, u16 ua)
> +{
> +	crd->update_act = cpu_to_le16(ua);
> +	crd->tblv_qact = NTMP_TBLV_QACT(tblv, qa);
> +}
> +
> +static void ntmp_fill_crd_eid(struct ntmp_req_by_eid *rbe, u8 tblv,
> +			      u8 qa, u16 ua, u32 entry_id)
> +{
> +	ntmp_fill_crd(&rbe->crd, tblv, qa, ua);
> +	rbe->entry_id = cpu_to_le32(entry_id);
> +}
> +
> +static int ntmp_delete_entry_by_id(struct ntmp_user *user, int tbl_id,
> +				   u8 tbl_ver, u32 entry_id, u32 req_len,
> +				   u32 resp_len)
> +{
> +	struct ntmp_dma_buf data = {.dev = user->dev};
> +	struct ntmp_req_by_eid *req;
> +	union netc_cbd cbd;
> +	u32 len;
> +	int err;
> +
> +	data.size = req_len >= resp_len ? req_len : resp_len;

max(req_len, resp_len)

It can also be placed as part of the "data" initializer:

	struct ntmp_dma_buf data = {
		.dev = user->dev,
		.size = max(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_hdr(&cbd, data.dma, len, tbl_id,
> +			      NTMP_CMD_DELETE, NTMP_AM_ENTRY_ID);
> +
> +	err = netc_xmit_ntmp_cmd(user, &cbd);
> +	if (err)
> +		dev_err(user->dev, "Delete table (id: %d) entry failed: %pe",
> +			tbl_id, ERR_PTR(err));

Could you also print the entry_id?

> +
> +	ntmp_free_data_mem(&data);
> +
> +	return err;
> +}
> +
> +static int ntmp_query_entry_by_id(struct ntmp_user *user, int tbl_id,
> +				  u32 len, struct ntmp_req_by_eid *req,
> +				  dma_addr_t dma, bool compare_eid)
> +{
> +	struct device *dev = user->dev;
> +	struct ntmp_cmn_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_hdr(&cbd, dma, len, tbl_id, cmd, NTMP_AM_ENTRY_ID);
> +	err = netc_xmit_ntmp_cmd(user, &cbd);
> +	if (err) {
> +		dev_err(dev, "Query table (id: %d) entry failed: %pe\n",
> +			tbl_id, ERR_PTR(err));

Could you print a string representation of the tbl_id instead? It should
be more user-friendly if something fails.

static const char *ntmp_table_name(enum ntmp_tbl_id tbl_id)
{
	switch (tbl_id) {
	case NTMP_MAFT_ID:
		return "MAC Address Filtering";
	case NTMP_RSST_ID:
		return "RSS";
	default:
		return "unknown";
	}
}

Also, similar comment about entry_id being absent here.

> +		return err;
> +	}
> +
> +	/* For a few tables, the first field of their response data is not
> +	 * entry_id, so directly return success.
> +	 */
> +	if (!compare_eid)
> +		return 0;
> +
> +	resp = (struct ntmp_cmn_resp_query *)req;
> +	if (unlikely(le32_to_cpu(resp->entry_id) != entry_id)) {
> +		dev_err(dev, "Table (id: %d) query EID: 0x%x, response EID: 0x%x\n",
> +			tbl_id, entry_id, le32_to_cpu(resp->entry_id));
> +		return -EIO;
> +	}
> +
> +	return 0;
> +}
> +
> +int ntmp_maft_add_entry(struct ntmp_user *user, u32 entry_id,
> +			struct maft_entry_data *maft)
> +{
> +	struct ntmp_dma_buf data = {.dev = user->dev};
> +	struct maft_req_add *req;
> +	union netc_cbd cbd;
> +	int err;
> +
> +	data.size = sizeof(*req);

Same comment about wrapping this into the struct initializer.

> +	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, user->tbl.maft_ver, 0, 0, entry_id);
> +	req->keye = maft->keye;
> +	req->cfge = maft->cfge;
> +
> +	ntmp_fill_request_hdr(&cbd, data.dma, NTMP_LEN(data.size, 0),
> +			      NTMP_MAFT_ID, NTMP_CMD_ADD, NTMP_AM_ENTRY_ID);
> +	err = netc_xmit_ntmp_cmd(user, &cbd);
> +	if (err)
> +		dev_err(user->dev, "Add MAFT entry failed (%pe)\n",
> +			ERR_PTR(err));
> +
> +	ntmp_free_data_mem(&data);
> +
> +	return err;
> +}
> +EXPORT_SYMBOL_GPL(ntmp_maft_add_entry);
> +
> +int ntmp_maft_query_entry(struct ntmp_user *user, u32 entry_id,
> +			  struct maft_entry_data *maft)
> +{
> +	struct ntmp_dma_buf data = {.dev = user->dev};
> +	struct maft_resp_query *resp;
> +	struct ntmp_req_by_eid *req;
> +	int err;
> +
> +	data.size = sizeof(*resp);

Same comment about struct initializer.

> +	err = ntmp_alloc_data_mem(&data, (void **)&req);
> +	if (err)
> +		return err;
> +
> +	ntmp_fill_crd_eid(req, user->tbl.maft_ver, 0, 0, entry_id);
> +	err = ntmp_query_entry_by_id(user, NTMP_MAFT_ID,
> +				     NTMP_LEN(sizeof(*req), 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);