[PATCH] net/ixgbe: Limit SDP3 check of TX_DISABLE to appropriate devices

[PATCH] net/ixgbe: Limit SDP3 check of TX_DISABLE to appropriate devices

[Jeff Daly]

1ca05831b9b added a check that SDP3 (used as a TX_DISABLE output to the
SFP cage on these cards) is not asserted to avoid incorrectly reporting
link up when the SFP's laser is turned off.

ff8162cb957 limited this workaround to fiber ports

This patch:
* Adds devarg 'fiber_sdp3_no_tx_disable' not all fiber ixgbe devs use
  SDP3 as TX_DISABLE

Fixes: 1ca05831b9b ("net/ixgbe: fix link status")
Fixes: ff8162cb957 ("net/ixgbe: fix link status")
Cc: stable@dpdk.org

Signed-off-by: Jeff Daly <jeffd@silicom-usa.com>
---
 doc/guides/nics/ixgbe.rst        | 17 ++++++++++++++
 drivers/net/ixgbe/ixgbe_ethdev.c | 39 +++++++++++++++++++++++++++++++-
 drivers/net/ixgbe/ixgbe_ethdev.h |  3 +++
 3 files changed, 58 insertions(+), 1 deletion(-)

diff --git a/doc/guides/nics/ixgbe.rst b/doc/guides/nics/ixgbe.rst
index 82fa453fa28e..ad1a3da6101e 100644
--- a/doc/guides/nics/ixgbe.rst
+++ b/doc/guides/nics/ixgbe.rst
@@ -101,6 +101,23 @@ To guarantee the constraint, capabilities in dev_conf.rxmode.offloads will be ch
 
 fdir_conf->mode will also be checked.
 
+Disable SDP3 TX_DISABLE for Fiber Links
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+The following ``devargs`` option can be enabled at runtime.  It must
+be passed as part of EAL arguments. For example,
+
+.. code-block:: console
+
+   dpdk-testpmd -a fiber_sdp3_no_tx_disable=1 -- -i
+
+- ``fiber_sdp3_no_tx_disable`` (default **0**)
+
+  Not all IXGBE implementations with SFP cages use the SDP3 signal as
+  TX_DISABLE as a means to disable the laser on fiber SFP modules.
+  This option informs the driver that in this case, SDP3 is not to be
+  used as a check for link up by testing for laser on/off.
+
 VF Runtime Options
 ^^^^^^^^^^^^^^^^^^
 
diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
index 2da3f67bbc78..f31bbb78956d 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -128,6 +128,13 @@
 #define IXGBE_EXVET_VET_EXT_SHIFT              16
 #define IXGBE_DMATXCTL_VT_MASK                 0xFFFF0000
 
