Message-Id: <185731c2-30c7-4199-b215-21ac215811eb@yunsilicon.com>
Date: Thu, 13 Feb 2025 10:09:34 +0800
From: "tianx" <tianx@...silicon.com>
To: "Przemek Kitszel" <przemyslaw.kitszel@...el.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 2025/1/16 18:30, Przemek Kitszel wrote:
> 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
>
thank you, that's an unused flag, will be removed.
> [..]
>
>
>> +    __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?

Thanks, that's a mistake, I will standardize it to big-endian.

>
>> +};
>> +
>> +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)
>
OK
>> +};
>> +
>> +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
>
As explained above.
>> +    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
>
OK
>> +};
>> +
>> +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
>
got it
> [..]
>
>> +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?
>
the |token| field is used to generate and maintain a unique identifier 
for commands:

token = cmd->token++ % 255 + 1;

It is protected by the token_lock

>> +    unsigned long    bitmask;
>
> again, this name is so generic, that only "data" is less generic
> BTW see DECLARE_BITMAP()
Is cmd_entry_mask better? it's a bitmap recording used cmd entry.
>
>> +    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?
>
we cannot use one of the kernel's standard workqueues, because we need 
to ensure that the commands are executed in the exact order they are 
issued. Therefore, we must use a single-threaded workqueue to maintain 
the execution sequence.
>> +    struct task_struct *cq_task;
>> +    struct semaphore sem;
>
> please write a documentation of how it is used

ok, The semaphore limits the number of working commands to |max_reg_cmds|.

Each |exec_cmd| call does a |down| to acquire the semaphore before 
execution and an |up| after completion, ensuring no more than 
|max_reg_cmds| commands run at the same time.

>
>> +    int    mode;
>
> you could just embed an enum here, same size
OK, will modify
>
>> +    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)
>
got it
>>   #include <linux/pci.h>
>
> separate "header groups" by a blank line
good suggestion.
>
>> +#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
>
OK
>> +    (((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
OK
>
>> +#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?
it's 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?
>
will use 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
>
Sorry, I couldn't find one 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
>
Yes, It's set to 0xff in the fw, and I'll add a comment.
>> +        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
>
got it
> [...]
>
>> +
>> +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
>
ok
>> +    struct xsc_core_device *xdev = container_of(cmd, struct 
>> xsc_core_device, cmd);

Powered by blists - more mailing lists