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

[Jeff Daly <jeffd@silicom-usa.com>]

> -----Original Message-----
> From: Zhang, Qi Z <qi.z.zhang@intel.com>
> Sent: Monday, May 23, 2022 7:22 PM
> To: Jeff Daly <jeffd@silicom-usa.com>; Thomas Monjalon
> <thomas@monjalon.net>; 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] 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 23, 2022 10:14 PM
> > To: Zhang, Qi Z <qi.z.zhang@intel.com>; Thomas Monjalon
> > <thomas@monjalon.net>; 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] 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 23, 2022 1:37 AM
> > > To: Jeff Daly <jeffd@silicom-usa.com>; Thomas Monjalon
> > > <thomas@monjalon.net>; 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] 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: Saturday, May 21, 2022 2:03 AM
> > > > To: Zhang, Qi Z <qi.z.zhang@intel.com>; Thomas Monjalon
> > > > <thomas@monjalon.net>; 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] 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: Thursday, May 19, 2022 8:15 PM
> > > > > To: Thomas Monjalon <thomas@monjalon.net>; dev@dpdk.org
> > > > > Cc: Stephen Douthit <stephend@silicom-usa.com>; Jeff Daly
> > > > > <jeffd@silicom- usa.com>; Yang, Qiming <qiming.yang@intel.com>;
> > > > > Wu,
> > > > > Wenjun1 <wenjun1.wu@intel.com>
> > > > > Subject: RE: [PATCH] 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: Thomas Monjalon <thomas@monjalon.net>
> > > > > > Sent: Wednesday, April 13, 2022 10:21 PM
> > > > > > To: dev@dpdk.org
> > > > > > Cc: Stephen Douthit <stephend@silicom-usa.com>; Jeff Daly
> > > > > > <jeffd@silicom- usa.com>; Wang, Haiyue
> > > > > > <haiyue.wang@intel.com>; Yang, Qiming <qiming.yang@intel.com>;
> > > > > > Wu, Wenjun1 <wenjun1.wu@intel.com>
> > > > > > Subject: Re: [PATCH] net/ixgbe: Treat 1G Cu SFPs as 1G SX on
> > > > > > the
> > > > > > X550 devices
> > > > > >
> > > > > > Please, could we have a review of this patch?
> > > > > > +Cc new ixgbe maintainers
> > > > > >
> > > > > >
> > > > > > 07/03/2022 23:34, jeffd@silicom-usa.com:
> > > > > > > From: Stephen Douthit <stephend@silicom-usa.com>
> > > > > > >
> > > > > > > 1G Cu SFPs are not officially supported on the X552/X553
> > > > > > > family of devices but 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: Stephen Douthit <stephend@silicom-usa.com>
> > > > > > > Signed-off-by: Jeff Daly <jeffd@silicom-usa.com>
> > > > >
> > > > > I think we need a devargs for this feature with well
> > > > > documentation So, it should not break existing behavior by
> > > > > default, but allow people to take risk if they know what they are
> doing.
> > > > >
> > > >
> > > > there was already a patch submitted to IWL mailing list for this
> > > > feature in the base driver, which was rejected.
> > > > https://patchwork.ozlabs.org/project/intel-wired-
> > > > lan/patch/20220414201329.27714-1-jeffd@silicom-usa.com/
> > >
> > > OK, thanks for sharing this,
> > >
> > > But base on the concern of the previous comment
> > >
> > > " As for 1G Cu SFP treating it as 1G SX, some 1G-Base-T SFP modules
> > > require the use of RX_ILOS and some Intel Ethernet products don't
> > > support
> > that."
> > >
> > > We may have a risk to accept the code as default behavior
> > >
> > > But devargs is allowed in DPDK for device-specific features.
> > >
> >
> > ok, I will submit a revised patch that uses a devargs (or whatever)
> > switch to allow the behavior when selected explicitly.
> >
> > But, can we *please* STOP marking patches as superseded when a
> > follow-up patch
> > hasn't been submitted yet!?    I've marked the patch as 'Changes
> Requested' for
> > now.
> 
> Sure, I should follow, thanks to correct his, but a little bit surprise, why this
> looks like a big deal, it just a shortcut when I expected a new version will
> come then I skip one status change, I think mailing list already have
> everything about the patch status for you.
> 

