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

[Wang, Haiyue]

> -----Original Message-----
> From: Stephen Douthit <stephend at silicom-usa.com>
> Sent: Tuesday, December 21, 2021 05:33
> To: Wang, Haiyue <haiyue.wang at intel.com>; Lu, Wenzhuo <wenzhuo.lu at intel.com>; Changchun Ouyang
> <changchun.ouyang at intel.com>; Zhang, Helin <helin.zhang at intel.com>
> Cc: dev at dpdk.org; Wang, Wen <wenw at silicom-usa.com>; stable at dpdk.org
> Subject: Re: [PATCH v2 3/7] net/ixgbe: Check that SFF-8472 soft rate select is supported before write
> 
> On 12/20/21 02:53, Wang, Haiyue wrote:
> >> -----Original Message-----
> >> From: Stephen Douthit <stephend at silicom-usa.com>
> >> Sent: Tuesday, December 7, 2021 06:19
> >> To: Wang, Haiyue <haiyue.wang at intel.com>; Lu, Wenzhuo <wenzhuo.lu at intel.com>; Changchun Ouyang
> >> <changchun.ouyang at intel.com>; Zhang, Helin <helin.zhang at intel.com>
> >> Cc: dev at dpdk.org; Wen Wang <wenw at silicom-usa.com>; Stephen Douthit <stephend at silicom-usa.com>;
> >> stable at dpdk.org
> >> Subject: [PATCH v2 3/7] net/ixgbe: Check that SFF-8472 soft rate select is supported before write
> >>
> >> 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 at dpdk.org
> >>
> >> Signed-off-by: Stephen Douthit <stephend at 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.

> 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

> 
> Please let me know if that addresses your concerns, or if I've missed
> your point.
> 





> Thanks,
> Steve
> 
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/ethernet/intel/ixg
> be/ixgbe_ethtool.c?h=v5.16-rc6
> 
> >>   #define IXGBE_SFF_SFF_8472_COMP              0x5E
> >>   #define IXGBE_SFF_SFF_8472_OSCB              0x6E
> >>   #define IXGBE_SFF_SFF_8472_ESCB              0x76
> >> @@ -48,6 +49,8 @@
> >>   #define IXGBE_SFF_SOFT_RS_SELECT_10G 0x8
> >> --
> >> 2.31.1
> >