| 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: <20240531145357.GJ123401@kernel.org>
Date: Fri, 31 May 2024 15:53:57 +0100
From: Simon Horman <horms@...nel.org>
To: Piotr Kwapulinski <piotr.kwapulinski@...el.com>
Cc: intel-wired-lan@...ts.osuosl.org, netdev@...r.kernel.org,
jacob.e.keller@...el.com, Stefan Wegrzyn <stefan.wegrzyn@...el.com>,
Jedrzej Jagielski <jedrzej.jagielski@...el.com>,
Jan Glaza <jan.glaza@...el.com>
Subject: Re: [PATCH iwl-next v7 3/7] ixgbe: Add link management support for
E610 device
On Mon, May 27, 2024 at 05:10:19PM +0200, Piotr Kwapulinski wrote:
> Add low level link management support for E610 device. Link management
> operations are handled via the Admin Command Interface. Add the following
> link management operations:
> - get link capabilities
> - set up link
> - get media type
> - get link status, link status events
> - link power management
>
> 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: Jan Glaza <jan.glaza@...el.com>
> Signed-off-by: Piotr Kwapulinski <piotr.kwapulinski@...el.com>
Hi Pitor, all,
Some more minor feedback from my side.
...
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
...
> +/**
> + * ixgbe_is_media_cage_present - check if media cage is present
> + * @hw: pointer to the HW struct
> + *
> + * Identify presence of media cage using the ACI command (0x06E0).
> + *
> + * Return: true if media cage is present, else false. If no cage, then
> + * media type is backplane or BASE-T.
> + */
> +static bool ixgbe_is_media_cage_present(struct ixgbe_hw *hw)
> +{
> + struct ixgbe_aci_cmd_get_link_topo *cmd;
> + struct ixgbe_aci_desc desc;
> +
> + cmd = &desc.params.get_link_topo;
> +
> + ixgbe_fill_dflt_direct_cmd_desc(&desc, ixgbe_aci_opc_get_link_topo);
> +
> + cmd->addr.topo_params.node_type_ctx =
> + FIELD_PREP(IXGBE_ACI_LINK_TOPO_NODE_CTX_M,
> + IXGBE_ACI_LINK_TOPO_NODE_CTX_PORT);
> +
> + /* Set node type. */
> + cmd->addr.topo_params.node_type_ctx |=
> + (IXGBE_ACI_LINK_TOPO_NODE_TYPE_M &
> + IXGBE_ACI_LINK_TOPO_NODE_TYPE_CAGE);
nit: Can this also use FIELD_PREP?
> +
> + /* Node type cage can be used to determine if cage is present. If AQC
> + * returns error (ENOENT), then no cage present. If no cage present then
> + * connection type is backplane or BASE-T.
> + */
> + return ixgbe_aci_get_netlist_node(hw, cmd, NULL, NULL);
> +}
...
> +/**
> + * ixgbe_get_link_status - get status of the HW network link
> + * @hw: pointer to the HW struct
> + * @link_up: pointer to bool (true/false = linkup/linkdown)
> + *
> + * Variable link_up is true if link is up, false if link is down.
> + * The variable link_up is invalid if status is non zero. As a
> + * result of this call, link status reporting becomes enabled
> + *
> + * Return: the exit code of the operation.
> + */
> +int ixgbe_get_link_status(struct ixgbe_hw *hw, bool *link_up)
> +{
> + int err = 0;
> +
> + if (!hw || !link_up)
> + return -EINVAL;
> +
> + if (hw->link.get_link_info) {
> + err = ixgbe_update_link_info(hw);
> + if (err)
> + return err;
> + }
> +
> + *link_up = hw->link.link_info.link_info & IXGBE_ACI_LINK_UP;
> +
> + return err;
nit: If the function used "return 0" here,
then there would be no need to initialise err to 0
and the scope of err could be reduced to the if clause where
ixgbe_update_link_info() is called.
> +}
...
> +/**
> + * ixgbe_fc_autoneg_e610 - Configure flow control
> + * @hw: pointer to hardware structure
> + *
> + * Configure Flow Control.
> + */
> +void ixgbe_fc_autoneg_e610(struct ixgbe_hw *hw)
> +{
> + int err;
> +
> + /* Get current link err.
> + * Current FC mode will be stored in the hw context.
> + */
> + err = ixgbe_aci_get_link_info(hw, false, NULL);
> + if (err)
> + goto out;
> +
> + /* Check if the link is up */
> + if (!(hw->link.link_info.link_info & IXGBE_ACI_LINK_UP)) {
> + err = -EIO;
> + goto out;
> + }
> +
> + /* Check if auto-negotiation has completed */
> + if (!(hw->link.link_info.an_info & IXGBE_ACI_AN_COMPLETED)) {
> + err = -EIO;
> + goto out;
> + }
> +
> +out:
> + if (!err) {
> + hw->fc.fc_was_autonegged = true;
> + } else {
> + hw->fc.fc_was_autonegged = false;
> + hw->fc.current_mode = hw->fc.requested_mode;
> + }
> +}
As out is only jumped to from error paths, and err is not returned,
perhaps this is a bit nicer (completely untested!):
void ixgbe_fc_autoneg_e610(struct ixgbe_hw *hw)
{
int err;
/* Get current link err.
* Current FC mode will be stored in the hw context.
*/
err = ixgbe_aci_get_link_info(hw, false, NULL);
if (err)
goto no_autoneg;
/* Check if the link is up */
if (!(hw->link.link_info.link_info & IXGBE_ACI_LINK_UP))
goto no_autoneg;
/* Check if auto-negotiation has completed */
if (!(hw->link.link_info.an_info & IXGBE_ACI_AN_COMPLETED))
goto no_autoneg;
hw->fc.fc_was_autonegged = true;
return;
no_autoneg:
hw->fc.fc_was_autonegged = false;
hw->fc.current_mode = hw->fc.requested_mode;
}
> +
> +/**
> + * ixgbe_disable_rx_e610 - Disable RX unit
> + * @hw: pointer to hardware structure
> + *
> + * Disable RX DMA unit on E610 with use of ACI command (0x000C).
> + *
> + * Return: the exit code of the operation.
> + */
> +void ixgbe_disable_rx_e610(struct ixgbe_hw *hw)
> +{
> + u32 rxctrl = IXGBE_READ_REG(hw, IXGBE_RXCTRL);
> +
> + if (rxctrl & IXGBE_RXCTRL_RXEN) {
FWIIW, this could be less indented using something like:
if (!(rxctrl & IXGBE_RXCTRL_RXEN))
return;
...
> + u32 pfdtxgswc;
> + int err;
> +
> + pfdtxgswc = IXGBE_READ_REG(hw, IXGBE_PFDTXGSWC);
> + if (pfdtxgswc & IXGBE_PFDTXGSWC_VT_LBEN) {
> + pfdtxgswc &= ~IXGBE_PFDTXGSWC_VT_LBEN;
> + IXGBE_WRITE_REG(hw, IXGBE_PFDTXGSWC, pfdtxgswc);
> + hw->mac.set_lben = true;
> + } else {
> + hw->mac.set_lben = false;
> + }
> +
> + err = ixgbe_aci_disable_rxen(hw);
> +
> + /* If we fail - disable RX using register write */
> + if (err) {
> + rxctrl = IXGBE_READ_REG(hw, IXGBE_RXCTRL);
> + if (rxctrl & IXGBE_RXCTRL_RXEN) {
> + rxctrl &= ~IXGBE_RXCTRL_RXEN;
> + IXGBE_WRITE_REG(hw, IXGBE_RXCTRL, rxctrl);
> + }
> + }
> + }
> +}
...
> +/**
> + * ixgbe_setup_phy_link_e610 - Sets up firmware-controlled PHYs
> + * @hw: pointer to hardware structure
> + *
> + * Set the parameters for the firmware-controlled PHYs.
> + *
> + * Return: the exit code of the operation.
> + */
> +int ixgbe_setup_phy_link_e610(struct ixgbe_hw *hw)
> +{
> + struct ixgbe_aci_cmd_get_phy_caps_data pcaps;
> + struct ixgbe_aci_cmd_set_phy_cfg_data pcfg;
> + int err;
> +
> + err = ixgbe_aci_get_link_info(hw, false, NULL);
> + if (err)
> + goto err;
> +
> + /* Set PHY Configuration only if media is available */
> + if (!(hw->link.link_info.link_info & IXGBE_ACI_MEDIA_AVAILABLE)) {
> + err = ixgbe_aci_set_link_restart_an(hw, false);
> + if (err)
> + goto err;
> +
> + return 0;
> + }
> +
> + err = ixgbe_aci_get_phy_caps(hw, false, IXGBE_ACI_REPORT_DFLT_CFG,
> + &pcaps);
> + if (err)
> + goto err;
nit: The goto label only returns err.
So IMHO it' slightly nicer to just do that here.
And return 0 right at the end of the function.
> +
> + ixgbe_copy_phy_caps_to_cfg(&pcaps, &pcfg);
> +
> + pcfg.caps |= IXGBE_ACI_PHY_ENA_LINK;
> + pcfg.caps |= IXGBE_ACI_PHY_ENA_AUTO_LINK_UPDT;
> +
> + /* Set default PHY types for a given speed */
> + pcfg.phy_type_low = 0;
> + pcfg.phy_type_high = 0;
> +
> + if (hw->phy.autoneg_advertised & IXGBE_LINK_SPEED_10_FULL) {
> + pcfg.phy_type_high |= IXGBE_PHY_TYPE_HIGH_10BASE_T;
> + pcfg.phy_type_high |= IXGBE_PHY_TYPE_HIGH_10M_SGMII;
> + }
> + if (hw->phy.autoneg_advertised & IXGBE_LINK_SPEED_100_FULL) {
> + pcfg.phy_type_low |= IXGBE_PHY_TYPE_LOW_100BASE_TX;
> + pcfg.phy_type_low |= IXGBE_PHY_TYPE_LOW_100M_SGMII;
> + pcfg.phy_type_high |= IXGBE_PHY_TYPE_HIGH_100M_USXGMII;
> + }
> + if (hw->phy.autoneg_advertised & IXGBE_LINK_SPEED_1GB_FULL) {
> + pcfg.phy_type_low |= IXGBE_PHY_TYPE_LOW_1000BASE_T;
> + pcfg.phy_type_low |= IXGBE_PHY_TYPE_LOW_1000BASE_SX;
> + pcfg.phy_type_low |= IXGBE_PHY_TYPE_LOW_1000BASE_LX;
> + pcfg.phy_type_low |= IXGBE_PHY_TYPE_LOW_1000BASE_KX;
> + pcfg.phy_type_low |= IXGBE_PHY_TYPE_LOW_1G_SGMII;
> + pcfg.phy_type_high |= IXGBE_PHY_TYPE_HIGH_1G_USXGMII;
> + }
> + if (hw->phy.autoneg_advertised & IXGBE_LINK_SPEED_2_5GB_FULL) {
> + pcfg.phy_type_low |= IXGBE_PHY_TYPE_LOW_2500BASE_T;
> + pcfg.phy_type_low |= IXGBE_PHY_TYPE_LOW_2500BASE_X;
> + pcfg.phy_type_low |= IXGBE_PHY_TYPE_LOW_2500BASE_KX;
> + pcfg.phy_type_high |= IXGBE_PHY_TYPE_HIGH_2500M_SGMII;
> + pcfg.phy_type_high |= IXGBE_PHY_TYPE_HIGH_2500M_USXGMII;
> + }
> + if (hw->phy.autoneg_advertised & IXGBE_LINK_SPEED_5GB_FULL) {
> + pcfg.phy_type_low |= IXGBE_PHY_TYPE_LOW_5GBASE_T;
> + pcfg.phy_type_low |= IXGBE_PHY_TYPE_LOW_5GBASE_KR;
> + pcfg.phy_type_high |= IXGBE_PHY_TYPE_HIGH_5G_USXGMII;
> + }
> + if (hw->phy.autoneg_advertised & IXGBE_LINK_SPEED_10GB_FULL) {
> + pcfg.phy_type_low |= IXGBE_PHY_TYPE_LOW_10GBASE_T;
> + pcfg.phy_type_low |= IXGBE_PHY_TYPE_LOW_10G_SFI_DA;
> + pcfg.phy_type_low |= IXGBE_PHY_TYPE_LOW_10GBASE_SR;
> + pcfg.phy_type_low |= IXGBE_PHY_TYPE_LOW_10GBASE_LR;
> + pcfg.phy_type_low |= IXGBE_PHY_TYPE_LOW_10GBASE_KR_CR1;
> + pcfg.phy_type_low |= IXGBE_PHY_TYPE_LOW_10G_SFI_AOC_ACC;
> + pcfg.phy_type_low |= IXGBE_PHY_TYPE_LOW_10G_SFI_C2C;
> + pcfg.phy_type_high |= IXGBE_PHY_TYPE_HIGH_10G_USXGMII;
> + }
> +
> + /* Mask the set values to avoid requesting unsupported link types */
> + pcfg.phy_type_low &= pcaps.phy_type_low;
> + pcfg.phy_type_high &= pcaps.phy_type_high;
> +
> + err = ixgbe_aci_set_phy_cfg(hw, &pcfg);
> + if (err)
> + goto err;
> +
> + /* Request link restart - without putting it down */
> + err = ixgbe_aci_set_link_restart_an(hw, true);
> + if (err)
> + goto err;
> +
> +err:
> + return err;
> +}
...
Powered by blists - more mailing lists