diff mbox series

[v6,2/2] net/ixgbe: Fix SFP detection and linking on hotplug

Message ID 20220412174220.31195-3-jeffd@silicom-usa.com (mailing list archive)
State Superseded, archived
Delegated to: Qi Zhang
Headers
Series ixgbe SFP handling fixes |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-x86_64-unit-testing success Testing PASS
ci/iol-abi-testing success Testing PASS
ci/iol-aarch64-unit-testing success Testing PASS
ci/iol-x86_64-compile-testing success Testing PASS
ci/iol-aarch64-compile-testing success Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/intel-Testing success Testing PASS

Commit Message

Jeff Daly April 12, 2022, 5:42 p.m. UTC 
  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 | 533 +++++++++++++++++++++++--------
 drivers/net/ixgbe/ixgbe_ethdev.h |  14 +-
 2 files changed, 410 insertions(+), 137 deletions(-)

Comments

Wang, Haiyue April 13, 2022, 2:46 a.m. UTC | #1 
> -----Original Message-----
> From: Jeff Daly <jeffd@silicom-usa.com>
> Sent: Wednesday, April 13, 2022 01:42
> To: dev@dpdk.org
> Cc: stable@dpdk.org; Stephen Douthit <stephend@silicom-usa.com>; Wang, Haiyue <haiyue.wang@intel.com>
> Subject: [PATCH v6 2/2] net/ixgbe: Fix SFP detection and linking on hotplug
> 
> 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
> 

So BIG change for new platform, DON'T CC to stable!

> Signed-off-by: Stephen Douthit <stephend@silicom-usa.com>
> Signed-off-by: Jeff Daly <jeffd@silicom-usa.com>
> ---
>  drivers/net/ixgbe/ixgbe_ethdev.c | 533 +++++++++++++++++++++++--------
>  drivers/net/ixgbe/ixgbe_ethdev.h |  14 +-
>  2 files changed, 410 insertions(+), 137 deletions(-)
> 
> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
> index f31bbb7895..9e720eee47 100644
> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> @@ -236,9 +236,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);
> 