Maybe I'm not understanding the terms being used then in the mailing list status.
If you expect a new version (that doesn't exist yet) then wouldn't this be more
aptly "Changes Requested" vs. "Superseded".   Superseded implies there's a new
version that exists and this current one no longer applies.  If I just came onto the 
mailing list and read a patch that was marked "Superseded" and looked for the 
new one and didn't find it, I'd be very confused.  If I read a patch that was marked
"Changes Requested", I'd know that this was the last patch sent by the developer
and that there would be a follow-up to this one sometime.

> > When I submit a follow-up I will set this one to superseded
> 
> Actually you did NOT change the below patch to superseded after you send
> a new version (I did this) and you didn't reply my last question yet.
> https://patchwork.dpdk.org/project/dpdk/list/?series=23046
> 
> 

why am I confused here?  The patch you linked above is not related to this patch.
The patch series linked above is an update to the hotplug patches that were 
requested prior.  (I screwed up the initial new series submission I admit, and marked
those as superseded).

*This* patch however I've not submitted a new update for yet.  

And I don't see where you asked a question ?

> >
> > >
> > > >
> > > > > Thanks
> > > > > Qi
> > > > >
> > > > >
> > > > > > > ---
> > > > > > >  drivers/net/ixgbe/base/ixgbe_x550.c | 14 +++++++++++++-
> > > > > > >  1 file changed, 13 insertions(+), 1 deletion(-)
> > > > > > >
> > > > > > > diff --git a/drivers/net/ixgbe/base/ixgbe_x550.c
> > > > > > > b/drivers/net/ixgbe/base/ixgbe_x550.c
> > > > > > > index 8810d1658e..8d1bc6c80d 100644
> > > > > > > --- a/drivers/net/ixgbe/base/ixgbe_x550.c
> > > > > > > +++ b/drivers/net/ixgbe/base/ixgbe_x550.c
> > > > > > > @@ -1538,9 +1538,21 @@ STATIC s32
> > > > > > ixgbe_supported_sfp_modules_X550em(struct ixgbe_hw *hw, bool
> > > > > > *linear)
> > > > > > >     case ixgbe_sfp_type_1g_lha_core1:
> > > > > > >             *linear = false;
> > > > > > >             break;
> > > > > > > -   case ixgbe_sfp_type_unknown:
> > > > > > > +   /* Copper SFPs are not officially supported for x550em
> > > > > > > + devices, but
> > > > > can
> > > > > > > +    * often be made to work at fixed 1G speeds.  Pretend
> > > > > > > + they're
> > > 1g_sx
> > > > > > > +    * modules here to allow g.Fast DSL SFPs to work.
> > > > > > > +    */
> > > > > > >     case ixgbe_sfp_type_1g_cu_core0:
> > > > > > > +           EWARN(hw, "Pretending that unsupported 1g_cu SFP
> > > > > > > + is
> > > > > > 1g_sx\n");
> > > > > > > +           *linear = false;
> > > > > > > +           hw->phy.sfp_type = ixgbe_sfp_type_1g_sx_core0;
> > > > > > > +           break;
> > > > > > >     case ixgbe_sfp_type_1g_cu_core1:
> > > > > > > +           EWARN(hw, "Pretending that unsupported 1g_cu SFP
> > > > > > > + is
> > > > > > 1g_sx\n");
> > > > > > > +           *linear = false;
> > > > > > > +           hw->phy.sfp_type = ixgbe_sfp_type_1g_sx_core1;
> > > > > > > +           break;
> > > > > > > +   case ixgbe_sfp_type_unknown:
> > > > > > >     default:
> > > > > > >             return IXGBE_ERR_SFP_NOT_SUPPORTED;
> > > > > > >     }
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >