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

["Morten Brørup" <mb@smartsharesystems.com>]

> From: Wang, Haiyue [mailto:haiyue.wang@intel.com]
> Sent: Wednesday, 13 April 2022 13.49
> 
> > From: Morten Brørup <mb@smartsharesystems.com>
> > Sent: Wednesday, April 13, 2022 15:20
> >
> > > From: Wang, Haiyue [mailto:haiyue.wang@intel.com]
> > > Sent: Wednesday, 13 April 2022 09.02
> > >
> > > > From: Morten Brørup <mb@smartsharesystems.com>
> > > > Sent: Wednesday, April 13, 2022 14:58
> > > >
> > > > > From: Wang, Haiyue [mailto:haiyue.wang@intel.com]
> > > > > Sent: Wednesday, 13 April 2022 04.47
> > > > > To: Daly, Jeff; dev@dpdk.org
> > > > > Cc: stable@dpdk.org; Stephen Douthit; Yang, Qiming
> > > > >
> > > > > > 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>
> > > > > >
> > > > > > 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
> > > > > >
> > > > >
> > > > > So BIG change for new platform, DON'T CC to stable!
> > > >
> > > > What do you mean by "new platform"? The ixgbe hardware and driver
> is
> > > not new.
> > > >
> > >
> > > It's soc NIC, ixgbe not support before.
> >
> > If the patch only fixes the driver for a new NIC that not supported
> by older DPDK versions, and that
> > NIC is not going to be supported by older DPDK versions, then I agree
> that there is no point in
> > backporting it or CC'ing stable.
> >
> > However, if the patch could also apply to any other ixgbe NIC that is
> potentially supported by older
> > DPDK versions, then it should be backported.
> 
> It's hard to say, these years, I see many ixgbe link related fixes.

The physical layers have always been tricky... Apparently, they still are. :-)

> At least now, no big link fix for normal ixgbe NICs.

I would not discriminate between normal and less common NICs. If they are not EOL according to Intel, the drivers should support them.

In the real world, though, driver development resources will be allocated to the important customers and/or the high volume products. So I do understand your concern! Not being directly involved in this work myself, it is easy to voice my opinion as a "backseat driver". :-)

> 
> This patch still have some kind of TODOs. And this is not acceptable
> for
> us to maintain this kind of code for released stable DPDK version. I
> don't
> want to see many follow fixes ...
> 
> And we have two DPDK development cycle (22.07 22.11) to make it for
> next
> stable release.
> 

I 100 % agree that all the TODOs should be solved first, so the driver is reliable and complete before any backporting starts. Adding follow-on fixes in multiple DPDK versions is a waste of maintainer time, and I agree with your pushback when this is the reason.

With that in mind, CC'ing stable could be postponed until the patch reaches backporting worthy quality.

> >
> > >
> > > > This patch fixes a bug (with a serious impact when occurring), so
> it
> > > should be backported. The size of
> > > > the patch does not disqualify it for backporting.
> > > >
> > > > -Morten
> > >
>