RE: [PATCH v4 3/3] net/ixgbe: Fix SFP detection and linking on hotplug

[Jeff Daly <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.