| 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: <a018ba52-ced5-3886-eaa9-4bfe3d6c1215@intel.com>
Date: Wed, 17 Apr 2024 16:57:23 -0700
From: Tony Nguyen <anthony.l.nguyen@...el.com>
To: Anil Samal <anil.samal@...el.com>, <intel-wired-lan@...ts.osuosl.org>
CC: <netdev@...r.kernel.org>, <jesse.brandeburg@...el.com>,
<leszek.pepiak@...el.com>, <jacob.e.keller@...el.com>,
<przemyslaw.kitszel@...el.com>, <lukasz.czapnik@...el.com>
Subject: Re: [PATCH iwl-next 1/4] ice: Implement new API to derive physical
topology of input port
On 4/12/2024 11:49 AM, Anil Samal wrote:
> Some phy configurations such as serdes equalizer parameters, are applied
> per serdes lane. Hence firmware requires serdes lane number to read
> serdes equalizer values. Similarly firmware requires PCS quad number
> and PCS port number to read FEC statistics. Current driver
> implementation does not maintain above physical properties of a port.
>
> Add new driver API to derive physical properties of an input port. These
> properties include PCS quad number, PCS port number, serdes lane count,
> primary serdes lane number.
>
> Reviewed-by: Jesse Brandeburg <jesse.brandeburg@...el.com>
> Signed-off-by: Anil Samal <anil.samal@...el.com>
> ---
> drivers/net/ethernet/intel/ice/ice_ethtool.c | 265 ++++++++++++++++++
> drivers/net/ethernet/intel/ice/ice_ethtool.h | 10 +
> .../net/ethernet/intel/ice/ice_hw_autogen.h | 2 +
> 3 files changed, 277 insertions(+)
>
> diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c b/drivers/net/ethernet/intel/ice/ice_ethtool.c
> index 78b833b3e1d7..6884b45c3b0a 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ethtool.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c
> @@ -466,6 +466,271 @@ static int ice_get_regs_len(struct net_device __always_unused *netdev)
> return sizeof(ice_regs_dump_list);
> }
>
> +/**
> + * ice_ethtool_get_maxspeed - Get the max speed for given lport
> + * @hw: pointer to the HW struct
> + * @lport: logical port for which max speed is requested
> + * @max_speed: return max speed for input lport
> + *
> + * Returns 0 on success, negative on failure.
This needs to be updated for kernel-doc -Wall. Please check others in
the patches/series as well.
> + */
> +static int ice_ethtool_get_maxspeed(struct ice_hw *hw, u8 lport, u8 *max_speed)
> +{
> + struct ice_aqc_get_port_options_elem options[ICE_AQC_PORT_OPT_MAX] = {};
> + bool active_valid = false, pending_valid = true;
> + u8 option_count = ICE_AQC_PORT_OPT_MAX;
> + u8 active_idx = 0, pending_idx = 0;
> + int status = 0;
initialization isn't needed as it's overwritten before use.
> +
> + if (!max_speed || !hw)
> + return -EINVAL;
This defensive check is unneccesary as hw is being checked by caller and
max_speed is always provided.
> +
> + *max_speed = 0;
> +
> + status = ice_aq_get_port_options(hw, options, &option_count, lport,
> + true, &active_idx, &active_valid,
> + &pending_idx, &pending_valid);
> +
> + if (status) {
Please no newline between function call and error check.
> + ice_debug(hw, ICE_DBG_PHY, "Port split read err: %d\n", status);
> + return -EIO;
> + }
> +
> + if (active_valid) {
> + ice_debug(hw, ICE_DBG_PHY, "Active idx: %d\n", active_idx);
> + } else {
> + ice_debug(hw, ICE_DBG_PHY, "No valid Active option\n");
> + return -EINVAL;
> + }
> + *max_speed = options[active_idx].max_lane_speed & ICE_AQC_PORT_OPT_MAX_LANE_M;
> + return 0;
> +}
> +
> +/**
> + * ice_is_serdes_muxed - returns whether serdes is muxed in hardware
> + * @hw: pointer to the HW struct
> + *
> + * Returns True : when serdes is muxed, False: when serdes is not muxed
> + */
> +static bool ice_is_serdes_muxed(struct ice_hw *hw)
> +{
> + u32 reg_value = rd32(hw, GLGEN_SWITCH_MODE_CONFIG);
> +
> + return FIELD_GET(GLGEN_SWITCH_MODE_CONFIG_SELECT_25X4_ON_SINGLE_QUAD_M,
> + reg_value);
> +}
> +
> +/**
> + * ice_map_port_topology_for_sfp - Fills port topology with pcsquad, pcsport,
> + * primary serdes lane number
> + * @port_topology: buffer to hold port topology
> + * @lport: logical port for which physical info requested
> + * @is_muxed: logical port for which physical info requested
> + *
> + * Returns 0 on success, negative on failure.
> + */
> +static int ice_map_port_topology_for_sfp(struct ice_port_topology *port_topology,
> + u8 lport, bool is_muxed)
> +{
> + if (!port_topology)
> + return -EINVAL;
The is already checked by caller and accessed before calling so it can't
be NULL here.
> +
> + switch (lport) {
> + case 0:
> + port_topology->pcs_quad_select = 0;
> + port_topology->pcs_port = 0;
> + port_topology->primary_serdes_lane = 0;
> + break;
> + case 1:
> + port_topology->pcs_quad_select = 1;
> + port_topology->pcs_port = 0;
> + if (is_muxed)
> + port_topology->primary_serdes_lane = 2;
> + else
> + port_topology->primary_serdes_lane = 4;
> + break;
> + case 2:
> + port_topology->pcs_quad_select = 0;
> + port_topology->pcs_port = 1;
> + port_topology->primary_serdes_lane = 1;
> + break;
> + case 3:
> + port_topology->pcs_quad_select = 1;
> + port_topology->pcs_port = 1;
> + if (is_muxed)
> + port_topology->primary_serdes_lane = 3;
> + else
> + port_topology->primary_serdes_lane = 5;
> + break;
> + case 4:
> + port_topology->pcs_quad_select = 0;
> + port_topology->pcs_port = 2;
> + port_topology->primary_serdes_lane = 2;
> + break;
> + case 5:
> + port_topology->pcs_quad_select = 1;
> + port_topology->pcs_port = 2;
> + port_topology->primary_serdes_lane = 6;
> + break;
> + case 6:
> + port_topology->pcs_quad_select = 0;
> + port_topology->pcs_port = 3;
> + port_topology->primary_serdes_lane = 3;
> + break;
> + case 7:
> + port_topology->pcs_quad_select = 1;
> + port_topology->pcs_port = 3;
> + port_topology->primary_serdes_lane = 7;
> + break;
> + default:
> + return -EINVAL;
> + }
> + return 0;
> +}
> +
> +/**
> + * ice_map_port_topology_for_qsfp - Fills port topology with pcsquad, pcsport,
> + * primary serdes lane number
> + * @port_topology: buffer to hold port topology
> + * @lport: logical port for which physical info requested
> + * @is_muxed: logical port for which physical info requested
> + *
> + * Returns 0 on success, negative on failure.
> + */
> +static int ice_map_port_topology_for_qsfp(struct ice_port_topology *port_topology,
> + u8 lport, bool is_muxed)
> +{
> + if (!port_topology)
> + return -EINVAL;
Same here. Can you check for unneeded defensive checks on the rest of
the series. If the function is static and we've already used, checked,
or always provide a value we can skip these extra checks in the call chain.
> +
> + switch (lport) {
> + case 0:
> + port_topology->pcs_quad_select = 0;
> + port_topology->pcs_port = 0;
> + port_topology->primary_serdes_lane = 0;
> + break;
> + case 1:
> + port_topology->pcs_quad_select = 1;
> + port_topology->pcs_port = 0;
> + if (is_muxed)
> + port_topology->primary_serdes_lane = 2;
> + else
> + port_topology->primary_serdes_lane = 4;
> + break;
> + case 2:
> + port_topology->pcs_quad_select = 0;
> + port_topology->pcs_port = 1;
> + port_topology->primary_serdes_lane = 1;
> + break;
> + case 3:
> + port_topology->pcs_quad_select = 1;
> + port_topology->pcs_port = 1;
> + if (is_muxed)
> + port_topology->primary_serdes_lane = 3;
> + else
> + port_topology->primary_serdes_lane = 5;
> + break;
> + case 4:
> + port_topology->pcs_quad_select = 0;
> + port_topology->pcs_port = 2;
> + port_topology->primary_serdes_lane = 2;
> + break;
> + case 5:
> + port_topology->pcs_quad_select = 1;
> + port_topology->pcs_port = 2;
> + port_topology->primary_serdes_lane = 6;
> + break;
> + case 6:
> + port_topology->pcs_quad_select = 0;
> + port_topology->pcs_port = 3;
> + port_topology->primary_serdes_lane = 3;
> + break;
> + case 7:
> + port_topology->pcs_quad_select = 1;
> + port_topology->pcs_port = 3;
> + port_topology->primary_serdes_lane = 7;
> + break;
> + default:
> + return -EINVAL;
> + }
> + return 0;
> +}
> +
> +/**
> + * ice_get_port_topology - returns physical topology like pcsquad, pcsport,
> + * serdes number
> + * @hw: pointer to the HW struct
> + * @lport: logical port for which physical info requested
> + * @port_topology: buffer to hold port topology
> + *
> + * Returns 0 on success, negative on failure.
> + */
> +static int ice_get_port_topology(struct ice_hw *hw, u8 lport,
> + struct ice_port_topology *port_topology)
This is reported as unused. The series probably needs to reorganized or
patches squashed to avoid these issues/warnings:
+../drivers/net/ethernet/intel/ice/ice_ethtool.c:668:12: warning: unused
function 'ice_get_port_topology' [-Wunused-function]
> +{
> + struct ice_aqc_get_link_topo cmd = {};
> + u16 node_handle = 0;
> + u8 cage_type = 0;
> + bool is_muxed;
> + int err;
> + u8 ctx;
> +
> + if (!hw || !port_topology)
> + return -EINVAL;
> +
> + ctx = ICE_AQC_LINK_TOPO_NODE_TYPE_CAGE << ICE_AQC_LINK_TOPO_NODE_TYPE_S;
> + ctx |= ICE_AQC_LINK_TOPO_NODE_CTX_PORT << ICE_AQC_LINK_TOPO_NODE_CTX_S;
> + cmd.addr.topo_params.node_type_ctx = ctx;
> +
> + err = ice_aq_get_netlist_node(hw, &cmd, &cage_type, &node_handle);
> + if (err)
> + return -EINVAL;
> +
> + is_muxed = ice_is_serdes_muxed(hw);
> + if (hw->device_id >= ICE_DEV_ID_E810_XXV_BACKPLANE) {
What are you trying to check? I don't believe you can infer anything
based on the device id's value in relation to values of other device ids.
> + port_topology->serdes_lane_count = 1;
> + if (lport == 0) {
> + port_topology->pcs_quad_select = 0;
> + port_topology->pcs_port = 0;
> + port_topology->primary_serdes_lane = 0;
> + } else if (lport == 1) {
> + port_topology->pcs_quad_select = 1;
> + port_topology->pcs_port = 0;
> + port_topology->primary_serdes_lane = 1;
> + } else {
> + return -EINVAL;
> + }
> + } else {
> + if (cage_type == 0x11 || /* SFP+ */
> + cage_type == 0x12) { /* SFP28 */
> + port_topology->serdes_lane_count = 1;
> + err = ice_map_port_topology_for_sfp(port_topology, lport, is_muxed);
> + if (err)
> + return err;
> + } else if (cage_type == 0x13 || /* QSFP */
> + cage_type == 0x14) { /* QSFP28 */
> + u8 max_speed = 0;
> +
> + err = ice_ethtool_get_maxspeed(hw, lport, &max_speed);
> + if (err)
> + return err;
> + if (max_speed == ICE_AQC_PORT_OPT_MAX_LANE_100G)
> + port_topology->serdes_lane_count = 4;
> + else if (max_speed == ICE_AQC_PORT_OPT_MAX_LANE_50G)
> + port_topology->serdes_lane_count = 2;
> + else
> + port_topology->serdes_lane_count = 1;
> +
> + err = ice_map_port_topology_for_qsfp(port_topology, lport, is_muxed);
> + if (err)
> + return err;
> + } else {
> + return -EINVAL;
> + }
> + }
> + return 0;
> +}
> +
> static void
> ice_get_regs(struct net_device *netdev, struct ethtool_regs *regs, void *p)
> {
> diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.h b/drivers/net/ethernet/intel/ice/ice_ethtool.h
> index b88e3da06f13..ffc8ad180e61 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ethtool.h
> +++ b/drivers/net/ethernet/intel/ice/ice_ethtool.h
> @@ -9,6 +9,16 @@ struct ice_phy_type_to_ethtool {
> u8 link_mode;
> };
>
> +/* Port topology from lport i.e.
> + * serdes mapping, pcsquad, macport, cage etc...
> + */
> +struct ice_port_topology {
> + u16 pcs_port;
> + u16 primary_serdes_lane;
> + u16 serdes_lane_count;
> + u16 pcs_quad_select;
> +};
> +
> /* Macro to make PHY type to Ethtool link mode table entry.
> * The index is the PHY type.
> */
> diff --git a/drivers/net/ethernet/intel/ice/ice_hw_autogen.h b/drivers/net/ethernet/intel/ice/ice_hw_autogen.h
> index cfac1d432c15..6604baa37c4a 100644
> --- a/drivers/net/ethernet/intel/ice/ice_hw_autogen.h
> +++ b/drivers/net/ethernet/intel/ice/ice_hw_autogen.h
> @@ -157,6 +157,8 @@
> #define GLGEN_RTRIG_CORER_M BIT(0)
> #define GLGEN_RTRIG_GLOBR_M BIT(1)
> #define GLGEN_STAT 0x000B612C
> +#define GLGEN_SWITCH_MODE_CONFIG 0x000B81E0 /* Reset Source: POR */
> +#define GLGEN_SWITCH_MODE_CONFIG_SELECT_25X4_ON_SINGLE_QUAD_M BIT(2)
Any way to make this shorter? It's length is a bit unwiedly
> #define GLGEN_VFLRSTAT(_i) (0x00093A04 + ((_i) * 4))
> #define PFGEN_CTRL 0x00091000
> #define PFGEN_CTRL_PFSWR_M BIT(0)
Thanks,
Tony
Powered by blists - more mailing lists