| 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: <20240329182816.GP651713@kernel.org>
Date: Fri, 29 Mar 2024 18:28:16 +0000
From: Simon Horman <horms@...nel.org>
To: Piotr Kwapulinski <piotr.kwapulinski@...el.com>
Cc: intel-wired-lan@...ts.osuosl.org, netdev@...r.kernel.org,
Stefan Wegrzyn <stefan.wegrzyn@...el.com>,
Jedrzej Jagielski <jedrzej.jagielski@...el.com>,
Michal Swiatkowski <michal.swiatkowski@...ux.intel.com>
Subject: Re: [PATCH iwl-next v1 1/5] ixgbe: Add support for E610 FW Admin
Command Interface
On Wed, Mar 27, 2024 at 04:54:18PM +0100, Piotr Kwapulinski wrote:
> Add low level support for Admin Command Interface (ACI). ACI is the
> Firmware interface used by a driver to communicate with E610 adapter. Add
> the following ACI features:
> - data structures, macros, register definitions
> - commands handling
> - events handling
>
> Co-developed-by: Stefan Wegrzyn <stefan.wegrzyn@...el.com>
> Signed-off-by: Stefan Wegrzyn <stefan.wegrzyn@...el.com>
> Co-developed-by: Jedrzej Jagielski <jedrzej.jagielski@...el.com>
> Signed-off-by: Jedrzej Jagielski <jedrzej.jagielski@...el.com>
> Reviewed-by: Michal Swiatkowski <michal.swiatkowski@...ux.intel.com>
> Signed-off-by: Piotr Kwapulinski <piotr.kwapulinski@...el.com>
> ---
> drivers/net/ethernet/intel/ixgbe/Makefile | 4 +-
> drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c | 505 ++++++++
> drivers/net/ethernet/intel/ixgbe/ixgbe_e610.h | 19 +
> drivers/net/ethernet/intel/ixgbe/ixgbe_type.h | 71 +-
> .../ethernet/intel/ixgbe/ixgbe_type_e610.h | 1063 +++++++++++++++++
> drivers/net/ethernet/intel/ixgbe/ixgbe_x550.h | 15 +
> 6 files changed, 1671 insertions(+), 6 deletions(-)
> create mode 100644 drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
> create mode 100644 drivers/net/ethernet/intel/ixgbe/ixgbe_e610.h
> create mode 100644 drivers/net/ethernet/intel/ixgbe/ixgbe_type_e610.h
> create mode 100644 drivers/net/ethernet/intel/ixgbe/ixgbe_x550.h
>
> diff --git a/drivers/net/ethernet/intel/ixgbe/Makefile b/drivers/net/ethernet/intel/ixgbe/Makefile
> index 4fb0d9e..e0444ae 100644
> --- a/drivers/net/ethernet/intel/ixgbe/Makefile
> +++ b/drivers/net/ethernet/intel/ixgbe/Makefile
> @@ -1,5 +1,5 @@
> # SPDX-License-Identifier: GPL-2.0
> -# Copyright(c) 1999 - 2018 Intel Corporation.
> +# Copyright(c) 1999 - 2024 Intel Corporation.
> #
> # Makefile for the Intel(R) 10GbE PCI Express ethernet driver
> #
> @@ -9,7 +9,7 @@ obj-$(CONFIG_IXGBE) += ixgbe.o
> ixgbe-objs := ixgbe_main.o ixgbe_common.o ixgbe_ethtool.o \
> ixgbe_82599.o ixgbe_82598.o ixgbe_phy.o ixgbe_sriov.o \
> ixgbe_mbx.o ixgbe_x540.o ixgbe_x550.o ixgbe_lib.o ixgbe_ptp.o \
> - ixgbe_xsk.o
> + ixgbe_xsk.o ixgbe_e610.o
>
> ixgbe-$(CONFIG_IXGBE_DCB) += ixgbe_dcb.o ixgbe_dcb_82598.o \
> ixgbe_dcb_82599.o ixgbe_dcb_nl.o
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
...
> +/**
> + * ixgbe_aci_send_cmd_execute - execute sending FW Admin Command to FW Admin
> + * Command Interface
> + * @hw: pointer to the HW struct
> + * @desc: descriptor describing the command
> + * @buf: buffer to use for indirect commands (NULL for direct commands)
> + * @buf_size: size of buffer for indirect commands (0 for direct commands)
> + *
> + * Admin Command is sent using CSR by setting descriptor and buffer in specific
> + * registers.
> + *
> + * Return: the exit code of the operation.
> + * * - 0 - success.
> + * * - -EIO - CSR mechanism is not enabled.
> + * * - -EBUSY - CSR mechanism is busy.
> + * * - -EINVAL - buf_size is too big or
> + * invalid argument buf or buf_size.
> + * * - -ETIME - Admin Command X command timeout.
> + * * - -EIO - Admin Command X invalid state of HICR register or
> + * Admin Command failed because of bad opcode was returned or
> + * Admin Command failed with error Y.
> + */
> +static int ixgbe_aci_send_cmd_execute(struct ixgbe_hw *hw,
> + struct ixgbe_aci_desc *desc,
> + void *buf, u16 buf_size)
> +{
> + u32 *tmp_buf __free(kfree) = NULL;
> + u32 *raw_desc = (u32 *)desc;
> + u32 hicr, i, tmp_buf_size;
> + bool valid_buf = false;
> + u16 opcode;
> +
> + hw->aci.last_status = IXGBE_ACI_RC_OK;
> +
> + /* It's necessary to check if mechanism is enabled */
> + hicr = IXGBE_READ_REG(hw, IXGBE_PF_HICR);
> +
> + if (!(hicr & IXGBE_PF_HICR_EN))
> + return -EIO;
> +
> + if (hicr & IXGBE_PF_HICR_C)
> + return -EBUSY;
> +
> + opcode = desc->opcode;
The type of opcode is u16, host byte order.
But the type of desc->opcode is __le16, little endien.
This does not seem right.
Flagged by Sparse.
There are a number of problems flagged by Sparse in this patch-set.
Please make sure the patchset is Sparse-clean..
> +
> + if (buf_size > IXGBE_ACI_MAX_BUFFER_SIZE)
> + return -EINVAL;
> +
> + if (buf)
> + desc->flags |= cpu_to_le16(IXGBE_ACI_FLAG_BUF);
> +
> + if (desc->flags & cpu_to_le16(IXGBE_ACI_FLAG_BUF)) {
> + if ((buf && !buf_size) ||
> + (!buf && buf_size))
> + return -EINVAL;
> + if (buf && buf_size)
> + valid_buf = true;
> + }
> +
> + if (valid_buf) {
> + if (buf_size % 4 == 0)
> + tmp_buf_size = buf_size;
> + else
> + /* Allow aligned PF_HIBA access */
> + tmp_buf_size = (buf_size & (u16)(~0x03)) + 4;
> +
> + tmp_buf = kmalloc(tmp_buf_size, GFP_KERNEL);
> + if (!tmp_buf)
> + return -ENOMEM;
> +
> + /* tmp_buf will be firstly filled with 0xFF and after
> + * that the content of buf will be written into it.
> + * This approach lets us use valid buf_size and
> + * prevents us from reading past buf area
> + * when buf_size mod 4 not equal to 0.
> + */
> + memset(tmp_buf, 0xFF, tmp_buf_size);
> + memcpy(tmp_buf, buf, buf_size);
> +
> + if (tmp_buf_size > IXGBE_ACI_LG_BUF)
> + desc->flags |= cpu_to_le16(IXGBE_ACI_FLAG_LB);
> +
> + desc->datalen = cpu_to_le16(buf_size);
> +
> + if (desc->flags & cpu_to_le16(IXGBE_ACI_FLAG_RD))
> + for (i = 0; i < tmp_buf_size / 4; i++)
> + IXGBE_WRITE_REG(hw, IXGBE_PF_HIBA(i),
> + le32_to_cpu(tmp_buf[i]));
> + }
> +
> + /* Descriptor is written to specific registers */
> + for (i = 0; i < IXGBE_ACI_DESC_SIZE_IN_DWORDS; i++)
> + IXGBE_WRITE_REG(hw, IXGBE_PF_HIDA(i),
> + le32_to_cpu(raw_desc[i]));
> +
> + /* SW has to set PF_HICR.C bit and clear PF_HICR.SV and
> + * PF_HICR_EV
> + */
> + hicr = (IXGBE_READ_REG(hw, IXGBE_PF_HICR) | IXGBE_PF_HICR_C) &
> + ~(IXGBE_PF_HICR_SV | IXGBE_PF_HICR_EV);
> + IXGBE_WRITE_REG(hw, IXGBE_PF_HICR, hicr);
> +
> +#define MAX_SLEEP_RESP_US 1000
> +#define MAX_TMOUT_RESP_SYNC_US 100000000
> +
> + /* Wait for sync Admin Command response */
> + read_poll_timeout(IXGBE_READ_REG, hicr,
> + (hicr & IXGBE_PF_HICR_SV) ||
> + !(hicr & IXGBE_PF_HICR_C),
> + MAX_SLEEP_RESP_US, MAX_TMOUT_RESP_SYNC_US, true, hw,
> + IXGBE_PF_HICR);
> +
> +#define MAX_TMOUT_RESP_ASYNC_US 150000000
> +
> + /* Wait for async Admin Command response */
> + read_poll_timeout(IXGBE_READ_REG, hicr,
> + (hicr & IXGBE_PF_HICR_EV) ||
> + !(hicr & IXGBE_PF_HICR_C),
> + MAX_SLEEP_RESP_US, MAX_TMOUT_RESP_ASYNC_US, true, hw,
> + IXGBE_PF_HICR);
> +
> + /* Read sync Admin Command response */
> + if ((hicr & IXGBE_PF_HICR_SV)) {
> + for (i = 0; i < IXGBE_ACI_DESC_SIZE_IN_DWORDS; i++) {
> + raw_desc[i] = IXGBE_READ_REG(hw, IXGBE_PF_HIDA(i));
> + raw_desc[i] = cpu_to_le32(raw_desc[i]);
> + }
> + }
> +
> + /* Read async Admin Command response */
> + if ((hicr & IXGBE_PF_HICR_EV) && !(hicr & IXGBE_PF_HICR_C)) {
> + for (i = 0; i < IXGBE_ACI_DESC_SIZE_IN_DWORDS; i++) {
> + raw_desc[i] = IXGBE_READ_REG(hw, IXGBE_PF_HIDA_2(i));
> + raw_desc[i] = cpu_to_le32(raw_desc[i]);
> + }
> + }
> +
> + /* Handle timeout and invalid state of HICR register */
> + if (hicr & IXGBE_PF_HICR_C)
> + return -ETIME;
> +
> + if (!(hicr & IXGBE_PF_HICR_SV) && !(hicr & IXGBE_PF_HICR_EV))
> + return -EIO;
> +
> + /* For every command other than 0x0014 treat opcode mismatch
> + * as an error. Response to 0x0014 command read from HIDA_2
> + * is a descriptor of an event which is expected to contain
> + * different opcode than the command.
> + */
> + if (desc->opcode != opcode &&
> + opcode != cpu_to_le16(ixgbe_aci_opc_get_fw_event))
> + return -EIO;
> +
> + if (desc->retval) {
> + hw->aci.last_status = (enum ixgbe_aci_err)desc->retval;
> + return -EIO;
> + }
> +
> + /* Write a response values to a buf */
> + if (valid_buf) {
> + for (i = 0; i < tmp_buf_size / 4; i++) {
> + tmp_buf[i] = IXGBE_READ_REG(hw, IXGBE_PF_HIBA(i));
> + tmp_buf[i] = cpu_to_le32(tmp_buf[i]);
> + }
> + memcpy(buf, tmp_buf, buf_size);
> + }
> +
> + return 0;
> +}
...
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_type_e610.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_type_e610.h
...
> +/**
> + * struct ixgbe_aq_desc - Admin Command (AC) descriptor
nit: ixgbe_aci_desc
./scripts/kernel-doc -none is your friend here
> + * @flags: IXGBE_ACI_FLAG_* flags
> + * @opcode: Admin command opcode
> + * @datalen: length in bytes of indirect/external data buffer
> + * @retval: return value from firmware
> + * @cookie_high: opaque data high-half
> + * @cookie_low: opaque data low-half
> + * @params: command-specific parameters
> + *
> + * Descriptor format for commands the driver posts via the
> + * Admin Command Interface (ACI).
> + * The firmware writes back onto the command descriptor and returns
> + * the result of the command. Asynchronous events that are not an immediate
> + * result of the command are written to the Admin Command Interface (ACI) using
> + * the same descriptor format. Descriptors are in little-endian notation with
> + * 32-bit words.
> + */
> +struct ixgbe_aci_desc {
> + __le16 flags;
> + __le16 opcode;
> + __le16 datalen;
> + __le16 retval;
> + __le32 cookie_high;
> + __le32 cookie_low;
> + union {
> + u8 raw[16];
> + struct ixgbe_aci_cmd_get_ver get_ver;
> + struct ixgbe_aci_cmd_driver_ver driver_ver;
> + struct ixgbe_aci_cmd_get_exp_err exp_err;
> + struct ixgbe_aci_cmd_req_res res_owner;
> + struct ixgbe_aci_cmd_list_caps get_cap;
> + struct ixgbe_aci_cmd_disable_rxen disable_rxen;
> + struct ixgbe_aci_cmd_get_phy_caps get_phy;
> + struct ixgbe_aci_cmd_set_phy_cfg set_phy;
> + struct ixgbe_aci_cmd_restart_an restart_an;
> + struct ixgbe_aci_cmd_get_link_status get_link_status;
> + struct ixgbe_aci_cmd_set_event_mask set_event_mask;
> + struct ixgbe_aci_cmd_get_link_topo get_link_topo;
> + struct ixgbe_aci_cmd_get_link_topo_pin get_link_topo_pin;
> + struct ixgbe_aci_cmd_sff_eeprom read_write_sff_param;
> + struct ixgbe_aci_cmd_nvm nvm;
> + struct ixgbe_aci_cmd_nvm_checksum nvm_checksum;
> + } params;
> +};
...
Powered by blists - more mailing lists