RE: [PATCH v2] net/ixgbe: Treat 1G Cu SFPs as 1G SX on the X550 devices

["Zhang, Qi Z" <qi.z.zhang@intel.com>]

> -----Original Message-----
> From: Jeff Daly <jeffd@silicom-usa.com>
> Sent: Tuesday, May 31, 2022 8:31 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; dev@dpdk.org
> Cc: Stephen Douthit <stephend@silicom-usa.com>; Yang, Qiming
> <qiming.yang@intel.com>; Wu, Wenjun1 <wenjun1.wu@intel.com>
> Subject: RE: [PATCH v2] net/ixgbe: Treat 1G Cu SFPs as 1G SX on the X550
> devices
> 
> 
> 
> > -----Original Message-----
> > From: Zhang, Qi Z <qi.z.zhang@intel.com>
> > Sent: Monday, May 30, 2022 9:51 AM
> > To: Jeff Daly <jeffd@silicom-usa.com>; dev@dpdk.org
> > Cc: Stephen Douthit <stephend@silicom-usa.com>; Yang, Qiming
> > <qiming.yang@intel.com>; Wu, Wenjun1 <wenjun1.wu@intel.com>
> > Subject: RE: [PATCH v2] net/ixgbe: Treat 1G Cu SFPs as 1G SX on the
> > X550 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: Monday, May 30, 2022 9:33 PM
> > > To: Zhang, Qi Z <qi.z.zhang@intel.com>; dev@dpdk.org
> > > Cc: Stephen Douthit <stephend@silicom-usa.com>; Yang, Qiming
> > > <qiming.yang@intel.com>; Wu, Wenjun1 <wenjun1.wu@intel.com>
> > > Subject: RE: [PATCH v2] net/ixgbe: Treat 1G Cu SFPs as 1G SX on the
> > > X550 devices
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Zhang, Qi Z <qi.z.zhang@intel.com>
> > > > Sent: Sunday, May 29, 2022 6:49 PM
> > > > To: Jeff Daly <jeffd@silicom-usa.com>; dev@dpdk.org
> > > > Cc: Stephen Douthit <stephend@silicom-usa.com>; Yang, Qiming
> > > > <qiming.yang@intel.com>; Wu, Wenjun1 <wenjun1.wu@intel.com>
> > > > Subject: RE: [PATCH v2] net/ixgbe: Treat 1G Cu SFPs as 1G SX on
> > > > the
> > > > X550 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: Friday, May 27, 2022 4:44 AM
> > > > > To: dev@dpdk.org
> > > > > Cc: Stephen Douthit <stephend@silicom-usa.com>; Yang, Qiming
> > > > > <qiming.yang@intel.com>; Wu, Wenjun1 <wenjun1.wu@intel.com>
> > > > > Subject: [PATCH v2] net/ixgbe: Treat 1G Cu SFPs as 1G SX on the
> > > > > X550 devices
> > > > >
> > > > > 1G Cu SFPs are not officially supported on the X552/X553 family
> > > > > of devices but create an option cu_sfp_as_sx to treat them as 1G
> > > > > SX modules since they usually work.  Print a warning though
> > > > > since support isn't validated, similar to what already happens
> > > > > for other unofficially supported SFPs enabled via the
> > > > > allow_unsupported_sfps
> > > > parameter inherited from the mainline Linux driver.
> > > > >
> > > > > Signed-off-by: Jeff Daly <jeffd@silicom-usa.com>
> > > > > Suggested-by: Stephen Douthit <stephend@silicom-usa.com>
> > > > > ---
> > > > > v2:
> > > > > * Introduced cu_sfp_as_sx option, default off.
> > > > > ---
> > > > >  doc/guides/nics/ixgbe.rst           | 16 ++++++++++++++
> > > > >  drivers/net/ixgbe/base/ixgbe_type.h |  1 +
> > > > > drivers/net/ixgbe/base/ixgbe_x550.c | 12 ++++++++++-
> > > > >  drivers/net/ixgbe/ixgbe_ethdev.c    | 33
> > > > +++++++++++++++++++++++++++++
> > > > >  drivers/net/ixgbe/ixgbe_ethdev.h    |  3 +++
> > > > >  5 files changed, 64 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/doc/guides/nics/ixgbe.rst
> > > > > b/doc/guides/nics/ixgbe.rst index
> > > > > 82fa453fa28e..5db63083eef8 100644
> > > > > --- a/doc/guides/nics/ixgbe.rst
> > > > > +++ b/doc/guides/nics/ixgbe.rst
> > > > > @@ -101,6 +101,22 @@ To guarantee the constraint, capabilities
> > > > > in dev_conf.rxmode.offloads will be ch
> > > > >
> > > > >  fdir_conf->mode will also be checked.
> > > > >
> > > > > +Runtime Options
> > > > > +^^^^^^^^^^^^^^^^^^
> > > > > +
> > > > > +The following ``devargs`` options can be enabled at runtime.
> > > > > +They must be passed as part of EAL arguments. For example,
> > > > > +
> > > > > +.. code-block:: console
> > > > > +
> > > > > +   dpdk-testpmd -a af:10.0,cu_sfp_as_sx=1 -- -i
> > > > > +
> > > > > +- ``cu_sfp_as_sx`` (default **0**)
> > > >
> > > > Can we make this devargs more generic e.g.: "allow_unsupported_phy"
> > > > So we don't need to add a devarg for similar requirement case by
> > > > case in future, of cause we still need to well explain all the
> > > > unsupported cases in the document.
> > > >
> > > >
> > >
> > > this patch is specifically to change the driver's recognition of Cu
> > > transceivers and treat them as optical transceivers.  so should we
> > > consider this an unsupported phy and use that same switch
> > > 'allow_unsupported_phy' or are you looking for a more generic name
> > > than 'cu_sfp_as_sx'?  if you are looking for a more generic name vs
> > > just reusing allow_unsupported_phy, then please pick something and
> > > I'll submit a new patch, but I don't want to guess what would be ok
> > > by
> > submitting patches.
> > >
> >
> > I'm not sure if there will be a situation we need to enable a
> > unsupported phys  in a different way, But as kernel driver take
> > allow_unsupported_spf as module_param, so I will prefer we keep the same
> in DPDK.
> >
> 
> edit: we should have been saying 'allow_unsupported_sfp' all along.  that's
> the kernel option. hopefully that didn't confuse anyone reading this thread......
> 
> so you realize that DPDK by default *always* sets allow_unsupported_sfp. 

No I didn't realize this

But now I realize allow_unsupported_sfp is hardcoded as 1.

The original implementation is wrapped by a compile option.

+#ifdef RTE_LIBRTE_IXGBE_ALLOW_UNSUPPORTED_SFP
+       hw->allow_unsupported_sfp = 1;
+#endif

I don't know why we decide to keep this feature already be on when we remove the #ifdef.  
It could be implemented as a devarg just like kernel driver does.

So can we take this chance to sync with kernel driver?  and the case of treating 1G Cu as 1G SX could be covered into this config.

> are
> you now suggesting that this patch functionality falls under the same option?
> meaning it will *always* be treating 1G Cu as 1G SX?