Message-ID: <0cf7fbc3-acc3-4178-bc3b-bd35cde1cafb@intel.com>
Date: Thu, 16 Jan 2025 11:30:22 +0100
From: Przemek Kitszel <przemyslaw.kitszel@...el.com>
To: Xin Tian <tianx@...silicon.com>
CC: <leon@...nel.org>, <andrew+netdev@...n.ch>, <kuba@...nel.org>,
	<pabeni@...hat.com>, <edumazet@...gle.com>, <davem@...emloft.net>,
	<jeff.johnson@....qualcomm.com>, <weihg@...silicon.com>,
	<wanry@...silicon.com>, <netdev@...r.kernel.org>
Subject: Re: [PATCH v3 02/14] net-next/yunsilicon: Enable CMDQ

On 1/15/25 11:22, Xin Tian wrote:
> Enable cmd queue to support driver-firmware communication.
> Hardware control will be performed through cmdq mostly.
> 

I didn't checked, but plese don't include anything that would not
be used at the end of the series. IOW: no dead code.

 > +	XSC_HW_THIRD_FEATURE			= BIT(2),
really? :D

[..]


> +	__be64		complete_reg;
> +	__be64		event_db;
> +	__be32		qp_rate_limit_min;
> +	__be32		qp_rate_limit_max;
> +	struct xsc_fw_version  fw_ver;
> +	u8	lag_logic_port_ofst;
> +};
> +
> +struct xsc_cmd_query_hca_cap_mbox_in {
> +	struct xsc_inbox_hdr	hdr;
> +	__be16			cpu_num;
> +	u8			rsvd[6];
> +};
> +
> +struct xsc_cmd_query_hca_cap_mbox_out {
> +	struct xsc_outbox_hdr	hdr;
> +	u8			rsvd0[8];
> +	struct xsc_hca_cap	hca_cap;
> +};
> +
> +struct xsc_query_vport_state_out {
> +	struct xsc_outbox_hdr	hdr;

this is BE

> +	u8			admin_state:4;
> +	u8			state:4;
> +};
> +
> +struct xsc_query_vport_state_in {
> +	struct xsc_inbox_hdr	hdr;

this is BE

> +	u32			other_vport:1;
> +	u32			vport_number:16;
> +	u32			rsvd0:15;

and this is CPU order, why?

> +};
> +
> +enum {
> +	XSC_CMD_EVENT_RESP_CHANGE_LINK		= BIT(0),
> +	XSC_CMD_EVENT_RESP_TEMP_WARN		= BIT(1),
> +	XSC_CMD_EVENT_RESP_OVER_TEMP_PROTECTION	= BIT(2)

(always add comma at the end, unless the entry is supposed
to be last forever)

> +};
> +
> +struct xsc_event_resp {
> +	u8			resp_cmd_type;
> +};
> +
> +struct xsc_event_query_type_mbox_in {
> +	struct xsc_inbox_hdr	hdr;
> +	u8			rsvd[2];
> +};
> +
> +struct xsc_event_query_type_mbox_out {
> +	struct xsc_outbox_hdr	hdr;
> +	struct xsc_event_resp	ctx;
> +};
> +
> +struct xsc_modify_raw_qp_request {
> +	u16		qpn;

hard to tell why you have switched from BE to CPU order
at this point

> +	u16		lag_id;
> +	u16		func_id;
> +	u8		dma_direct;
> +	u8		prio;
> +	u8		qp_out_port;
> +	u8		rsvd[7];
> +};
> +
> +struct xsc_modify_raw_qp_mbox_in {
> +	struct xsc_inbox_hdr	hdr;
> +	u8			pcie_no;
> +	u8			rsvd[7];
> +	struct xsc_modify_raw_qp_request	req;
> +};
> +
> +struct xsc_modify_raw_qp_mbox_out {
> +	struct xsc_outbox_hdr	hdr;
> +	u8			rsvd[8];
> +};
> +
> +struct xsc_set_mtu_mbox_in {
> +	struct xsc_inbox_hdr	hdr;
> +	__be16			mtu;
> +	__be16			rx_buf_sz_min;
> +	u8			mac_port;
> +	u8			rsvd;
> +};
> +
> +struct xsc_set_mtu_mbox_out {
> +	struct xsc_outbox_hdr	hdr;
> +};
> +
> +struct xsc_query_eth_mac_mbox_in {
> +	struct xsc_inbox_hdr	hdr;
> +	u8			index;
> +};
> +
> +struct xsc_query_eth_mac_mbox_out {
> +	struct xsc_outbox_hdr	hdr;
> +	u8			mac[6];

ETH_ALEN

> +};
> +
> +enum {
> +	XSC_TBM_CAP_HASH_PPH = 0,
> +	XSC_TBM_CAP_RSS,
> +	XSC_TBM_CAP_PP_BYPASS,
> +	XSC_TBM_CAP_PCT_DROP_CONFIG,
> +};
> +
> +struct xsc_nic_attr {
> +	__be16	caps;
> +	__be16	caps_mask;
> +	u8	mac_addr[6];
> +};
> +
> +struct xsc_rss_attr {
> +	u8	rss_en;
> +	u8	hfunc;
> +	__be16	rqn_base;
> +	__be16	rqn_num;
> +	__be32	hash_tmpl;
> +};
> +
> +struct xsc_cmd_enable_nic_hca_mbox_in {
> +	struct xsc_inbox_hdr	hdr;
> +	struct xsc_nic_attr	nic;
> +	struct xsc_rss_attr	rss;
> +};
> +
> +struct xsc_cmd_enable_nic_hca_mbox_out {
> +	struct xsc_outbox_hdr	hdr;
> +	u8			rsvd0[2];
> +};
> +
> +struct xsc_nic_dis_attr {
> +	__be16	caps;
> +};
> +
> +struct xsc_cmd_disable_nic_hca_mbox_in {
> +	struct xsc_inbox_hdr		hdr;
> +	struct xsc_nic_dis_attr		nic;
> +};
> +
> +struct xsc_cmd_disable_nic_hca_mbox_out {
> +	struct xsc_outbox_hdr	hdr;
> +	u8			rsvd0[4];
> +};
> +
> +struct xsc_function_reset_mbox_in {
> +	struct xsc_inbox_hdr	hdr;
> +	__be16			glb_func_id;
> +	u8			rsvd[6];
> +};
> +
> +struct xsc_function_reset_mbox_out {
> +	struct xsc_outbox_hdr	hdr;
> +	u8			rsvd[8];
> +};
> +
> +struct xsc_cmd_query_guid_mbox_in {
> +	struct xsc_inbox_hdr	hdr;
> +	u8			rsvd[8];
> +};
> +
> +struct xsc_cmd_query_guid_mbox_out {
> +	struct xsc_outbox_hdr	hdr;
> +	__be64			guid;
> +};
> +
> +struct xsc_cmd_activate_hw_config_mbox_in {
> +	struct xsc_inbox_hdr	hdr;
> +	u8			rsvd[8];
> +};
> +
> +struct xsc_cmd_activate_hw_config_mbox_out {
> +	struct xsc_outbox_hdr	hdr;
> +	u8			rsvd[8];
> +};
> +
> +struct xsc_event_set_port_admin_status_mbox_in {
> +	struct xsc_inbox_hdr	hdr;
> +	u16			admin_status;

reapeating "admin" in the "admin" command seems redundant

[..]

> +struct xsc_cmd {
> +	struct xsc_cmd_reg reg;
> +	void	       *cmd_buf;
> +	void	       *cq_buf;
> +	dma_addr_t	dma;
> +	dma_addr_t	cq_dma;
> +	u16     cmd_pid;
> +	u16     cq_cid;
> +	u8      owner_bit;
> +	u8		cmdif_rev;
> +	u8		log_sz;
> +	u8		log_stride;
> +	int		max_reg_cmds;
> +	int		events;
> +	u32 __iomem    *vector;
> +
> +	spinlock_t	alloc_lock;	/* protect command queue allocations */
> +	spinlock_t	token_lock;	/* protect token allocations */
> +	spinlock_t	doorbell_lock;	/* protect cmdq req pid doorbell */
> +	u8		token;

you have a whole lock for token allocations, but then there is a field
named @token, what it does?

> +	unsigned long	bitmask;

again, this name is so generic, that only "data" is less generic
BTW see DECLARE_BITMAP()

> +	char		wq_name[XSC_CMD_WQ_MAX_NAME];
> +	struct workqueue_struct *wq;

sorry if I have already asked you, do you really need custom WQ instead
of just using one of the kernel ones?

> +	struct task_struct *cq_task;
> +	struct semaphore sem;

please write a documentation of how it is used

> +	int	mode;

you could just embed an enum here, same size

> +	struct xsc_cmd_work_ent *ent_arr[XSC_MAX_COMMANDS];
> +	struct dma_pool *pool;
> +	struct xsc_cmd_debug dbg;
> +	struct cmd_msg_cache cache;
> +	int checksum_disabled;
> +	struct xsc_cmd_stats stats[XSC_CMD_OP_MAX];
> +	unsigned int	irqn;
> +	u8	ownerbit_learned;
> +	u8	cmd_status;
> +};
> +
> +#endif
> diff --git a/drivers/net/ethernet/yunsilicon/xsc/common/xsc_core.h b/drivers/net/ethernet/yunsilicon/xsc/common/xsc_core.h
> index 2c4e8e731..3b4b77948 100644
> --- a/drivers/net/ethernet/yunsilicon/xsc/common/xsc_core.h
> +++ b/drivers/net/ethernet/yunsilicon/xsc/common/xsc_core.h
> @@ -8,6 +8,7 @@
>   
>   #include <linux/kernel.h>

(sorry for commenting in the wrong patch)

you should avoid including kernel.h
(include what you use IWYU, kernel.h is a historic header
with much baggage, don't inclde if not strictly needed)

>   #include <linux/pci.h>

separate "header groups" by a blank line

> +#include "common/xsc_cmdq.h"
>   
>   #define XSC_PCI_VENDOR_ID		0x1f67
>   
> @@ -26,6 +27,11 @@
>   #define XSC_MV_HOST_VF_DEV_ID		0x1152
>   #define XSC_MV_SOC_PF_DEV_ID		0x1153
>   
> +#define REG_ADDR(dev, offset)						\

no need to put "\" so far to the right
remainder about xsc prefix

> +	(((dev)->bar) + ((offset) - 0xA0000000))
> +
> +#define REG_WIDTH_TO_STRIDE(width)	((width) / 8)
> +
>   struct xsc_dev_resource {
>   	struct mutex alloc_mutex;	/* protect buffer alocation according to numa node */
>   };
> @@ -35,6 +41,10 @@ enum xsc_pci_state {
>   	XSC_PCI_STATE_ENABLED,
>   };
>   
> +enum xsc_interface_state {
> +	XSC_INTERFACE_STATE_UP = BIT(0),
> +};
> +
>   struct xsc_core_device {
>   	struct pci_dev		*pdev;
>   	struct device		*device;
> @@ -44,6 +54,9 @@ struct xsc_core_device {
>   	void __iomem		*bar;
>   	int			bar_num;
>   
> +	struct xsc_cmd		cmd;
> +	u16			cmdq_ver;
> +
>   	struct mutex		pci_state_mutex;	/* protect pci_state */
>   	enum xsc_pci_state	pci_state;
>   	struct mutex		intf_state_mutex;	/* protect intf_state */
> diff --git a/drivers/net/ethernet/yunsilicon/xsc/common/xsc_driver.h b/drivers/net/ethernet/yunsilicon/xsc/common/xsc_driver.h
> new file mode 100644
> index 000000000..72b2df6c9
> --- /dev/null
> +++ b/drivers/net/ethernet/yunsilicon/xsc/common/xsc_driver.h
> @@ -0,0 +1,25 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright (C) 2021-2025, Shanghai Yunsilicon Technology Co., Ltd.
> + * All rights reserved.
> + */
> +
> +#ifndef __XSC_DRIVER_H
> +#define __XSC_DRIVER_H
> +
> +#include "common/xsc_core.h"
> +#include "common/xsc_cmd.h"
> +
> +int xsc_cmd_init(struct xsc_core_device *xdev);
> +void xsc_cmd_cleanup(struct xsc_core_device *xdev);
> +void xsc_cmd_use_events(struct xsc_core_device *xdev);
> +void xsc_cmd_use_polling(struct xsc_core_device *xdev);
> +int xsc_cmd_err_handler(struct xsc_core_device *xdev);
> +void xsc_cmd_resp_handler(struct xsc_core_device *xdev);
> +int xsc_cmd_status_to_err(struct xsc_outbox_hdr *hdr);
> +int xsc_cmd_exec(struct xsc_core_device *xdev, void *in, int in_size, void *out,
> +		 int out_size);
> +int xsc_cmd_version_check(struct xsc_core_device *xdev);
> +const char *xsc_command_str(int command);
> +
> +#endif
> +
> diff --git a/drivers/net/ethernet/yunsilicon/xsc/pci/Makefile b/drivers/net/ethernet/yunsilicon/xsc/pci/Makefile
> index 709270df8..5e0f0a205 100644
> --- a/drivers/net/ethernet/yunsilicon/xsc/pci/Makefile
> +++ b/drivers/net/ethernet/yunsilicon/xsc/pci/Makefile
> @@ -6,4 +6,4 @@ ccflags-y += -I$(srctree)/drivers/net/ethernet/yunsilicon/xsc
>   
>   obj-$(CONFIG_YUNSILICON_XSC_PCI) += xsc_pci.o
>   
> -xsc_pci-y := main.o
> +xsc_pci-y := main.o cmdq.o
> diff --git a/drivers/net/ethernet/yunsilicon/xsc/pci/cmdq.c b/drivers/net/ethernet/yunsilicon/xsc/pci/cmdq.c
> new file mode 100644
> index 000000000..028970151
> --- /dev/null
> +++ b/drivers/net/ethernet/yunsilicon/xsc/pci/cmdq.c
> @@ -0,0 +1,1555 @@
> +// SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB
> +/*
> + * Copyright (c) 2021-2025, Shanghai Yunsilicon Technology Co., Ltd.
> + * All rights reserved.
> + * Copyright (c) 2013-2016, Mellanox Technologies. All rights reserved.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/init.h>

nope, you are either a module-like or builtin-only, remove init.h

> +#include <linux/errno.h>
> +#include <linux/pci.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/slab.h>
> +#include <linux/delay.h>
> +#include <linux/random.h>
> +#include <linux/kthread.h>
> +#include <linux/io-mapping.h>
> +#include "common/xsc_driver.h"
> +#include "common/xsc_cmd.h"
> +#include "common/xsc_auto_hw.h"
> +#include "common/xsc_core.h"
> +
> +enum {
> +	CMD_IF_REV = 3,
> +};
> +
> +enum {
> +	CMD_MODE_POLLING,
> +	CMD_MODE_EVENTS
> +};
> +
> +enum {
> +	NUM_LONG_LISTS	  = 2,
> +	NUM_MED_LISTS	  = 64,
> +	LONG_LIST_SIZE	  = (2ULL * 1024 * 1024 * 1024 / PAGE_SIZE) * 8 + 16 +
> +				XSC_CMD_DATA_BLOCK_SIZE,
> +	MED_LIST_SIZE	  = 16 + XSC_CMD_DATA_BLOCK_SIZE,
> +};
> +
> +enum {
> +	XSC_CMD_DELIVERY_STAT_OK			= 0x0,
> +	XSC_CMD_DELIVERY_STAT_SIGNAT_ERR		= 0x1,
> +	XSC_CMD_DELIVERY_STAT_TOK_ERR			= 0x2,
> +	XSC_CMD_DELIVERY_STAT_BAD_BLK_NUM_ERR		= 0x3,
> +	XSC_CMD_DELIVERY_STAT_OUT_PTR_ALIGN_ERR	= 0x4,
> +	XSC_CMD_DELIVERY_STAT_IN_PTR_ALIGN_ERR		= 0x5,
> +	XSC_CMD_DELIVERY_STAT_FW_ERR			= 0x6,
> +	XSC_CMD_DELIVERY_STAT_IN_LENGTH_ERR		= 0x7,
> +	XSC_CMD_DELIVERY_STAT_OUT_LENGTH_ERR		= 0x8,
> +	XSC_CMD_DELIVERY_STAT_RES_FLD_NOT_CLR_ERR	= 0x9,
> +	XSC_CMD_DELIVERY_STAT_CMD_DESCR_ERR		= 0x10,
> +};
> +
> +static struct xsc_cmd_work_ent *alloc_cmd(struct xsc_cmd *cmd,
> +					  struct xsc_cmd_msg *in,
> +					  struct xsc_rsp_msg *out)
> +{
> +	struct xsc_cmd_work_ent *ent;
> +
> +	ent = kzalloc(sizeof(*ent), GFP_KERNEL);
> +	if (!ent)
> +		return ERR_PTR(-ENOMEM);
> +
> +	ent->in		= in;
> +	ent->out	= out;
> +	ent->cmd	= cmd;
> +
> +	return ent;
> +}
> +
> +static u8 alloc_token(struct xsc_cmd *cmd)

remainted about xsc prefix in the names

> +{
> +	u8 token;
> +
> +	spin_lock(&cmd->token_lock);
> +	token = cmd->token++ % 255 + 1;

token==0 is wrong or reserved?

> +	spin_unlock(&cmd->token_lock);
> +
> +	return token;
> +}
> +
> +static int alloc_ent(struct xsc_cmd *cmd)
> +{
> +	unsigned long flags;
> +	int ret;
> +
> +	spin_lock_irqsave(&cmd->alloc_lock, flags);
> +	ret = find_first_bit(&cmd->bitmask, cmd->max_reg_cmds);
> +	if (ret < cmd->max_reg_cmds)
> +		clear_bit(ret, &cmd->bitmask);
> +	spin_unlock_irqrestore(&cmd->alloc_lock, flags);
> +
> +	return ret < cmd->max_reg_cmds ? ret : -ENOMEM;

ENOMEM is strictly for kmalloc() family failures, perhaps ENOSPC?

> +}
> +
> +static void free_ent(struct xsc_cmd *cmd, int idx)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&cmd->alloc_lock, flags);
> +	set_bit(idx, &cmd->bitmask);
> +	spin_unlock_irqrestore(&cmd->alloc_lock, flags);
> +}
> +
> +static struct xsc_cmd_layout *get_inst(struct xsc_cmd *cmd, int idx)
> +{
> +	return cmd->cmd_buf + (idx << cmd->log_stride);
> +}
> +
> +static struct xsc_rsp_layout *get_cq_inst(struct xsc_cmd *cmd, int idx)
> +{
> +	return cmd->cq_buf + (idx << cmd->log_stride);
> +}
> +
> +static u8 xor8_buf(void *buf, int len)

double check if there is already something like this in the kernel

> +{
> +	u8 *ptr = buf;
> +	u8 sum = 0;
> +	int i;
> +
> +	for (i = 0; i < len; i++)
> +		sum ^= ptr[i];
> +
> +	return sum;
> +}
> +
> +static int verify_block_sig(struct xsc_cmd_prot_block *block)
> +{
> +	if (xor8_buf(block->rsvd0, sizeof(*block) - sizeof(block->data) - 1) != 0xff)

you force rsvd0 to be 0xff? the usual approach is that reserved fields
are 0
if this is somethins that your FW/HW already has baked in, please add
a comment at this particular rsvd0 declaration

> +		return -EINVAL;
> +
> +	if (xor8_buf(block, sizeof(*block)) != 0xff)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static void calc_block_sig(struct xsc_cmd_prot_block *block, u8 token)
> +{
> +	block->token = token;
> +	block->ctrl_sig = ~xor8_buf(block->rsvd0, sizeof(*block) - sizeof(block->data) - 2);
> +	block->sig = ~xor8_buf(block, sizeof(*block) - 1);
> +}
> +
> +static void calc_chain_sig(struct xsc_cmd_mailbox *head, u8 token)
> +{
> +	struct xsc_cmd_mailbox *next = head;
> +
> +	while (next) {
> +		calc_block_sig(next->buf, token);
> +		next = next->next;
> +	}
> +}
> +
> +static void set_signature(struct xsc_cmd_work_ent *ent)
> +{
> +	ent->lay->sig = ~xor8_buf(ent->lay, sizeof(*ent->lay));
> +	calc_chain_sig(ent->in->next, ent->token);
> +	calc_chain_sig(ent->out->next, ent->token);
> +}
> +
> +static void free_cmd(struct xsc_cmd_work_ent *ent)

please move near the "alloc_cmd"
remainder about the xsc prefixes

[...]

> +
> +static void cmd_work_handler(struct work_struct *work)
> +{
> +	struct xsc_cmd_work_ent *ent = container_of(work, struct xsc_cmd_work_ent, work);
> +	struct xsc_cmd *cmd = ent->cmd;

reverse xmass tree violated

> +	struct xsc_core_device *xdev = container_of(cmd, struct xsc_core_device, cmd);

Powered by blists - more mailing lists