From: Jeff Daly <jeffd@silicom-usa.com>
To: Jeff Daly <jeffd@silicom-usa.com>, "dev@dpdk.org" <dev@dpdk.org>
Cc: "stable@dpdk.org" <stable@dpdk.org>,
Stephen Douthit <stephend@silicom-usa.com>,
Haiyue Wang <haiyue.wang@intel.com>
Subject: RE: [PATCH v4 3/3] net/ixgbe: Fix SFP detection and linking on hotplug
Date: Sat, 12 Mar 2022 13:03:07 +0000 [thread overview]
Message-ID: <AM0PR0402MB35060FEEC09D00733F089225EA0D9@AM0PR0402MB3506.eurprd04.prod.outlook.com> (raw)
In-Reply-To: <20220228152937.21247-4-jeffd@silicom-usa.com>
> -----Original Message-----
> From: Jeff Daly <jeffd@silicom-usa.com>
> Sent: Monday, February 28, 2022 10:30 AM
> To: dev@dpdk.org
> Cc: stable@dpdk.org; Stephen Douthit <stephend@silicom-usa.com>; Haiyue
> Wang <haiyue.wang@intel.com>
> Subject: [PATCH v4 3/3] net/ixgbe: Fix SFP detection and linking on hotplug
>
> Caution: This is an external email. Please take care when clicking links or
> opening attachments.
>
>
> Currently the ixgbe driver does not ID any SFP except for the first one plugged in.
> This can lead to no-link, or incorrect speed conditions.
>
> For example:
>
> * If link is initially established with a 1G SFP, and later a 1G/10G multispeed part
> is later installed, then the MAC link setup functions are never called to change
> from 1000BASE-X to 10GBASE-R mode, and the link stays running at the slower
> rate.
>
> * If link is initially established with a 1G SFP, and later a 10G only module is later
> installed, no link is established, since we are still trasnsmitting in 1000BASE-X
> mode to a 10GBASE-R only partner.
>
> Refactor the SFP ID/setup, and link setup code, to more closely match the flow
> of the mainline kernel driver which does not have these issues. In that driver a
> service task runs periodically to handle these operations based on bit flags that
> have been set (usually via interrupt or userspace request), and then get cleared
> once the requested subtask has been completed.
>
> Fixes: af75078fece ("first public release")
> Cc: stable@dpdk.org
>
> Signed-off-by: Stephen Douthit <stephend@silicom-usa.com>
> Signed-off-by: Jeff Daly <jeffd@silicom-usa.com>
> ---
> drivers/net/ixgbe/ixgbe_ethdev.c | 499 +++++++++++++++++++++++--------
> drivers/net/ixgbe/ixgbe_ethdev.h | 14 +-
> 2 files changed, 392 insertions(+), 121 deletions(-)
>
> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
> index e8f07cb405..b70985bb1d 100644
> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> @@ -229,9 +229,6 @@ static int ixgbe_dev_interrupt_get_status(struct
> rte_eth_dev *dev); static int ixgbe_dev_interrupt_action(struct rte_eth_dev
> *dev); static void ixgbe_dev_interrupt_handler(void *param); static void
> ixgbe_dev_interrupt_delayed_handler(void *param); -static void
> *ixgbe_dev_setup_link_thread_handler(void *param); -static int
> ixgbe_dev_wait_setup_link_complete(struct rte_eth_dev *dev,
> - uint32_t timeout_ms);
>
> static int ixgbe_add_rar(struct rte_eth_dev *dev,
> struct rte_ether_addr *mac_addr, @@ -766,6 +763,33 @@ static
> const struct rte_ixgbe_xstats_name_off rte_ixgbevf_stats_strings[] = { #define
> IXGBEVF_NB_XSTATS (sizeof(rte_ixgbevf_stats_strings) / \
> sizeof(rte_ixgbevf_stats_strings[0]))
>
> +/**
> + * This function is the same as ixgbe_need_crosstalk_fix() in
> +base/ixgbe_common.c
> + *
> + * ixgbe_need_crosstalk_fix - Determine if we need to do cross talk fix
> + * @hw: pointer to hardware structure
> + *
> + * Contains the logic to identify if we need to verify link for the
> + * crosstalk fix
> + **/
> +static bool ixgbe_need_crosstalk_fix(struct ixgbe_hw *hw) {
> + /* Does FW say we need the fix */
> + if (!hw->need_crosstalk_fix)
> + return false;
> +
> + /* Only consider SFP+ PHYs i.e. media type fiber */
> + switch (ixgbe_get_media_type(hw)) {
> + case ixgbe_media_type_fiber:
> + case ixgbe_media_type_fiber_qsfp:
> + break;
> + default:
> + return false;
> + }
> +
> + return true;
> +}
> +
> /*
> * This function is the same as ixgbe_is_sfp() in base/ixgbe.h.
> */
> @@ -1032,6 +1056,306 @@ ixgbe_swfw_lock_reset(struct ixgbe_hw *hw)
> ixgbe_release_swfw_semaphore(hw, mask); }
>
> +/**
> + * ixgbe_check_sfp_cage - Find present status of SFP module
> + * @hw: pointer to hardware structure
> + *
> + * Find if a SFP module is present and if this device supports SFPs
> +**/ enum ixgbe_sfp_cage_status ixgbe_check_sfp_cage(struct ixgbe_hw
> +*hw) {
> + enum ixgbe_sfp_cage_status sfp_cage_status;
> +
> + /* If we're not a fiber/fiber_qsfp, no cage to check */
> + switch (hw->mac.ops.get_media_type(hw)) {
> + case ixgbe_media_type_fiber:
> + case ixgbe_media_type_fiber_qsfp:
> + break;
> + default:
> + return IXGBE_SFP_CAGE_NOCAGE;
> + }
> +
> + switch (hw->mac.type) {
> + case ixgbe_mac_82599EB:
> + sfp_cage_status = !!(IXGBE_READ_REG(hw, IXGBE_ESDP) &
> + IXGBE_ESDP_SDP2);
> + break;
> + case ixgbe_mac_X550EM_x:
> + case ixgbe_mac_X550EM_a:
> + /* SDP0 is the active low signal PRSNT#, so invert this */
> + sfp_cage_status = !(IXGBE_READ_REG(hw, IXGBE_ESDP) &
> + IXGBE_ESDP_SDP0);
> + break;
> + default:
> + /* Don't know how to check this device type yet */
> + sfp_cage_status = IXGBE_SFP_CAGE_UNKNOWN;
> + DEBUGOUT("IXGBE_SFP_CAGE_UNKNOWN, unknown mac type
> %d\n",
> + hw->mac.type);
> + break;
> + }
> +
> + DEBUGOUT("sfp status %d for mac type %d\n", sfp_cage_status, hw-
> >mac.type);
> + return sfp_cage_status;
> +}
> +
> +static s32
> +ixgbe_sfp_id_and_setup(struct rte_eth_dev *dev) {
> + struct ixgbe_hw *hw =
> + IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> + enum ixgbe_sfp_cage_status sfp_cage_status;
> + s32 err;
> +
> + /* Can't ID or setup SFP if it's not plugged in */
> + sfp_cage_status = ixgbe_check_sfp_cage(hw);
> + if (sfp_cage_status == IXGBE_SFP_CAGE_EMPTY ||
> + sfp_cage_status == IXGBE_SFP_CAGE_NOCAGE)
> + return IXGBE_ERR_SFP_NOT_PRESENT;
> +
> + /* Something's in the cage, ID it */
> + hw->phy.ops.identify_sfp(hw);
> +
> + /* Unknown module type, give up */
> + if (hw->phy.sfp_type == ixgbe_sfp_type_unknown) {
> + PMD_DRV_LOG(ERR, "unknown SFP type, giving up");
> + return IXGBE_ERR_SFP_NOT_SUPPORTED;
> + }
> +
> + /* This should be a redundant check, since we looked at the
> + * PRSNT# signal from the cage above, but just in case this is
> + * an SFP that's slow to respond to I2C pokes correctly, try it
> + * again later
> + */
> + if (hw->phy.sfp_type == ixgbe_sfp_type_not_present) {
> + PMD_DRV_LOG(ERR, "IDed SFP as absent but cage PRSNT# active!?");
> + return IXGBE_ERR_SFP_NOT_PRESENT;
> + }
> +
> + /* SFP is present and identified, try to set it up */
> + err = hw->mac.ops.setup_sfp(hw);
> + if (err)
> + PMD_DRV_LOG(ERR, "setup_sfp() failed %d", err);
> +
> + return err;
> +}
> +
> +static void
> +ixgbe_sfp_service(struct rte_eth_dev *dev) {
> + struct ixgbe_hw *hw =
> + IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> + struct ixgbe_interrupt *intr =
> + IXGBE_DEV_PRIVATE_TO_INTR(dev->data->dev_private);
> + enum ixgbe_sfp_cage_status sfp_cage_status;
> + s32 err;
> + u8 sff_id;
> + bool have_int = false;
> +
> + /* If there's no module cage, then there's nothing to service */
> + sfp_cage_status = ixgbe_check_sfp_cage(hw);
> + if (sfp_cage_status == IXGBE_SFP_CAGE_NOCAGE) {
> + PMD_DRV_LOG(DEBUG, "No SFP to service\n");
> + return;
> + }
> +
> + /* TODO - Even for platforms where ixgbe_check_sfp_cage() gives a clear
> + * status result, if there's no interrupts, or no interrupt for the SFP
> + * cage present pin, even if other interrupts exist, then we still need
> + * to poll here to set the flag.
> + */
> +#ifndef RTE_EXEC_ENV_FREEBSD
> + struct rte_pci_device *pci_dev = RTE_ETH_DEV_TO_PCI(dev);
> + struct rte_intr_handle *intr_handle = pci_dev->intr_handle;
> + if (rte_intr_allow_others(intr_handle)) {
> + /* check if lsc interrupt is enabled */
> + if (dev->data->dev_conf.intr_conf.lsc)
> + have_int = true;
> + }
> +#endif /* #ifdef RTE_EXEC_ENV_FREEBSD */
> +
> + if (!have_int && sfp_cage_status == IXGBE_SFP_CAGE_EMPTY) {
> + intr->flags |= IXGBE_FLAG_NEED_SFP_SETUP;
> + PMD_DRV_LOG(DEBUG, "No SFP, no LSC, set NEED_SFP_SETUP\n");
> + }
> +
> + /* For platforms that don't have a way to read the PRESENT# signal from
> + * the SFP cage, fallback to doing an I2C read and seeing if it's ACKed
> + * to determine if a module is present
> + */
> + if (sfp_cage_status == IXGBE_SFP_CAGE_UNKNOWN) {
> + PMD_DRV_LOG(DEBUG,
> + "SFP present unknown (int? %d), try I2C read\n",
> + have_int);
> +
> + /* Rather than calling identify_sfp, which will read a lot of I2C
> + * registers (and in a slow processor intensive fashion due to
> + * bit-banging, just read the SFF ID register, which is at a
> + * common address across SFP/SFP+/QSFP modules and see if
> + * there's a NACK. This works since we only expect a NACK if no
> + * module is present
> + */
> + err = ixgbe_read_i2c_eeprom(hw, IXGBE_SFF_IDENTIFIER, &sff_id);
> + if (err != IXGBE_SUCCESS) {
> + PMD_DRV_LOG(DEBUG, "Received I2C NAK from SFP, set
> NEED_SFP_SETUP flag\n");
> + intr->flags |= IXGBE_FLAG_NEED_SFP_SETUP;
> + sfp_cage_status = IXGBE_SFP_CAGE_EMPTY;
> + } else {
> + PMD_DRV_LOG(DEBUG, "SFP ID read ACKed");
> + sfp_cage_status = IXGBE_SFP_CAGE_FULL;
> + }
> + }
> +
> + if (sfp_cage_status == IXGBE_SFP_CAGE_EMPTY) {
> + PMD_DRV_LOG(DEBUG, "SFP absent, cage_status %d\n",
> sfp_cage_status);
> + return;
> + }
> +
> + /* No setup requested? Nothing to do */
> + if (!(intr->flags & IXGBE_FLAG_NEED_SFP_SETUP))
> + return;
> +
> + err = ixgbe_sfp_id_and_setup(dev);
> + if (err) {
> + PMD_DRV_LOG(DEBUG, "failed to ID & setup SFP %d", err);
> + return;
> + }
> +
> + /* Setup is done, clear the flag, but make sure link config runs for new SFP
> */
> + intr->flags &= ~IXGBE_FLAG_NEED_SFP_SETUP;
> + intr->flags |= IXGBE_FLAG_NEED_LINK_CONFIG;
> +
> + /*
> + * Since this is a new SFP, clear the old advertised speed mask so we don't
> + * end up using an old slower rate
> + */
> + hw->phy.autoneg_advertised = 0;
> +}
> +
> +static void
> +ixgbe_link_service(struct rte_eth_dev *dev) {
> + struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data-
> >dev_private);
> + struct ixgbe_interrupt *intr =
> + IXGBE_DEV_PRIVATE_TO_INTR(dev->data->dev_private);
> + bool link_up, autoneg = false, have_int = false;
> + u32 speed;
> + s32 err;
> +
> + /* Test if we have a LSC interrupt for this platform, if not we need to
> + * manually check the link register since IXGBE_FLAG_NEED_LINK_CONFIG
> + * will never be set in the interrupt handler
> + */
> +#ifndef RTE_EXEC_ENV_FREEBSD
> + struct rte_pci_device *pci_dev = RTE_ETH_DEV_TO_PCI(dev);
> + struct rte_intr_handle *intr_handle = pci_dev->intr_handle;
> + if (rte_intr_allow_others(intr_handle)) {
> + /* check if lsc interrupt is enabled */
> + if (dev->data->dev_conf.intr_conf.lsc)
> + have_int = true;
> + }
> +#endif /* #ifdef RTE_EXEC_ENV_FREEBSD */
> +
> + /* Skip if we still need to setup an SFP, or if no link config requested
> + */
> + if ((intr->flags & IXGBE_FLAG_NEED_SFP_SETUP) ||
> + (!(intr->flags & IXGBE_FLAG_NEED_LINK_CONFIG) && have_int))
> + return;
> +
> + if (!have_int && !(intr->flags & IXGBE_FLAG_NEED_LINK_CONFIG)) {
> + err = ixgbe_check_link(hw, &speed, &link_up, 0);
> + if (!err && !link_up) {
> + intr->flags |= IXGBE_FLAG_NEED_LINK_CONFIG;
> + PMD_DRV_LOG(DEBUG, "Link down, no LSC, set
> NEED_LINK_CONFIG\n");
> + } else {
> + return;
> + }
> + }
> +
> + speed = hw->phy.autoneg_advertised;
> + if (!speed)
> + ixgbe_get_link_capabilities(hw, &speed, &autoneg);
> +
> + err = ixgbe_setup_link(hw, speed, true);
> + if (err) {
> + PMD_DRV_LOG(ERR, "ixgbe_setup_link failed %d", err);
> + return;
> + }
> +
> + intr->flags &= ~IXGBE_FLAG_NEED_LINK_CONFIG; }
> +
> +static void
> +ixgbe_link_update_service(struct rte_eth_dev *dev) {
> + /* Update internal link status, waiting for link */
> + if (!ixgbe_dev_link_update(dev, 0)) {
> + ixgbe_dev_link_status_print(dev);
> + rte_eth_dev_callback_process(dev, RTE_ETH_EVENT_INTR_LSC,
> NULL);
> + }
> +}
> +
> +/*
> + * Service task thread to handle periodic tasks */ static void *
> +ixgbe_dev_service_thread_handler(void *param) {
> + struct rte_eth_dev *dev = (struct rte_eth_dev *)param;
> + struct ixgbe_hw *hw =
> + IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> + uint64_t start, ticks, service_ms;
> + uint32_t speed;
> + s32 err;
> + bool link_up;
> +
> + while (1) {
> + ixgbe_sfp_service(dev);
> + ixgbe_link_service(dev);
> + ixgbe_link_update_service(dev);
> +
> + /* Run the service thread handler more frequently when link is
> + * down to reduce link up latency (every 200ms vs 1s)
> + *
> + * Use a number of smaller sleeps to decrease exit latency when
> + * ixgbe_dev_stop() wants this thread to join
> + */
> + err = ixgbe_check_link(hw, &speed, &link_up, 0);
> + if (err == IXGBE_SUCCESS && link_up)
> + service_ms = 2000;
> + else
> + service_ms = 100;
> +
> + /* Call msec_delay in a loop with several smaller sleeps to
> + * provide periodic thread cancellation points
> + */
> + start = rte_get_timer_cycles();
> + ticks = (uint64_t)service_ms * rte_get_timer_hz() / 1E3;
> + while ((rte_get_timer_cycles() - start) < ticks)
> + msec_delay(100);
> + }
> +
> + /* Never return */
> + return NULL;
> +}
> +
> +static s32
> +eth_ixgbe_check_mac_link_generic(struct ixgbe_hw *hw, ixgbe_link_speed
> *speed,
> + bool *link_up, bool
> +link_up_wait_to_complete) {
> + if (ixgbe_need_crosstalk_fix(hw)) {
> + enum ixgbe_sfp_cage_status sfp_cage_status;
> +
> + sfp_cage_status = ixgbe_check_sfp_cage(hw);
> + if (sfp_cage_status != IXGBE_SFP_CAGE_FULL) {
> + *link_up = false;
> + *speed = IXGBE_LINK_SPEED_UNKNOWN;
> + return IXGBE_SUCCESS;
> + }
> + }
> +
> + return ixgbe_check_mac_link_generic(hw, speed, link_up,
> +link_up_wait_to_complete); }
> +
> /*
> * This function is based on code in ixgbe_attach() in base/ixgbe.c.
> * It returns 0 on success.
> @@ -1054,6 +1378,7 @@ eth_ixgbe_dev_init(struct rte_eth_dev *eth_dev, void
> *init_params __rte_unused)
> IXGBE_DEV_PRIVATE_TO_FILTER_INFO(eth_dev->data->dev_private);
> struct ixgbe_bw_conf *bw_conf =
> IXGBE_DEV_PRIVATE_TO_BW_CONF(eth_dev->data->dev_private);
> + struct ixgbe_mac_info *mac = &hw->mac;
> uint32_t ctrl_ext;
> uint16_t csum;
> int diag, i, ret;
> @@ -1124,6 +1449,17 @@ eth_ixgbe_dev_init(struct rte_eth_dev *eth_dev,
> void *init_params __rte_unused)
> return -EIO;
> }
>
> + /* override mac_link_check to check for sfp cage full/empty */
> + switch (hw->mac.type) {
> + case ixgbe_mac_X550EM_x:
> + case ixgbe_mac_X550EM_a:
> + case ixgbe_mac_82599EB:
> + mac->ops.check_link = eth_ixgbe_check_mac_link_generic;
> + break;
> + default:
> + break;
> + }
> +
> /* pick up the PCI bus settings for reporting later */
> ixgbe_get_bus_info(hw);
>
> @@ -2558,8 +2894,11 @@ ixgbe_flow_ctrl_enable(struct rte_eth_dev *dev,
> struct ixgbe_hw *hw) static int ixgbe_dev_start(struct rte_eth_dev *dev) {
> + struct ixgbe_adapter *ad = dev->data->dev_private;
> struct ixgbe_hw *hw =
> IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> + struct ixgbe_interrupt *intr =
> + IXGBE_DEV_PRIVATE_TO_INTR(dev->data->dev_private);
> struct ixgbe_vf_info *vfinfo =
> *IXGBE_DEV_PRIVATE_TO_P_VFDATA(dev->data->dev_private);
> struct rte_pci_device *pci_dev = RTE_ETH_DEV_TO_PCI(dev); @@ -2580,9
> +2919,6 @@ ixgbe_dev_start(struct rte_eth_dev *dev)
>
> PMD_INIT_FUNC_TRACE();
>
> - /* Stop the link setup handler before resetting the HW. */
> - ixgbe_dev_wait_setup_link_complete(dev, 0);
> -
> /* disable uio/vfio intr/eventfd mapping */
> rte_intr_disable(intr_handle);
>
> @@ -2815,6 +3151,20 @@ ixgbe_dev_start(struct rte_eth_dev *dev)
> ixgbe_l2_tunnel_conf(dev);
> ixgbe_filter_restore(dev);
>
> + /* Spawn service thread */
> + if (ixgbe_is_sfp(hw)) {
> + intr->flags |= IXGBE_FLAG_NEED_SFP_SETUP;
> + err = rte_ctrl_thread_create(&ad->service_thread_tid,
> + "ixgbe-service-thread",
> + NULL,
> + ixgbe_dev_service_thread_handler,
> + dev);
> + if (err) {
> + PMD_DRV_LOG(ERR, "service_thread err");
> + goto error;
> + }
> + }
> +
> if (tm_conf->root && !tm_conf->committed)
> PMD_DRV_LOG(WARNING,
> "please call hierarchy_commit() "
> @@ -2860,13 +3210,21 @@ ixgbe_dev_stop(struct rte_eth_dev *dev)
> int vf;
> struct ixgbe_tm_conf *tm_conf =
> IXGBE_DEV_PRIVATE_TO_TM_CONF(dev->data->dev_private);
> + void *res;
> + s32 err;
>
> if (hw->adapter_stopped)
> return 0;
>
> PMD_INIT_FUNC_TRACE();
>
> - ixgbe_dev_wait_setup_link_complete(dev, 0);
> + /* Cancel the service thread, and wait for it to join */
> + err = pthread_cancel(adapter->service_thread_tid);
> + if (err)
> + PMD_DRV_LOG(ERR, "failed to cancel service thread %d", err);
> + err = pthread_join(adapter->service_thread_tid, &res);
> + if (err)
> + PMD_DRV_LOG(ERR, "failed to join service thread %d",
> + err);
>
> /* disable interrupts */
> ixgbe_disable_intr(hw);
> @@ -2945,7 +3303,6 @@ ixgbe_dev_set_link_up(struct rte_eth_dev *dev)
> } else {
> /* Turn on the laser */
> ixgbe_enable_tx_laser(hw);
> - ixgbe_dev_link_update(dev, 0);
> }
>
> return 0;
> @@ -2976,7 +3333,6 @@ ixgbe_dev_set_link_down(struct rte_eth_dev *dev)
> } else {
> /* Turn off the laser */
> ixgbe_disable_tx_laser(hw);
> - ixgbe_dev_link_update(dev, 0);
> }
>
> return 0;
> @@ -4128,57 +4484,6 @@ ixgbevf_check_link(struct ixgbe_hw *hw,
> ixgbe_link_speed *speed,
> return ret_val;
> }
>
> -/*
> - * If @timeout_ms was 0, it means that it will not return until link complete.
> - * It returns 1 on complete, return 0 on timeout.
> - */
> -static int
> -ixgbe_dev_wait_setup_link_complete(struct rte_eth_dev *dev, uint32_t
> timeout_ms) -{
> -#define WARNING_TIMEOUT 9000 /* 9s in total */
> - struct ixgbe_adapter *ad = dev->data->dev_private;
> - uint32_t timeout = timeout_ms ? timeout_ms : WARNING_TIMEOUT;
> -
> - while (rte_atomic32_read(&ad->link_thread_running)) {
> - msec_delay(1);
> - timeout--;
> -
> - if (timeout_ms) {
> - if (!timeout)
> - return 0;
> - } else if (!timeout) {
> - /* It will not return until link complete */
> - timeout = WARNING_TIMEOUT;
> - PMD_DRV_LOG(ERR, "IXGBE link thread not complete too long
> time!");
> - }
> - }
> -
> - return 1;
> -}
> -
> -static void *
> -ixgbe_dev_setup_link_thread_handler(void *param) -{
> - struct rte_eth_dev *dev = (struct rte_eth_dev *)param;
> - struct ixgbe_adapter *ad = dev->data->dev_private;
> - struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data-
> >dev_private);
> - struct ixgbe_interrupt *intr =
> - IXGBE_DEV_PRIVATE_TO_INTR(dev->data->dev_private);
> - u32 speed;
> - bool autoneg = false;
> -
> - pthread_detach(pthread_self());
> - speed = hw->phy.autoneg_advertised;
> - if (!speed)
> - ixgbe_get_link_capabilities(hw, &speed, &autoneg);
> -
> - ixgbe_setup_link(hw, speed, true);
> -
> - intr->flags &= ~IXGBE_FLAG_NEED_LINK_CONFIG;
> - rte_atomic32_clear(&ad->link_thread_running);
> - return NULL;
> -}
> -
> /*
> * In freebsd environment, nic_uio drivers do not support interrupts,
> * rte_intr_callback_register() will fail to register interrupts.
> @@ -4218,11 +4523,8 @@ ixgbe_dev_link_update_share(struct rte_eth_dev
> *dev,
> int wait_to_complete, int vf) {
> struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data-
> >dev_private);
> - struct ixgbe_adapter *ad = dev->data->dev_private;
> struct rte_eth_link link;
> ixgbe_link_speed link_speed = IXGBE_LINK_SPEED_UNKNOWN;
> - struct ixgbe_interrupt *intr =
> - IXGBE_DEV_PRIVATE_TO_INTR(dev->data->dev_private);
> bool link_up;
> int diag;
> int wait = 1;
> @@ -4237,9 +4539,6 @@ ixgbe_dev_link_update_share(struct rte_eth_dev
> *dev,
>
> hw->mac.get_link_status = true;
>
> - if (intr->flags & IXGBE_FLAG_NEED_LINK_CONFIG)
> - return rte_eth_linkstatus_set(dev, &link);
> -
> /* check if it needs to wait to complete, if lsc interrupt is enabled */
> if (wait_to_complete == 0 || dev->data->dev_conf.intr_conf.lsc != 0)
> wait = 0;
> @@ -4254,7 +4553,7 @@ ixgbe_dev_link_update_share(struct rte_eth_dev
> *dev,
> else
> diag = ixgbe_check_link(hw, &link_speed, &link_up, wait);
>
> - if (diag != 0) {
> + if (diag != 0 || !link_up) {
> link.link_speed = RTE_ETH_SPEED_NUM_100M;
> link.link_duplex = RTE_ETH_LINK_FULL_DUPLEX;
> return rte_eth_linkstatus_set(dev, &link); @@ -4267,32 +4566,6 @@
> ixgbe_dev_link_update_share(struct rte_eth_dev *dev,
> link_up = 0;
> }
>
> - if (link_up == 0) {
> - if (ixgbe_get_media_type(hw) == ixgbe_media_type_fiber) {
> - ixgbe_dev_wait_setup_link_complete(dev, 0);
> - if (rte_atomic32_test_and_set(&ad->link_thread_running)) {
> - /* To avoid race condition between threads, set
> - * the IXGBE_FLAG_NEED_LINK_CONFIG flag only
> - * when there is no link thread running.
> - */
> - intr->flags |= IXGBE_FLAG_NEED_LINK_CONFIG;
> - if (rte_ctrl_thread_create(&ad->link_thread_tid,
> - "ixgbe-link-handler",
> - NULL,
> - ixgbe_dev_setup_link_thread_handler,
> - dev) < 0) {
> - PMD_DRV_LOG(ERR,
> - "Create link thread failed!");
> - rte_atomic32_clear(&ad->link_thread_running);
> - }
> - } else {
> - PMD_DRV_LOG(ERR,
> - "Other link thread is running now!");
> - }
> - }
> - return rte_eth_linkstatus_set(dev, &link);
> - }
> -
> link.link_status = RTE_ETH_LINK_UP;
> link.link_duplex = RTE_ETH_LINK_FULL_DUPLEX;
>
> @@ -4498,8 +4771,6 @@ ixgbe_dev_interrupt_get_status(struct rte_eth_dev
> *dev)
> eicr = IXGBE_READ_REG(hw, IXGBE_EICR);
> PMD_DRV_LOG(DEBUG, "eicr %x", eicr);
>
> - intr->flags = 0;
> -
> /* set flag for async link update */
> if (eicr & IXGBE_EICR_LSC)
> intr->flags |= IXGBE_FLAG_NEED_LINK_UPDATE; @@ -4515,6
> +4786,14 @@ ixgbe_dev_interrupt_get_status(struct rte_eth_dev *dev)
> (eicr & IXGBE_EICR_GPI_SDP0_X550EM_x))
> intr->flags |= IXGBE_FLAG_PHY_INTERRUPT;
>
> + /* Check for loss of SFP */
> + /* TODO - For platforms that don't have this flag, do we need to set
> + * NEED_SFP_SETUP on LSC if we're a SFP platform?
> + */
> + if (hw->mac.type == ixgbe_mac_X550EM_a &&
> + (eicr & IXGBE_EICR_GPI_SDP0_X550EM_a))
> + intr->flags |= IXGBE_FLAG_NEED_SFP_SETUP;
> +
> return 0;
> }
>
> @@ -4566,11 +4845,13 @@ ixgbe_dev_link_status_print(struct rte_eth_dev
> *dev) static int ixgbe_dev_interrupt_action(struct rte_eth_dev *dev) {
> + struct rte_pci_device *pci_dev = RTE_ETH_DEV_TO_PCI(dev);
> + struct rte_intr_handle *intr_handle = pci_dev->intr_handle;
> struct ixgbe_interrupt *intr =
> IXGBE_DEV_PRIVATE_TO_INTR(dev->data->dev_private);
> - int64_t timeout;
> struct ixgbe_hw *hw =
> IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> + int64_t timeout;
>
> PMD_DRV_LOG(DEBUG, "intr action type %d", intr->flags);
>
> @@ -4605,16 +4886,14 @@ ixgbe_dev_interrupt_action(struct rte_eth_dev
> *dev)
> if (rte_eal_alarm_set(timeout * 1000,
> ixgbe_dev_interrupt_delayed_handler, (void *)dev) < 0)
> PMD_DRV_LOG(ERR, "Error setting alarm");
> - else {
> - /* remember original mask */
> - intr->mask_original = intr->mask;
> + else
> /* only disable lsc interrupt */
> intr->mask &= ~IXGBE_EIMS_LSC;
> - }
> }
>
> PMD_DRV_LOG(DEBUG, "enable intr immediately");
> ixgbe_enable_intr(dev);
> + rte_intr_ack(intr_handle);
>
> return 0;
> }
> @@ -4637,8 +4916,6 @@ static void
> ixgbe_dev_interrupt_delayed_handler(void *param) {
> struct rte_eth_dev *dev = (struct rte_eth_dev *)param;
> - struct rte_pci_device *pci_dev = RTE_ETH_DEV_TO_PCI(dev);
> - struct rte_intr_handle *intr_handle = pci_dev->intr_handle;
> struct ixgbe_interrupt *intr =
> IXGBE_DEV_PRIVATE_TO_INTR(dev->data->dev_private);
> struct ixgbe_hw *hw =
> @@ -4668,13 +4945,10 @@ ixgbe_dev_interrupt_delayed_handler(void
> *param)
> intr->flags &= ~IXGBE_FLAG_MACSEC;
> }
>
> - /* restore original mask */
> - intr->mask = intr->mask_original;
> - intr->mask_original = 0;
> + if (dev->data->dev_conf.intr_conf.lsc != 0)
> + intr->mask |= IXGBE_EICR_LSC;
>
> - PMD_DRV_LOG(DEBUG, "enable intr in delayed handler S[%08x]", eicr);
> ixgbe_enable_intr(dev);
> - rte_intr_ack(intr_handle);
> }
>
> /**
> @@ -5316,9 +5590,6 @@ ixgbevf_dev_start(struct rte_eth_dev *dev)
>
> PMD_INIT_FUNC_TRACE();
>
> - /* Stop the link setup handler before resetting the HW. */
> - ixgbe_dev_wait_setup_link_complete(dev, 0);
> -
> err = hw->mac.ops.reset_hw(hw);
>
> /**
> @@ -5398,12 +5669,6 @@ ixgbevf_dev_start(struct rte_eth_dev *dev)
> /* Re-enable interrupt for VF */
> ixgbevf_intr_enable(dev);
>
> - /*
> - * Update link status right before return, because it may
> - * start link configuration process in a separate thread.
> - */
> - ixgbevf_dev_link_update(dev, 0);
> -
> hw->adapter_stopped = false;
>
> return 0;
> @@ -5422,8 +5687,6 @@ ixgbevf_dev_stop(struct rte_eth_dev *dev)
>
> PMD_INIT_FUNC_TRACE();
>
> - ixgbe_dev_wait_setup_link_complete(dev, 0);
> -
> ixgbevf_intr_disable(dev);
>
> dev->data->dev_started = 0;
> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.h
> b/drivers/net/ixgbe/ixgbe_ethdev.h
> index 69e0e82a5b..d243e417e9 100644
> --- a/drivers/net/ixgbe/ixgbe_ethdev.h
> +++ b/drivers/net/ixgbe/ixgbe_ethdev.h
> @@ -29,6 +29,7 @@
> #define IXGBE_FLAG_PHY_INTERRUPT (uint32_t)(1 << 2)
> #define IXGBE_FLAG_MACSEC (uint32_t)(1 << 3)
> #define IXGBE_FLAG_NEED_LINK_CONFIG (uint32_t)(1 << 4)
> +#define IXGBE_FLAG_NEED_SFP_SETUP ((uint32_t)(1 << 5))
>
> /*
> * Defines that were not part of ixgbe_type.h as they are not used by the @@ -
> 223,8 +224,6 @@ struct ixgbe_rte_flow_rss_conf { struct ixgbe_interrupt {
> uint32_t flags;
> uint32_t mask;
> - /*to save original mask during delayed handler */
> - uint32_t mask_original;
> };
>
> struct ixgbe_stat_mapping_registers {
> @@ -507,7 +506,7 @@ struct ixgbe_adapter {
> uint8_t pflink_fullchk;
> uint8_t mac_ctrl_frame_fwd;
> rte_atomic32_t link_thread_running;
> - pthread_t link_thread_tid;
> + pthread_t service_thread_tid;
> };
>
> struct ixgbe_vf_representor {
> @@ -670,6 +669,15 @@ int ixgbe_syn_filter_set(struct rte_eth_dev *dev,
> struct rte_eth_syn_filter *filter,
> bool add);
>
> +enum ixgbe_sfp_cage_status {
> + IXGBE_SFP_CAGE_EMPTY = 0,
> + IXGBE_SFP_CAGE_FULL,
> + IXGBE_SFP_CAGE_UNKNOWN = -1,
> + IXGBE_SFP_CAGE_NOCAGE = -2,
> +};
> +enum ixgbe_sfp_cage_status ixgbe_check_sfp_cage(struct ixgbe_hw *hw);
> +
> +
> /**
> * l2 tunnel configuration.
> */
> --
> 2.25.1
Hm, testing on a different platform there appears to be an issue when plugging in a module with a different speed from a prior module does not correctly update the link speed. Currently investigating, could be a platform-specific issue.
next prev parent reply other threads:[~2022-03-12 13:03 UTC|newest]
Thread overview: 69+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20211206221922.644187-1-stephend@silicom-usa.com>
2021-12-06 22:19 ` [PATCH v2 1/7] net/ixgbe: Fix ixgbe_is_sfp() to return valid result for X550EM_a devs Stephen Douthit
2021-12-20 7:45 ` Wang, Haiyue
2021-12-20 21:32 ` Stephen Douthit
2021-12-06 22:19 ` [PATCH v2 2/7] net/ixgbe: Add ixgbe_check_sfp_cage() for testing state of PRSNT# signal Stephen Douthit
2021-12-06 22:19 ` [PATCH v2 3/7] net/ixgbe: Check that SFF-8472 soft rate select is supported before write Stephen Douthit
2021-12-20 7:53 ` Wang, Haiyue
2021-12-20 21:32 ` Stephen Douthit
2021-12-21 1:15 ` Wang, Haiyue
2021-12-21 8:57 ` Morten Brørup
2021-12-22 1:24 ` Wang, Haiyue
2021-12-22 10:43 ` Morten Brørup
2021-12-22 16:03 ` Wang, Haiyue
2021-12-22 19:13 ` Morten Brørup
2021-12-22 21:44 ` Stephen Douthit
2021-12-23 0:55 ` Wang, Haiyue
2022-01-18 21:06 ` Stephen Douthit
2022-01-19 0:31 ` Wang, Haiyue
2022-02-07 16:04 ` Ferruh Yigit
2022-02-08 13:50 ` Jeff Daly
2022-02-08 14:52 ` Ferruh Yigit
2022-02-09 4:00 ` Wang, Haiyue
2022-02-09 13:33 ` Ferruh Yigit
2022-02-09 13:43 ` Wang, Haiyue
2021-12-21 14:05 ` Stephen Douthit
2021-12-06 22:19 ` [PATCH v2 4/7] net/ixgbe: Run 82599 link status workaround only on affected devices Stephen Douthit
2021-12-06 22:19 ` [PATCH v2 5/7] net/ixgbe: Fix SFP detection and linking on hotplug Stephen Douthit
2022-02-07 16:07 ` Ferruh Yigit
[not found] ` <20220224152357.12277-1-jeffd@silicom-usa.com>
2022-02-24 15:23 ` [PATCH v3 1/3] net/ixgbe: Fix ixgbe_is_sfp() to return valid result for X550EM_a devs Jeff Daly
2022-02-24 15:23 ` [PATCH v3 2/3] net/ixgbe: Limit SDP3 check of TX_DISABLE to appropriate devices Jeff Daly
2022-02-24 15:23 ` [PATCH v3 3/3] net/ixgbe: Fix SFP detection and linking on hotplug Jeff Daly
2022-02-25 1:56 ` Wang, Haiyue
2022-02-25 20:50 ` [PATCH v4 " Jeff Daly
2022-02-26 15:57 ` Ferruh Yigit
[not found] ` <20220228152937.21247-1-jeffd@silicom-usa.com>
2022-02-28 15:29 ` [PATCH v4 1/3] net/ixgbe: Fix ixgbe_is_sfp() to return valid result for X550EM_a devs Jeff Daly
2022-03-01 5:56 ` Wang, Haiyue
2022-03-01 11:18 ` Zhang, Qi Z
2022-03-06 17:56 ` Thomas Monjalon
2022-03-08 15:01 ` Jeff Daly
2022-02-28 15:29 ` [PATCH v4 2/3] net/ixgbe: Limit SDP3 check of TX_DISABLE to appropriate devices Jeff Daly
2022-02-28 15:29 ` [PATCH v4 3/3] net/ixgbe: Fix SFP detection and linking on hotplug Jeff Daly
2022-03-12 13:03 ` Jeff Daly [this message]
[not found] ` <20220412173445.30810-1-jeffd@silicom-usa.com>
2022-04-12 17:34 ` [PATCH v5 1/2] net/ixgbe: Limit SDP3 check of TX_DISABLE to appropriate devices Jeff Daly
2022-04-12 17:34 ` [PATCH v5 2/2] net/ixgbe: Fix SFP detection and linking on hotplug Jeff Daly
[not found] ` <20220412174220.31195-1-jeffd@silicom-usa.com>
2022-04-12 17:42 ` [PATCH v6 1/2] net/ixgbe: Limit SDP3 check of TX_DISABLE to appropriate devices Jeff Daly
2022-04-13 1:21 ` Wang, Haiyue
2022-04-13 15:32 ` Jeff Daly
2022-04-14 1:56 ` Wang, Haiyue
2022-04-12 17:42 ` [PATCH v6 2/2] net/ixgbe: Fix SFP detection and linking on hotplug Jeff Daly
2022-04-13 2:46 ` Wang, Haiyue
2022-04-13 6:57 ` Morten Brørup
2022-04-13 7:01 ` Wang, Haiyue
2022-04-13 7:19 ` Morten Brørup
2022-04-13 11:49 ` Wang, Haiyue
2022-04-13 12:54 ` Morten Brørup
2022-04-13 15:23 ` Jeff Daly
2022-04-14 10:49 ` Jeff Daly
2022-04-14 11:08 ` Jeff Daly
2022-04-14 2:49 ` Wang, Haiyue
2022-04-14 2:59 ` Wang, Haiyue
2022-04-14 10:40 ` Jeff Daly
2022-04-14 12:11 ` Wang, Haiyue
2022-04-18 21:54 ` Jeff Daly
2022-04-19 2:05 ` Wang, Haiyue
2022-04-19 17:33 ` Jeff Daly
2022-04-20 1:09 ` Wang, Haiyue
2022-04-21 17:31 ` Jeff Daly
2022-04-22 2:11 ` Wang, Haiyue
2022-05-12 1:26 ` Zhang, Qi Z
2022-05-25 16:55 ` Jeff Daly
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=AM0PR0402MB35060FEEC09D00733F089225EA0D9@AM0PR0402MB3506.eurprd04.prod.outlook.com \
--to=jeffd@silicom-usa.com \
--cc=dev@dpdk.org \
--cc=haiyue.wang@intel.com \
--cc=stable@dpdk.org \
--cc=stephend@silicom-usa.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).