RE: [PATCH v2 3/7] net/ixgbe: Check that SFF-8472 soft rate select is supported before write

From: "Morten Brørup" <mb@smartsharesystems.com>
To: "Wang, Haiyue" <haiyue.wang@intel.com>,
	<stephend@silicom-usa.com>, "Lu, Wenzhuo" <wenzhuo.lu@intel.com>,
	"Changchun Ouyang" <changchun.ouyang@intel.com>,
	"Zhang, Helin" <helin.zhang@intel.com>
Cc: <dev@dpdk.org>, "Wang, Wen" <wenw@silicom-usa.com>, <stable@dpdk.org>
Subject: RE: [PATCH v2 3/7] net/ixgbe: Check that SFF-8472 soft rate select is supported before write
Date: Tue, 21 Dec 2021 09:57:57 +0100	[thread overview]
Message-ID: <98CBD80474FA8B44BF855DF32C47DC35D86D8B@smartserver.smartshare.dk> (raw)
In-Reply-To: <BYAPR11MB34957184FD7BED1AE8BD286FF77C9@BYAPR11MB3495.namprd11.prod.outlook.com>

> From: Wang, Haiyue [mailto:haiyue.wang@intel.com]
> Sent: Tuesday, 21 December 2021 02.15
> 
> > -----Original Message-----
> > From: Stephen Douthit <stephend@silicom-usa.com>
> > Sent: Tuesday, December 21, 2021 05:33
> >
> > On 12/20/21 02:53, Wang, Haiyue wrote:
> > >> -----Original Message-----
> > >> From: Stephen Douthit <stephend@silicom-usa.com>
> > >> Sent: Tuesday, December 7, 2021 06:19
> > >>
> > >> Make sure an SFP is really a SFF-8472 device that supports the
> optional
> > >> soft rate select feature before just blindly poking those I2C
> registers.
> > >>
> > >> Skip all I2C traffic if we know there's no SFP.
> > >>
> > >> Fixes: f3430431aba ("ixgbe/base: add SFP+ dual-speed support")
> > >> Cc: stable@dpdk.org
> > >>
> > >> Signed-off-by: Stephen Douthit <stephend@silicom-usa.com>
> > >> ---
> > >
> > >
> > >>        /* Set RS0 */
> > >>        status = hw->phy.ops.read_i2c_byte(hw,
> IXGBE_SFF_SFF_8472_OSCB,
> > >>
> IXGBE_I2C_EEPROM_DEV_ADDR2,
> > >> diff --git a/drivers/net/ixgbe/base/ixgbe_phy.h
> b/drivers/net/ixgbe/base/ixgbe_phy.h
> > >> index ceefbb3e68..cd57ce040f 100644
> > >> --- a/drivers/net/ixgbe/base/ixgbe_phy.h
> > >> +++ b/drivers/net/ixgbe/base/ixgbe_phy.h
> > >> @@ -21,6 +21,7 @@
> > >>   #define IXGBE_SFF_CABLE_TECHNOLOGY   0x8
> > >>   #define IXGBE_SFF_CABLE_SPEC_COMP    0x3C
> > >>   #define IXGBE_SFF_SFF_8472_SWAP              0x5C
> > >> +#define IXGBE_SFF_SFF_8472_EOPT              0x5D
> > >
> > > Looks like this is YOUR platform specific, then this patchset can't
> be
> > > merged. : - (
> >
> > This isn't anything unique to our hardware, these values are coming
> from
> > the SFF-8472 SFP+ I2C specification.
> >
> > The ability to do a soft rate select via I2C is an optional feature,
> and
> > modules that support it are supposed to set bit 3 in byte 93 (0x5d),
> the
> > "Enhanced Options" register, to advertise the functionality.
> >
> > Please see section 8.10 and Table 8-6 in the SFF-8472 spec.
> >
> > Checking the RATE_SELECT bit flag may be overkill since the
> transceiver
> > is supposed to ignore writes to rate select control bits if the
> feature
> > isn't implemented.  I can drop that check if you like, but the other
> > checks for a 8472 device (vs 8079) aren't anything different than
> what
> > already happens in the driver elsewhere[1].  I'd argue that testing
> that
> > a feature is supported in hardware before trying to use it is normal
> > driver behavior.
> >
> > If instead you mean that the entire series is somehow applicable only
> to
> > our hardware, I'm not sure why.
> >
> > That hotplug issue isn't seen on the same hardware when using the
> Linux
> > driver; so it's a dpdk problem (at least on C3000 ixgbe devs), and
> not a
> 
> I can't find your related fix in two official Linux drivers:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree
> /drivers/net/ethernet/intel/ixgbe
> https://www.intel.com/content/www/us/en/download/14302/14687/intel-
> network-adapter-driver-for-pcie-intel-10-gigabit-ethernet-network-
> connections-under-linux.html?
> 
> Normally, DPDK keeps sync with this kind of release.
> 

Working with the Linux kernel mainline drivers is good advice.

The official Intel Linux drivers seem to be ages behind the Kernel mainline, and they don't fully support the C3000 NICs, so don’t waste any time there! We recently tried using the official Intel Linux drivers for a C3338 based project (using Kernel 3.19 in 32 bit mode with x2APIC disabled), and they didn't work at all. We ended up backporting the necessary changes from the kernel mainline instead.

> > hardware problem.  Fixing the hotplug/rateswap issue was my primary
> > goal, the other patches fix problems I found along the way while
> > debugging.
> >
> > I can also reproduce the hotplug/rateswap issue on the PLCC-B, an
> Intel
> > reference design for the C3000 family, so again, not unique to this
> > platform.
> 
> I guess this is just in C3000 reference board SDK ?
> 
> I recommend you submit the fix to kernel firstly, you will get more
> experts' reviews and fully test:
> 
> https://patchwork.ozlabs.org/project/intel-wired-lan/list/
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
> 