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

[Stephen Douthit <stephend@silicom-usa.com>]

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
>> To: Wang, Haiyue <haiyue.wang@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>; Changchun Ouyang
>> <changchun.ouyang@intel.com>; Zhang, Helin <helin.zhang@intel.com>
>> Cc: dev@dpdk.org; Wen Wang <wenw@silicom-usa.com>; Stephen Douthit <stephend@silicom-usa.com>;
>> stable@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@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
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.

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/ixgbe/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
>