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: 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
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