| 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: <DM6PR11MB4610B0DD83E935236A0674CBF33D2@DM6PR11MB4610.namprd11.prod.outlook.com>
Date: Wed, 3 Apr 2024 07:27:21 +0000
From: "Kwapulinski, Piotr" <piotr.kwapulinski@...el.com>
To: Simon Horman <horms@...nel.org>, "intel-wired-lan@...ts.osuosl.org"
<intel-wired-lan@...ts.osuosl.org>, "netdev@...r.kernel.org"
<netdev@...r.kernel.org>
CC: "Wegrzyn, Stefan" <stefan.wegrzyn@...el.com>, "Jagielski, Jedrzej"
<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
>-----Original Message-----
>From: Simon Horman <horms@...nel.org>
>Sent: Friday, March 29, 2024 7:28 PM
>To: Kwapulinski, Piotr <piotr.kwapulinski@...el.com>
>Cc: intel-wired-lan@...ts.osuosl.org; netdev@...r.kernel.org; Wegrzyn, Stefan <stefan.wegrzyn@...el.com>; Jagielski, Jedrzej <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..
Will do, working on it.
>> +
>> + 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
Will do
>> + * @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;
>> +};
>
>...
Thank you for review
Piotr
Powered by blists - more mailing lists