* [PATCH 1/3] ixgbe: make link update thread periodic
2022-05-19 18:02 [PATCH 0/3] ixgbe: Fix SFP hotplug detection Jeff Daly
@ 2022-05-19 18:02 ` Jeff Daly
2022-05-29 23:25 ` Zhang, Qi Z
2022-05-19 18:02 ` [PATCH 2/3] ixgbe: move periodic link service work into separate function Jeff Daly
2022-05-19 18:02 ` [PATCH 3/3] ixgbe: make hotplug detection aware of changed SFPs Jeff Daly
2 siblings, 1 reply; 12+ messages in thread
From: Jeff Daly @ 2022-05-19 18:02 UTC (permalink / raw)
To: dev, Qiming Yang, Wenjun Wu; +Cc: Stephen Douthit
Rather than run-to-completion, allow the link update thread to be
periodic. This will set the stage for properly handling hot-plugging.
Signed-off-by: Jeff Daly <jeffd@silicom-usa.com>
Inspired-by: Stephen Douthit <stephend@silicom-usa.com>
---
drivers/net/ixgbe/base/ixgbe_common.c | 4 +-
drivers/net/ixgbe/ixgbe_ethdev.c | 180 ++++++++++----------------
2 files changed, 71 insertions(+), 113 deletions(-)
diff --git a/drivers/net/ixgbe/base/ixgbe_common.c b/drivers/net/ixgbe/base/ixgbe_common.c
index aa843bd5c4a5..712062306491 100644
--- a/drivers/net/ixgbe/base/ixgbe_common.c
+++ b/drivers/net/ixgbe/base/ixgbe_common.c
@@ -4154,8 +4154,8 @@ s32 ixgbe_check_mac_link_generic(struct ixgbe_hw *hw, ixgbe_link_speed *speed,
break;
case ixgbe_mac_X550EM_x:
case ixgbe_mac_X550EM_a:
- sfp_cage_full = IXGBE_READ_REG(hw, IXGBE_ESDP) &
- IXGBE_ESDP_SDP0;
+ sfp_cage_full = !(IXGBE_READ_REG(hw, IXGBE_ESDP) &
+ IXGBE_ESDP_SDP0);
break;
default:
/* sanity check - No SFP+ devices here */
diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
index 2da3f67bbc78..81b15ad28212 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -230,8 +230,6 @@ 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,
@@ -1039,7 +1037,6 @@ ixgbe_swfw_lock_reset(struct ixgbe_hw *hw)
static int
eth_ixgbe_dev_init(struct rte_eth_dev *eth_dev, void *init_params __rte_unused)
{
- struct ixgbe_adapter *ad = eth_dev->data->dev_private;
struct rte_pci_device *pci_dev = RTE_ETH_DEV_TO_PCI(eth_dev);
struct rte_intr_handle *intr_handle = pci_dev->intr_handle;
struct ixgbe_hw *hw =
@@ -1094,7 +1091,6 @@ eth_ixgbe_dev_init(struct rte_eth_dev *eth_dev, void *init_params __rte_unused)
return 0;
}
- rte_atomic32_clear(&ad->link_thread_running);
rte_eth_copy_pci_info(eth_dev, pci_dev);
eth_dev->data->dev_flags |= RTE_ETH_DEV_AUTOFILL_QUEUE_XSTATS;
@@ -1547,7 +1543,6 @@ eth_ixgbevf_dev_init(struct rte_eth_dev *eth_dev)
{
int diag;
uint32_t tc, tcs;
- struct ixgbe_adapter *ad = eth_dev->data->dev_private;
struct rte_pci_device *pci_dev = RTE_ETH_DEV_TO_PCI(eth_dev);
struct rte_intr_handle *intr_handle = pci_dev->intr_handle;
struct ixgbe_hw *hw =
@@ -1590,7 +1585,6 @@ eth_ixgbevf_dev_init(struct rte_eth_dev *eth_dev)
return 0;
}
- rte_atomic32_clear(&ad->link_thread_running);
ixgbevf_parse_devargs(eth_dev->data->dev_private,
pci_dev->device.devargs);
@@ -2558,8 +2552,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 +2577,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);
@@ -2700,8 +2694,16 @@ ixgbe_dev_start(struct rte_eth_dev *dev)
if (ixgbe_is_sfp(hw) && hw->phy.multispeed_fiber) {
err = hw->mac.ops.setup_sfp(hw);
- if (err)
+ intr->flags |= IXGBE_FLAG_NEED_LINK_CONFIG;
+ err = rte_ctrl_thread_create(&ad->link_thread_tid,
+ "ixgbe-service-tid",
+ NULL,
+ ixgbe_dev_setup_link_thread_handler,
+ dev);
+ if (err) {
+ PMD_DRV_LOG(ERR, "service_thread err");
goto error;
+ }
}
if (hw->mac.ops.get_media_type(hw) == ixgbe_media_type_copper) {
@@ -2825,12 +2827,6 @@ ixgbe_dev_start(struct rte_eth_dev *dev)
if (err)
goto error;
- /*
- * Update link status right before return, because it may
- * start link configuration process in a separate thread.
- */
- ixgbe_dev_link_update(dev, 0);
-
/* setup the macsec setting register */
if (macsec_setting->offload_en)
ixgbe_dev_macsec_register_enable(dev, macsec_setting);
@@ -2860,13 +2856,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 */
+ err = pthread_cancel(adapter->link_thread_tid);
+ if (err)
+ PMD_DRV_LOG(ERR, "failed to cancel service thread %d", err);
+ err = pthread_join(adapter->link_thread_tid, &res);
+ if (err)
+ PMD_DRV_LOG(ERR, "failed to join service thread %d", err);
/* disable interrupts */
ixgbe_disable_intr(hw);
@@ -2945,7 +2949,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 +2979,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;
@@ -4129,54 +4131,58 @@ 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;
+ u32 speed, start, ticks, service_ms;
+ s32 err;
+ bool link_up, 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);
+ while (1) {
+ service_ms = 100;
+ if (intr->flags & IXGBE_FLAG_NEED_LINK_CONFIG) {
+ speed = hw->phy.autoneg_advertised;
+
+ if (!speed)
+ ixgbe_get_link_capabilities(hw, &speed, &autoneg);
+
+ err = ixgbe_setup_link(hw, speed, true);
+
+ if (err == IXGBE_SUCCESS)
+ err = ixgbe_check_link(hw, &speed, &link_up, 0);
+
+ /* 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
+ */
+ if (err == IXGBE_SUCCESS && link_up) {
+ service_ms = 2000;
+ intr->flags &= ~IXGBE_FLAG_NEED_LINK_CONFIG;
+ }
+
+ 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);
+ }
+ }
+
+ /* 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);
+ }
- intr->flags &= ~IXGBE_FLAG_NEED_LINK_CONFIG;
- rte_atomic32_clear(&ad->link_thread_running);
return NULL;
}
@@ -4219,11 +4225,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;
@@ -4238,9 +4241,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;
@@ -4255,7 +4255,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 +4267,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;
@@ -4566,6 +4540,8 @@ 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;
@@ -4605,16 +4581,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 +4611,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 +4640,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 +5285,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 +5364,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 +5382,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;
--
2.25.1
^ permalink raw reply [flat|nested] 12+ messages in thread* RE: [PATCH 1/3] ixgbe: make link update thread periodic
2022-05-19 18:02 ` [PATCH 1/3] ixgbe: make link update thread periodic Jeff Daly
@ 2022-05-29 23:25 ` Zhang, Qi Z
2022-05-30 13:46 ` Jeff Daly
0 siblings, 1 reply; 12+ messages in thread
From: Zhang, Qi Z @ 2022-05-29 23:25 UTC (permalink / raw)
To: Daly, Jeff, dev, Yang, Qiming, Wu, Wenjun1; +Cc: Stephen Douthit
> -----Original Message-----
> From: Jeff Daly <jeffd@silicom-usa.com>
> Sent: Friday, May 20, 2022 2:03 AM
> To: dev@dpdk.org; Yang, Qiming <qiming.yang@intel.com>; Wu, Wenjun1
> <wenjun1.wu@intel.com>
> Cc: Stephen Douthit <stephend@silicom-usa.com>
> Subject: [PATCH 1/3] ixgbe: make link update thread periodic
>
> Rather than run-to-completion, allow the link update thread to be periodic.
> This will set the stage for properly handling hot-plugging.
Could you explain more about what's the hot-plugging issue with run-to-completion you try to fix?
>
> Signed-off-by: Jeff Daly <jeffd@silicom-usa.com>
> Inspired-by: Stephen Douthit <stephend@silicom-usa.com>
> ---
> drivers/net/ixgbe/base/ixgbe_common.c | 4 +-
> drivers/net/ixgbe/ixgbe_ethdev.c | 180 ++++++++++----------------
> 2 files changed, 71 insertions(+), 113 deletions(-)
>
> diff --git a/drivers/net/ixgbe/base/ixgbe_common.c
> b/drivers/net/ixgbe/base/ixgbe_common.c
> index aa843bd5c4a5..712062306491 100644
> --- a/drivers/net/ixgbe/base/ixgbe_common.c
> +++ b/drivers/net/ixgbe/base/ixgbe_common.c
> @@ -4154,8 +4154,8 @@ s32 ixgbe_check_mac_link_generic(struct
> ixgbe_hw *hw, ixgbe_link_speed *speed,
> break;
> case ixgbe_mac_X550EM_x:
> case ixgbe_mac_X550EM_a:
> - sfp_cage_full = IXGBE_READ_REG(hw, IXGBE_ESDP)
> &
> - IXGBE_ESDP_SDP0;
> + sfp_cage_full = !(IXGBE_READ_REG(hw, IXGBE_ESDP)
> &
> + IXGBE_ESDP_SDP0);
> break;
> default:
> /* sanity check - No SFP+ devices here */ diff --git
Looks like you change the behavior of link status check for x550.
I'm not an ixgbe expert, but I know this is not kernel driver's implementation.
So do you think this is a fix for both DPDK and kernel driver? if it is, please move this change into a separate patch and we need to reach the right expert to approve this.
^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [PATCH 1/3] ixgbe: make link update thread periodic
2022-05-29 23:25 ` Zhang, Qi Z
@ 2022-05-30 13:46 ` Jeff Daly
2022-05-30 14:21 ` Zhang, Qi Z
0 siblings, 1 reply; 12+ messages in thread
From: Jeff Daly @ 2022-05-30 13:46 UTC (permalink / raw)
To: Zhang, Qi Z, dev, Yang, Qiming, Wu, Wenjun1; +Cc: Stephen Douthit
> -----Original Message-----
> From: Zhang, Qi Z <qi.z.zhang@intel.com>
> Sent: Sunday, May 29, 2022 7:25 PM
> To: Jeff Daly <jeffd@silicom-usa.com>; dev@dpdk.org; Yang, Qiming
> <qiming.yang@intel.com>; Wu, Wenjun1 <wenjun1.wu@intel.com>
> Cc: Stephen Douthit <stephend@silicom-usa.com>
> Subject: RE: [PATCH 1/3] ixgbe: make link update thread periodic
>
> Caution: This is an external email. Please take care when clicking links or
> opening attachments.
>
>
> > -----Original Message-----
> > From: Jeff Daly <jeffd@silicom-usa.com>
> > Sent: Friday, May 20, 2022 2:03 AM
> > To: dev@dpdk.org; Yang, Qiming <qiming.yang@intel.com>; Wu, Wenjun1
> > <wenjun1.wu@intel.com>
> > Cc: Stephen Douthit <stephend@silicom-usa.com>
> > Subject: [PATCH 1/3] ixgbe: make link update thread periodic
> >
> > Rather than run-to-completion, allow the link update thread to be
> periodic.
> > This will set the stage for properly handling hot-plugging.
>
> Could you explain more about what's the hot-plugging issue with run-to-
> completion you try to fix?
>
it doesn't work right when you have SFPs. (at least not on our platform or on an
82599 dual SFP add-in card we have). run-to-completion only works 1x. if you
remove and plug in a different SFP it doesn't work. This patch series should have
been taking in context with the original SFP hotplug patch but apparently since
I can't ever seem to get the patch submission threading to do what I mean perhaps
some context has gone missing. the SFP hotplug fix has been in the queue since
Dec 2021, has been reworked several times, has gone through a change in Intel
maintainership. this patch series makes the SFP hot plugging work like the Intel
kernel driver does.
> >
> > Signed-off-by: Jeff Daly <jeffd@silicom-usa.com>
> > Inspired-by: Stephen Douthit <stephend@silicom-usa.com>
> > ---
> > drivers/net/ixgbe/base/ixgbe_common.c | 4 +-
> > drivers/net/ixgbe/ixgbe_ethdev.c | 180 ++++++++++----------------
> > 2 files changed, 71 insertions(+), 113 deletions(-)
> >
> > diff --git a/drivers/net/ixgbe/base/ixgbe_common.c
> > b/drivers/net/ixgbe/base/ixgbe_common.c
> > index aa843bd5c4a5..712062306491 100644
> > --- a/drivers/net/ixgbe/base/ixgbe_common.c
> > +++ b/drivers/net/ixgbe/base/ixgbe_common.c
> > @@ -4154,8 +4154,8 @@ s32 ixgbe_check_mac_link_generic(struct
> > ixgbe_hw *hw, ixgbe_link_speed *speed,
> > break;
> > case ixgbe_mac_X550EM_x:
> > case ixgbe_mac_X550EM_a:
> > - sfp_cage_full = IXGBE_READ_REG(hw, IXGBE_ESDP)
> > &
> > - IXGBE_ESDP_SDP0;
> > + sfp_cage_full = !(IXGBE_READ_REG(hw, IXGBE_ESDP)
> > &
> > + IXGBE_ESDP_SDP0);
> > break;
> > default:
> > /* sanity check - No SFP+ devices here */ diff
> > --git
>
> Looks like you change the behavior of link status check for x550.
> I'm not an ixgbe expert, but I know this is not kernel driver's
> implementation.
>
sigh. this was supposed to be part of a different patch which also had some
question about functionality. the SDP0 bit check doesn't specifically need to be
a check for a '1', since the bit reflects the state of the pin on the platform. Intel's
platform implementations have an inverter on the board to switch the state.
MOD_ABS from an SFP will be '0' when an SFP is plugged in. with an inverter in
the platform the signal will be '1' when an SFP is plugged in. there's no guidance
from Intel's platform design guide that an inverter needs to be between the SFP
and the NIC SDP pin so having it only follow Intel's platform implementations is
hard to justify.
> So do you think this is a fix for both DPDK and kernel driver? if it is, please
> move this change into a separate patch and we need to reach the right
> expert to approve this.
>
>
no, as explained above.
^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [PATCH 1/3] ixgbe: make link update thread periodic
2022-05-30 13:46 ` Jeff Daly
@ 2022-05-30 14:21 ` Zhang, Qi Z
2022-05-30 14:38 ` Zhang, Qi Z
2022-05-31 12:25 ` Jeff Daly
0 siblings, 2 replies; 12+ messages in thread
From: Zhang, Qi Z @ 2022-05-30 14:21 UTC (permalink / raw)
To: Daly, Jeff, dev, Yang, Qiming, Wu, Wenjun1; +Cc: Stephen Douthit
> -----Original Message-----
> From: Jeff Daly <jeffd@silicom-usa.com>
> Sent: Monday, May 30, 2022 9:47 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; dev@dpdk.org; Yang, Qiming
> <qiming.yang@intel.com>; Wu, Wenjun1 <wenjun1.wu@intel.com>
> Cc: Stephen Douthit <stephend@silicom-usa.com>
> Subject: RE: [PATCH 1/3] ixgbe: make link update thread periodic
>
>
>
> > -----Original Message-----
> > From: Zhang, Qi Z <qi.z.zhang@intel.com>
> > Sent: Sunday, May 29, 2022 7:25 PM
> > To: Jeff Daly <jeffd@silicom-usa.com>; dev@dpdk.org; Yang, Qiming
> > <qiming.yang@intel.com>; Wu, Wenjun1 <wenjun1.wu@intel.com>
> > Cc: Stephen Douthit <stephend@silicom-usa.com>
> > Subject: RE: [PATCH 1/3] ixgbe: make link update thread periodic
> >
> > Caution: This is an external email. Please take care when clicking
> > links or opening attachments.
> >
> >
> > > -----Original Message-----
> > > From: Jeff Daly <jeffd@silicom-usa.com>
> > > Sent: Friday, May 20, 2022 2:03 AM
> > > To: dev@dpdk.org; Yang, Qiming <qiming.yang@intel.com>; Wu, Wenjun1
> > > <wenjun1.wu@intel.com>
> > > Cc: Stephen Douthit <stephend@silicom-usa.com>
> > > Subject: [PATCH 1/3] ixgbe: make link update thread periodic
> > >
> > > Rather than run-to-completion, allow the link update thread to be
> > periodic.
> > > This will set the stage for properly handling hot-plugging.
> >
> > Could you explain more about what's the hot-plugging issue with
> > run-to- completion you try to fix?
> >
>
> it doesn't work right when you have SFPs. (at least not on our platform or on
> an
> 82599 dual SFP add-in card we have). run-to-completion only works 1x. if
> you remove and plug in a different SFP it doesn't work. This patch series
> should have been taking in context with the original SFP hotplug patch but
> apparently since I can't ever seem to get the patch submission threading to do
> what I mean perhaps some context has gone missing. the SFP hotplug fix has
> been in the queue since Dec 2021, has been reworked several times, has gone
> through a change in Intel maintainership. this patch series makes the SFP hot
> plugging work like the Intel kernel driver does.
>
> > >
> > > Signed-off-by: Jeff Daly <jeffd@silicom-usa.com>
> > > Inspired-by: Stephen Douthit <stephend@silicom-usa.com>
> > > ---
> > > drivers/net/ixgbe/base/ixgbe_common.c | 4 +-
> > > drivers/net/ixgbe/ixgbe_ethdev.c | 180 ++++++++++----------------
> > > 2 files changed, 71 insertions(+), 113 deletions(-)
> > >
> > > diff --git a/drivers/net/ixgbe/base/ixgbe_common.c
> > > b/drivers/net/ixgbe/base/ixgbe_common.c
> > > index aa843bd5c4a5..712062306491 100644
> > > --- a/drivers/net/ixgbe/base/ixgbe_common.c
> > > +++ b/drivers/net/ixgbe/base/ixgbe_common.c
> > > @@ -4154,8 +4154,8 @@ s32 ixgbe_check_mac_link_generic(struct
> > > ixgbe_hw *hw, ixgbe_link_speed *speed,
> > > break;
> > > case ixgbe_mac_X550EM_x:
> > > case ixgbe_mac_X550EM_a:
> > > - sfp_cage_full = IXGBE_READ_REG(hw, IXGBE_ESDP)
> > > &
> > > - IXGBE_ESDP_SDP0;
> > > + sfp_cage_full = !(IXGBE_READ_REG(hw,
> > > + IXGBE_ESDP)
> > > &
> > > + IXGBE_ESDP_SDP0);
> > > break;
> > > default:
> > > /* sanity check - No SFP+ devices here */ diff
> > > --git
> >
> > Looks like you change the behavior of link status check for x550.
> > I'm not an ixgbe expert, but I know this is not kernel driver's
> > implementation.
> >
>
> sigh. this was supposed to be part of a different patch which also had some
> question about functionality. the SDP0 bit check doesn't specifically need to
> be a check for a '1', since the bit reflects the state of the pin on the platform.
> Intel's platform implementations have an inverter on the board to switch the
> state.
> MOD_ABS from an SFP will be '0' when an SFP is plugged in. with an inverter
> in the platform the signal will be '1' when an SFP is plugged in. there's no
> guidance from Intel's platform design guide that an inverter needs to be
> between the SFP and the NIC SDP pin so having it only follow Intel's platform
> implementations is hard to justify.
OK, I assume the existing code should be proved for the normal scenario (remove and plug in with the same SFP)
So how can we guarantee this change will not break something?
Could you help me to understand why we should ignore the difference when SDP0 is 1 in normal scenario?
Before change
we will continue to read the link register and return the link speed.
after change
we return SPEED_UNKNOWN immediately .
>
> > So do you think this is a fix for both DPDK and kernel driver? if it
> > is, please move this change into a separate patch and we need to
> > reach the right expert to approve this.
> >
> >
>
> no, as explained above.
^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [PATCH 1/3] ixgbe: make link update thread periodic
2022-05-30 14:21 ` Zhang, Qi Z
@ 2022-05-30 14:38 ` Zhang, Qi Z
2022-05-31 12:25 ` Jeff Daly
1 sibling, 0 replies; 12+ messages in thread
From: Zhang, Qi Z @ 2022-05-30 14:38 UTC (permalink / raw)
To: Zhang, Qi Z, Daly, Jeff, dev, Yang, Qiming, Wu, Wenjun1; +Cc: Stephen Douthit
> -----Original Message-----
> From: Zhang, Qi Z <qi.z.zhang@intel.com>
> Sent: Monday, May 30, 2022 10:21 PM
> To: Daly, Jeff <jeffd@silicom-usa.com>; dev@dpdk.org; Yang, Qiming
> <qiming.yang@intel.com>; Wu, Wenjun1 <wenjun1.wu@intel.com>
> Cc: Stephen Douthit <stephend@silicom-usa.com>
> Subject: RE: [PATCH 1/3] ixgbe: make link update thread periodic
>
>
>
> > -----Original Message-----
> > From: Jeff Daly <jeffd@silicom-usa.com>
> > Sent: Monday, May 30, 2022 9:47 PM
> > To: Zhang, Qi Z <qi.z.zhang@intel.com>; dev@dpdk.org; Yang, Qiming
> > <qiming.yang@intel.com>; Wu, Wenjun1 <wenjun1.wu@intel.com>
> > Cc: Stephen Douthit <stephend@silicom-usa.com>
> > Subject: RE: [PATCH 1/3] ixgbe: make link update thread periodic
> >
> >
> >
> > > -----Original Message-----
> > > From: Zhang, Qi Z <qi.z.zhang@intel.com>
> > > Sent: Sunday, May 29, 2022 7:25 PM
> > > To: Jeff Daly <jeffd@silicom-usa.com>; dev@dpdk.org; Yang, Qiming
> > > <qiming.yang@intel.com>; Wu, Wenjun1 <wenjun1.wu@intel.com>
> > > Cc: Stephen Douthit <stephend@silicom-usa.com>
> > > Subject: RE: [PATCH 1/3] ixgbe: make link update thread periodic
> > >
> > > Caution: This is an external email. Please take care when clicking
> > > links or opening attachments.
> > >
> > >
> > > > -----Original Message-----
> > > > From: Jeff Daly <jeffd@silicom-usa.com>
> > > > Sent: Friday, May 20, 2022 2:03 AM
> > > > To: dev@dpdk.org; Yang, Qiming <qiming.yang@intel.com>; Wu,
> > > > Wenjun1 <wenjun1.wu@intel.com>
> > > > Cc: Stephen Douthit <stephend@silicom-usa.com>
> > > > Subject: [PATCH 1/3] ixgbe: make link update thread periodic
> > > >
> > > > Rather than run-to-completion, allow the link update thread to be
> > > periodic.
> > > > This will set the stage for properly handling hot-plugging.
> > >
> > > Could you explain more about what's the hot-plugging issue with
> > > run-to- completion you try to fix?
> > >
> >
> > it doesn't work right when you have SFPs. (at least not on our
> > platform or on an
> > 82599 dual SFP add-in card we have). run-to-completion only works 1x.
What is 1x means?
> > if you remove and plug in a different SFP it doesn't work. This patch
> > series should have been taking in context with the original SFP
> > hotplug patch but apparently since I can't ever seem to get the patch
> > submission threading to do what I mean perhaps some context has gone
> > missing. the SFP hotplug fix has been in the queue since Dec 2021,
> > has been reworked several times, has gone through a change in Intel
> > maintainership. this patch series makes the SFP hot plugging work like the
> Intel kernel driver does.
Basically, I'm asking for adding more detail information in the commit log , so people can understand why run-to-completion does not work with hot plugin
if you think some patches already be merged that helps to understand the design, you may add a commit ID as reference.
> >
> > > >
> > > > Signed-off-by: Jeff Daly <jeffd@silicom-usa.com>
> > > > Inspired-by: Stephen Douthit <stephend@silicom-usa.com>
> > > > ---
> > > > drivers/net/ixgbe/base/ixgbe_common.c | 4 +-
> > > > drivers/net/ixgbe/ixgbe_ethdev.c | 180 ++++++++++----------------
> > > > 2 files changed, 71 insertions(+), 113 deletions(-)
> > > >
> > > > diff --git a/drivers/net/ixgbe/base/ixgbe_common.c
> > > > b/drivers/net/ixgbe/base/ixgbe_common.c
> > > > index aa843bd5c4a5..712062306491 100644
> > > > --- a/drivers/net/ixgbe/base/ixgbe_common.c
> > > > +++ b/drivers/net/ixgbe/base/ixgbe_common.c
> > > > @@ -4154,8 +4154,8 @@ s32 ixgbe_check_mac_link_generic(struct
> > > > ixgbe_hw *hw, ixgbe_link_speed *speed,
> > > > break;
> > > > case ixgbe_mac_X550EM_x:
> > > > case ixgbe_mac_X550EM_a:
> > > > - sfp_cage_full = IXGBE_READ_REG(hw, IXGBE_ESDP)
> > > > &
> > > > - IXGBE_ESDP_SDP0;
> > > > + sfp_cage_full = !(IXGBE_READ_REG(hw,
> > > > + IXGBE_ESDP)
> > > > &
> > > > + IXGBE_ESDP_SDP0);
> > > > break;
> > > > default:
> > > > /* sanity check - No SFP+ devices here */
> > > > diff --git
> > >
> > > Looks like you change the behavior of link status check for x550.
> > > I'm not an ixgbe expert, but I know this is not kernel driver's
> > > implementation.
> > >
> >
> > sigh. this was supposed to be part of a different patch which also
> > had some question about functionality. the SDP0 bit check doesn't
> > specifically need to be a check for a '1', since the bit reflects the state of the
> pin on the platform.
> > Intel's platform implementations have an inverter on the board to
> > switch the state.
> > MOD_ABS from an SFP will be '0' when an SFP is plugged in. with an
> > inverter in the platform the signal will be '1' when an SFP is plugged
> > in. there's no guidance from Intel's platform design guide that an
> > inverter needs to be between the SFP and the NIC SDP pin so having it
> > only follow Intel's platform implementations is hard to justify.
>
> OK, I assume the existing code should be proved for the normal scenario
> (remove and plug in with the same SFP) So how can we guarantee this change
> will not break something?
>
> Could you help me to understand why we should ignore the difference when
> SDP0 is 1 in normal scenario?
>
> Before change
> we will continue to read the link register and return the link speed.
>
> after change
> we return SPEED_UNKNOWN immediately .
>
>
>
>
> >
> > > So do you think this is a fix for both DPDK and kernel driver? if
> > > it is, please move this change into a separate patch and we need to
> > > reach the right expert to approve this.
> > >
> > >
> >
> > no, as explained above.
^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [PATCH 1/3] ixgbe: make link update thread periodic
2022-05-30 14:21 ` Zhang, Qi Z
2022-05-30 14:38 ` Zhang, Qi Z
@ 2022-05-31 12:25 ` Jeff Daly
2022-05-31 13:03 ` Zhang, Qi Z
1 sibling, 1 reply; 12+ messages in thread
From: Jeff Daly @ 2022-05-31 12:25 UTC (permalink / raw)
To: Zhang, Qi Z, dev, Yang, Qiming, Wu, Wenjun1; +Cc: Stephen Douthit
> -----Original Message-----
> From: Zhang, Qi Z <qi.z.zhang@intel.com>
> Sent: Monday, May 30, 2022 10:21 AM
> To: Jeff Daly <jeffd@silicom-usa.com>; dev@dpdk.org; Yang, Qiming
> <qiming.yang@intel.com>; Wu, Wenjun1 <wenjun1.wu@intel.com>
> Cc: Stephen Douthit <stephend@silicom-usa.com>
> Subject: RE: [PATCH 1/3] ixgbe: make link update thread periodic
>
> Caution: This is an external email. Please take care when clicking links or
> opening attachments.
>
>
> > -----Original Message-----
> > From: Jeff Daly <jeffd@silicom-usa.com>
> > Sent: Monday, May 30, 2022 9:47 PM
> > To: Zhang, Qi Z <qi.z.zhang@intel.com>; dev@dpdk.org; Yang, Qiming
> > <qiming.yang@intel.com>; Wu, Wenjun1 <wenjun1.wu@intel.com>
> > Cc: Stephen Douthit <stephend@silicom-usa.com>
> > Subject: RE: [PATCH 1/3] ixgbe: make link update thread periodic
> >
> >
> >
> > > -----Original Message-----
> > > From: Zhang, Qi Z <qi.z.zhang@intel.com>
> > > Sent: Sunday, May 29, 2022 7:25 PM
> > > To: Jeff Daly <jeffd@silicom-usa.com>; dev@dpdk.org; Yang, Qiming
> > > <qiming.yang@intel.com>; Wu, Wenjun1 <wenjun1.wu@intel.com>
> > > Cc: Stephen Douthit <stephend@silicom-usa.com>
> > > Subject: RE: [PATCH 1/3] ixgbe: make link update thread periodic
> > >
> > > Caution: This is an external email. Please take care when clicking
> > > links or opening attachments.
> > >
> > >
> > > > -----Original Message-----
> > > > From: Jeff Daly <jeffd@silicom-usa.com>
> > > > Sent: Friday, May 20, 2022 2:03 AM
> > > > To: dev@dpdk.org; Yang, Qiming <qiming.yang@intel.com>; Wu,
> > > > Wenjun1 <wenjun1.wu@intel.com>
> > > > Cc: Stephen Douthit <stephend@silicom-usa.com>
> > > > Subject: [PATCH 1/3] ixgbe: make link update thread periodic
> > > >
> > > > Rather than run-to-completion, allow the link update thread to be
> > > periodic.
> > > > This will set the stage for properly handling hot-plugging.
> > >
> > > Could you explain more about what's the hot-plugging issue with
> > > run-to- completion you try to fix?
> > >
> >
> > it doesn't work right when you have SFPs. (at least not on our
> > platform or on an
> > 82599 dual SFP add-in card we have). run-to-completion only works 1x.
> > if you remove and plug in a different SFP it doesn't work. This patch
> > series should have been taking in context with the original SFP
> > hotplug patch but apparently since I can't ever seem to get the patch
> > submission threading to do what I mean perhaps some context has gone
> > missing. the SFP hotplug fix has been in the queue since Dec 2021,
> > has been reworked several times, has gone through a change in Intel
> > maintainership. this patch series makes the SFP hot plugging work like the
> Intel kernel driver does.
> >
> > > >
> > > > Signed-off-by: Jeff Daly <jeffd@silicom-usa.com>
> > > > Inspired-by: Stephen Douthit <stephend@silicom-usa.com>
> > > > ---
> > > > drivers/net/ixgbe/base/ixgbe_common.c | 4 +-
> > > > drivers/net/ixgbe/ixgbe_ethdev.c | 180 ++++++++++----------------
> > > > 2 files changed, 71 insertions(+), 113 deletions(-)
> > > >
> > > > diff --git a/drivers/net/ixgbe/base/ixgbe_common.c
> > > > b/drivers/net/ixgbe/base/ixgbe_common.c
> > > > index aa843bd5c4a5..712062306491 100644
> > > > --- a/drivers/net/ixgbe/base/ixgbe_common.c
> > > > +++ b/drivers/net/ixgbe/base/ixgbe_common.c
> > > > @@ -4154,8 +4154,8 @@ s32 ixgbe_check_mac_link_generic(struct
> > > > ixgbe_hw *hw, ixgbe_link_speed *speed,
> > > > break;
> > > > case ixgbe_mac_X550EM_x:
> > > > case ixgbe_mac_X550EM_a:
> > > > - sfp_cage_full = IXGBE_READ_REG(hw, IXGBE_ESDP)
> > > > &
> > > > - IXGBE_ESDP_SDP0;
> > > > + sfp_cage_full = !(IXGBE_READ_REG(hw,
> > > > + IXGBE_ESDP)
> > > > &
> > > > + IXGBE_ESDP_SDP0);
> > > > break;
> > > > default:
> > > > /* sanity check - No SFP+ devices here */
> > > > diff --git
> > >
> > > Looks like you change the behavior of link status check for x550.
> > > I'm not an ixgbe expert, but I know this is not kernel driver's
> > > implementation.
> > >
> >
> > sigh. this was supposed to be part of a different patch which also
> > had some question about functionality. the SDP0 bit check doesn't
> > specifically need to be a check for a '1', since the bit reflects the state of
> the pin on the platform.
> > Intel's platform implementations have an inverter on the board to
> > switch the state.
> > MOD_ABS from an SFP will be '0' when an SFP is plugged in. with an
> > inverter in the platform the signal will be '1' when an SFP is plugged
> > in. there's no guidance from Intel's platform design guide that an
> > inverter needs to be between the SFP and the NIC SDP pin so having it
> > only follow Intel's platform implementations is hard to justify.
>
> OK, I assume the existing code should be proved for the normal scenario
> (remove and plug in with the same SFP) So how can we guarantee this
> change will not break something?
>
> Could you help me to understand why we should ignore the difference when
> SDP0 is 1 in normal scenario?
>
'normal scenario'? meaning on any Intel-implemented platform?
the code needs a devargs-like option so that platforms that don't implement
the inverter can still use this code in the expected way.
> Before change
> we will continue to read the link register and return the link speed.
>
> after change
> we return SPEED_UNKNOWN immediately .
>
>
>
>
> >
> > > So do you think this is a fix for both DPDK and kernel driver? if
> > > it is, please move this change into a separate patch and we need to
> > > reach the right expert to approve this.
> > >
> > >
> >
> > no, as explained above.
^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [PATCH 1/3] ixgbe: make link update thread periodic
2022-05-31 12:25 ` Jeff Daly
@ 2022-05-31 13:03 ` Zhang, Qi Z
0 siblings, 0 replies; 12+ messages in thread
From: Zhang, Qi Z @ 2022-05-31 13:03 UTC (permalink / raw)
To: Daly, Jeff, dev, Yang, Qiming, Wu, Wenjun1; +Cc: Stephen Douthit
> -----Original Message-----
> From: Jeff Daly <jeffd@silicom-usa.com>
> Sent: Tuesday, May 31, 2022 8:25 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; dev@dpdk.org; Yang, Qiming
> <qiming.yang@intel.com>; Wu, Wenjun1 <wenjun1.wu@intel.com>
> Cc: Stephen Douthit <stephend@silicom-usa.com>
> Subject: RE: [PATCH 1/3] ixgbe: make link update thread periodic
>
>
>
> > -----Original Message-----
> > From: Zhang, Qi Z <qi.z.zhang@intel.com>
> > Sent: Monday, May 30, 2022 10:21 AM
> > To: Jeff Daly <jeffd@silicom-usa.com>; dev@dpdk.org; Yang, Qiming
> > <qiming.yang@intel.com>; Wu, Wenjun1 <wenjun1.wu@intel.com>
> > Cc: Stephen Douthit <stephend@silicom-usa.com>
> > Subject: RE: [PATCH 1/3] ixgbe: make link update thread periodic
> >
> > Caution: This is an external email. Please take care when clicking
> > links or opening attachments.
> >
> >
> > > -----Original Message-----
> > > From: Jeff Daly <jeffd@silicom-usa.com>
> > > Sent: Monday, May 30, 2022 9:47 PM
> > > To: Zhang, Qi Z <qi.z.zhang@intel.com>; dev@dpdk.org; Yang, Qiming
> > > <qiming.yang@intel.com>; Wu, Wenjun1 <wenjun1.wu@intel.com>
> > > Cc: Stephen Douthit <stephend@silicom-usa.com>
> > > Subject: RE: [PATCH 1/3] ixgbe: make link update thread periodic
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Zhang, Qi Z <qi.z.zhang@intel.com>
> > > > Sent: Sunday, May 29, 2022 7:25 PM
> > > > To: Jeff Daly <jeffd@silicom-usa.com>; dev@dpdk.org; Yang, Qiming
> > > > <qiming.yang@intel.com>; Wu, Wenjun1 <wenjun1.wu@intel.com>
> > > > Cc: Stephen Douthit <stephend@silicom-usa.com>
> > > > Subject: RE: [PATCH 1/3] ixgbe: make link update thread periodic
> > > >
> > > > Caution: This is an external email. Please take care when clicking
> > > > links or opening attachments.
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Jeff Daly <jeffd@silicom-usa.com>
> > > > > Sent: Friday, May 20, 2022 2:03 AM
> > > > > To: dev@dpdk.org; Yang, Qiming <qiming.yang@intel.com>; Wu,
> > > > > Wenjun1 <wenjun1.wu@intel.com>
> > > > > Cc: Stephen Douthit <stephend@silicom-usa.com>
> > > > > Subject: [PATCH 1/3] ixgbe: make link update thread periodic
> > > > >
> > > > > Rather than run-to-completion, allow the link update thread to
> > > > > be
> > > > periodic.
> > > > > This will set the stage for properly handling hot-plugging.
> > > >
> > > > Could you explain more about what's the hot-plugging issue with
> > > > run-to- completion you try to fix?
> > > >
> > >
> > > it doesn't work right when you have SFPs. (at least not on our
> > > platform or on an
> > > 82599 dual SFP add-in card we have). run-to-completion only works 1x.
> > > if you remove and plug in a different SFP it doesn't work. This
> > > patch series should have been taking in context with the original
> > > SFP hotplug patch but apparently since I can't ever seem to get the
> > > patch submission threading to do what I mean perhaps some context
> > > has gone missing. the SFP hotplug fix has been in the queue since
> > > Dec 2021, has been reworked several times, has gone through a change
> > > in Intel maintainership. this patch series makes the SFP hot
> > > plugging work like the
> > Intel kernel driver does.
> > >
> > > > >
> > > > > Signed-off-by: Jeff Daly <jeffd@silicom-usa.com>
> > > > > Inspired-by: Stephen Douthit <stephend@silicom-usa.com>
> > > > > ---
> > > > > drivers/net/ixgbe/base/ixgbe_common.c | 4 +-
> > > > > drivers/net/ixgbe/ixgbe_ethdev.c | 180 ++++++++++----------------
> > > > > 2 files changed, 71 insertions(+), 113 deletions(-)
> > > > >
> > > > > diff --git a/drivers/net/ixgbe/base/ixgbe_common.c
> > > > > b/drivers/net/ixgbe/base/ixgbe_common.c
> > > > > index aa843bd5c4a5..712062306491 100644
> > > > > --- a/drivers/net/ixgbe/base/ixgbe_common.c
> > > > > +++ b/drivers/net/ixgbe/base/ixgbe_common.c
> > > > > @@ -4154,8 +4154,8 @@ s32 ixgbe_check_mac_link_generic(struct
> > > > > ixgbe_hw *hw, ixgbe_link_speed *speed,
> > > > > break;
> > > > > case ixgbe_mac_X550EM_x:
> > > > > case ixgbe_mac_X550EM_a:
> > > > > - sfp_cage_full = IXGBE_READ_REG(hw, IXGBE_ESDP)
> > > > > &
> > > > > - IXGBE_ESDP_SDP0;
> > > > > + sfp_cage_full = !(IXGBE_READ_REG(hw,
> > > > > + IXGBE_ESDP)
> > > > > &
> > > > > + IXGBE_ESDP_SDP0);
> > > > > break;
> > > > > default:
> > > > > /* sanity check - No SFP+ devices here */
> > > > > diff --git
> > > >
> > > > Looks like you change the behavior of link status check for x550.
> > > > I'm not an ixgbe expert, but I know this is not kernel driver's
> > > > implementation.
> > > >
> > >
> > > sigh. this was supposed to be part of a different patch which also
> > > had some question about functionality. the SDP0 bit check doesn't
> > > specifically need to be a check for a '1', since the bit reflects
> > > the state of
> > the pin on the platform.
> > > Intel's platform implementations have an inverter on the board to
> > > switch the state.
> > > MOD_ABS from an SFP will be '0' when an SFP is plugged in. with an
> > > inverter in the platform the signal will be '1' when an SFP is
> > > plugged in. there's no guidance from Intel's platform design guide
> > > that an inverter needs to be between the SFP and the NIC SDP pin so
> > > having it only follow Intel's platform implementations is hard to justify.
> >
> > OK, I assume the existing code should be proved for the normal
> > scenario (remove and plug in with the same SFP) So how can we
> > guarantee this change will not break something?
> >
> > Could you help me to understand why we should ignore the difference
> > when
> > SDP0 is 1 in normal scenario?
> >
>
> 'normal scenario'? meaning on any Intel-implemented platform?
>
> the code needs a devargs-like option so that platforms that don't implement
> the inverter can still use this code in the expected way.
Yes, please move this change into a separate patch and use a devargs for SDP pin invert case.
>
> > Before change
> > we will continue to read the link register and return the link speed.
> >
> > after change
> > we return SPEED_UNKNOWN immediately .
> >
> >
> >
> >
> > >
> > > > So do you think this is a fix for both DPDK and kernel driver? if
> > > > it is, please move this change into a separate patch and we need
> > > > to reach the right expert to approve this.
> > > >
> > > >
> > >
> > > no, as explained above.
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 2/3] ixgbe: move periodic link service work into separate function
2022-05-19 18:02 [PATCH 0/3] ixgbe: Fix SFP hotplug detection Jeff Daly
2022-05-19 18:02 ` [PATCH 1/3] ixgbe: make link update thread periodic Jeff Daly
@ 2022-05-19 18:02 ` Jeff Daly
2022-05-19 18:02 ` [PATCH 3/3] ixgbe: make hotplug detection aware of changed SFPs Jeff Daly
2 siblings, 0 replies; 12+ messages in thread
From: Jeff Daly @ 2022-05-19 18:02 UTC (permalink / raw)
To: dev, Qiming Yang, Wenjun Wu; +Cc: Stephen Douthit
The link update originally direct coded into the periodic service thread
and is made separate in preparation for additional SFP handling code.
Signed-off-by: Jeff Daly <jeffd@silicom-usa.com>
Inspired-by: Stephen Douthit <stephend@silicom-usa.com>
---
drivers/net/ixgbe/ixgbe_ethdev.c | 99 ++++++++++++++++++++++----------
drivers/net/ixgbe/ixgbe_ethdev.h | 1 +
2 files changed, 71 insertions(+), 29 deletions(-)
diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
index 81b15ad28212..f1694ef76b14 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -4131,48 +4131,89 @@ ixgbevf_check_link(struct ixgbe_hw *hw, ixgbe_link_speed *speed,
return ret_val;
}
-static void *
-ixgbe_dev_setup_link_thread_handler(void *param)
+static void
+ixgbe_link_service(struct rte_eth_dev *dev)
{
- struct rte_eth_dev *dev = (struct rte_eth_dev *)param;
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, start, ticks, service_ms;
+ bool link_up, autoneg = false, have_int = false;
+ u32 speed;
s32 err;
- bool link_up, autoneg = false;
- pthread_detach(pthread_self());
+ /* 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 */
- while (1) {
- service_ms = 100;
- if (intr->flags & IXGBE_FLAG_NEED_LINK_CONFIG) {
- speed = hw->phy.autoneg_advertised;
+ /* 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);
- 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\n", err);
+ return;
+ }
- err = ixgbe_setup_link(hw, speed, true);
+ intr->flags &= ~IXGBE_FLAG_NEED_LINK_CONFIG;
+}
- if (err == IXGBE_SUCCESS)
- err = ixgbe_check_link(hw, &speed, &link_up, 0);
+static void *
+ixgbe_dev_setup_link_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);
+ u32 speed, start, ticks, service_ms;
+ s32 err;
+ bool link_up = false;
- /* 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
- */
- if (err == IXGBE_SUCCESS && link_up) {
- service_ms = 2000;
- intr->flags &= ~IXGBE_FLAG_NEED_LINK_CONFIG;
- }
+ pthread_detach(pthread_self());
- 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);
- }
+ while (1) {
+ ixgbe_link_service(dev);
+
+ 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);
}
+ /* 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
diff --git a/drivers/net/ixgbe/ixgbe_ethdev.h b/drivers/net/ixgbe/ixgbe_ethdev.h
index 69e0e82a5b1a..f61706c9eae6 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
--
2.25.1
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH 3/3] ixgbe: make hotplug detection aware of changed SFPs
2022-05-19 18:02 [PATCH 0/3] ixgbe: Fix SFP hotplug detection Jeff Daly
2022-05-19 18:02 ` [PATCH 1/3] ixgbe: make link update thread periodic Jeff Daly
2022-05-19 18:02 ` [PATCH 2/3] ixgbe: move periodic link service work into separate function Jeff Daly
@ 2022-05-19 18:02 ` Jeff Daly
2 siblings, 0 replies; 12+ messages in thread
From: Jeff Daly @ 2022-05-19 18:02 UTC (permalink / raw)
To: dev, Qiming Yang, Wenjun Wu; +Cc: Stephen Douthit
SFP hotplug may require setup/configuration of the SFP in the event that
the SFP is different from the previously installed SFP. Ensure that the
periodic service task can handle this situation. Teach the interrupt
path that an SFP removal means that an SFP insertion will need to be
setup again.
Signed-off-by: Jeff Daly <jeffd@silicom-usa.com>
Inspired-by: Stephen Douthit <stephend@silicom-usa.com>
---
drivers/net/ixgbe/ixgbe_ethdev.c | 272 ++++++++++++++++++++++++++++++-
drivers/net/ixgbe/ixgbe_ethdev.h | 8 +
2 files changed, 273 insertions(+), 7 deletions(-)
diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
index f1694ef76b14..8464e30d6dd7 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -135,6 +135,8 @@ static const char * const ixgbevf_valid_arguments[] = {
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);
static int eth_ixgbe_dev_init(struct rte_eth_dev *eth_dev, void *init_params);
static int eth_ixgbe_dev_uninit(struct rte_eth_dev *eth_dev);
static int ixgbe_fdir_filter_init(struct rte_eth_dev *eth_dev);
@@ -764,6 +766,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.
*/
@@ -1051,6 +1080,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;
@@ -1123,6 +1153,18 @@ eth_ixgbe_dev_init(struct rte_eth_dev *eth_dev, void *init_params __rte_unused)
/* pick up the PCI bus settings for reporting later */
ixgbe_get_bus_info(hw);
+ /* 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;
+ }
+
+
/* Unlock any pending hardware semaphore */
ixgbe_swfw_lock_reset(hw);
@@ -2386,6 +2428,8 @@ ixgbe_dev_configure(struct rte_eth_dev *dev)
struct ixgbe_interrupt *intr =
IXGBE_DEV_PRIVATE_TO_INTR(dev->data->dev_private);
struct ixgbe_adapter *adapter = dev->data->dev_private;
+ struct ixgbe_hw *hw =
+ IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
int ret;
PMD_INIT_FUNC_TRACE();
@@ -2404,6 +2448,10 @@ ixgbe_dev_configure(struct rte_eth_dev *dev)
/* set flag to update link status after init */
intr->flags |= IXGBE_FLAG_NEED_LINK_UPDATE;
+ /* set flag to setup SFP after init */
+ if (ixgbe_is_sfp(hw))
+ intr->flags |= IXGBE_FLAG_NEED_SFP_SETUP;
+
/*
* Initialize to TRUE. If any of Rx queues doesn't meet the bulk
* allocation or vector Rx preconditions we will reset it.
@@ -2423,13 +2471,24 @@ ixgbe_dev_phy_intr_setup(struct rte_eth_dev *dev)
IXGBE_DEV_PRIVATE_TO_INTR(dev->data->dev_private);
uint32_t gpie;
- /* only set up it on X550EM_X */
+ /* only set up it on X550EM_X (external PHY interrupt)
+ * or on X550EM_a_* for SFP_PRSNT# de-assertion (SFP removal)
+ * */
if (hw->mac.type == ixgbe_mac_X550EM_x) {
gpie = IXGBE_READ_REG(hw, IXGBE_GPIE);
gpie |= IXGBE_SDP0_GPIEN_X550EM_x;
IXGBE_WRITE_REG(hw, IXGBE_GPIE, gpie);
if (hw->phy.type == ixgbe_phy_x550em_ext_t)
intr->mask |= IXGBE_EICR_GPI_SDP0_X550EM_x;
+ } else if (hw->mac.type == ixgbe_mac_X550EM_a) {
+ gpie = IXGBE_READ_REG(hw, IXGBE_GPIE);
+ gpie |= IXGBE_SDP0_GPIEN_X550EM_a;
+ IXGBE_WRITE_REG(hw, IXGBE_GPIE, gpie);
+ intr->mask |= IXGBE_EICR_GPI_SDP0_X550EM_a;
+ } else {
+ PMD_DRV_LOG(DEBUG,
+ "No PHY/SFP interrupt for MAC %d, PHY %d\n",
+ hw->mac.type, hw->phy.type);
}
}
@@ -2692,9 +2751,9 @@ ixgbe_dev_start(struct rte_eth_dev *dev)
}
}
- if (ixgbe_is_sfp(hw) && hw->phy.multispeed_fiber) {
+ if (ixgbe_is_sfp(hw)) {
err = hw->mac.ops.setup_sfp(hw);
- intr->flags |= IXGBE_FLAG_NEED_LINK_CONFIG;
+ intr->flags |= IXGBE_FLAG_NEED_SFP_SETUP;
err = rte_ctrl_thread_create(&ad->link_thread_tid,
"ixgbe-service-tid",
NULL,
@@ -4131,6 +4190,181 @@ ixgbevf_check_link(struct ixgbe_hw *hw, ixgbe_link_speed *speed,
return ret_val;
}
+/**
+ * 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;
+ }
+
+ /* 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)
{
@@ -4189,13 +4423,15 @@ ixgbe_dev_setup_link_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);
- u32 speed, start, ticks, service_ms;
+ uint64_t start, ticks, service_ms;
+ uint32_t speed;
s32 err;
bool link_up = false;
- pthread_detach(pthread_self());
+// pthread_detach(pthread_self());
while (1) {
+ ixgbe_sfp_service(dev);
ixgbe_link_service(dev);
if (!ixgbe_dev_link_update(dev, 0)) {
@@ -4227,6 +4463,25 @@ ixgbe_dev_setup_link_thread_handler(void *param)
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);
+}
+
+
/*
* In freebsd environment, nic_uio drivers do not support interrupts,
* rte_intr_callback_register() will fail to register interrupts.
@@ -4513,8 +4768,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;
@@ -4530,6 +4783,11 @@ 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 */
+ if (hw->mac.type == ixgbe_mac_X550EM_a &&
+ (eicr & IXGBE_EICR_GPI_SDP0_X550EM_a))
+ intr->flags |= IXGBE_FLAG_NEED_SFP_SETUP;
+
return 0;
}
diff --git a/drivers/net/ixgbe/ixgbe_ethdev.h b/drivers/net/ixgbe/ixgbe_ethdev.h
index f61706c9eae6..2037cdaa621e 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.h
+++ b/drivers/net/ixgbe/ixgbe_ethdev.h
@@ -517,6 +517,14 @@ struct ixgbe_vf_representor {
struct rte_eth_dev *pf_ethdev;
};
+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);
+
int ixgbe_vf_representor_init(struct rte_eth_dev *ethdev, void *init_params);
int ixgbe_vf_representor_uninit(struct rte_eth_dev *ethdev);
--
2.25.1
^ permalink raw reply [flat|nested] 12+ messages in thread