| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20250304132145.GD3666230@kernel.org>
Date: Tue, 4 Mar 2025 13:21:45 +0000
From: Simon Horman <horms@...nel.org>
To: Xin Tian <tianx@...silicon.com>
Cc: netdev@...r.kernel.org, leon@...nel.org, andrew+netdev@...n.ch,
kuba@...nel.org, pabeni@...hat.com, edumazet@...gle.com,
davem@...emloft.net, jeff.johnson@....qualcomm.com,
przemyslaw.kitszel@...el.com, weihg@...silicon.com,
wanry@...silicon.com, jacky@...silicon.com,
parthiban.veerasooran@...rochip.com, masahiroy@...nel.org
Subject: Re: [PATCH net-next v7 02/14] xsc: Enable command queue
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@...silicon.com>
> Signed-off-by: Honggang Wei <weihg@...silicon.com>
> Co-developed-by: Lei Yan <jacky@...silicon.com>
> Signed-off-by: Lei Yan <jacky@...silicon.com>
> Signed-off-by: Xin Tian <tianx@...silicon.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;
> +}
...
Powered by blists - more mailing lists