RE: [PATCH v6 2/2] net/ixgbe: Fix SFP detection and linking on hotplug

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

> -----Original Message-----
> From: Wang, Haiyue <haiyue.wang@intel.com>
> Sent: Tuesday, April 19, 2022 9:09 PM
> To: Jeff Daly <jeffd@silicom-usa.com>; dev@dpdk.org
> Cc: stable@dpdk.org; Yang, Qiming <qiming.yang@intel.com>
> Subject: RE: [PATCH v6 2/2] net/ixgbe: Fix SFP detection and linking on
> hotplug
> 
> 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 20, 2022 01:34
> > To: Wang, Haiyue <haiyue.wang@intel.com>; dev@dpdk.org
> > Cc: stable@dpdk.org; Yang, Qiming <qiming.yang@intel.com>
> > Subject: RE: [PATCH v6 2/2] net/ixgbe: Fix SFP detection and linking
> > on hotplug
> >
> >
> >
> > > -----Original Message-----
> > > From: Wang, Haiyue <haiyue.wang@intel.com>
> > > Sent: Monday, April 18, 2022 10:05 PM
> > > To: Jeff Daly <jeffd@silicom-usa.com>; dev@dpdk.org
> > > Cc: stable@dpdk.org; Yang, Qiming <qiming.yang@intel.com>
> > > Subject: RE: [PATCH v6 2/2] net/ixgbe: Fix SFP detection and linking
> > > on hotplug
> > >
> > > 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: Tuesday, April 19, 2022 05:55
> > > > To: Wang, Haiyue <haiyue.wang@intel.com>; dev@dpdk.org
> > > > Cc: stable@dpdk.org
> > > > Subject: RE: [PATCH v6 2/2] net/ixgbe: Fix SFP detection and
> > > > linking on hotplug
> > > >
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Wang, Haiyue <haiyue.wang@intel.com>
> > > > > Sent: Thursday, April 14, 2022 8:11 AM
> > > > > To: Jeff Daly <jeffd@silicom-usa.com>; dev@dpdk.org
> > > > > Cc: stable@dpdk.org
> > > > > Subject: RE: [PATCH v6 2/2] net/ixgbe: Fix SFP detection and
> > > > > linking on hotplug
> > > > >
> > > > > 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: Thursday, April 14, 2022 18:41
> > > > > > To: Wang, Haiyue <haiyue.wang@intel.com>; dev@dpdk.org
> > > > > > Cc: stable@dpdk.org
> > > > > > Subject: RE: [PATCH v6 2/2] net/ixgbe: Fix SFP detection and
> > > > > > linking on hotplug
> > > > > >
> > > > > >
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Wang, Haiyue <haiyue.wang@intel.com>
> > > > > > > Sent: Wednesday, April 13, 2022 11:00 PM
> > > > > > > To: Jeff Daly <jeffd@silicom-usa.com>; dev@dpdk.org
> > > > > > > Cc: stable@dpdk.org; Stephen Douthit
> > > > > > > <stephend@silicom-usa.com>
> > > > > > > Subject: RE: [PATCH v6 2/2] net/ixgbe: Fix SFP detection and
> > > > > > > linking on hotplug
> > > > > > >
> > > > > > > Caution: This is an external email. Please take care when
> > > > > > > clicking links or opening attachments.
> > > > > > >
> > > > > > >
> > > > > > > > -----Original Message-----
> > > > > > > > From: Wang, Haiyue
> > > > > > > > Sent: Thursday, April 14, 2022 10:49
> > > > > > > > To: Jeff Daly <jeffd@silicom-usa.com>; dev@dpdk.org
> > > > > > > > Cc: stable@dpdk.org; Stephen Douthit
> > > > > > > > <stephend@silicom-usa.com>
> > > > > > > > Subject: RE: [PATCH v6 2/2] net/ixgbe: Fix SFP detection
> > > > > > > > and linking on hotplug
> > > > > > > >
> > > > > > > > > -----Original Message-----
> > > > > > > > > From: Jeff Daly <jeffd@silicom-usa.com>
> > > > > > > > > Sent: Wednesday, April 13, 2022 01:42
> > > > > > > > > To: dev@dpdk.org
> > > > > > > > > Cc: stable@dpdk.org; Stephen Douthit
> > > > > > > > > <stephend@silicom-usa.com>; Wang, Haiyue
> > > > > > > > > <haiyue.wang@intel.com>
> > > > > > > > > Subject: [PATCH v6 2/2] net/ixgbe: Fix SFP detection and
> > > > > > > > > linking on hotplug
> > > > > > > > >
> > > > > > > > > Currently the ixgbe driver does not ID any SFP except
> > > > > > > > > for the first one plugged in. This can lead to no-link,
> > > > > > > > > or incorrect speed
> > > > > conditions.
> > > > > > > > >
> > > > > > > > > For example:
> > > > > > > > >
> > > > > > > > > * If link is initially established with a 1G SFP, and
> > > > > > > > > later a 1G/10G multispeed part is later installed, then
> > > > > > > > > the MAC link setup functions are never called to change
> > > > > > > > > from 1000BASE-X to 10GBASE-R mode, and the link stays
> > > > > > > > > running at the
> > > slower rate.
> > > > > > > > >
> > > > > > > > > * If link is initially established with a 1G SFP, and
> > > > > > > > > later a 10G only module is later installed, no link is
> > > > > > > > > established, since we are still trasnsmitting in
> > > > > > > > > 1000BASE-X mode to a 10GBASE-R
> > > > > only partner.
> > > > > > > > >
> > > > > > > > > Refactor the SFP ID/setup, and link setup code, to more
> > > > > > > > > closely match the flow of the mainline kernel driver
> > > > > > > > > which does not have these issues.  In that driver a
> > > > > > > > > service task runs periodically to handle these
> > > > > > > > > operations based on bit flags that have been set
> > > > > > > > > (usually via interrupt or userspace request), and then
> > > > > > > > > get cleared once the requested subtask has
> > > been completed.
> > > > > > > > >
> > > > > > > > > Fixes: af75078fece ("first public release")
> > > > > > > > > Cc: stable@dpdk.org
> > > > > > > > >
> > > > > > > > > Signed-off-by: Stephen Douthit
> > > > > > > > > <stephend@silicom-usa.com>
> > > > > > > > > Signed-off-by: Jeff Daly <jeffd@silicom-usa.com>
> > > > > > > > > ---
> > > > > > > > >  drivers/net/ixgbe/ixgbe_ethdev.c | 533
> > > > > > > > > +++++++++++++++++++++++--------
> > > > > > > > > +++++++++++++++++++++++drivers/net/ixgbe/ixgbe_ethdev.h
> > > > > > > > > +++++++++++++++++++++++|
> > > > > > > > > 14 +-
> > > > > > > > >  2 files changed, 410 insertions(+), 137 deletions(-)
> > > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > >
> > > > > > > > >  struct ixgbe_stat_mapping_registers { @@ -510,7 +509,7
> > > > > > > > > @@ struct ixgbe_adapter {
> > > > > > > > >     uint8_t pflink_fullchk;
> > > > > > > > >     uint8_t mac_ctrl_frame_fwd;
> > > > > > > > >     rte_atomic32_t link_thread_running;
> > > > > > > > > -   pthread_t link_thread_tid;
> > > > > > > > > +   pthread_t service_thread_tid;
> > > > > > > >
> > > > > > > > No need to rename this variable,
> > > > > > >
> > > > > > > Let's do link related service now, so we can keep it, I
> > > > > > > missed to add my comment. ;-)
> > > > > > >
> > > > > >
> > > > > > I don't understand this reply, are you still asking to rework
> > > > > > the patch or
> > > > > not?
> > > > > >
> > > > >
> > > > > Different thing.
> > > > >
> > > > > 1. This var can be kept to trace the created thread. (change
> > > > > less code to
> > > keep
> > > > >    the patch clean.)
> > > > > 2. Yes, two patches.
> > > > >
> > > >
> > > > ok, I guess I'm just being thick-headed here, but I still don't
> > > > understand why you are saying it should be split into
> > > > 2 patches.  if I understand *what* you are asking, you're saying
> > > > make the original thread periodic to continuously
> > >
> > > Well, ...
> > >
> > > Your patch merges the original 'ixgbe_setup_link' task into one,
> > > this will make us hard to review the whole design. So what I said
> > > is: firstly, let's change the thread to a service thread to handle
> > > the 'ixgbe_setup_link' subtask firstly. Which is 'ixgbe_link_service'
> > > in your whole patch.
> > >
> >
> > still not 100%, are you suggesting that the original
> > ixgbe_dev_setup_link_thread_handler()
> > which currently is not periodic and only really calls
> > ixgbe_setup_link() be changed to be a periodic task that essentially
> > does what the patch's ixgbe_link_service() function does which would
> > only be checking whether link config is needed and if so calls
> ixgbe_setup_link() as before?
> >
> > if I'm following the code correctly, it ends up going down to
> > ixgbe_check_mac_link_generic() which looks at SDP0 (in the case of
> > needing xtalk fix) which incorrectly will set sfp_cage_full when in
> > fact the PRESENT# signal (or MOD_ABS#) is active low.  the code is
> > extremely convoluted in it's effort to be smart so I may be missing
> > something, but I *believe* that what we end up with is completely
> unnecessary probing of i2c busses looking for SFPs that don't exist.  even
> when making it periodic, I don't think it's going to end up with working code.
> 
> 
> I'm lost ...
> 
> The 'periodic' is the service thread running mode, but the subtask is ONLY
> called when be scheduled, like:
> 
> if (!(adapter->flags & IXGBE_FLAG_NEED_LINK_CONFIG))
>                 return;
> 

this makes no sense.  the *only* time right now when IXGBE_FLAG_NEED_LINK_CONFIG is set is
immediately before the thread is created!  there's no 'hotplug' part of the patch vs fixing the 
'original issue' (your words, below).  the 'original issue' is that the code doesn't work for hotplug
(SFP cages), so there's no breaking up the patch to fix any original issue first.  the only issue is
that for SFP cages, the code doesn't work.  

> >
> > > Small patch is good for us to review, and try to do one thing.
> > >
> > > Hope this time, I can make myself clear. ;-)
> > >
> > > > do ixgbe_link_setup() ?  I believe the problem with the setup is
> > > > that the sfp_type is only detected once at initialization time and
> > > > if nothing is in the cage then the code just returns
> > > > IXGBE_SUCCESS, in which case making this task periodic is useless.
> > > > the whole issue of hotplug is only addressed by the entire patch
> > > > which
> > > > 1) makes the
> > > > task periodic, 2) changes the actions of the task to look for
> > > > whether the cage has something in it and whether its been changed
> > > > and needs to be configured again.
> > > >
> > > >
> > > > > > > > we can separate this patch as least into two patches:
> > > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > > 1st, change the thread handle
> > > 'ixgbe_dev_setup_link_thread_handler'
> > > > > > > > from
> > > > > > > >
> > > > > > > > run-once to as periodical, to handle the original issue.
> > > > > > > >
> > > > > > > > The name 'ixgbe_dev_setup_link_thread_handler' may be not
> > > > > > > > suitable now, as it is a service thread.
> > > > > > > >
> > > > > > > > We can change it to "'ixgbe_link_service_thread_handler'"
> > > > > > > > to reflect the change purpose.
> > > > > > > >
> > > > > > > > 2nd, add the SFP hotplug in this patch.
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > >  };
> > > > > > > > >
> > > > > > > > > --
> > > > > > > > > 2.25.1