> +	/* 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;


> +	/* 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;
>  }


TODO ?

> --
> 2.25.1
Morten Brørup April 13, 2022, 6:57 a.m. UTC | #2 
> From: Wang, Haiyue [mailto:haiyue.wang@intel.com]
> Sent: Wednesday, 13 April 2022 04.47
> To: Daly, Jeff; dev@dpdk.org
> Cc: stable@dpdk.org; Stephen Douthit; Yang, Qiming
> 
> > From: Jeff Daly <jeffd@silicom-usa.com>
> > Sent: Wednesday, April 13, 2022 01:42
> > To: dev@dpdk.org
> > Cc: stable@dpdk.org; Stephen Douthit <stephend@silicom-usa.com>;
> Wang, Haiyue <haiyue.wang@intel.com>
> >
> > 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
> >
> 
> So BIG change for new platform, DON'T CC to stable!

What do you mean by "new platform"? The ixgbe hardware and driver is not new.

This patch fixes a bug (with a serious impact when occurring), so it should be backported. The size of the patch does not disqualify it for backporting.

-Morten
Wang, Haiyue April 13, 2022, 7:01 a.m. UTC | #3 
> -----Original Message-----
> From: Morten Brørup <mb@smartsharesystems.com>
> Sent: Wednesday, April 13, 2022 14:58
> To: Wang, Haiyue <haiyue.wang@intel.com>; Daly, Jeff <jeffd@silicom-usa.com>; dev@dpdk.org
> Cc: stable@dpdk.org; Stephen Douthit <stephend@silicom-usa.com>; Yang, Qiming <qiming.yang@intel.com>
> Subject: RE: [PATCH v6 2/2] net/ixgbe: Fix SFP detection and linking on hotplug
> 
> > From: Wang, Haiyue [mailto:haiyue.wang@intel.com]
> > Sent: Wednesday, 13 April 2022 04.47
> > To: Daly, Jeff; dev@dpdk.org
> > Cc: stable@dpdk.org; Stephen Douthit; Yang, Qiming
> >
> > > From: Jeff Daly <jeffd@silicom-usa.com>
> > > Sent: Wednesday, April 13, 2022 01:42
> > > To: dev@dpdk.org
> > > Cc: stable@dpdk.org; Stephen Douthit <stephend@silicom-usa.com>;
> > Wang, Haiyue <haiyue.wang@intel.com>
> > >
> > > 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
> > >
> >
> > So BIG change for new platform, DON'T CC to stable!
> 
> What do you mean by "new platform"? The ixgbe hardware and driver is not new.
> 

It's soc NIC, ixgbe not support before.

> This patch fixes a bug (with a serious impact when occurring), so it should be backported. The size of
> the patch does not disqualify it for backporting.
> 
> -Morten
Morten Brørup April 13, 2022, 7:19 a.m. UTC | #4 
> From: Wang, Haiyue [mailto:haiyue.wang@intel.com]
> Sent: Wednesday, 13 April 2022 09.02
> 
> > From: Morten Brørup <mb@smartsharesystems.com>
> > Sent: Wednesday, April 13, 2022 14:58
> >
> > > From: Wang, Haiyue [mailto:haiyue.wang@intel.com]
> > > Sent: Wednesday, 13 April 2022 04.47
> > > To: Daly, Jeff; dev@dpdk.org
> > > Cc: stable@dpdk.org; Stephen Douthit; Yang, Qiming
> > >
> > > > From: Jeff Daly <jeffd@silicom-usa.com>
> > > > Sent: Wednesday, April 13, 2022 01:42
> > > > To: dev@dpdk.org
> > > > Cc: stable@dpdk.org; Stephen Douthit <stephend@silicom-usa.com>;
> > > Wang, Haiyue <haiyue.wang@intel.com>
> > > >
> > > > 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
> > > >
> > >
> > > So BIG change for new platform, DON'T CC to stable!
> >
> > What do you mean by "new platform"? The ixgbe hardware and driver is
> not new.
> >
> 
> It's soc NIC, ixgbe not support before.

If the patch only fixes the driver for a new NIC that not supported by older DPDK versions, and that NIC is not going to be supported by older DPDK versions, then I agree that there is no point in backporting it or CC'ing stable.

However, if the patch could also apply to any other ixgbe NIC that is potentially supported by older DPDK versions, then it should be backported.

> 
> > This patch fixes a bug (with a serious impact when occurring), so it
> should be backported. The size of
> > the patch does not disqualify it for backporting.
> >
> > -Morten
>
Wang, Haiyue April 13, 2022, 11:49 a.m. UTC | #5 
> -----Original Message-----
> From: Morten Brørup <mb@smartsharesystems.com>
> Sent: Wednesday, April 13, 2022 15:20
> To: Wang, Haiyue <haiyue.wang@intel.com>; Daly, Jeff <jeffd@silicom-usa.com>; dev@dpdk.org
> Cc: stable@dpdk.org; Stephen Douthit <stephend@silicom-usa.com>; Yang, Qiming <qiming.yang@intel.com>
> Subject: RE: [PATCH v6 2/2] net/ixgbe: Fix SFP detection and linking on hotplug
> 
> > From: Wang, Haiyue [mailto:haiyue.wang@intel.com]
> > Sent: Wednesday, 13 April 2022 09.02
> >
> > > From: Morten Brørup <mb@smartsharesystems.com>
> > > Sent: Wednesday, April 13, 2022 14:58
> > >
> > > > From: Wang, Haiyue [mailto:haiyue.wang@intel.com]
> > > > Sent: Wednesday, 13 April 2022 04.47
> > > > To: Daly, Jeff; dev@dpdk.org
> > > > Cc: stable@dpdk.org; Stephen Douthit; Yang, Qiming
> > > >
> > > > > From: Jeff Daly <jeffd@silicom-usa.com>
> > > > > Sent: Wednesday, April 13, 2022 01:42
> > > > > To: dev@dpdk.org
> > > > > Cc: stable@dpdk.org; Stephen Douthit <stephend@silicom-usa.com>;
> > > > Wang, Haiyue <haiyue.wang@intel.com>
> > > > >
> > > > > 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
> > > > >
> > > >
> > > > So BIG change for new platform, DON'T CC to stable!
> > >
> > > What do you mean by "new platform"? The ixgbe hardware and driver is
> > not new.
> > >
> >
> > It's soc NIC, ixgbe not support before.
> 
> If the patch only fixes the driver for a new NIC that not supported by older DPDK versions, and that
> NIC is not going to be supported by older DPDK versions, then I agree that there is no point in
> backporting it or CC'ing stable.
> 
> However, if the patch could also apply to any other ixgbe NIC that is potentially supported by older
> DPDK versions, then it should be backported.

It's hard to say, these years, I see many ixgbe link related fixes.
At least now, no big link fix for normal ixgbe NICs.

This patch still have some kind of TODOs. And this is not acceptable for
us to maintain this kind of code for released stable DPDK version. I don't
want to see many follow fixes ...

And we have two DPDK development cycle (22.07 22.11) to make it for next
stable release.

> 
> >
> > > This patch fixes a bug (with a serious impact when occurring), so it
> > should be backported. The size of
> > > the patch does not disqualify it for backporting.
> > >
> > > -Morten
> >
Morten Brørup April 13, 2022, 12:54 p.m. UTC | #6 
> From: Wang, Haiyue [mailto:haiyue.wang@intel.com]
> Sent: Wednesday, 13 April 2022 13.49
> 
> > From: Morten Brørup <mb@smartsharesystems.com>
> > Sent: Wednesday, April 13, 2022 15:20
> >
> > > From: Wang, Haiyue [mailto:haiyue.wang@intel.com]
> > > Sent: Wednesday, 13 April 2022 09.02
> > >
> > > > From: Morten Brørup <mb@smartsharesystems.com>
> > > > Sent: Wednesday, April 13, 2022 14:58
> > > >
> > > > > From: Wang, Haiyue [mailto:haiyue.wang@intel.com]
> > > > > Sent: Wednesday, 13 April 2022 04.47
> > > > > To: Daly, Jeff; dev@dpdk.org
> > > > > Cc: stable@dpdk.org; Stephen Douthit; Yang, Qiming
> > > > >
> > > > > > From: Jeff Daly <jeffd@silicom-usa.com>
> > > > > > Sent: Wednesday, April 13, 2022 01:42
> > > > > > To: dev@dpdk.org
> > > > > > Cc: stable@dpdk.org; Stephen Douthit <stephend@silicom-
> usa.com>;
> > > > > Wang, Haiyue <haiyue.wang@intel.com>
> > > > > >
> > > > > > 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
> > > > > >
> > > > >
> > > > > So BIG change for new platform, DON'T CC to stable!
> > > >
> > > > What do you mean by "new platform"? The ixgbe hardware and driver
> is
> > > not new.
> > > >
> > >
> > > It's soc NIC, ixgbe not support before.
> >
> > If the patch only fixes the driver for a new NIC that not supported
> by older DPDK versions, and that
> > NIC is not going to be supported by older DPDK versions, then I agree
> that there is no point in
> > backporting it or CC'ing stable.
> >
> > However, if the patch could also apply to any other ixgbe NIC that is
> potentially supported by older
> > DPDK versions, then it should be backported.
> 
> It's hard to say, these years, I see many ixgbe link related fixes.

The physical layers have always been tricky... Apparently, they still are. :-)

> At least now, no big link fix for normal ixgbe NICs.

I would not discriminate between normal and less common NICs. If they are not EOL according to Intel, the drivers should support them.

In the real world, though, driver development resources will be allocated to the important customers and/or the high volume products. So I do understand your concern! Not being directly involved in this work myself, it is easy to voice my opinion as a "backseat driver". :-)

> 
> This patch still have some kind of TODOs. And this is not acceptable
> for
> us to maintain this kind of code for released stable DPDK version. I
> don't
> want to see many follow fixes ...
> 
> And we have two DPDK development cycle (22.07 22.11) to make it for
> next
> stable release.
> 

I 100 % agree that all the TODOs should be solved first, so the driver is reliable and complete before any backporting starts. Adding follow-on fixes in multiple DPDK versions is a waste of maintainer time, and I agree with your pushback when this is the reason.

With that in mind, CC'ing stable could be postponed until the patch reaches backporting worthy quality.

> >
> > >
> > > > This patch fixes a bug (with a serious impact when occurring), so
> it
> > > should be backported. The size of
> > > > the patch does not disqualify it for backporting.
> > > >
> > > > -Morten
> > >
>
Jeff Daly April 13, 2022, 3:23 p.m. UTC | #7 
> -----Original Message-----
> From: Morten Brørup <mb@smartsharesystems.com>
> Sent: Wednesday, April 13, 2022 3:20 AM
> To: Wang, Haiyue <haiyue.wang@intel.com>; Jeff Daly <jeffd@silicom-
> usa.com>; dev@dpdk.org
> Cc: stable@dpdk.org; Stephen Douthit <stephend@silicom-usa.com>; Yang,
> Qiming <qiming.yang@intel.com>
> Subject: RE: [PATCH v6 2/2] net/ixgbe: Fix SFP detection and linking on
> hotplug
> 
> Caution: This is an external email. Please take care when clicking links or
> opening attachments.
> 
> 
> > From: Wang, Haiyue [mailto:haiyue.wang@intel.com]
> > Sent: Wednesday, 13 April 2022 09.02
> >
> > > From: Morten Brørup <mb@smartsharesystems.com>
> > > Sent: Wednesday, April 13, 2022 14:58
> > >
> > > > From: Wang, Haiyue [mailto:haiyue.wang@intel.com]
> > > > Sent: Wednesday, 13 April 2022 04.47
> > > > To: Daly, Jeff; dev@dpdk.org
> > > > Cc: stable@dpdk.org; Stephen Douthit; Yang, Qiming
> > > >
> > > > > From: Jeff Daly <jeffd@silicom-usa.com>
> > > > > Sent: Wednesday, April 13, 2022 01:42
> > > > > To: dev@dpdk.org
> > > > > Cc: stable@dpdk.org; Stephen Douthit <stephend@silicom-usa.com>;
> > > > Wang, Haiyue <haiyue.wang@intel.com>
> > > > >
> > > > > 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
> > > > >
> > > >
> > > > So BIG change for new platform, DON'T CC to stable!
> > >
> > > What do you mean by "new platform"? The ixgbe hardware and driver is
> > not new.
> > >
> >
> > It's soc NIC, ixgbe not support before.
> 
> If the patch only fixes the driver for a new NIC that not supported by older
> DPDK versions, and that NIC is not going to be supported by older DPDK
> versions, then I agree that there is no point in backporting it or CC'ing stable.
> 
> However, if the patch could also apply to any other ixgbe NIC that is
> potentially supported by older DPDK versions, then it should be backported.
> 

This patch is not *only* for soc NIC, it's for *all* IXGBE supported NIC that have SFP cages. 
The code was also validated using a dual port 82599ES 10G SFI/SFP+ PCIe NIC adapter.

> >
> > > This patch fixes a bug (with a serious impact when occurring), so it
> > should be backported. The size of
> > > the patch does not disqualify it for backporting.
> > >
> > > -Morten
> >
Wang, Haiyue April 14, 2022, 2:49 a.m. UTC | #8 
> -----Original Message-----
> From: Jeff Daly <jeffd@silicom-usa.com>
> Sent: Wednesday, April 13, 2022 01:42
> To: dev@dpdk.org
> Cc: stable@dpdk.org; Stephen Douthit <stephend@silicom-usa.com>; Wang, Haiyue <haiyue.wang@intel.com>
> Subject: [PATCH v6 2/2] net/ixgbe: Fix SFP detection and linking on hotplug
> 
> 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 | 533 +++++++++++++++++++++++--------
>  drivers/net/ixgbe/ixgbe_ethdev.h |  14 +-
>  2 files changed, 410 insertions(+), 137 deletions(-)
> 


> 
>  struct ixgbe_stat_mapping_registers {
> @@ -510,7 +509,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;

No need to rename this variable, we can separate this patch as least into
two patches:

1st, change the thread handle 'ixgbe_dev_setup_link_thread_handler' from

run-once to as periodical, to handle the original issue.

The name 'ixgbe_dev_setup_link_thread_handler' may be not suitable now,
as it is a service thread.

We can change it to "'ixgbe_link_service_thread_handler'" to reflect the
change purpose.

2nd, add the SFP hotplug in this patch.



>  };
> 
> --
> 2.25.1
Wang, Haiyue April 14, 2022, 2:59 a.m. UTC | #9 
> -----Original Message-----
> From: Wang, Haiyue
> Sent: Thursday, April 14, 2022 10:49
> To: Jeff Daly <jeffd@silicom-usa.com>; dev@dpdk.org
> Cc: stable@dpdk.org; Stephen Douthit <stephend@silicom-usa.com>
> Subject: RE: [PATCH v6 2/2] net/ixgbe: Fix SFP detection and linking on hotplug
> 
> > -----Original Message-----
> > From: Jeff Daly <jeffd@silicom-usa.com>
> > Sent: Wednesday, April 13, 2022 01:42
> > To: dev@dpdk.org
> > Cc: stable@dpdk.org; Stephen Douthit <stephend@silicom-usa.com>; Wang, Haiyue <haiyue.wang@intel.com>
> > Subject: [PATCH v6 2/2] net/ixgbe: Fix SFP detection and linking on hotplug
> >
> > 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 | 533 +++++++++++++++++++++++--------
> >  drivers/net/ixgbe/ixgbe_ethdev.h |  14 +-
> >  2 files changed, 410 insertions(+), 137 deletions(-)
> >
> 
> 
> >
> >  struct ixgbe_stat_mapping_registers {
> > @@ -510,7 +509,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;
> 
> No need to rename this variable, 

Let's do link related service now, so we can keep it, I missed to add my
comment. ;-)

> we can separate this patch as least into
> two patches:


> 
> 1st, change the thread handle 'ixgbe_dev_setup_link_thread_handler' from
> 
> run-once to as periodical, to handle the original issue.
> 
> The name 'ixgbe_dev_setup_link_thread_handler' may be not suitable now,
> as it is a service thread.
> 
> We can change it to "'ixgbe_link_service_thread_handler'" to reflect the
> change purpose.
> 
> 2nd, add the SFP hotplug in this patch.
> 
> 
> 
> >  };
> >
> > --
> > 2.25.1
Jeff Daly April 14, 2022, 10:40 a.m. UTC | #10 
> -----Original Message-----
> From: Wang, Haiyue <haiyue.wang@intel.com>
> Sent: Wednesday, April 13, 2022 11:00 PM
> To: Jeff Daly <jeffd@silicom-usa.com>; dev@dpdk.org
> Cc: stable@dpdk.org; Stephen Douthit <stephend@silicom-usa.com>
> Subject: RE: [PATCH v6 2/2] net/ixgbe: Fix SFP detection and linking on
> hotplug
> 
> Caution: This is an external email. Please take care when clicking links or
> opening attachments.
> 
> 
> > -----Original Message-----
> > From: Wang, Haiyue
> > Sent: Thursday, April 14, 2022 10:49
> > To: Jeff Daly <jeffd@silicom-usa.com>; dev@dpdk.org
> > Cc: stable@dpdk.org; Stephen Douthit <stephend@silicom-usa.com>
> > Subject: RE: [PATCH v6 2/2] net/ixgbe: Fix SFP detection and linking
> > on hotplug
> >
> > > -----Original Message-----
> > > From: Jeff Daly <jeffd@silicom-usa.com>
> > > Sent: Wednesday, April 13, 2022 01:42
> > > To: dev@dpdk.org
> > > Cc: stable@dpdk.org; Stephen Douthit <stephend@silicom-usa.com>;
> > > Wang, Haiyue <haiyue.wang@intel.com>
> > > Subject: [PATCH v6 2/2] net/ixgbe: Fix SFP detection and linking on
> > > hotplug
> > >
> > > 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 | 533
> > > +++++++++++++++++++++++--------  drivers/net/ixgbe/ixgbe_ethdev.h |
> > > 14 +-
> > >  2 files changed, 410 insertions(+), 137 deletions(-)
> > >
> >
> >
> > >
> > >  struct ixgbe_stat_mapping_registers { @@ -510,7 +509,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;
> >
> > No need to rename this variable,
> 
> Let's do link related service now, so we can keep it, I missed to add my
> comment. ;-)
> 

I don't understand this reply, are you still asking to rework the patch or not?

> > we can separate this patch as least into two patches:
> 
> 
> >
> > 1st, change the thread handle 'ixgbe_dev_setup_link_thread_handler'
> > from
> >
> > run-once to as periodical, to handle the original issue.
> >
> > The name 'ixgbe_dev_setup_link_thread_handler' may be not suitable
> > now, as it is a service thread.
> >
> > We can change it to "'ixgbe_link_service_thread_handler'" to reflect
> > the change purpose.
> >
> > 2nd, add the SFP hotplug in this patch.
> >
> >
> >
> > >  };
> > >
> > > --
> > > 2.25.1
Jeff Daly April 14, 2022, 10:49 a.m. UTC | #11 
> -----Original Message-----
> From: Wang, Haiyue <haiyue.wang@intel.com>
> Sent: Tuesday, April 12, 2022 10:47 PM
> To: Jeff Daly <jeffd@silicom-usa.com>; dev@dpdk.org
> Cc: stable@dpdk.org; Stephen Douthit <stephend@silicom-usa.com>; Yang,
> Qiming <qiming.yang@intel.com>
> Subject: RE: [PATCH v6 2/2] net/ixgbe: Fix SFP detection and linking on
> hotplug
> 
> 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: Wednesday, April 13, 2022 01:42
> > To: dev@dpdk.org
> > Cc: stable@dpdk.org; Stephen Douthit <stephend@silicom-usa.com>;
> Wang,
> > Haiyue <haiyue.wang@intel.com>
> > Subject: [PATCH v6 2/2] net/ixgbe: Fix SFP detection and linking on
> > hotplug
> >
> > 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
> >
> 
> So BIG change for new platform, DON'T CC to stable!
> 
> > Signed-off-by: Stephen Douthit <stephend@silicom-usa.com>
> > Signed-off-by: Jeff Daly <jeffd@silicom-usa.com>
> > ---
> >  drivers/net/ixgbe/ixgbe_ethdev.c | 533
> > +++++++++++++++++++++++--------  drivers/net/ixgbe/ixgbe_ethdev.h |
> > 14 +-
> >  2 files changed, 410 insertions(+), 137 deletions(-)
> >
> > diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> > b/drivers/net/ixgbe/ixgbe_ethdev.c
> > index f31bbb7895..9e720eee47 100644
> > --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> > +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> > @@ -236,9 +236,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);
> >
> 
> 
> > +     /* 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;
> 
> 
> > +     /* 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;
> >  }
> 
> 
> TODO ?
> 

These were notes that some further refinements could be possible.  I can remove the comments if that makes it more acceptable.  The first TODO was a note that (for FreeBSD mostly) w/o interrupts the code below (not shown, which checks for 'have_int') will run vs. exiting early and relying on the NEED_SFP_SETUP flag being set elsewhere.  The 2nd TODO was a question from the original developer to himself where (I believe) he was considering other implementations which may not use MOD_ABS the same that other platforms do.  I don't think this is an issue (that I know of) so removing this commend and just moving on would be the best thing, and if someone else knows of a situation where this consideration might need to be made, then another patch can be submitted to address it.

> > --
> > 2.25.1
Jeff Daly April 14, 2022, 11:08 a.m. UTC | #12 
> -----Original Message-----
> From: Jeff Daly <jeffd@silicom-usa.com>
> Sent: Thursday, April 14, 2022 6:50 AM
> To: Wang, Haiyue <haiyue.wang@intel.com>; dev@dpdk.org
> Cc: stable@dpdk.org; Yang, Qiming <qiming.yang@intel.com>
> Subject: RE: [PATCH v6 2/2] net/ixgbe: Fix SFP detection and linking on
> hotplug
> 
> Caution: This is an external email. Please take care when clicking links or
> opening attachments.
> 
> 
> > -----Original Message-----
> > From: Wang, Haiyue <haiyue.wang@intel.com>
> > Sent: Tuesday, April 12, 2022 10:47 PM
> > To: Jeff Daly <jeffd@silicom-usa.com>; dev@dpdk.org
> > Cc: stable@dpdk.org; Stephen Douthit <stephend@silicom-usa.com>; Yang,
> > Qiming <qiming.yang@intel.com>
> > Subject: RE: [PATCH v6 2/2] net/ixgbe: Fix SFP detection and linking
> > on hotplug
> >
> > 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: Wednesday, April 13, 2022 01:42
> > > To: dev@dpdk.org
> > > Cc: stable@dpdk.org; Stephen Douthit <stephend@silicom-usa.com>;
> > Wang,
> > > Haiyue <haiyue.wang@intel.com>
> > > Subject: [PATCH v6 2/2] net/ixgbe: Fix SFP detection and linking on
> > > hotplug
> > >
> > > 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
> > >
> >
> > So BIG change for new platform, DON'T CC to stable!
> >
> > > Signed-off-by: Stephen Douthit <stephend@silicom-usa.com>
> > > Signed-off-by: Jeff Daly <jeffd@silicom-usa.com>
> > > ---
> > >  drivers/net/ixgbe/ixgbe_ethdev.c | 533
> > > +++++++++++++++++++++++--------  drivers/net/ixgbe/ixgbe_ethdev.h |
> > > 14 +-
> > >  2 files changed, 410 insertions(+), 137 deletions(-)
> > >
> > > diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> > > b/drivers/net/ixgbe/ixgbe_ethdev.c
> > > index f31bbb7895..9e720eee47 100644
> > > --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> > > +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> > > @@ -236,9 +236,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);
> > >
> >
> >
> > > +     /* 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;
> >
> >
> > > +     /* 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;
> > >  }
> >
> >
> > TODO ?
> >
> 
> These were notes that some further refinements could be possible.  I can
> remove the comments if that makes it more acceptable.  The first TODO was
> a note that (for FreeBSD mostly) w/o interrupts the code below (not shown,
> which checks for 'have_int') will run vs. exiting early and relying on the
> NEED_SFP_SETUP flag being set elsewhere.  The 2nd TODO was a question
> from the original developer to himself where (I believe) he was considering
> other implementations which may not use MOD_ABS the same that other
> platforms do.  I don't think this is an issue (that I know of) so removing this
> commend and just moving on would be the best thing, and if someone else
> knows of a situation where this consideration might need to be made, then
> another patch can be submitted to address it.
> 

Ugh.  Apologies for the dumb Outlook editor not wrapping the lines properly,
if you are reading this on patches.dpdk.org it runs off the page.  Repeating below:

These were notes that some further refinements could be possible.  I can 
remove the comments if that makes it more acceptable.  The first TODO was
a note that (for FreeBSD mostly) w/o interrupts the code below (not shown, 
which checks for 'have_int') will run vs. exiting early and relying on the 
NEED_SFP_SETUP flag being set elsewhere.  The 2nd TODO was a question 
from the original developer to himself where (I believe) he was considering
other implementations which may not use MOD_ABS the same that other
platforms do.  I don't think this is an issue (that I know of) so removing this
comment and just moving on would be the best thing, and if someone else
knows of a situation where this consideration might need to be made, then
another patch can be submitted to address it.

> > > --
> > > 2.25.1
Wang, Haiyue April 14, 2022, 12:11 p.m. UTC | #13 
> -----Original Message-----
> From: Jeff Daly <jeffd@silicom-usa.com>
> Sent: Thursday, April 14, 2022 18:41
> To: Wang, Haiyue <haiyue.wang@intel.com>; dev@dpdk.org
> Cc: stable@dpdk.org
> Subject: RE: [PATCH v6 2/2] net/ixgbe: Fix SFP detection and linking on hotplug
> 
> 
> 
> > -----Original Message-----
> > From: Wang, Haiyue <haiyue.wang@intel.com>
> > Sent: Wednesday, April 13, 2022 11:00 PM
> > To: Jeff Daly <jeffd@silicom-usa.com>; dev@dpdk.org
> > Cc: stable@dpdk.org; Stephen Douthit <stephend@silicom-usa.com>
> > Subject: RE: [PATCH v6 2/2] net/ixgbe: Fix SFP detection and linking on
> > hotplug
> >
> > Caution: This is an external email. Please take care when clicking links or
> > opening attachments.
> >
> >
> > > -----Original Message-----
> > > From: Wang, Haiyue
> > > Sent: Thursday, April 14, 2022 10:49
> > > To: Jeff Daly <jeffd@silicom-usa.com>; dev@dpdk.org
> > > Cc: stable@dpdk.org; Stephen Douthit <stephend@silicom-usa.com>
> > > Subject: RE: [PATCH v6 2/2] net/ixgbe: Fix SFP detection and linking
> > > on hotplug
> > >
> > > > -----Original Message-----
> > > > From: Jeff Daly <jeffd@silicom-usa.com>
> > > > Sent: Wednesday, April 13, 2022 01:42
> > > > To: dev@dpdk.org
> > > > Cc: stable@dpdk.org; Stephen Douthit <stephend@silicom-usa.com>;
> > > > Wang, Haiyue <haiyue.wang@intel.com>
> > > > Subject: [PATCH v6 2/2] net/ixgbe: Fix SFP detection and linking on
> > > > hotplug
> > > >
> > > > 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 | 533
> > > > +++++++++++++++++++++++--------  drivers/net/ixgbe/ixgbe_ethdev.h |
> > > > 14 +-
> > > >  2 files changed, 410 insertions(+), 137 deletions(-)
> > > >
> > >
> > >
> > > >
> > > >  struct ixgbe_stat_mapping_registers { @@ -510,7 +509,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;
> > >
> > > No need to rename this variable,
> >
> > Let's do link related service now, so we can keep it, I missed to add my
> > comment. ;-)
> >
> 
> I don't understand this reply, are you still asking to rework the patch or not?
> 

Different thing.

1. This var can be kept to trace the created thread. (change less code to keep
   the patch clean.)
2. Yes, two patches.

> > > we can separate this patch as least into two patches:
> >
> >
> > >
> > > 1st, change the thread handle 'ixgbe_dev_setup_link_thread_handler'
> > > from
> > >
> > > run-once to as periodical, to handle the original issue.
> > >
> > > The name 'ixgbe_dev_setup_link_thread_handler' may be not suitable
> > > now, as it is a service thread.
> > >
> > > We can change it to "'ixgbe_link_service_thread_handler'" to reflect
> > > the change purpose.
> > >
> > > 2nd, add the SFP hotplug in this patch.
> > >
> > >
> > >
> > > >  };
> > > >
> > > > --
> > > > 2.25.1
Jeff Daly April 18, 2022, 9:54 p.m. UTC | #14 
> -----Original Message-----
> From: Wang, Haiyue <haiyue.wang@intel.com>
> Sent: Thursday, April 14, 2022 8:11 AM
> To: Jeff Daly <jeffd@silicom-usa.com>; dev@dpdk.org
> Cc: stable@dpdk.org
> Subject: RE: [PATCH v6 2/2] net/ixgbe: Fix SFP detection and linking on
> hotplug
> 
> 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: Thursday, April 14, 2022 18:41
> > To: Wang, Haiyue <haiyue.wang@intel.com>; dev@dpdk.org
> > Cc: stable@dpdk.org
> > Subject: RE: [PATCH v6 2/2] net/ixgbe: Fix SFP detection and linking
> > on hotplug
> >
> >
> >
> > > -----Original Message-----
> > > From: Wang, Haiyue <haiyue.wang@intel.com>
> > > Sent: Wednesday, April 13, 2022 11:00 PM
> > > To: Jeff Daly <jeffd@silicom-usa.com>; dev@dpdk.org
> > > Cc: stable@dpdk.org; Stephen Douthit <stephend@silicom-usa.com>
> > > Subject: RE: [PATCH v6 2/2] net/ixgbe: Fix SFP detection and linking
> > > on hotplug
> > >
> > > Caution: This is an external email. Please take care when clicking
> > > links or opening attachments.
> > >
> > >
> > > > -----Original Message-----
> > > > From: Wang, Haiyue
> > > > Sent: Thursday, April 14, 2022 10:49
> > > > To: Jeff Daly <jeffd@silicom-usa.com>; dev@dpdk.org
> > > > Cc: stable@dpdk.org; Stephen Douthit <stephend@silicom-usa.com>
> > > > Subject: RE: [PATCH v6 2/2] net/ixgbe: Fix SFP detection and
> > > > linking on hotplug
> > > >
> > > > > -----Original Message-----
> > > > > From: Jeff Daly <jeffd@silicom-usa.com>
> > > > > Sent: Wednesday, April 13, 2022 01:42
> > > > > To: dev@dpdk.org
> > > > > Cc: stable@dpdk.org; Stephen Douthit <stephend@silicom-usa.com>;
> > > > > Wang, Haiyue <haiyue.wang@intel.com>
> > > > > Subject: [PATCH v6 2/2] net/ixgbe: Fix SFP detection and linking
> > > > > on hotplug
> > > > >
> > > > > 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 | 533
> > > > > +++++++++++++++++++++++--------
> > > > > +++++++++++++++++++++++drivers/net/ixgbe/ixgbe_ethdev.h |
> > > > > 14 +-
> > > > >  2 files changed, 410 insertions(+), 137 deletions(-)
> > > > >
> > > >
> > > >
> > > > >
> > > > >  struct ixgbe_stat_mapping_registers { @@ -510,7 +509,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;
> > > >
> > > > No need to rename this variable,
> > >
> > > Let's do link related service now, so we can keep it, I missed to
> > > add my comment. ;-)
> > >
> >
> > I don't understand this reply, are you still asking to rework the patch or
> not?
> >
> 
> Different thing.
> 
> 1. This var can be kept to trace the created thread. (change less code to keep
>    the patch clean.)
> 2. Yes, two patches.
> 

ok, I guess I'm just being thick-headed here, but I still don't understand why you are saying it should be split into 
2 patches.  if I understand *what* you are asking, you're saying make the original thread periodic to continuously
do ixgbe_link_setup() ?  I believe the problem with the setup is that the sfp_type is only detected once
at initialization time and if nothing is in the cage then the code just returns IXGBE_SUCCESS, in which case making 
this task periodic is useless.  the whole issue of hotplug is only addressed by the entire patch which 1) makes the
task periodic, 2) changes the actions of the task to look for whether the cage has something in it and whether its
been changed and needs to be configured again.


> > > > we can separate this patch as least into two patches:
> > >
> > >
> > > >
> > > > 1st, change the thread handle 'ixgbe_dev_setup_link_thread_handler'
> > > > from
> > > >
> > > > run-once to as periodical, to handle the original issue.
> > > >
> > > > The name 'ixgbe_dev_setup_link_thread_handler' may be not suitable
> > > > now, as it is a service thread.
> > > >
> > > > We can change it to "'ixgbe_link_service_thread_handler'" to
> > > > reflect the change purpose.
> > > >
> > > > 2nd, add the SFP hotplug in this patch.
> > > >
> > > >
> > > >
> > > > >  };
> > > > >
> > > > > --
> > > > > 2.25.1
Wang, Haiyue April 19, 2022, 2:05 a.m. UTC | #15 
> -----Original Message-----
> From: Jeff Daly <jeffd@silicom-usa.com>
> Sent: Tuesday, April 19, 2022 05:55
> To: Wang, Haiyue <haiyue.wang@intel.com>; dev@dpdk.org
> Cc: stable@dpdk.org
> Subject: RE: [PATCH v6 2/2] net/ixgbe: Fix SFP detection and linking on hotplug
> 
> 
> 
> > -----Original Message-----
> > From: Wang, Haiyue <haiyue.wang@intel.com>
> > Sent: Thursday, April 14, 2022 8:11 AM
> > To: Jeff Daly <jeffd@silicom-usa.com>; dev@dpdk.org
> > Cc: stable@dpdk.org
> > Subject: RE: [PATCH v6 2/2] net/ixgbe: Fix SFP detection and linking on
> > hotplug
> >
> > 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: Thursday, April 14, 2022 18:41
> > > To: Wang, Haiyue <haiyue.wang@intel.com>; dev@dpdk.org
> > > Cc: stable@dpdk.org
> > > Subject: RE: [PATCH v6 2/2] net/ixgbe: Fix SFP detection and linking
> > > on hotplug
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Wang, Haiyue <haiyue.wang@intel.com>
> > > > Sent: Wednesday, April 13, 2022 11:00 PM
> > > > To: Jeff Daly <jeffd@silicom-usa.com>; dev@dpdk.org
> > > > Cc: stable@dpdk.org; Stephen Douthit <stephend@silicom-usa.com>
> > > > Subject: RE: [PATCH v6 2/2] net/ixgbe: Fix SFP detection and linking
> > > > on hotplug
> > > >
> > > > Caution: This is an external email. Please take care when clicking
> > > > links or opening attachments.
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Wang, Haiyue
> > > > > Sent: Thursday, April 14, 2022 10:49
> > > > > To: Jeff Daly <jeffd@silicom-usa.com>; dev@dpdk.org
> > > > > Cc: stable@dpdk.org; Stephen Douthit <stephend@silicom-usa.com>
> > > > > Subject: RE: [PATCH v6 2/2] net/ixgbe: Fix SFP detection and
> > > > > linking on hotplug
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Jeff Daly <jeffd@silicom-usa.com>
> > > > > > Sent: Wednesday, April 13, 2022 01:42
> > > > > > To: dev@dpdk.org
> > > > > > Cc: stable@dpdk.org; Stephen Douthit <stephend@silicom-usa.com>;
> > > > > > Wang, Haiyue <haiyue.wang@intel.com>
> > > > > > Subject: [PATCH v6 2/2] net/ixgbe: Fix SFP detection and linking
> > > > > > on hotplug
> > > > > >
> > > > > > 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 | 533
> > > > > > +++++++++++++++++++++++--------
> > > > > > +++++++++++++++++++++++drivers/net/ixgbe/ixgbe_ethdev.h |
> > > > > > 14 +-
> > > > > >  2 files changed, 410 insertions(+), 137 deletions(-)
> > > > > >
> > > > >
> > > > >
> > > > > >
> > > > > >  struct ixgbe_stat_mapping_registers { @@ -510,7 +509,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;
> > > > >
> > > > > No need to rename this variable,
> > > >
> > > > Let's do link related service now, so we can keep it, I missed to
> > > > add my comment. ;-)
> > > >
> > >
> > > I don't understand this reply, are you still asking to rework the patch or
> > not?
> > >
> >
> > Different thing.
> >
> > 1. This var can be kept to trace the created thread. (change less code to keep
> >    the patch clean.)
> > 2. Yes, two patches.
> >
> 
> ok, I guess I'm just being thick-headed here, but I still don't understand why you are saying it
> should be split into
> 2 patches.  if I understand *what* you are asking, you're saying make the original thread periodic to
> continuously

Well, ...

Your patch merges the original 'ixgbe_setup_link' task into one,
this will make us hard to review the whole design. So what I said
is: firstly, let's change the thread to a service thread to handle
the 'ixgbe_setup_link' subtask firstly. Which is 'ixgbe_link_service'
in your whole patch.

Small patch is good for us to review, and try to do one thing.

Hope this time, I can make myself clear. ;-)

> do ixgbe_link_setup() ?  I believe the problem with the setup is that the sfp_type is only detected
> once
> at initialization time and if nothing is in the cage then the code just returns IXGBE_SUCCESS, in
> which case making
> this task periodic is useless.  the whole issue of hotplug is only addressed by the entire patch which
> 1) makes the
> task periodic, 2) changes the actions of the task to look for whether the cage has something in it and
> whether its
> been changed and needs to be configured again.
> 
> 
> > > > > we can separate this patch as least into two patches:
> > > >
> > > >
> > > > >
> > > > > 1st, change the thread handle 'ixgbe_dev_setup_link_thread_handler'
> > > > > from
> > > > >
> > > > > run-once to as periodical, to handle the original issue.
> > > > >
> > > > > The name 'ixgbe_dev_setup_link_thread_handler' may be not suitable
> > > > > now, as it is a service thread.
> > > > >
> > > > > We can change it to "'ixgbe_link_service_thread_handler'" to
> > > > > reflect the change purpose.
> > > > >
> > > > > 2nd, add the SFP hotplug in this patch.
> > > > >
> > > > >
> > > > >
> > > > > >  };
> > > > > >
> > > > > > --
> > > > > > 2.25.1
Jeff Daly April 19, 2022, 5:33 p.m. UTC | #16 
> -----Original Message-----
> From: Wang, Haiyue <haiyue.wang@intel.com>
> Sent: Monday, April 18, 2022 10:05 PM
> To: Jeff Daly <jeffd@silicom-usa.com>; dev@dpdk.org
> Cc: stable@dpdk.org; Yang, Qiming <qiming.yang@intel.com>
> Subject: RE: [PATCH v6 2/2] net/ixgbe: Fix SFP detection and linking on
> hotplug
> 
> 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: Tuesday, April 19, 2022 05:55
> > To: Wang, Haiyue <haiyue.wang@intel.com>; dev@dpdk.org
> > Cc: stable@dpdk.org
> > Subject: RE: [PATCH v6 2/2] net/ixgbe: Fix SFP detection and linking
> > on hotplug
> >
> >
> >
> > > -----Original Message-----
> > > From: Wang, Haiyue <haiyue.wang@intel.com>
> > > Sent: Thursday, April 14, 2022 8:11 AM
> > > To: Jeff Daly <jeffd@silicom-usa.com>; dev@dpdk.org
> > > Cc: stable@dpdk.org
> > > Subject: RE: [PATCH v6 2/2] net/ixgbe: Fix SFP detection and linking
> > > on hotplug
> > >
> > > 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: Thursday, April 14, 2022 18:41
> > > > To: Wang, Haiyue <haiyue.wang@intel.com>; dev@dpdk.org
> > > > Cc: stable@dpdk.org
> > > > Subject: RE: [PATCH v6 2/2] net/ixgbe: Fix SFP detection and
> > > > linking on hotplug
> > > >
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Wang, Haiyue <haiyue.wang@intel.com>
> > > > > Sent: Wednesday, April 13, 2022 11:00 PM
> > > > > To: Jeff Daly <jeffd@silicom-usa.com>; dev@dpdk.org
> > > > > Cc: stable@dpdk.org; Stephen Douthit <stephend@silicom-usa.com>
> > > > > Subject: RE: [PATCH v6 2/2] net/ixgbe: Fix SFP detection and
> > > > > linking on hotplug
> > > > >
> > > > > Caution: This is an external email. Please take care when
> > > > > clicking links or opening attachments.
> > > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Wang, Haiyue
> > > > > > Sent: Thursday, April 14, 2022 10:49
> > > > > > To: Jeff Daly <jeffd@silicom-usa.com>; dev@dpdk.org
> > > > > > Cc: stable@dpdk.org; Stephen Douthit
> > > > > > <stephend@silicom-usa.com>
> > > > > > Subject: RE: [PATCH v6 2/2] net/ixgbe: Fix SFP detection and
> > > > > > linking on hotplug
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Jeff Daly <jeffd@silicom-usa.com>
> > > > > > > Sent: Wednesday, April 13, 2022 01:42
> > > > > > > To: dev@dpdk.org
> > > > > > > Cc: stable@dpdk.org; Stephen Douthit
> > > > > > > <stephend@silicom-usa.com>; Wang, Haiyue
> > > > > > > <haiyue.wang@intel.com>
> > > > > > > Subject: [PATCH v6 2/2] net/ixgbe: Fix SFP detection and
> > > > > > > linking on hotplug
> > > > > > >
> > > > > > > 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 | 533
> > > > > > > +++++++++++++++++++++++--------
> > > > > > > +++++++++++++++++++++++drivers/net/ixgbe/ixgbe_ethdev.h |
> > > > > > > 14 +-
> > > > > > >  2 files changed, 410 insertions(+), 137 deletions(-)
> > > > > > >
> > > > > >
> > > > > >
> > > > > > >
> > > > > > >  struct ixgbe_stat_mapping_registers { @@ -510,7 +509,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;
> > > > > >
> > > > > > No need to rename this variable,
> > > > >
> > > > > Let's do link related service now, so we can keep it, I missed
> > > > > to add my comment. ;-)
> > > > >
> > > >
> > > > I don't understand this reply, are you still asking to rework the
> > > > patch or
> > > not?
> > > >
> > >
> > > Different thing.
> > >
> > > 1. This var can be kept to trace the created thread. (change less code to
> keep
> > >    the patch clean.)
> > > 2. Yes, two patches.
> > >
> >
> > ok, I guess I'm just being thick-headed here, but I still don't
> > understand why you are saying it should be split into
> > 2 patches.  if I understand *what* you are asking, you're saying make
> > the original thread periodic to continuously
> 
> Well, ...
> 
> Your patch merges the original 'ixgbe_setup_link' task into one, this will
> make us hard to review the whole design. So what I said
> is: firstly, let's change the thread to a service thread to handle the
> 'ixgbe_setup_link' subtask firstly. Which is 'ixgbe_link_service'
> in your whole patch.
> 

still not 100%, are you suggesting that the original ixgbe_dev_setup_link_thread_handler()
which currently is not periodic and only really calls ixgbe_setup_link() be changed to be a 
periodic task that essentially does what the patch's ixgbe_link_service() function does which
would only be checking whether link config is needed and if so calls ixgbe_setup_link() as 
before?

if I'm following the code correctly, it ends up going down to ixgbe_check_mac_link_generic()
which looks at SDP0 (in the case of needing xtalk fix) which incorrectly will set sfp_cage_full
when in fact the PRESENT# signal (or MOD_ABS#) is active low.  the code is extremely convoluted
in it's effort to be smart so I may be missing something, but I *believe* that what we end up
with is completely unnecessary probing of i2c busses looking for SFPs that don't exist.  even when
making it periodic, I don't think it's going to end up with working code.

> Small patch is good for us to review, and try to do one thing.
> 
> Hope this time, I can make myself clear. ;-)
> 
> > do ixgbe_link_setup() ?  I believe the problem with the setup is that
> > the sfp_type is only detected once at initialization time and if
> > nothing is in the cage then the code just returns IXGBE_SUCCESS, in
> > which case making this task periodic is useless.  the whole issue of
> > hotplug is only addressed by the entire patch which
> > 1) makes the
> > task periodic, 2) changes the actions of the task to look for whether
> > the cage has something in it and whether its been changed and needs to
> > be configured again.
> >
> >
> > > > > > we can separate this patch as least into two patches:
> > > > >
> > > > >
> > > > > >
> > > > > > 1st, change the thread handle
> 'ixgbe_dev_setup_link_thread_handler'
> > > > > > from
> > > > > >
> > > > > > run-once to as periodical, to handle the original issue.
> > > > > >
> > > > > > The name 'ixgbe_dev_setup_link_thread_handler' may be not
> > > > > > suitable now, as it is a service thread.
> > > > > >
> > > > > > We can change it to "'ixgbe_link_service_thread_handler'" to
> > > > > > reflect the change purpose.
> > > > > >
> > > > > > 2nd, add the SFP hotplug in this patch.
> > > > > >
> > > > > >
> > > > > >
> > > > > > >  };
> > > > > > >
> > > > > > > --
> > > > > > > 2.25.1
Wang, Haiyue April 20, 2022, 1:09 a.m. UTC | #17 
> -----Original Message-----
> From: Jeff Daly <jeffd@silicom-usa.com>
> Sent: Wednesday, April 20, 2022 01:34
> To: Wang, Haiyue <haiyue.wang@intel.com>; dev@dpdk.org
> Cc: stable@dpdk.org; Yang, Qiming <qiming.yang@intel.com>
> Subject: RE: [PATCH v6 2/2] net/ixgbe: Fix SFP detection and linking on hotplug
> 
> 
> 
> > -----Original Message-----
> > From: Wang, Haiyue <haiyue.wang@intel.com>
> > Sent: Monday, April 18, 2022 10:05 PM
> > To: Jeff Daly <jeffd@silicom-usa.com>; dev@dpdk.org
> > Cc: stable@dpdk.org; Yang, Qiming <qiming.yang@intel.com>
> > Subject: RE: [PATCH v6 2/2] net/ixgbe: Fix SFP detection and linking on
> > hotplug
> >
> > 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: Tuesday, April 19, 2022 05:55
> > > To: Wang, Haiyue <haiyue.wang@intel.com>; dev@dpdk.org
> > > Cc: stable@dpdk.org
> > > Subject: RE: [PATCH v6 2/2] net/ixgbe: Fix SFP detection and linking
> > > on hotplug
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Wang, Haiyue <haiyue.wang@intel.com>
> > > > Sent: Thursday, April 14, 2022 8:11 AM
> > > > To: Jeff Daly <jeffd@silicom-usa.com>; dev@dpdk.org
> > > > Cc: stable@dpdk.org
> > > > Subject: RE: [PATCH v6 2/2] net/ixgbe: Fix SFP detection and linking
> > > > on hotplug
> > > >
> > > > 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: Thursday, April 14, 2022 18:41
> > > > > To: Wang, Haiyue <haiyue.wang@intel.com>; dev@dpdk.org
> > > > > Cc: stable@dpdk.org
> > > > > Subject: RE: [PATCH v6 2/2] net/ixgbe: Fix SFP detection and
> > > > > linking on hotplug
> > > > >
> > > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Wang, Haiyue <haiyue.wang@intel.com>
> > > > > > Sent: Wednesday, April 13, 2022 11:00 PM
> > > > > > To: Jeff Daly <jeffd@silicom-usa.com>; dev@dpdk.org
> > > > > > Cc: stable@dpdk.org; Stephen Douthit <stephend@silicom-usa.com>
> > > > > > Subject: RE: [PATCH v6 2/2] net/ixgbe: Fix SFP detection and
> > > > > > linking on hotplug
> > > > > >
> > > > > > Caution: This is an external email. Please take care when
> > > > > > clicking links or opening attachments.
> > > > > >
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Wang, Haiyue
> > > > > > > Sent: Thursday, April 14, 2022 10:49
> > > > > > > To: Jeff Daly <jeffd@silicom-usa.com>; dev@dpdk.org
> > > > > > > Cc: stable@dpdk.org; Stephen Douthit
> > > > > > > <stephend@silicom-usa.com>
> > > > > > > Subject: RE: [PATCH v6 2/2] net/ixgbe: Fix SFP detection and
> > > > > > > linking on hotplug
> > > > > > >
> > > > > > > > -----Original Message-----
> > > > > > > > From: Jeff Daly <jeffd@silicom-usa.com>
> > > > > > > > Sent: Wednesday, April 13, 2022 01:42
> > > > > > > > To: dev@dpdk.org
> > > > > > > > Cc: stable@dpdk.org; Stephen Douthit
> > > > > > > > <stephend@silicom-usa.com>; Wang, Haiyue
> > > > > > > > <haiyue.wang@intel.com>
> > > > > > > > Subject: [PATCH v6 2/2] net/ixgbe: Fix SFP detection and
> > > > > > > > linking on hotplug
> > > > > > > >
> > > > > > > > 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 | 533
> > > > > > > > +++++++++++++++++++++++--------
> > > > > > > > +++++++++++++++++++++++drivers/net/ixgbe/ixgbe_ethdev.h |
> > > > > > > > 14 +-
> > > > > > > >  2 files changed, 410 insertions(+), 137 deletions(-)
> > > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > >  struct ixgbe_stat_mapping_registers { @@ -510,7 +509,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;
> > > > > > >
> > > > > > > No need to rename this variable,
> > > > > >
> > > > > > Let's do link related service now, so we can keep it, I missed
> > > > > > to add my comment. ;-)
> > > > > >
> > > > >
> > > > > I don't understand this reply, are you still asking to rework the
> > > > > patch or
> > > > not?
> > > > >
> > > >
> > > > Different thing.
> > > >
> > > > 1. This var can be kept to trace the created thread. (change less code to
> > keep
> > > >    the patch clean.)
> > > > 2. Yes, two patches.
> > > >
> > >
> > > ok, I guess I'm just being thick-headed here, but I still don't
> > > understand why you are saying it should be split into
> > > 2 patches.  if I understand *what* you are asking, you're saying make
> > > the original thread periodic to continuously
> >
> > Well, ...
> >
> > Your patch merges the original 'ixgbe_setup_link' task into one, this will
> > make us hard to review the whole design. So what I said
> > is: firstly, let's change the thread to a service thread to handle the
> > 'ixgbe_setup_link' subtask firstly. Which is 'ixgbe_link_service'
> > in your whole patch.
> >
> 
> still not 100%, are you suggesting that the original ixgbe_dev_setup_link_thread_handler()
> which currently is not periodic and only really calls ixgbe_setup_link() be changed to be a
> periodic task that essentially does what the patch's ixgbe_link_service() function does which
> would only be checking whether link config is needed and if so calls ixgbe_setup_link() as
> before?
> 
> if I'm following the code correctly, it ends up going down to ixgbe_check_mac_link_generic()
> which looks at SDP0 (in the case of needing xtalk fix) which incorrectly will set sfp_cage_full
> when in fact the PRESENT# signal (or MOD_ABS#) is active low.  the code is extremely convoluted
> in it's effort to be smart so I may be missing something, but I *believe* that what we end up
> with is completely unnecessary probing of i2c busses looking for SFPs that don't exist.  even when
> making it periodic, I don't think it's going to end up with working code.


I'm lost ...

The 'periodic' is the service thread running mode, but the subtask is
ONLY called when be scheduled, like:

if (!(adapter->flags & IXGBE_FLAG_NEED_LINK_CONFIG))
		return;

> 
> > Small patch is good for us to review, and try to do one thing.
> >
> > Hope this time, I can make myself clear. ;-)
> >
> > > do ixgbe_link_setup() ?  I believe the problem with the setup is that
> > > the sfp_type is only detected once at initialization time and if
> > > nothing is in the cage then the code just returns IXGBE_SUCCESS, in
> > > which case making this task periodic is useless.  the whole issue of
> > > hotplug is only addressed by the entire patch which
> > > 1) makes the
> > > task periodic, 2) changes the actions of the task to look for whether
> > > the cage has something in it and whether its been changed and needs to
> > > be configured again.
> > >
> > >
> > > > > > > we can separate this patch as least into two patches:
> > > > > >
> > > > > >
> > > > > > >
> > > > > > > 1st, change the thread handle
> > 'ixgbe_dev_setup_link_thread_handler'
> > > > > > > from
> > > > > > >
> > > > > > > run-once to as periodical, to handle the original issue.
> > > > > > >
> > > > > > > The name 'ixgbe_dev_setup_link_thread_handler' may be not
> > > > > > > suitable now, as it is a service thread.
> > > > > > >
> > > > > > > We can change it to "'ixgbe_link_service_thread_handler'" to
> > > > > > > reflect the change purpose.
> > > > > > >
> > > > > > > 2nd, add the SFP hotplug in this patch.
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > >  };
> > > > > > > >
> > > > > > > > --
> > > > > > > > 2.25.1
Jeff Daly April 21, 2022, 5:31 p.m. UTC | #18 
> -----Original Message-----
> From: Wang, Haiyue <haiyue.wang@intel.com>
> Sent: Tuesday, April 19, 2022 9:09 PM
> To: Jeff Daly <jeffd@silicom-usa.com>; dev@dpdk.org
> Cc: stable@dpdk.org; Yang, Qiming <qiming.yang@intel.com>
> Subject: RE: [PATCH v6 2/2] net/ixgbe: Fix SFP detection and linking on
> hotplug
> 
> 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: Wednesday, April 20, 2022 01:34
> > To: Wang, Haiyue <haiyue.wang@intel.com>; dev@dpdk.org
> > Cc: stable@dpdk.org; Yang, Qiming <qiming.yang@intel.com>
> > Subject: RE: [PATCH v6 2/2] net/ixgbe: Fix SFP detection and linking
> > on hotplug
> >
> >
> >
> > > -----Original Message-----
> > > From: Wang, Haiyue <haiyue.wang@intel.com>
> > > Sent: Monday, April 18, 2022 10:05 PM
> > > To: Jeff Daly <jeffd@silicom-usa.com>; dev@dpdk.org
> > > Cc: stable@dpdk.org; Yang, Qiming <qiming.yang@intel.com>
> > > Subject: RE: [PATCH v6 2/2] net/ixgbe: Fix SFP detection and linking
> > > on hotplug
> > >
> > > 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: Tuesday, April 19, 2022 05:55
> > > > To: Wang, Haiyue <haiyue.wang@intel.com>; dev@dpdk.org
> > > > Cc: stable@dpdk.org
> > > > Subject: RE: [PATCH v6 2/2] net/ixgbe: Fix SFP detection and
> > > > linking on hotplug
> > > >
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Wang, Haiyue <haiyue.wang@intel.com>
> > > > > Sent: Thursday, April 14, 2022 8:11 AM
> > > > > To: Jeff Daly <jeffd@silicom-usa.com>; dev@dpdk.org
> > > > > Cc: stable@dpdk.org
> > > > > Subject: RE: [PATCH v6 2/2] net/ixgbe: Fix SFP detection and
> > > > > linking on hotplug
> > > > >
> > > > > 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: Thursday, April 14, 2022 18:41
> > > > > > To: Wang, Haiyue <haiyue.wang@intel.com>; dev@dpdk.org
> > > > > > Cc: stable@dpdk.org
> > > > > > Subject: RE: [PATCH v6 2/2] net/ixgbe: Fix SFP detection and
> > > > > > linking on hotplug
> > > > > >
> > > > > >
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Wang, Haiyue <haiyue.wang@intel.com>
> > > > > > > Sent: Wednesday, April 13, 2022 11:00 PM
> > > > > > > To: Jeff Daly <jeffd@silicom-usa.com>; dev@dpdk.org
> > > > > > > Cc: stable@dpdk.org; Stephen Douthit
> > > > > > > <stephend@silicom-usa.com>
> > > > > > > Subject: RE: [PATCH v6 2/2] net/ixgbe: Fix SFP detection and
> > > > > > > linking on hotplug
> > > > > > >
> > > > > > > Caution: This is an external email. Please take care when
> > > > > > > clicking links or opening attachments.
> > > > > > >
> > > > > > >
> > > > > > > > -----Original Message-----
> > > > > > > > From: Wang, Haiyue
> > > > > > > > Sent: Thursday, April 14, 2022 10:49
> > > > > > > > To: Jeff Daly <jeffd@silicom-usa.com>; dev@dpdk.org
> > > > > > > > Cc: stable@dpdk.org; Stephen Douthit
> > > > > > > > <stephend@silicom-usa.com>
> > > > > > > > Subject: RE: [PATCH v6 2/2] net/ixgbe: Fix SFP detection
> > > > > > > > and linking on hotplug
> > > > > > > >
> > > > > > > > > -----Original Message-----
> > > > > > > > > From: Jeff Daly <jeffd@silicom-usa.com>
> > > > > > > > > Sent: Wednesday, April 13, 2022 01:42
> > > > > > > > > To: dev@dpdk.org
> > > > > > > > > Cc: stable@dpdk.org; Stephen Douthit
> > > > > > > > > <stephend@silicom-usa.com>; Wang, Haiyue
> > > > > > > > > <haiyue.wang@intel.com>
> > > > > > > > > Subject: [PATCH v6 2/2] net/ixgbe: Fix SFP detection and
> > > > > > > > > linking on hotplug
> > > > > > > > >
> > > > > > > > > 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 | 533
> > > > > > > > > +++++++++++++++++++++++--------
> > > > > > > > > +++++++++++++++++++++++drivers/net/ixgbe/ixgbe_ethdev.h
> > > > > > > > > +++++++++++++++++++++++|
> > > > > > > > > 14 +-
> > > > > > > > >  2 files changed, 410 insertions(+), 137 deletions(-)
> > > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > >
> > > > > > > > >  struct ixgbe_stat_mapping_registers { @@ -510,7 +509,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;
> > > > > > > >
> > > > > > > > No need to rename this variable,
> > > > > > >
> > > > > > > Let's do link related service now, so we can keep it, I
> > > > > > > missed to add my comment. ;-)
> > > > > > >
> > > > > >
> > > > > > I don't understand this reply, are you still asking to rework
> > > > > > the patch or
> > > > > not?
> > > > > >
> > > > >
> > > > > Different thing.
> > > > >
> > > > > 1. This var can be kept to trace the created thread. (change
> > > > > less code to
> > > keep
> > > > >    the patch clean.)
> > > > > 2. Yes, two patches.
> > > > >
> > > >
> > > > ok, I guess I'm just being thick-headed here, but I still don't
> > > > understand why you are saying it should be split into
> > > > 2 patches.  if I understand *what* you are asking, you're saying
> > > > make the original thread periodic to continuously
> > >
> > > Well, ...
> > >
> > > Your patch merges the original 'ixgbe_setup_link' task into one,
> > > this will make us hard to review the whole design. So what I said
> > > is: firstly, let's change the thread to a service thread to handle
> > > the 'ixgbe_setup_link' subtask firstly. Which is 'ixgbe_link_service'
> > > in your whole patch.
> > >
> >
> > still not 100%, are you suggesting that the original
> > ixgbe_dev_setup_link_thread_handler()
> > which currently is not periodic and only really calls
> > ixgbe_setup_link() be changed to be a periodic task that essentially
> > does what the patch's ixgbe_link_service() function does which would
> > only be checking whether link config is needed and if so calls
> ixgbe_setup_link() as before?
> >
> > if I'm following the code correctly, it ends up going down to
> > ixgbe_check_mac_link_generic() which looks at SDP0 (in the case of
> > needing xtalk fix) which incorrectly will set sfp_cage_full when in
> > fact the PRESENT# signal (or MOD_ABS#) is active low.  the code is
> > extremely convoluted in it's effort to be smart so I may be missing
> > something, but I *believe* that what we end up with is completely
> unnecessary probing of i2c busses looking for SFPs that don't exist.  even
> when making it periodic, I don't think it's going to end up with working code.
> 
> 
> I'm lost ...
> 
> The 'periodic' is the service thread running mode, but the subtask is ONLY
> called when be scheduled, like:
> 
> if (!(adapter->flags & IXGBE_FLAG_NEED_LINK_CONFIG))
>                 return;
> 

this makes no sense.  the *only* time right now when IXGBE_FLAG_NEED_LINK_CONFIG is set is
immediately before the thread is created!  there's no 'hotplug' part of the patch vs fixing the 
'original issue' (your words, below).  the 'original issue' is that the code doesn't work for hotplug
(SFP cages), so there's no breaking up the patch to fix any original issue first.  the only issue is
that for SFP cages, the code doesn't work.  

> >
> > > Small patch is good for us to review, and try to do one thing.
> > >
> > > Hope this time, I can make myself clear. ;-)
> > >
> > > > do ixgbe_link_setup() ?  I believe the problem with the setup is
> > > > that the sfp_type is only detected once at initialization time and
> > > > if nothing is in the cage then the code just returns
> > > > IXGBE_SUCCESS, in which case making this task periodic is useless.
> > > > the whole issue of hotplug is only addressed by the entire patch
> > > > which
> > > > 1) makes the
> > > > task periodic, 2) changes the actions of the task to look for
> > > > whether the cage has something in it and whether its been changed
> > > > and needs to be configured again.
> > > >
> > > >
> > > > > > > > we can separate this patch as least into two patches:
> > > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > > 1st, change the thread handle
> > > 'ixgbe_dev_setup_link_thread_handler'
> > > > > > > > from
> > > > > > > >
> > > > > > > > run-once to as periodical, to handle the original issue.
> > > > > > > >
> > > > > > > > The name 'ixgbe_dev_setup_link_thread_handler' may be not
> > > > > > > > suitable now, as it is a service thread.
> > > > > > > >
> > > > > > > > We can change it to "'ixgbe_link_service_thread_handler'"
> > > > > > > > to reflect the change purpose.
> > > > > > > >
> > > > > > > > 2nd, add the SFP hotplug in this patch.
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > >  };
> > > > > > > > >
> > > > > > > > > --
> > > > > > > > > 2.25.1
Wang, Haiyue April 22, 2022, 2:11 a.m. UTC | #19 
> -----Original Message-----
> From: Jeff Daly <jeffd@silicom-usa.com>
> Sent: Friday, April 22, 2022 01:31
> To: Wang, Haiyue <haiyue.wang@intel.com>; dev@dpdk.org
> Cc: stable@dpdk.org; Yang, Qiming <qiming.yang@intel.com>
> Subject: RE: [PATCH v6 2/2] net/ixgbe: Fix SFP detection and linking on hotplug
> 
> 
> 
> > -----Original Message-----
> > From: Wang, Haiyue <haiyue.wang@intel.com>
> > Sent: Tuesday, April 19, 2022 9:09 PM
> > To: Jeff Daly <jeffd@silicom-usa.com>; dev@dpdk.org
> > Cc: stable@dpdk.org; Yang, Qiming <qiming.yang@intel.com>
> > Subject: RE: [PATCH v6 2/2] net/ixgbe: Fix SFP detection and linking on
> > hotplug
> >
> > 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: Wednesday, April 20, 2022 01:34
> > > To: Wang, Haiyue <haiyue.wang@intel.com>; dev@dpdk.org
> > > Cc: stable@dpdk.org; Yang, Qiming <qiming.yang@intel.com>
> > > Subject: RE: [PATCH v6 2/2] net/ixgbe: Fix SFP detection and linking
> > > on hotplug
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Wang, Haiyue <haiyue.wang@intel.com>
> > > > Sent: Monday, April 18, 2022 10:05 PM
> > > > To: Jeff Daly <jeffd@silicom-usa.com>; dev@dpdk.org
> > > > Cc: stable@dpdk.org; Yang, Qiming <qiming.yang@intel.com>
> > > > Subject: RE: [PATCH v6 2/2] net/ixgbe: Fix SFP detection and linking
> > > > on hotplug
> > > >
> > > > 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: Tuesday, April 19, 2022 05:55
> > > > > To: Wang, Haiyue <haiyue.wang@intel.com>; dev@dpdk.org
> > > > > Cc: stable@dpdk.org
> > > > > Subject: RE: [PATCH v6 2/2] net/ixgbe: Fix SFP detection and
> > > > > linking on hotplug
> > > > >
> > > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Wang, Haiyue <haiyue.wang@intel.com>
> > > > > > Sent: Thursday, April 14, 2022 8:11 AM
> > > > > > To: Jeff Daly <jeffd@silicom-usa.com>; dev@dpdk.org
> > > > > > Cc: stable@dpdk.org
> > > > > > Subject: RE: [PATCH v6 2/2] net/ixgbe: Fix SFP detection and
> > > > > > linking on hotplug
> > > > > >
> > > > > > 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: Thursday, April 14, 2022 18:41
> > > > > > > To: Wang, Haiyue <haiyue.wang@intel.com>; dev@dpdk.org
> > > > > > > Cc: stable@dpdk.org
> > > > > > > Subject: RE: [PATCH v6 2/2] net/ixgbe: Fix SFP detection and
> > > > > > > linking on hotplug
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > > -----Original Message-----
> > > > > > > > From: Wang, Haiyue <haiyue.wang@intel.com>
> > > > > > > > Sent: Wednesday, April 13, 2022 11:00 PM
> > > > > > > > To: Jeff Daly <jeffd@silicom-usa.com>; dev@dpdk.org
> > > > > > > > Cc: stable@dpdk.org; Stephen Douthit
> > > > > > > > <stephend@silicom-usa.com>
> > > > > > > > Subject: RE: [PATCH v6 2/2] net/ixgbe: Fix SFP detection and
> > > > > > > > linking on hotplug
> > > > > > > >
> > > > > > > > Caution: This is an external email. Please take care when
> > > > > > > > clicking links or opening attachments.
> > > > > > > >
> > > > > > > >
> > > > > > > > > -----Original Message-----
> > > > > > > > > From: Wang, Haiyue
> > > > > > > > > Sent: Thursday, April 14, 2022 10:49
> > > > > > > > > To: Jeff Daly <jeffd@silicom-usa.com>; dev@dpdk.org
> > > > > > > > > Cc: stable@dpdk.org; Stephen Douthit
> > > > > > > > > <stephend@silicom-usa.com>
> > > > > > > > > Subject: RE: [PATCH v6 2/2] net/ixgbe: Fix SFP detection
> > > > > > > > > and linking on hotplug
> > > > > > > > >
> > > > > > > > > > -----Original Message-----
> > > > > > > > > > From: Jeff Daly <jeffd@silicom-usa.com>
> > > > > > > > > > Sent: Wednesday, April 13, 2022 01:42
> > > > > > > > > > To: dev@dpdk.org
> > > > > > > > > > Cc: stable@dpdk.org; Stephen Douthit
> > > > > > > > > > <stephend@silicom-usa.com>; Wang, Haiyue
> > > > > > > > > > <haiyue.wang@intel.com>
> > > > > > > > > > Subject: [PATCH v6 2/2] net/ixgbe: Fix SFP detection and
> > > > > > > > > > linking on hotplug
> > > > > > > > > >
> > > > > > > > > > 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 | 533
> > > > > > > > > > +++++++++++++++++++++++--------
> > > > > > > > > > +++++++++++++++++++++++drivers/net/ixgbe/ixgbe_ethdev.h
> > > > > > > > > > +++++++++++++++++++++++|
> > > > > > > > > > 14 +-
> > > > > > > > > >  2 files changed, 410 insertions(+), 137 deletions(-)
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > >  struct ixgbe_stat_mapping_registers { @@ -510,7 +509,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;
> > > > > > > > >
> > > > > > > > > No need to rename this variable,
> > > > > > > >
> > > > > > > > Let's do link related service now, so we can keep it, I
> > > > > > > > missed to add my comment. ;-)
> > > > > > > >
> > > > > > >
> > > > > > > I don't understand this reply, are you still asking to rework
> > > > > > > the patch or
> > > > > > not?
> > > > > > >
> > > > > >
> > > > > > Different thing.
> > > > > >
> > > > > > 1. This var can be kept to trace the created thread. (change
> > > > > > less code to
> > > > keep
> > > > > >    the patch clean.)
> > > > > > 2. Yes, two patches.
> > > > > >
> > > > >
> > > > > ok, I guess I'm just being thick-headed here, but I still don't
> > > > > understand why you are saying it should be split into
> > > > > 2 patches.  if I understand *what* you are asking, you're saying
> > > > > make the original thread periodic to continuously
> > > >
> > > > Well, ...
> > > >
> > > > Your patch merges the original 'ixgbe_setup_link' task into one,
> > > > this will make us hard to review the whole design. So what I said
> > > > is: firstly, let's change the thread to a service thread to handle
> > > > the 'ixgbe_setup_link' subtask firstly. Which is 'ixgbe_link_service'
> > > > in your whole patch.
> > > >
> > >
> > > still not 100%, are you suggesting that the original
> > > ixgbe_dev_setup_link_thread_handler()
> > > which currently is not periodic and only really calls
> > > ixgbe_setup_link() be changed to be a periodic task that essentially
> > > does what the patch's ixgbe_link_service() function does which would
> > > only be checking whether link config is needed and if so calls
> > ixgbe_setup_link() as before?
> > >
> > > if I'm following the code correctly, it ends up going down to
> > > ixgbe_check_mac_link_generic() which looks at SDP0 (in the case of
> > > needing xtalk fix) which incorrectly will set sfp_cage_full when in
> > > fact the PRESENT# signal (or MOD_ABS#) is active low.  the code is
> > > extremely convoluted in it's effort to be smart so I may be missing
> > > something, but I *believe* that what we end up with is completely
> > unnecessary probing of i2c busses looking for SFPs that don't exist.  even
> > when making it periodic, I don't think it's going to end up with working code.
> >
> >
> > I'm lost ...
> >
> > The 'periodic' is the service thread running mode, but the subtask is ONLY
> > called when be scheduled, like:
> >
> > if (!(adapter->flags & IXGBE_FLAG_NEED_LINK_CONFIG))
> >                 return;
> >
> 
> this makes no sense.  the *only* time right now when IXGBE_FLAG_NEED_LINK_CONFIG is set is
> immediately before the thread is created!  there's no 'hotplug' part of the patch vs fixing the
> 'original issue' (your words, below).  the 'original issue' is that the code doesn't work for hotplug
> (SFP cages), so there's no breaking up the patch to fix any original issue first.  the only issue is
> that for SFP cages, the code doesn't work.

No!

Your code is right, but looks like you still be lost in your hotplug issue.

+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); 
                 ^
                 |
                 +- 1st patch, just handle the 'ixgbe_link_service_thread_handler' issue.

I've no words to comment again, hope this makes thing clear.

> 
> > >
> > > > Small patch is good for us to review, and try to do one thing.
> > > >
> > > > Hope this time, I can make myself clear. ;-)
> > > >
> > > > > do ixgbe_link_setup() ?  I believe the problem with the setup is
> > > > > that the sfp_type is only detected once at initialization time and
> > > > > if nothing is in the cage then the code just returns
> > > > > IXGBE_SUCCESS, in which case making this task periodic is useless.
> > > > > the whole issue of hotplug is only addressed by the entire patch
> > > > > which
> > > > > 1) makes the
> > > > > task periodic, 2) changes the actions of the task to look for
> > > > > whether the cage has something in it and whether its been changed
> > > > > and needs to be configured again.
> > > > >
> > > > >
> > > > > > > > > we can separate this patch as least into two patches:
> > > > > > > >
> > > > > > > >
> > > > > > > > >
> > > > > > > > > 1st, change the thread handle
> > > > 'ixgbe_dev_setup_link_thread_handler'
> > > > > > > > > from
> > > > > > > > >
> > > > > > > > > run-once to as periodical, to handle the original issue.
> > > > > > > > >
> > > > > > > > > The name 'ixgbe_dev_setup_link_thread_handler' may be not
> > > > > > > > > suitable now, as it is a service thread.
> > > > > > > > >
> > > > > > > > > We can change it to "'ixgbe_link_service_thread_handler'"
> > > > > > > > > to reflect the change purpose.
> > > > > > > > >
> > > > > > > > > 2nd, add the SFP hotplug in this patch.
> > > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > >  };
> > > > > > > > > >
> > > > > > > > > > --
> > > > > > > > > > 2.25.1
Qi Zhang May 12, 2022, 1:26 a.m. UTC | #20 
> -----Original Message-----
> From: Jeff Daly <jeffd@silicom-usa.com>
> Sent: Wednesday, April 13, 2022 1:42 AM
> To: dev@dpdk.org
> Cc: stable@dpdk.org; Stephen Douthit <stephend@silicom-usa.com>; Wang,
> Haiyue <haiyue.wang@intel.com>
> Subject: [PATCH v6 2/2] net/ixgbe: Fix SFP detection and linking on hotplug
> 
> 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.

Does kernel driver has the same issue for this?

> 
> 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.

If kernel driver don't have this issue, Is this the same way that kernel driver handle this issue?

Btw, could you break down the patch for easy review?

Thanks
Qi
Jeff Daly May 25, 2022, 4:55 p.m. UTC | #21 
> -----Original Message-----
> From: Zhang, Qi Z <qi.z.zhang@intel.com>
> Sent: Wednesday, May 11, 2022 9:27 PM
> To: Jeff Daly <jeffd@silicom-usa.com>; dev@dpdk.org
> Cc: stable@dpdk.org; Stephen Douthit <stephend@silicom-usa.com>
> Subject: RE: [PATCH v6 2/2] net/ixgbe: Fix SFP detection and linking on
> hotplug
> 
> 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: Wednesday, April 13, 2022 1:42 AM
> > To: dev@dpdk.org
> > Cc: stable@dpdk.org; Stephen Douthit <stephend@silicom-usa.com>;
> Wang,
> > Haiyue <haiyue.wang@intel.com>
> > Subject: [PATCH v6 2/2] net/ixgbe: Fix SFP detection and linking on
> > hotplug
> >
> > 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.
> 
> Does kernel driver has the same issue for this?
> 

No, the kernel driver does the correct thing.

> >
> > 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.
> 
> If kernel driver don't have this issue, Is this the same way that kernel driver
> handle this issue?
>

The history can probably be searched back for, but yes the mechanism
implemented in this patch tries to follow the kernel's method for handling
hotplug.  The patch has been split into 3 chunks, starting with:

https://patchwork.dpdk.org/project/dpdk/patch/ffbdaf3aa487241a51bb6512bbb4701da17e69fe.1652988826.git.jeffd@silicom-usa.com/

it was refactored according to the wishes of the prior maintainer, so it's not
exactly the same as this one which is why it was submitted as a new one, 
not a revision of this.  

HTH.
 
> Btw, could you break down the patch for easy review?
> 
> Thanks
> Qi
diff mbox series

Patch

diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
index f31bbb7895..9e720eee47 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -236,9 +236,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,
@@ -775,6 +772,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.
  */
@@ -1064,6 +1088,306 @@  ixgbe_parse_devargs(struct ixgbe_adapter *adapter,
 	rte_kvargs_free(kvlist);
 }
 
+/**
+ * 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.
@@ -1071,7 +1395,6 @@  ixgbe_parse_devargs(struct ixgbe_adapter *adapter,
 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 =
@@ -1086,6 +1409,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;
@@ -1126,7 +1450,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);
 	ixgbe_parse_devargs(eth_dev->data->dev_private,
 			    pci_dev->device.devargs);
 	rte_eth_copy_pci_info(eth_dev, pci_dev);
@@ -1158,6 +1481,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);
 
@@ -1581,7 +1915,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 =
@@ -1624,7 +1957,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);
 
@@ -2426,6 +2758,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();
@@ -2444,6 +2778,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.
@@ -2463,13 +2801,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);
 	}
 }
 
@@ -2592,8 +2941,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);
@@ -2614,9 +2966,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);
 
@@ -2732,12 +3081,6 @@  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)
-			goto error;
-	}
-
 	if (hw->mac.ops.get_media_type(hw) == ixgbe_media_type_copper) {
 		/* Turn on the copper */
 		ixgbe_set_phy_power(hw, true);
@@ -2849,6 +3192,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() "
@@ -2859,12 +3216,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);
@@ -2894,13 +3245,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);
@@ -2979,7 +3338,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;
@@ -3010,7 +3368,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;
@@ -4163,57 +4520,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.
@@ -4256,8 +4562,6 @@  ixgbe_dev_link_update_share(struct rte_eth_dev *dev,
 	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;
@@ -4272,9 +4576,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;
@@ -4289,7 +4590,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);
@@ -4302,32 +4603,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;
 
@@ -4533,8 +4808,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;
@@ -4550,6 +4823,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;
 }
 
@@ -4601,11 +4882,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);
 
@@ -4640,16 +4923,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;
 }
@@ -4672,8 +4953,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 =
@@ -4703,13 +4982,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);
 }
 
 /**
@@ -5351,9 +5627,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);
 
 	/**
@@ -5433,12 +5706,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;
@@ -5457,8 +5724,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 cc6049a66a..d8d0bebd04 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 {
@@ -510,7 +509,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 {
@@ -673,6 +672,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.
  */