Re: [PATCH v1 04/15] net/xsc: add the command queue

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] net/xsc: leaked command resources and wq hang on timeout in _xsc_cmd_exec()
- [High] net/xsc: IRQ deadlock and unchecked kthread_create() in xsc_cmd_use_polling()
- [High] net/xsc: infinite loop and premature up() in xsc_cmd_use_events()
- [Medium] net/xsc: wrong 16-byte inline threshold in xsc_alloc_msg() corrupts commands
- [Medium] net/xsc: broken CQ owner-bit learning in xsc_cmd_cq_polling()
- [Medium] net/xsc: unsynchronized shared board_info access in xsc_hw_setup()
- [Medium] net/xsc: unguarded V2 capability fields and 32-bit shifts in xsc_cmd_query_hca_cap()
--

commit bb1aecb9bb4cc115b4d4581a9902a3b7bb2ae064
Author: Xin Tian <tianx@yunsilicon.com>

net/xsc: add the command queue

This commit adds the firmware command queue implementation, mailbox memory
management, and initial hardware capability and board configuration queries
for the Yunsilicon XSC PCI driver.

> 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 0000000000000..c001b312224d6
> --- /dev/null
> +++ b/drivers/net/ethernet/yunsilicon/xsc/pci/cmdq.c
> @@ -0,0 +1,1244 @@

[ ... ]

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

[ ... ]

> +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;
> +	int err = 0;
> +
> +	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 err_free;
> +	}
> +
> +	err = xsc_wait_func(xdev, ent);
> +	if (err == -ETIMEDOUT)
> +		/* leave the entry for the completion handler; the driver
> +		 * is marked timed out and rejects all further commands
> +		 */
> +		return err;

[ ... ]

> +void xsc_cmd_use_events(struct xsc_core_device *xdev)
> +{
> +	struct xsc_cmd *cmd = &xdev->cmd;
> +	int i;
> +
> +	/* flush first: cmd_work_handler() takes a semaphore permit, so
> +	 * flushing after all permits are held would deadlock against a
> +	 * just-queued work item
> +	 */
> +	flush_workqueue(cmd->wq);
> +
> +	/* keep the cmdq interrupt out while the polling kthread is still
> +	 * draining the queue
> +	 */
> +	disable_irq(cmd->irqn);
> +
> +	for (i = 0; i < cmd->max_reg_cmds; i++)
> +		down(&cmd->sem);
> +
> +	cmd->mode = XSC_CMD_MODE_EVENTS;
> +
> +	while (cmd->cmd_pid != cmd->cq_cid)
> +		msleep(20);
> +	kthread_stop(cmd->cq_task);
> +	cmd->cq_task = NULL;
> +
> +	for (i = 0; i < cmd->max_reg_cmds; i++)
> +		up(&cmd->sem);
> +
> +	enable_irq(cmd->irqn);
> +}

[Severity: High]
Can the while (cmd->cmd_pid != cmd->cq_cid) loop in xsc_cmd_use_events()
spin forever if the request producer index and response consumer index start
at different offsets?

In xsc_cmd_init(), cmd->cmd_pid and cmd->cq_cid are initialized from separate
hardware registers:

    cmd->cmd_pid = xsc_get_cmdq_req_pid(xdev) & XSC_CMDQ_INDEX_MASK;
    cmd->cq_cid = xsc_get_cmdq_rsp_cid(xdev) & XSC_CMDQ_INDEX_MASK;

If the hardware request and response indices do not have the exact same
modulo-32 value after a kexec, FLR, or prior command error, cmd->cmd_pid and
cmd->cq_cid maintain a constant non-zero offset even when all commands have
completed, so cmd->cmd_pid != cmd->cq_cid will never become false.

In addition, in xsc_cmd_cq_polling() and xsc_cmd_resp_handler(),
xsc_cmd_comp_handler() calls up(&cmd->sem) before the caller advances
cmd->cq_cid, updates hardware via xsc_update_cmdq_rsp_cid(), and flips
cmd->owner_bit. Doesn't that allow xsc_cmd_use_events() or
xsc_cmd_use_polling() to acquire the final semaphore permit before the
completion handler finishes updating the CQ consumer state?

