> -----Original Message----- > From: Jeff Daly <je...@silicom-usa.com> > Sent: Tuesday, May 31, 2022 8:25 PM > To: Zhang, Qi Z <qi.z.zh...@intel.com>; dev@dpdk.org; Yang, Qiming > <qiming.y...@intel.com>; Wu, Wenjun1 <wenjun1...@intel.com> > Cc: Stephen Douthit <steph...@silicom-usa.com> > Subject: RE: [PATCH 1/3] ixgbe: make link update thread periodic > > > > > -----Original Message----- > > From: Zhang, Qi Z <qi.z.zh...@intel.com> > > Sent: Monday, May 30, 2022 10:21 AM > > To: Jeff Daly <je...@silicom-usa.com>; dev@dpdk.org; Yang, Qiming > > <qiming.y...@intel.com>; Wu, Wenjun1 <wenjun1...@intel.com> > > Cc: Stephen Douthit <steph...@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 <je...@silicom-usa.com> > > > Sent: Monday, May 30, 2022 9:47 PM > > > To: Zhang, Qi Z <qi.z.zh...@intel.com>; dev@dpdk.org; Yang, Qiming > > > <qiming.y...@intel.com>; Wu, Wenjun1 <wenjun1...@intel.com> > > > Cc: Stephen Douthit <steph...@silicom-usa.com> > > > Subject: RE: [PATCH 1/3] ixgbe: make link update thread periodic > > > > > > > > > > > > > -----Original Message----- > > > > From: Zhang, Qi Z <qi.z.zh...@intel.com> > > > > Sent: Sunday, May 29, 2022 7:25 PM > > > > To: Jeff Daly <je...@silicom-usa.com>; dev@dpdk.org; Yang, Qiming > > > > <qiming.y...@intel.com>; Wu, Wenjun1 <wenjun1...@intel.com> > > > > Cc: Stephen Douthit <steph...@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 <je...@silicom-usa.com> > > > > > Sent: Friday, May 20, 2022 2:03 AM > > > > > To: dev@dpdk.org; Yang, Qiming <qiming.y...@intel.com>; Wu, > > > > > Wenjun1 <wenjun1...@intel.com> > > > > > Cc: Stephen Douthit <steph...@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 <je...@silicom-usa.com> > > > > > Inspired-by: Stephen Douthit <steph...@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.
Yes, please move this change into a separate patch and use a devargs for SDP pin invert case. > > > 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.
