Re: [PATCH net-next v7 02/14] xsc: Enable command queue

[Simon Horman <horms@kernel.org>]

On Fri, Feb 28, 2025 at 11:41:26PM +0800, Xin Tian wrote:
> The command queue is a hardware channel for sending
> commands between the driver and the firmware.
> xsc_cmd.h defines the command protocol structures.
> The logic for command allocation, sending,
> completion handling, and error handling is implemented
> in cmdq.c.
> 
> Co-developed-by: Honggang Wei <weihg@yunsilicon.com>
> Signed-off-by: Honggang Wei <weihg@yunsilicon.com>
> Co-developed-by: Lei Yan <jacky@yunsilicon.com>
> Signed-off-by: Lei Yan <jacky@yunsilicon.com>
> Signed-off-by: Xin Tian <tianx@yunsilicon.com>

Hi Xin Tian, all,

Some minor nits from my side.

...

> diff --git a/drivers/net/ethernet/yunsilicon/xsc/pci/cmdq.c b/drivers/net/ethernet/yunsilicon/xsc/pci/cmdq.c

...

> +/*  Notes:
> + *    1. Callback functions may not sleep
> + *    2. page queue commands do not support asynchrous completion
> + */
> +static int xsc_cmd_invoke(struct xsc_core_device *xdev, struct xsc_cmd_msg *in,
> +			  struct xsc_rsp_msg *out, u8 *status)
> +{
> +	struct xsc_cmd *cmd = &xdev->cmd;
> +	struct xsc_cmd_work_ent *ent;
> +	struct xsc_cmd_stats *stats;
> +	ktime_t t1, t2, delta;
> +	struct semaphore *sem;
> +	int err = 0;
> +	s64 ds;
> +	u16 op;
> +
> +	ent = xsc_alloc_cmd(cmd, in, out);
> +	if (IS_ERR(ent))
> +		return PTR_ERR(ent);
> +
> +	init_completion(&ent->done);
> +	INIT_WORK(&ent->work, cmd_work_handler);
> +	if (!queue_work(cmd->wq, &ent->work)) {
> +		pci_err(xdev->pdev, "failed to queue work\n");
> +		err = -ENOMEM;
> +		goto out_free;
> +	}
> +
> +	err = xsc_wait_func(xdev, ent);
> +	if (err == -ETIMEDOUT)
> +		goto out;
> +	t1 = timespec64_to_ktime(ent->ts1);
> +	t2 = timespec64_to_ktime(ent->ts2);
> +	delta = ktime_sub(t2, t1);
> +	ds = ktime_to_ns(delta);
> +	op = be16_to_cpu(((struct xsc_inbox_hdr *)in->first.data)->opcode);
> +	if (op < ARRAY_SIZE(cmd->stats)) {
> +		stats = &cmd->stats[op];
> +		spin_lock(&stats->lock);
> +		stats->sum += ds;
> +		++stats->n;
> +		spin_unlock(&stats->lock);
> +	}
> +	*status = ent->status;
> +	xsc_free_cmd(ent);
> +
> +	return err;
> +
> +out:

Maybe err_sem_up would be a better name for this label.
Likewise for other cases where out or our_* is used
for paths only used for unwinding in the case of error.

> +	sem = &cmd->sem;
> +	up(sem);
> +out_free:

And err_free would be a better name for this label.

Also, in this patch (set) sometimes labels are named err_something,
and sometimes they are called something_err. It would be nice
to make that consistent (personally, I would go for err_somthing).

> +	xsc_free_cmd(ent);
> +	return err;
> +}
> +
> +static int xsc_copy_to_cmd_msg(struct xsc_cmd_msg *to, void *from, int size)
> +{
> +	struct xsc_cmd_prot_block *block;
> +	struct xsc_cmd_mailbox *next;
> +	int copy;
> +
> +	if (!to || !from)
> +		return -ENOMEM;
> +
> +	copy = min_t(int, size, sizeof(to->first.data));
> +	memcpy(to->first.data, from, copy);
> +	size -= copy;
> +	from += copy;
> +
> +	next = to->next;
> +	while (size) {
> +		if (!next) {
> +			/* this is a BUG */

Maybe WARN_ONCE() or similar would be appropriate here?

> +			return -ENOMEM;
> +		}
> +
> +		copy = min_t(int, size, XSC_CMD_DATA_BLOCK_SIZE);
> +		block = next->buf;
> +		memcpy(block->data, from, copy);
> +		block->owner_status = 0;
> +		from += copy;
> +		size -= copy;
> +		next = next->next;
> +	}
> +
> +	return 0;
> +}
> +
> +static int xsc_copy_from_rsp_msg(void *to, struct xsc_rsp_msg *from, int size)
> +{
> +	struct xsc_cmd_prot_block *block;
> +	struct xsc_cmd_mailbox *next;
> +	int copy;
> +
> +	if (!to || !from)
> +		return -ENOMEM;
> +
> +	copy = min_t(int, size, sizeof(from->first.data));
> +	memcpy(to, from->first.data, copy);
> +	size -= copy;
> +	to += copy;
> +
> +	next = from->next;
> +	while (size) {
> +		if (!next) {
> +			/* this is a BUG */

Ditto.

> +			return -ENOMEM;
> +		}
> +
> +		copy = min_t(int, size, XSC_CMD_DATA_BLOCK_SIZE);
> +		block = next->buf;
> +		if (!block->owner_status)
> +			pr_err("block ownership check failed\n");
> +
> +		memcpy(to, block->data, copy);
> +		to += copy;
> +		size -= copy;
> +		next = next->next;
> +	}
> +
> +	return 0;
> +}