> +static int xsc_cmd_cq_polling(void *data);
> +void xsc_cmd_use_polling(struct xsc_core_device *xdev)
> +{
> +	struct xsc_cmd *cmd = &xdev->cmd;
> +	int i;
> +
> +	flush_workqueue(cmd->wq);
> +
> +	disable_irq(cmd->irqn);
> +
> +	for (i = 0; i < cmd->max_reg_cmds; i++)
> +		down(&cmd->sem);
> +
> +	cmd->mode = XSC_CMD_MODE_POLLING;
> +	cmd->cq_task = kthread_create(xsc_cmd_cq_polling,
> +				      (void *)xdev,
> +				      "xsc_cmd_cq_polling");
> +	if (!IS_ERR(cmd->cq_task))
> +		wake_up_process(cmd->cq_task);
> +
> +	for (i = 0; i < cmd->max_reg_cmds; i++)
> +		up(&cmd->sem);
> +
> +	enable_irq(cmd->irqn);
> +}

[Severity: High]
Won't xsc_cmd_use_polling() deadlock on cmd->sem if a command is still in
flight when disable_irq(cmd->irqn) is called?

In XSC_CMD_MODE_EVENTS, cmd->cq_task is NULL and completions are driven by
the interrupt handler on cmd->irqn calling xsc_cmd_resp_handler() ->
xsc_cmd_comp_handler() -> up(&cmd->sem). Because cmd_work_handler() returns
as soon as it rings the doorbell while the in-flight command continues
holding its permit on cmd->sem, calling disable_irq(cmd->irqn) before
acquiring all cmd->max_reg_cmds permits masks the completion interrupt while
no polling thread is running, so up(&cmd->sem) cannot be called.

Also, why does xsc_cmd_use_polling() call enable_irq(cmd->irqn) at the end
after starting cmd->cq_task? With the interrupt enabled in polling mode,
xsc_cmd_resp_handler() and xsc_cmd_cq_polling() can race on cmd->cq_cid and
cmd->owner_bit.

Finally, if kthread_create() fails in xsc_cmd_use_polling(), cmd->cq_task
stores an ERR_PTR while cmd->mode is set to XSC_CMD_MODE_POLLING, and
xsc_cmd_use_events() later calls kthread_stop(cmd->cq_task) without checking
IS_ERR_OR_NULL(cmd->cq_task).

[ ... ]

> +static struct xsc_cmd_msg *xsc_alloc_msg(struct xsc_core_device *xdev,
> +					 unsigned int in_size)
> +{
> +	struct xsc_cmd_msg *msg = ERR_PTR(-ENOMEM);
> +	struct xsc_cmd *cmd = &xdev->cmd;
> +	struct cache_ent *ent = NULL;
> +
> +	if (in_size > MED_LIST_SIZE && in_size <= LONG_LIST_SIZE)
> +		ent = &cmd->cache.large;
> +	else if (in_size > 16 && in_size <= MED_LIST_SIZE)
> +		ent = &cmd->cache.med;
> +
> +	if (ent) {
> +		spin_lock(&ent->lock);
> +		if (!list_empty(&ent->head)) {
> +			msg = list_entry(ent->head.next, typeof(*msg), list);
> +			/* For cached lists, we must explicitly state what is
> +			 * the real size
> +			 */
> +			msg->len = in_size;
> +			list_del(&msg->list);
> +		}
> +		spin_unlock(&ent->lock);
> +	}
> +
> +	if (IS_ERR(msg))
> +		msg = xsc_alloc_cmd_msg(xdev, GFP_KERNEL, in_size);
> +
> +	return msg;
> +}

[Severity: Medium]
Should the inline threshold in xsc_alloc_msg(), MED_LIST_SIZE, and
LONG_LIST_SIZE be sizeof(msg->first.data) (32 bytes) instead of 16?