+#define IXGBE_DEVARG_FIBER_SDP3_NOT_TX_DISABLE	"fiber_sdp3_no_tx_disable"
+
+static const char * const ixgbe_valid_arguments[] = {
+	IXGBE_DEVARG_FIBER_SDP3_NOT_TX_DISABLE,
+	NULL
+};
+
 #define IXGBEVF_DEVARG_PFLINK_FULLCHK		"pflink_fullchk"
 
 static const char * const ixgbevf_valid_arguments[] = {
@@ -348,6 +355,8 @@ static int ixgbe_dev_udp_tunnel_port_del(struct rte_eth_dev *dev,
 static int ixgbe_filter_restore(struct rte_eth_dev *dev);
 static void ixgbe_l2_tunnel_conf(struct rte_eth_dev *dev);
 static int ixgbe_wait_for_link_up(struct ixgbe_hw *hw);
+static int devarg_handle_int(__rte_unused const char *key, const char *value,
+			     void *extra_args);
 
 /*
  * Define VF Stats MACRO for Non "cleared on read" register
@@ -1032,6 +1041,29 @@ ixgbe_swfw_lock_reset(struct ixgbe_hw *hw)
 	ixgbe_release_swfw_semaphore(hw, mask);
 }
 
+static void
+ixgbe_parse_devargs(struct ixgbe_adapter *adapter,
+		      struct rte_devargs *devargs)
+{
+	struct rte_kvargs *kvlist;
+	uint16_t sdp3_no_tx_disable;
+
+	if (devargs == NULL)
+		return;
+
+	kvlist = rte_kvargs_parse(devargs->args, ixgbe_valid_arguments);
+	if (kvlist == NULL)
+		return;
+
+	if (rte_kvargs_count(kvlist, IXGBE_DEVARG_FIBER_SDP3_NOT_TX_DISABLE) == 1 &&
+	    rte_kvargs_process(kvlist, IXGBE_DEVARG_FIBER_SDP3_NOT_TX_DISABLE,
+			       devarg_handle_int, &sdp3_no_tx_disable) == 0 &&
+	    sdp3_no_tx_disable == 1)
+		adapter->sdp3_no_tx_disable = 1;
+
+	rte_kvargs_free(kvlist);
+}
+
 /*
  * This function is based on code in ixgbe_attach() in base/ixgbe.c.
  * It returns 0 on success.
@@ -1095,6 +1127,8 @@ eth_ixgbe_dev_init(struct rte_eth_dev *eth_dev, void *init_params __rte_unused)
 	}
 
 	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);
 	eth_dev->data->dev_flags |= RTE_ETH_DEV_AUTOFILL_QUEUE_XSTATS;
 
@@ -4261,7 +4295,8 @@ ixgbe_dev_link_update_share(struct rte_eth_dev *dev,
 		return rte_eth_linkstatus_set(dev, &link);
 	}
 
-	if (ixgbe_get_media_type(hw) == ixgbe_media_type_fiber) {
+	if (ixgbe_get_media_type(hw) == ixgbe_media_type_fiber &&
+	    !ad->sdp3_no_tx_disable) {
 		esdp_reg = IXGBE_READ_REG(hw, IXGBE_ESDP);
 		if ((esdp_reg & IXGBE_ESDP_SDP3))
 			link_up = 0;
@@ -8250,6 +8285,8 @@ ixgbe_dev_macsec_register_disable(struct rte_eth_dev *dev)
 RTE_PMD_REGISTER_PCI(net_ixgbe, rte_ixgbe_pmd);
 RTE_PMD_REGISTER_PCI_TABLE(net_ixgbe, pci_id_ixgbe_map);
 RTE_PMD_REGISTER_KMOD_DEP(net_ixgbe, "* igb_uio | uio_pci_generic | vfio-pci");
+RTE_PMD_REGISTER_PARAM_STRING(net_ixgbe,
+			      IXGBE_DEVARG_FIBER_SDP3_NOT_TX_DISABLE "=<0|1>");
 RTE_PMD_REGISTER_PCI(net_ixgbe_vf, rte_ixgbevf_pmd);
 RTE_PMD_REGISTER_PCI_TABLE(net_ixgbe_vf, pci_id_ixgbevf_map);
 RTE_PMD_REGISTER_KMOD_DEP(net_ixgbe_vf, "* igb_uio | vfio-pci");
diff --git a/drivers/net/ixgbe/ixgbe_ethdev.h b/drivers/net/ixgbe/ixgbe_ethdev.h
index 69e0e82a5b1a..cc6049a66ad1 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.h
+++ b/drivers/net/ixgbe/ixgbe_ethdev.h
@@ -501,6 +501,9 @@ struct ixgbe_adapter {
 	/* For RSS reta table update */
 	uint8_t rss_reta_updated;
 
+	/* Used for limiting SDP3 TX_DISABLE checks */
+	uint8_t sdp3_no_tx_disable;
+
 	/* Used for VF link sync with PF's physical and logical (by checking
 	 * mailbox status) link status.
 	 */
-- 
2.25.1

RE: [PATCH] net/ixgbe: Limit SDP3 check of TX_DISABLE to appropriate devices

[Zhang, Qi Z]

> -----Original Message-----
> From: Jeff Daly <jeffd@silicom-usa.com>
> Sent: Wednesday, May 11, 2022 2:57 AM
> To: Yang, Qiming <qiming.yang@intel.com>; Wu, Wenjun1
> <wenjun1.wu@intel.com>; Zhao1, Wei <wei.zhao1@intel.com>; Xiao Zhang
> <xiao.zhang@intel.com>; Xiaolong Ye <xiaolong.ye@intel.com>; Lunyuan Cui
> <lunyuanx.cui@intel.com>
> Cc: dev@dpdk.org; stable@dpdk.org
> Subject: [PATCH] net/ixgbe: Limit SDP3 check of TX_DISABLE to appropriate
> devices
> 
> 1ca05831b9b added a check that SDP3 (used as a TX_DISABLE output to the SFP
> cage on these cards) is not asserted to avoid incorrectly reporting link up when
> the SFP's laser is turned off.
> 
> ff8162cb957 limited this workaround to fiber ports
> 
> This patch:
> * Adds devarg 'fiber_sdp3_no_tx_disable' not all fiber ixgbe devs use
>   SDP3 as TX_DISABLE
> 
> Fixes: 1ca05831b9b ("net/ixgbe: fix link status")
> Fixes: ff8162cb957 ("net/ixgbe: fix link status")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Jeff Daly <jeffd@silicom-usa.com>

Acked-by: Qi Zhang <qi.z.zhang@intel.com>

Applied to dpdk-next-net-intel.

Thanks
Qi

Re: [PATCH] net/ixgbe: Limit SDP3 check of TX_DISABLE to appropriate devices

[Thomas Monjalon]

11/05/2022 13:43, Zhang, Qi Z:
> From: Jeff Daly <jeffd@silicom-usa.com>
> > 
> > 1ca05831b9b added a check that SDP3 (used as a TX_DISABLE output to the SFP
> > cage on these cards) is not asserted to avoid incorrectly reporting link up when
> > the SFP's laser is turned off.
> > 
> > ff8162cb957 limited this workaround to fiber ports
> > 
> > This patch:
> > * Adds devarg 'fiber_sdp3_no_tx_disable' not all fiber ixgbe devs use
> >   SDP3 as TX_DISABLE
> > 
> > Fixes: 1ca05831b9b ("net/ixgbe: fix link status")
> > Fixes: ff8162cb957 ("net/ixgbe: fix link status")
> > Cc: stable@dpdk.org
> > 
> > Signed-off-by: Jeff Daly <jeffd@silicom-usa.com>
> 
> Acked-by: Qi Zhang <qi.z.zhang@intel.com>
> 
> Applied to dpdk-next-net-intel.

There is a lack of context in this description.
I don't know what SDP3 and TX_DISABLE refers to.
Please make more complete sentences, thanks.

RE: [PATCH] net/ixgbe: Limit SDP3 check of TX_DISABLE to appropriate devices

[Jeff Daly]

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Wednesday, May 11, 2022 10:51 AM
> To: Jeff Daly <jeffd@silicom-usa.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: Yang, Qiming <qiming.yang@intel.com>; Wu, Wenjun1
> <wenjun1.wu@intel.com>; Zhao1, Wei <wei.zhao1@intel.com>; Xiao Zhang
> <xiao.zhang@intel.com>; Xiaolong Ye <xiaolong.ye@intel.com>; Lunyuan Cui
> <lunyuanx.cui@intel.com>; dev@dpdk.org; stable@dpdk.org
> Subject: Re: [PATCH] net/ixgbe: Limit SDP3 check of TX_DISABLE to
> appropriate devices
> 
> Caution: This is an external email. Please take care when clicking links or
> opening attachments.
> 
> 
> 11/05/2022 13:43, Zhang, Qi Z:
> > From: Jeff Daly <jeffd@silicom-usa.com>
> > >
> > > 1ca05831b9b added a check that SDP3 (used as a TX_DISABLE output to
> > > the SFP cage on these cards) is not asserted to avoid incorrectly
> > > reporting link up when the SFP's laser is turned off.
> > >
> > > ff8162cb957 limited this workaround to fiber ports
> > >
> > > This patch:
> > > * Adds devarg 'fiber_sdp3_no_tx_disable' not all fiber ixgbe devs use
> > >   SDP3 as TX_DISABLE
> > >
> > > Fixes: 1ca05831b9b ("net/ixgbe: fix link status")
> > > Fixes: ff8162cb957 ("net/ixgbe: fix link status")
> > > Cc: stable@dpdk.org
> > >
> > > Signed-off-by: Jeff Daly <jeffd@silicom-usa.com>
> >
> > Acked-by: Qi Zhang <qi.z.zhang@intel.com>
> >
> > Applied to dpdk-next-net-intel.
> 
> There is a lack of context in this description.
> I don't know what SDP3 and TX_DISABLE refers to.
> Please make more complete sentences, thanks.
> 

I don't want to sound obtuse here, but this is a fix to a specific Intel NIC driver.  Any symbols or abbreviations or definitions used in a device driver are almost always in the manual.  While SDP3 means something specific to the Intel 82599 and X550 (and probably others), it probably doesn't appear in a driver from Marvell for example.  So, in this case [S]oftware [D]efined [P]ins [3] (out of 0-3) is specifically talking about the Intel X550.  I'm familiar enough with the hardware to recognize that, but if I was to look at a Marvell driver and saw something I didn't recognize like that, I'd be checking the Marvell manual.  

What I'm describing here is the fact that the TX_DISABLE signal (a signal defined in the SFP spec) from the NIC as implemented by the Software Defined Pin (3) by many (most?) implementations that are using this Intel driver, is not specifically the *only* use of SDP3.  A later patch limited the check (correctly) to fiber implementations (which is the only thing that makes sense), and *this* patch adds a module switch for platforms to disable this check in the event that they (as they are perfectly allowed to) don't use SDP3 as TX_DISABLE.

Re: [PATCH] net/ixgbe: Limit SDP3 check of TX_DISABLE to appropriate devices

[Thomas Monjalon]

12/05/2022 19:01, Jeff Daly:
> From: Thomas Monjalon <thomas@monjalon.net>
> > 11/05/2022 13:43, Zhang, Qi Z:
> > > From: Jeff Daly <jeffd@silicom-usa.com>
> > > >
> > > > 1ca05831b9b added a check that SDP3 (used as a TX_DISABLE output to
> > > > the SFP cage on these cards) is not asserted to avoid incorrectly
> > > > reporting link up when the SFP's laser is turned off.
> > > >
> > > > ff8162cb957 limited this workaround to fiber ports
> > > >
> > > > This patch:
> > > > * Adds devarg 'fiber_sdp3_no_tx_disable' not all fiber ixgbe devs use
> > > >   SDP3 as TX_DISABLE
> > > >
> > > > Fixes: 1ca05831b9b ("net/ixgbe: fix link status")
> > > > Fixes: ff8162cb957 ("net/ixgbe: fix link status")
> > > > Cc: stable@dpdk.org
> > > >
> > > > Signed-off-by: Jeff Daly <jeffd@silicom-usa.com>
> > >
> > > Acked-by: Qi Zhang <qi.z.zhang@intel.com>
> > >
> > > Applied to dpdk-next-net-intel.
> > 
> > There is a lack of context in this description.
> > I don't know what SDP3 and TX_DISABLE refers to.
> > Please make more complete sentences, thanks.
> > 
> 
> I don't want to sound obtuse here, but this is a fix to a specific Intel NIC driver.  Any symbols or abbreviations or definitions used in a device driver are almost always in the manual.  While SDP3 means something specific to the Intel 82599 and X550 (and probably others), it probably doesn't appear in a driver from Marvell for example.  So, in this case [S]oftware [D]efined [P]ins [3] (out of 0-3) is specifically talking about the Intel X550.  I'm familiar enough with the hardware to recognize that, but if I was to look at a Marvell driver and saw something I didn't recognize like that, I'd be checking the Marvell manual.

Of course we can spend hours checking code and manuals.
But if we need to look to a lot of commits,
it is a lot more convenient to have a summary of this information
in the commit log.

> What I'm describing here is the fact that the TX_DISABLE signal (a signal defined in the SFP spec) from the NIC as implemented by the Software Defined Pin (3) by many (most?) implementations that are using this Intel driver, is not specifically the *only* use of SDP3.  A later patch limited the check (correctly) to fiber implementations (which is the only thing that makes sense), and *this* patch adds a module switch for platforms to disable this check in the event that they (as they are perfectly allowed to) don't use SDP3 as TX_DISABLE.

Thank you for the explanation.