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

["Wang, Haiyue" <haiyue.wang@intel.com>]

> -----Original Message-----
> From: Jeff Daly <jeffd@silicom-usa.com>
> Sent: Wednesday, April 13, 2022 23:32
> To: Wang, Haiyue <haiyue.wang@intel.com>; dev@dpdk.org
> Cc: stable@dpdk.org; Xiaolong Ye <xiaolong.ye@intel.com>; Xiao Zhang <xiao.zhang@intel.com>; Zhao1,
> Wei <wei.zhao1@intel.com>; Lunyuan Cui <lunyuanx.cui@intel.com>
> Subject: RE: [PATCH v6 1/2] net/ixgbe: Limit SDP3 check of TX_DISABLE to appropriate devices
> 
> 
> 
> > -----Original Message-----
> > From: Wang, Haiyue <haiyue.wang@intel.com>
> > Sent: Tuesday, April 12, 2022 9:22 PM
> > To: Jeff Daly <jeffd@silicom-usa.com>; dev@dpdk.org
> > Cc: stable@dpdk.org; Xiaolong Ye <xiaolong.ye@intel.com>; Xiao Zhang
> > <xiao.zhang@intel.com>; Zhao1, Wei <wei.zhao1@intel.com>; Lunyuan Cui
> > <lunyuanx.cui@intel.com>
> > Subject: RE: [PATCH v6 1/2] 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.
> >
> >
> > > -----Original Message-----
> > > From: Jeff Daly <jeffd@silicom-usa.com>
> > > Sent: Wednesday, April 13, 2022 01:42
> > > To: dev@dpdk.org
> > > Cc: stable@dpdk.org; Wang, Haiyue <haiyue.wang@intel.com>; Xiaolong Ye
> > > <xiaolong.ye@intel.com>; Xiao Zhang <xiao.zhang@intel.com>; Zhao1, Wei
> > > <wei.zhao1@intel.com>; Lunyuan Cui <lunyuanx.cui@intel.com>
> > > Subject: [PATCH v6 1/2] 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
> >
> > This is new for soc for BIG change, not cc to stable.
> >
> > >
> > > Signed-off-by: Jeff Daly <jeffd@silicom-usa.com>
> > > ---
> > >  drivers/net/ixgbe/ixgbe_ethdev.c | 39
> > > +++++++++++++++++++++++++++++++-  drivers/net/ixgbe/ixgbe_ethdev.h |
> > > 3 +++
> > >  2 files changed, 41 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> > > b/drivers/net/ixgbe/ixgbe_ethdev.c
> > > index 2da3f67bbc..f31bbb7895 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"

> > > +
> >
> > 'platform' may be a good arg for the soc related change.
> >
> > dpdk-testpmd -a af:10.0,platform=xxx - -i
> >
> > enum ixgbe_platform_type {
> >         ixgbe_platform_unknown = 0,
> >         ixgbe_platform_soc_atom, ??? You can specify it.
> >
> >
> >
> > enum ixgbe_media_type ixgbe_get_platform_type(xxx) {
> >         return xxx;
> > }
> >
> 
> This patchset is not explicitly for SoC platform support.  *Any* implementation may or may not use
> SDP3 as TX_DISABLE.   The previous version of the patch added a check for the mac being an 82599 that
> uses fiber SFP rather than just a fiber SFP.  Our platform specifically can be fiber SFP, but
> TX_DISABLE
> is not SDP3.  However, our platform may not be the only implementation that doesn't use SDP3 this way.
> It does seem that *most* implementations out there do use SDP3 this way, so our platform would be
> the setting this option to 1 to skip this check while any others would work the same as before.
> 

OK, the reason looks good to me.

Please also update the doc:
https://doc.dpdk.org/guides/nics/ixgbe.html

And one more session before "VF Runtime Options", to describe the devarg.

> > >
> > > +     /* Used for limiting SDP3 TX_DISABLE checks */

This comment can be enhanced as your commit message said:


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