In struct xsc_cmd_first, data is 32 bytes (__be32 data[8]), and both
xsc_alloc_cmd_msg() and xsc_copy_to_cmd_msg() copy up to
sizeof(to->first.data) (32 bytes) inline before using overflow mailboxes in
to->next.

Because xsc_alloc_msg() checks in_size > 16, a command with 16 < in_size <= 32
(such as XSC_CMD_OP_ENABLE_NIC_HCA or XSC_CMD_OP_MODIFY_RAW_QP) is served from
cmd->cache.med, which has a pre-allocated overflow mailbox in msg->next.
xsc_copy_to_cmd_msg() consumes all in_size bytes into to->first.data (leaving
the remaining 32 - in_size inline bytes with stale cached data) and never
initializes to->next->buf, yet cmd_work_handler() still copies all 32 bytes
of ent->in->first.data into lay->in and sets lay->in_ptr to ent->in->next->dma.

[ ... ]

> +static int _xsc_cmd_exec(struct xsc_core_device *xdev,
> +			 void *in, unsigned int in_size,
> +			 void *out, unsigned int out_size)
> +{
> +	struct xsc_cmd *cmd = &xdev->cmd;
> +	struct xsc_rsp_msg *outb;
> +	struct xsc_cmd_msg *inb;
> +	u8 status = 0;
> +	int err;
> +
> +	if (cmd->cmd_status == XSC_CMD_STATUS_TIMEDOUT)
> +		return -ETIMEDOUT;
> +
> +	inb = xsc_alloc_msg(xdev, in_size);
> +	if (IS_ERR(inb)) {
> +		err = PTR_ERR(inb);
> +		return err;
> +	}
> +
> +	err = xsc_copy_to_cmd_msg(inb, in, in_size);
> +	if (err) {
> +		pci_err(xdev->pdev, "copy to cmd_msg err %d\n", err);
> +		goto err_free_msg;
> +	}
> +
> +	outb = xsc_alloc_rsp_msg(xdev, GFP_KERNEL, out_size);
> +	if (IS_ERR(outb)) {
> +		err = PTR_ERR(outb);
> +		goto err_free_msg;
> +	}
> +
> +	err = xsc_cmd_invoke(xdev, inb, outb, &status);
> +	if (err == -ETIMEDOUT)
> +		/* the entry stays registered for the completion handler
> +		 * and keeps its mailboxes
> +		 */
> +		return err;

[ ... ]

> +static void xsc_cmd_comp_handler(struct xsc_core_device *xdev, u8 idx,
> +				 struct xsc_rsp_layout *rsp)
> +{

[ ... ]

> +	xsc_free_ent(cmd, ent->idx);
> +	complete(&ent->done);
> +	up(&cmd->sem);
> +}
> +
> +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 = xsc_get_cmdq_rsp_pid(xdev);
> +		if (cmd->cq_cid == cq_pid) {
> +			usleep_range(1000, 2000);
> +			continue;
> +		}
> +
> +		rsp = xsc_get_cq_inst(cmd, cmd->cq_cid);
> +		if (!cmd->ownerbit_learned) {
> +			cmd->ownerbit_learned = 1;
> +			cmd->owner_bit = xsc_rsp_owner_bit(rsp);
> +		}
> +		if (cmd->owner_bit != xsc_rsp_owner_bit(rsp)) {
> +			dev_err_ratelimited(&xdev->pdev->dev,
> +				"hw update cq doorbell but buf not ready %u %u\n",
> +				cmd->cq_cid, cq_pid);
> +			msleep(20);
> +			continue;
> +		}

[Severity: Medium]
Does learning cmd->owner_bit from xsc_rsp_owner_bit(rsp) right before
checking cmd->owner_bit != xsc_rsp_owner_bit(rsp) bypass the readiness check
on the first response in xsc_cmd_cq_polling() and xsc_cmd_resp_handler()?

In xsc_cmd_init(), cmd->cq_buf is zeroed with memset(cmd->cq_buf, 0,
PAGE_SIZE) and cmd->ownerbit_learned is set to 0. When the first completion
is polled, if the MMIO read of rsp_pid sees the updated producer index before
the device DMA write of the response descriptor reaches cmd->cq_buf,
xsc_rsp_owner_bit(rsp) reads 0 from the zeroed buffer, sets cmd->owner_bit = 0,
and immediately passes the cmd->owner_bit != xsc_rsp_owner_bit(rsp) check with
an all-zero descriptor while latching the wrong owner-bit polarity for subsequent
slots.

[ ... ]

> +void xsc_cmd_cleanup(struct xsc_core_device *xdev)
> +{
> +	struct xsc_cmd *cmd = &xdev->cmd;
> +
> +	destroy_workqueue(cmd->wq);
> +	if (!IS_ERR_OR_NULL(cmd->cq_task))
> +		kthread_stop(cmd->cq_task);
> +	xsc_destroy_msg_cache(xdev);
> +	dma_free_coherent(&xdev->pdev->dev, PAGE_SIZE, cmd->cq_buf,
> +			  cmd->cq_dma);
> +	dma_free_coherent(&xdev->pdev->dev, PAGE_SIZE, cmd->cmd_buf,
> +			  cmd->dma);
> +	dma_pool_destroy(cmd->pool);
> +}

[Severity: High]
When a command times out in xsc_cmd_invoke() and _xsc_cmd_exec(), where are
ent, inb, and outb freed?

On -ETIMEDOUT, xsc_cmd_invoke() leaves ent allocated and _xsc_cmd_exec()
returns early without freeing outb or returning/freeing inb. However, neither
xsc_cmd_comp_handler() (if a late completion arrives) nor xsc_cmd_cleanup()
(during error unwind in xsc_hw_setup() or device teardown) ever frees ent,
inb, or outb. Because inb (when detached from cmd->cache or allocated on
demand) and outb (when out_size > sizeof(outb->first.data), such as in
xsc_cmd_query_hca_cap()) hold blocks allocated from cmd->pool,
dma_pool_destroy(cmd->pool) in xsc_cmd_cleanup() will fail to destroy the
pool due to still-allocated DMA blocks and leak coherent DMA memory.

Furthermore, if cmd->max_reg_cmds commands time out while another work item
is queued on cmd->wq, cmd_work_handler() for that queued item will block
indefinitely in down(&cmd->sem), causing destroy_workqueue(cmd->wq) in
xsc_cmd_cleanup() to hang.

> diff --git a/drivers/net/ethernet/yunsilicon/xsc/pci/hw.c b/drivers/net/ethernet/yunsilicon/xsc/pci/hw.c
> new file mode 100644
> index 0000000000000..0358a40cca9b8
> --- /dev/null
> +++ b/drivers/net/ethernet/yunsilicon/xsc/pci/hw.c
> @@ -0,0 +1,316 @@

[ ... ]

> +int xsc_cmd_query_hca_cap(struct xsc_core_device *xdev,
> +			  struct xsc_caps *caps)
> +{

[ ... ]

> +	caps->max_qp_mcg = be16_to_cpu(out->hca_cap.max_qp_mcg);
> +	caps->max_ra_res_qp = 1 << (out->hca_cap.log_max_ra_res_qp & 0x3f);
> +	caps->max_ra_req_qp = 1 << (out->hca_cap.log_max_ra_req_qp & 0x3f);
> +	caps->max_srq_wqes = 1 << out->hca_cap.log_max_srq_sz;

[ ... ]

> +	caps->max_qp = be32_to_cpu(out->hca_cap.max_qp);
> +	caps->max_cq = be32_to_cpu(out->hca_cap.max_cq);
> +	caps->max_pd = be32_to_cpu(out->hca_cap.max_pd);
> +
> +	if (be16_to_cpu(out->hdr.ver) >= XSC_CMD_QUERY_HCA_CAP_V2)
> +		caps->max_mtt = be32_to_cpu(out->hca_cap.max_mtt);
> +	else
> +		caps->max_mtt = 1 << (out->hca_cap.log_max_mtt & 0x3f);

[Severity: Medium]
In struct xsc_hca_cap, max_qp, max_cq, and max_pd are defined alongside
max_mtt in the V2 section of the mailbox. When be16_to_cpu(out->hdr.ver) is
earlier than XSC_CMD_QUERY_HCA_CAP_V2, won't reading out->hca_cap.max_qp,
out->hca_cap.max_cq, and out->hca_cap.max_pd unconditionally leave
caps->max_qp, caps->max_cq, and caps->max_pd as 0 instead of falling back to
1U << caps->log_max_qp, 1U << caps->log_max_cq, and 1U << caps->log_max_pd?

Also, in expressions like 1 << (out->hca_cap.log_max_mtt & 0x3f),
1 << (out->hca_cap.log_max_ra_res_qp & 0x3f), and
1 << out->hca_cap.log_max_srq_sz, can shifting the signed 32-bit integer
literal 1 by a 6-bit or 8-bit field overflow a 32-bit signed int?

[ ... ]

> +static int xsc_cmd_query_guid(struct xsc_core_device *xdev)
> +{
> +	struct xsc_cmd_query_guid_mbox_out out;
> +	struct xsc_cmd_query_guid_mbox_in in = {};
> +	int err;
> +
> +	in.hdr.opcode = cpu_to_be16(XSC_CMD_OP_QUERY_GUID);
> +	err = xsc_cmd_exec(xdev, &in, sizeof(in), &out, sizeof(out));
> +	if (err)
> +		return err;
> +
> +	xdev->board_info->guid = out.guid;
> +	xdev->board_info->guid_valid = 1;
> +	return 0;
> +}
> +
> +int xsc_query_guid(struct xsc_core_device *xdev)
> +{
> +	if (xdev->board_info->guid_valid)
> +		return 0;
> +
> +	return xsc_cmd_query_guid(xdev);
> +}
> +
> +static int xsc_cmd_activate_hw_config(struct xsc_core_device *xdev)
> +{
> +	struct xsc_cmd_activate_hw_config_mbox_out out;
> +	struct xsc_cmd_activate_hw_config_mbox_in in = {};
> +	int err = 0;
> +
> +	in.hdr.opcode = cpu_to_be16(XSC_CMD_OP_ACTIVATE_HW_CONFIG);
> +	err = xsc_cmd_exec(xdev, &in, sizeof(in), &out, sizeof(out));
> +	if (err)
> +		return err;
> +
> +	xdev->board_info->hw_config_activated = 1;
> +	return 0;
> +}
> +
> +int xsc_activate_hw_config(struct xsc_core_device *xdev)
> +{
> +	if (xdev->board_info->hw_config_activated)
> +		return 0;
> +
> +	return xsc_cmd_activate_hw_config(xdev);
> +}

[Severity: Medium]
Since xdev->board_info is shared across all PCI functions of the same board,
can concurrent probes of multiple functions on the same card race in
xsc_query_guid(), xsc_activate_hw_config(), and xsc_hw_setup()?

In xsc_cmd_query_hca_cap(), board_info_lock protects the lookup and
allocation of xdev->board_info, but xsc_query_guid() and
xsc_activate_hw_config() check and update guid_valid, guid, and
hw_config_activated (and xsc_hw_setup() updates xdev->board_info->msg_opcode)
without holding board_info_lock or using memory barriers. Two PCI functions
probing in parallel can both see guid_valid == 0 or hw_config_activated == 0
and issue duplicate XSC_CMD_OP_QUERY_GUID and XSC_CMD_OP_ACTIVATE_HW_CONFIG
commands or observe guid_valid == 1 before the store to guid is visible.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104155650.37053-1-tianx@yunsilicon.com?part=4