RE: [PATCH 1/3] ixgbe: make link update thread periodic

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

> -----Original Message-----
> From: Zhang, Qi Z <qi.z.zhang@intel.com>
> Sent: Monday, May 30, 2022 10:21 AM
> To: Jeff Daly <jeffd@silicom-usa.com>; dev@dpdk.org; Yang, Qiming
> <qiming.yang@intel.com>; Wu, Wenjun1 <wenjun1.wu@intel.com>
> Cc: Stephen Douthit <stephend@silicom-usa.com>
> Subject: RE: [PATCH 1/3] ixgbe: make link update thread periodic
> 
> 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:47 PM
> > To: Zhang, Qi Z <qi.z.zhang@intel.com>; dev@dpdk.org; Yang, Qiming
> > <qiming.yang@intel.com>; Wu, Wenjun1 <wenjun1.wu@intel.com>
> > Cc: Stephen Douthit <stephend@silicom-usa.com>
> > Subject: RE: [PATCH 1/3] ixgbe: make link update thread periodic
> >
> >
> >
> > > -----Original Message-----
> > > From: Zhang, Qi Z <qi.z.zhang@intel.com>
> > > Sent: Sunday, May 29, 2022 7:25 PM
> > > To: Jeff Daly <jeffd@silicom-usa.com>; dev@dpdk.org; Yang, Qiming
> > > <qiming.yang@intel.com>; Wu, Wenjun1 <wenjun1.wu@intel.com>
> > > Cc: Stephen Douthit <stephend@silicom-usa.com>
> > > Subject: RE: [PATCH 1/3] ixgbe: make link update thread periodic
> > >
> > > 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 20, 2022 2:03 AM
> > > > To: dev@dpdk.org; Yang, Qiming <qiming.yang@intel.com>; Wu,
> > > > Wenjun1 <wenjun1.wu@intel.com>
> > > > Cc: Stephen Douthit <stephend@silicom-usa.com>
> > > > Subject: [PATCH 1/3] ixgbe: make link update thread periodic
> > > >
> > > > Rather than run-to-completion, allow the link update thread to be
> > > periodic.
> > > > This will set the stage for properly handling hot-plugging.
> > >
> > > Could you explain more about what's the hot-plugging issue with
> > > run-to- completion you try to fix?
> > >
> >
> > it doesn't work right when you have SFPs.  (at least not on our
> > platform or on an
> > 82599 dual SFP add-in card we have).  run-to-completion only works 1x.
> > if you remove and plug in a different SFP it doesn't work.  This patch
> > series should have been taking in context with the original SFP
> > hotplug patch but apparently since I can't ever seem to get the patch
> > submission threading to do what I mean perhaps some context has gone
> > missing.  the SFP hotplug fix has been in the queue since Dec 2021,
> > has been reworked several times, has gone through a change in Intel
> > maintainership.  this patch series makes the SFP hot plugging work like the
> Intel kernel driver does.
> >
> > > >
> > > > Signed-off-by: Jeff Daly <jeffd@silicom-usa.com>
> > > > Inspired-by: Stephen Douthit <stephend@silicom-usa.com>
> > > > ---
> > > >  drivers/net/ixgbe/base/ixgbe_common.c |   4 +-
> > > >  drivers/net/ixgbe/ixgbe_ethdev.c      | 180 ++++++++++----------------
> > > >  2 files changed, 71 insertions(+), 113 deletions(-)
> > > >
> > > > diff --git a/drivers/net/ixgbe/base/ixgbe_common.c
> > > > b/drivers/net/ixgbe/base/ixgbe_common.c
> > > > index aa843bd5c4a5..712062306491 100644
> > > > --- a/drivers/net/ixgbe/base/ixgbe_common.c
> > > > +++ b/drivers/net/ixgbe/base/ixgbe_common.c
> > > > @@ -4154,8 +4154,8 @@ s32 ixgbe_check_mac_link_generic(struct
> > > > ixgbe_hw *hw, ixgbe_link_speed *speed,
> > > >                       break;
> > > >               case ixgbe_mac_X550EM_x:
> > > >               case ixgbe_mac_X550EM_a:
> > > > -                     sfp_cage_full = IXGBE_READ_REG(hw, IXGBE_ESDP)
> > > > &
> > > > -                                     IXGBE_ESDP_SDP0;
> > > > +                     sfp_cage_full = !(IXGBE_READ_REG(hw,
> > > > + IXGBE_ESDP)
> > > > &
> > > > +                                       IXGBE_ESDP_SDP0);
> > > >                       break;
> > > >               default:
> > > >                       /* sanity check - No SFP+ devices here */
> > > > diff --git
> > >
> > > Looks like you change the behavior of link status check for x550.
> > > I'm not an ixgbe expert, but I know this is not kernel driver's
> > > implementation.
> > >
> >
> > sigh.  this was supposed to be part of a different patch which also
> > had some question about functionality.  the SDP0 bit check doesn't
> > specifically need to be a check for a '1', since the bit reflects the state of
> the pin on the platform.
> > Intel's platform implementations have an inverter on the board to
> > switch the state.
> > MOD_ABS from an SFP will be '0' when an SFP is plugged in.  with an
> > inverter in the platform the signal will be '1' when an SFP is plugged
> > in.  there's no guidance from Intel's platform design guide that an
> > inverter needs to be between the SFP and the NIC SDP pin so having it
> > only follow Intel's platform implementations is hard to justify.
> 
> OK, I assume the existing code should be proved for the normal scenario
> (remove and plug in with the same SFP) So how can we guarantee this
> change will not break something?
> 
> Could you help me to understand why we should ignore the difference when
> SDP0 is 1 in normal scenario?
> 

'normal scenario'?  meaning on any Intel-implemented platform?  

the code needs a devargs-like option so that platforms that don't implement
the inverter can still use this code in the expected way.

> Before change
> we will continue to read the link register and return the link speed.
> 
> after change
> we return SPEED_UNKNOWN immediately .
> 
> 
> 
> 
> >
> > > So do you think this is a fix for both DPDK and kernel driver?  if
> > > it is, please move this change into a  separate patch and we need to
> > > reach the right expert to approve this.
> > >
> > >
> >
> > no, as explained above.