| 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: <173cb6c9-18d5-4e5d-bf52-5e23653f27d1@intel.com>
Date: Wed, 18 Dec 2024 15:46:28 +0100
From: Przemek Kitszel <przemyslaw.kitszel@...el.com>
To: Xin Tian <tianx@...silicon.com>
CC: <andrew+netdev@...n.ch>, <kuba@...nel.org>, <netdev@...r.kernel.org>,
<pabeni@...hat.com>, <edumazet@...gle.com>, <davem@...emloft.net>,
<jeff.johnson@....qualcomm.com>, <weihg@...silicon.com>,
<wanry@...silicon.com>
Subject: Re: [PATCH v1 02/16] net-next/yunsilicon: Enable CMDQ
On 12/18/24 11:50, Xin Tian wrote:
> Enable cmd queue to support driver-firmware communication.
> Hardware control will be performed through cmdq mostly.
>
>
> Co-developed-by: Honggang Wei <weihg@...silicon.com>
> Co-developed-by: Lei Yan <Jacky@...silicon.com>
> Signed-off-by: Xin Tian <tianx@...silicon.com>
> +
> +#ifndef XSC_CMD_H
> +#define XSC_CMD_H
> +
> +#define CMDQ_VERSION 0x32
> +
> +#define MAX_MBOX_OUT_LEN 2048
> +
> +#define QOS_PRIO_MAX 7
> +#define QOS_DSCP_MAX 63
weird formatting
> +#define MAC_PORT_DSCP_SHIFT 6
> +#define QOS_PCP_MAX 7
> +#define DSCP_PCP_UNSET 255
> +#define MAC_PORT_PCP_SHIFT 3
> +#define XSC_MAX_MAC_NUM 8
> +#define XSC_BOARD_SN_LEN 32
> +#define MAX_PKT_LEN 9800
you don't want to collide with future defines in core kernel
> +#define XSC_RTT_CFG_QPN_MAX 32
> +
> +#define XSC_PCIE_LAT_CFG_INTERVAL_MAX 8
> +#define XSC_PCIE_LAT_CFG_HISTOGRAM_MAX 9
> +#define XSC_PCIE_LAT_EN_DISABLE 0
> +#define XSC_PCIE_LAT_EN_ENABLE 1
> +#define XSC_PCIE_LAT_PERIOD_MIN 1
> +#define XSC_PCIE_LAT_PERIOD_MAX 20
> +#define DPU_PORT_WGHT_CFG_MAX 1
> +
> +enum {
> + XSC_CMD_STAT_OK = 0x0,
> + XSC_CMD_STAT_INT_ERR = 0x1,
> + XSC_CMD_STAT_BAD_OP_ERR = 0x2,
> + XSC_CMD_STAT_BAD_PARAM_ERR = 0x3,
> + XSC_CMD_STAT_BAD_SYS_STATE_ERR = 0x4,
> + XSC_CMD_STAT_BAD_RES_ERR = 0x5,
> + XSC_CMD_STAT_RES_BUSY = 0x6,
> + XSC_CMD_STAT_LIM_ERR = 0x8,
> + XSC_CMD_STAT_BAD_RES_STATE_ERR = 0x9,
> + XSC_CMD_STAT_IX_ERR = 0xa,
> + XSC_CMD_STAT_NO_RES_ERR = 0xf,
> + XSC_CMD_STAT_BAD_INP_LEN_ERR = 0x50,
> + XSC_CMD_STAT_BAD_OUTP_LEN_ERR = 0x51,
> + XSC_CMD_STAT_BAD_QP_STATE_ERR = 0x10,
I would keep the list numerically sorted
> + XSC_CMD_STAT_BAD_PKT_ERR = 0x30,
> + XSC_CMD_STAT_BAD_SIZE_OUTS_CQES_ERR = 0x40,
> +};
> +
> +enum {
> + DPU_PORT_WGHT_TARGET_HOST,
> + DPU_PORT_WGHT_TARGET_SOC,
> + DPU_PORT_WGHT_TARGET_NUM,
> +};
> +
> +enum {
> + DPU_PRIO_WGHT_TARGET_HOST2SOC,
> + DPU_PRIO_WGHT_TARGET_SOC2HOST,
> + DPU_PRIO_WGHT_TARGET_HOSTSOC2LAG,
> + DPU_PRIO_WGHT_TARGET_NUM,
> +};
> +
> +#define XSC_AP_FEAT_UDP_SPORT_MIN 1024
> +#define XSC_AP_FEAT_UDP_SPORT_MAX 65535
there is really nothing to reuse from code networking?
> +
> +enum {
> + XSC_CMD_OP_QUERY_HCA_CAP = 0x100,
> + XSC_CMD_OP_QUERY_ADAPTER = 0x101,
> + XSC_CMD_OP_INIT_HCA = 0x102,
> + XSC_CMD_OP_TEARDOWN_HCA = 0x103,
> + XSC_CMD_OP_ENABLE_HCA = 0x104,
> + XSC_CMD_OP_DISABLE_HCA = 0x105,
> + XSC_CMD_OP_MODIFY_HCA = 0x106,
> + XSC_CMD_OP_QUERY_PAGES = 0x107,
> + XSC_CMD_OP_MANAGE_PAGES = 0x108,
> + XSC_CMD_OP_SET_HCA_CAP = 0x109,
> + XSC_CMD_OP_QUERY_CMDQ_VERSION = 0x10a,
> + XSC_CMD_OP_QUERY_MSIX_TBL_INFO = 0x10b,
> + XSC_CMD_OP_FUNCTION_RESET = 0x10c,
> + XSC_CMD_OP_DUMMY = 0x10d,
I didn't checked, but please limit the scope of added defines
to cover only what this series uses (IOW: no dead code)
[...]
> +
> +enum xsc_eth_qp_num_sel {
> + XSC_ETH_QP_NUM_8K_SEL = 0,
> + XSC_ETH_QP_NUM_8K_8TC_SEL,
> + XSC_ETH_QP_NUM_SEL_MAX,
no comma after items that you don't expect addition after
> +};
> +
> +enum xsc_eth_vf_num_sel {
> + XSC_ETH_VF_NUM_SEL_8 = 0,
> + XSC_ETH_VF_NUM_SEL_16,
> + XSC_ETH_VF_NUM_SEL_32,
> + XSC_ETH_VF_NUM_SEL_64,
> + XSC_ETH_VF_NUM_SEL_128,
> + XSC_ETH_VF_NUM_SEL_256,
> + XSC_ETH_VF_NUM_SEL_512,
> + XSC_ETH_VF_NUM_SEL_1024,
> + XSC_ETH_VF_NUM_SEL_MAX
> +};
> +
> +enum {
> + LINKSPEED_MODE_UNKNOWN = -1,
> + LINKSPEED_MODE_10G = 10000,
> + LINKSPEED_MODE_25G = 25000,
> + LINKSPEED_MODE_40G = 40000,
> + LINKSPEED_MODE_50G = 50000,
> + LINKSPEED_MODE_100G = 100000,
> + LINKSPEED_MODE_200G = 200000,
> + LINKSPEED_MODE_400G = 400000,
reminder about prefixing your enums by XSC_
would be also good to add your max supported speed into cover letter
(to get some more atteniton :))
> +};
> +
> +enum {
> + MODULE_SPEED_UNKNOWN,
> + MODULE_SPEED_10G,
> + MODULE_SPEED_25G,
> + MODULE_SPEED_40G_R4,
> + MODULE_SPEED_50G_R,
> + MODULE_SPEED_50G_R2,
> + MODULE_SPEED_100G_R2,
> + MODULE_SPEED_100G_R4,
> + MODULE_SPEED_200G_R4,
> + MODULE_SPEED_200G_R8,
> + MODULE_SPEED_400G_R8,
> +};
> +
> +enum xsc_dma_direct {
> + DMA_DIR_TO_MAC,
> + DMA_DIR_READ,
> + DMA_DIR_WRITE,
> + DMA_DIR_LOOPBACK,
> + DMA_DIR_MAX,
> +};
> +
> +/* hw feature bitmap, 32bit */
> +enum xsc_hw_feature_flag {
> + XSC_HW_RDMA_SUPPORT = 0x1,
= BIT(0)
> + XSC_HW_PFC_PRIO_STATISTIC_SUPPORT = 0x2,
= BIT(1), etc
> + XSC_HW_THIRD_FEATURE = 0x4,
> + XSC_HW_PFC_STALL_STATS_SUPPORT = 0x8,
> + XSC_HW_RDMA_CM_SUPPORT = 0x20,
> +
> + XSC_HW_LAST_FEATURE = 0x80000000,
> +};
> +
> +enum xsc_lldp_dcbx_sub_cmd {
> + XSC_OS_HANDLE_LLDP_STATUS = 0x1,
> + XSC_DCBX_STATUS
> +};
> +
> +struct xsc_inbox_hdr {
> + __be16 opcode;
> + u8 rsvd[4];
> + __be16 ver;
this is command version? (vs API as a whole, or HW ver, etc)
> +};
> +
> +struct xsc_outbox_hdr {
> + u8 status;
> + u8 rsvd[5];
> + __be16 ver;
> +};
> +
> +struct xsc_alloc_ia_lock_mbox_in {
> + struct xsc_inbox_hdr hdr;
> + u8 lock_num;
> + u8 rsvd[7];
> +};
> +
> +#define XSC_RES_NUM_IAE_GRP 16
> +
> +struct xsc_alloc_ia_lock_mbox_out {
> + struct xsc_outbox_hdr hdr;
> + u8 lock_idx[XSC_RES_NUM_IAE_GRP];
> +};
> +
> +struct xsc_release_ia_lock_mbox_in {
> + struct xsc_inbox_hdr hdr;
> + u8 lock_idx[XSC_RES_NUM_IAE_GRP];
> +};
> +
> +struct xsc_release_ia_lock_mbox_out {
> + struct xsc_outbox_hdr hdr;
> + u8 rsvd[8];
> +};
> +
> +struct xsc_pci_driver_init_params_in {
> + struct xsc_inbox_hdr hdr;
> + __be32 s_wqe_mode;
> + __be32 r_wqe_mode;
> + __be32 local_timeout_retrans;
> + u8 mac_lossless_prio[XSC_MAX_MAC_NUM];
please look up your formatting (through the series)
> + __be32 group_mod;
> +};
> +
> +struct xsc_pci_driver_init_params_out {
> + struct xsc_outbox_hdr hdr;
> + u8 rsvd[8];
> +};
> +
> +/*CQ mbox*/
> +struct xsc_cq_context {
> + __be16 eqn;
> + __be16 pa_num;
> + __be16 glb_func_id;
> + u8 log_cq_sz;
> + u8 cq_type;
> +};
> +
> +struct xsc_create_cq_mbox_in {
> + struct xsc_inbox_hdr hdr;
> + struct xsc_cq_context ctx;
> + __be64 pas[];
would be great to explain your shortcuts: eqn, pas, and all other non
obvious ones
[...]
> +/*PD mbox*/
> +struct xsc_alloc_pd_request {
> + u8 rsvd[8];
> +};
really? what this struct is for?
> +
> +struct xsc_alloc_pd_mbox_in {
> + struct xsc_inbox_hdr hdr;
> + struct xsc_alloc_pd_request req;
this is the only use of the above
[...]
> +/* vport mbox */
> +struct xsc_nic_vport_context {
> + __be32 min_wqe_inline_mode:3;
> + __be32 disable_mc_local_lb:1;
> + __be32 disable_uc_local_lb:1;
> + __be32 roce_en:1;
> +
> + __be32 arm_change_event:1;
> + __be32 event_on_mtu:1;
> + __be32 event_on_promisc_change:1;
> + __be32 event_on_vlan_change:1;
> + __be32 event_on_mc_address_change:1;
> + __be32 event_on_uc_address_change:1;
> + __be32 affiliation_criteria:4;
I guess you will have hard time reading those bitfields out
look at FIELD_GET() and similar
> + __be32 affiliated_vhca_id;
> +
> + __be16 mtu;
> +
> + __be64 system_image_guid;
> + __be64 port_guid;
> + __be64 node_guid;
> +
> + __be32 qkey_violation_counter;
> +
> + __be16 spoofchk:1;
> + __be16 trust:1;
> + __be16 promisc:1;
> + __be16 allmcast:1;
> + __be16 vlan_allowed:1;
> + __be16 allowed_list_type:3;
> + __be16 allowed_list_size:10;
> +
> + __be16 vlan_proto;
> + __be16 vlan;
> + u8 qos;
> + u8 permanent_address[6];
> + u8 current_address[6];
> + u8 current_uc_mac_address[0][2];
> +};
> +
> +enum {
> + XSC_HCA_VPORT_SEL_PORT_GUID = 1 << 0,
> + XSC_HCA_VPORT_SEL_NODE_GUID = 1 << 1,
> + XSC_HCA_VPORT_SEL_STATE_POLICY = 1 << 2,
BIT(0), BIT(1), BIT(2)
[...]
> +
> +struct xsc_array128 {
> + u8 array128[16];
> +};
both struct name and member name are wrong, this is an
u128 type basically
> +
> +struct xsc_query_hca_vport_gid_out {
> + struct xsc_outbox_hdr hdr;
> + u16 gids_num;
> + struct xsc_array128 gid[];
__counted_by(gids_num)
> +};
> +
> +struct xsc_query_hca_vport_gid_in {
> + struct xsc_inbox_hdr hdr;
> + u32 other_vport:1;
most other things you have in Big Endian, and now CPU endiannes,
it's intentional?
> + u32 port_num:4;
> + u32 vport_number:16;
> + u32 rsvd0:11;
> + u16 gid_index;
> +};
> +
> +struct xsc_pkey {
> + u16 pkey;
> +};
> +
> +struct xsc_query_hca_vport_pkey_out {
> + struct xsc_outbox_hdr hdr;
> + struct xsc_pkey pkey[];
> +};
> +
> +struct xsc_query_hca_vport_pkey_in {
> + struct xsc_inbox_hdr hdr;
> + u32 other_vport:1;
> + u32 port_num:4;
> + u32 vport_number:16;
> + u32 rsvd0:11;
> + u16 pkey_index;
> +};
> +
> +struct xsc_query_vport_state_out {
> + struct xsc_outbox_hdr hdr;
> + u8 admin_state:4;
> + u8 state:4;
> +};
> +
> +struct xsc_query_vport_state_in {
> + struct xsc_inbox_hdr hdr;
> + u32 other_vport:1;
> + u32 vport_number:16;
> + u32 rsvd0:15;
> +};
> +
> +struct xsc_modify_vport_state_out {
> + struct xsc_outbox_hdr hdr;
> +};
> +
> +struct xsc_modify_vport_state_in {
> + struct xsc_inbox_hdr hdr;
> + u32 other_vport:1;
> + u32 vport_number:16;
> + u32 rsvd0:15;
> + u8 admin_state:4;
> + u8 rsvd1:4;
> +};
> +
> +struct xsc_traffic_counter {
> + u64 packets;
> + u64 bytes;
> +};
> +
> +struct xsc_query_vport_counter_out {
> + struct xsc_outbox_hdr hdr;
> + struct xsc_traffic_counter received_errors;
> + struct xsc_traffic_counter transmit_errors;
> + struct xsc_traffic_counter received_ib_unicast;
> + struct xsc_traffic_counter transmitted_ib_unicast;
> + struct xsc_traffic_counter received_ib_multicast;
> + struct xsc_traffic_counter transmitted_ib_multicast;
> + struct xsc_traffic_counter received_eth_broadcast;
> + struct xsc_traffic_counter transmitted_eth_broadcast;
> + struct xsc_traffic_counter received_eth_unicast;
> + struct xsc_traffic_counter transmitted_eth_unicast;
> + struct xsc_traffic_counter received_eth_multicast;
> + struct xsc_traffic_counter transmitted_eth_multicast;
> +};
> +
not related to "getting counters from the vport", but please make
sure to be familiar with struct rtnl_link_stats64 for actual reporting
needs
> +#define ETH_ALEN 6
please remove
[...]
> +
> +struct xsc_event_linkstatus_resp {
> + u8 linkstatus; /*0:down, 1:up*/
> +};
> +
> +struct xsc_event_linkinfo {
> + u8 linkstatus; /*0:down, 1:up*/
> + u8 port;
> + u8 duplex;
> + u8 autoneg;
> + u32 linkspeed;
> + u64 supported;
> + u64 advertising;
> + u64 supported_fec; /* reserved, not support currently */
> + u64 advertised_fec; /* reserved, not support currently */
> + u64 supported_speed[2];
> + u64 advertising_speed[2];
> +};
heads up: please make our link speed, pause, fec code commits
clearly marked, so reviewers interested in that topic would recognize
and in general: please elaborate a bit in the commit message what it
does in terms of standard networking/kernel (not limiting to your
3-4 letter acronyms used for name of channel between your driver and HW)
this patch is so long that I will end here
[...]
> +
> +struct hwc_set_t {
add prefix xsc_ in the name
don't name structs _t, as that would be confused for typedefed one
[...]
> +
> + u8 dword_0[0x20];
> + u8 dword_1[0x20];
> + u8 dword_2[0x20];
> + u8 dword_3[0x20];
> + u8 dword_4[0x20];
> + u8 dword_5[0x20];
> + u8 dword_6[0x20];
> + u8 dword_7[0x20];
> + u8 dword_8[0x20];
> + u8 dword_9[0x20];
> + u8 dword_10[0x20];
> + u8 dword_11[0x20];
sizeof(dword) is neither sizeof(u8) nor 0x20
(bad name)
> +++ b/drivers/net/ethernet/yunsilicon/xsc/common/xsc_cmdq.h
> @@ -0,0 +1,218 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright (C) 2021-2025, Shanghai Yunsilicon Technology Co., Ltd.
> + * All rights reserved.
> + */
> +
> +#ifndef XSC_CMDQ_H
> +#define XSC_CMDQ_H
> +
> +#include "common/xsc_cmd.h"
> +
> +enum {
> + /* one minute for the sake of bringup. Generally, commands must always
outdated comment
> + * complete and we may need to increase this timeout value
> + */
> + XSC_CMD_TIMEOUT_MSEC = 10 * 1000,
> + XSC_CMD_WQ_MAX_NAME = 32,
take a look at the abundant kernel provided work queues, not need to
spawn your own most of the time
Powered by blists - more mailing lists