...

> +static int xsc_request_pid_cid_mismatch_restore(struct xsc_core_device *xdev)
> +{
> +	struct xsc_cmd *cmd = &xdev->cmd;
> +	u16 req_pid, req_cid;
> +	u16 gap;
> +
> +	int err;
> +
> +	req_pid = readl(XSC_REG_ADDR(xdev, cmd->reg.req_pid_addr));
> +	req_cid = readl(XSC_REG_ADDR(xdev, cmd->reg.req_cid_addr));
> +	if (req_pid >= (1 << cmd->log_sz) || req_cid >= (1 << cmd->log_sz)) {
> +		pci_err(xdev->pdev,
> +			"req_pid %d, req_cid %d, out of normal range!!! max value is %d\n",
> +			req_pid, req_cid, (1 << cmd->log_sz));
> +		return -1;
> +	}
> +
> +	if (req_pid == req_cid)
> +		return 0;
> +
> +	gap = (req_pid > req_cid) ? (req_pid - req_cid)
> +	      : ((1 << cmd->log_sz) + req_pid - req_cid);
> +
> +	err = xsc_send_dummy_cmd(xdev, gap, req_cid);
> +	if (err) {
> +		pci_err(xdev->pdev, "Send dummy cmd failed\n");
> +		goto send_dummy_fail;

I think that it would be nicer to simply return err here
and drop the send_dummy_fail label here as no unwind is occurring.
Likewise for other similar cases in this patch (set).

> +	}
> +
> +send_dummy_fail:
> +	return err;
> +}

...

> +static int xsc_cmd_cq_polling(void *data)
> +{
> +	struct xsc_core_device *xdev = data;
> +	struct xsc_cmd *cmd = &xdev->cmd;
> +	struct xsc_rsp_layout *rsp;
> +	u32 cq_pid;
> +
> +	while (!kthread_should_stop()) {
> +		if (need_resched())
> +			schedule();
> +		cq_pid = readl(XSC_REG_ADDR(xdev, cmd->reg.rsp_pid_addr));
> +		if (cmd->cq_cid == cq_pid) {
> +			mdelay(3);
> +			continue;
> +		}
> +
> +		rsp = xsc_get_cq_inst(cmd, cmd->cq_cid);
> +		if (!cmd->ownerbit_learned) {
> +			cmd->ownerbit_learned = 1;
> +			cmd->owner_bit = rsp->owner_bit;
> +		}
> +		if (cmd->owner_bit != rsp->owner_bit) {
> +			pci_err(xdev->pdev, "hw update cq doorbell but buf not ready %u %u\n",
> +				cmd->cq_cid, cq_pid);
> +			continue;
> +		}
> +
> +		xsc_cmd_comp_handler(xdev, rsp->idx, rsp);
> +
> +		cmd->cq_cid = (cmd->cq_cid + 1) % (1 << cmd->log_sz);
> +
> +		writel(cmd->cq_cid, XSC_REG_ADDR(xdev, cmd->reg.rsp_cid_addr));
> +		if (cmd->cq_cid == 0)
> +			cmd->owner_bit = !cmd->owner_bit;
> +	}

super nit: blank line here please

> +	return 0;
> +}

...

> +static int xsc_load(struct xsc_core_device *xdev)
> +{
> +	int err = 0;
> +
> +	mutex_lock(&xdev->intf_state_mutex);
> +	if (test_bit(XSC_INTERFACE_STATE_UP, &xdev->intf_state))
> +		goto out;
> +
> +	err = xsc_hw_setup(xdev);
> +	if (err) {
> +		pci_err(xdev->pdev, "xsc_hw_setup failed %d\n", err);
> +		goto out;
> +	}
> +
> +	set_bit(XSC_INTERFACE_STATE_UP, &xdev->intf_state);
> +	mutex_unlock(&xdev->intf_state_mutex);
> +
> +	return 0;
> +out:
> +	mutex_unlock(&xdev->intf_state_mutex);
> +	return err;
> +}
> +
> +static int xsc_unload(struct xsc_core_device *xdev)
> +{
> +	mutex_lock(&xdev->intf_state_mutex);
> +	if (!test_bit(XSC_INTERFACE_STATE_UP, &xdev->intf_state)) {
> +		xsc_hw_cleanup(xdev);
> +		goto out;
> +	}
> +
> +	clear_bit(XSC_INTERFACE_STATE_UP, &xdev->intf_state);
> +
> +	xsc_hw_cleanup(xdev);
> +
> +out:
> +	mutex_unlock(&xdev->intf_state_mutex);

super nit: maybe no blank line here.

> +
> +	return 0;
> +}

...