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 20:15, Wang, Haiyue wrote:
>> -----Original Message-----
>> From: Stephen Douthit <stephend@silicom-usa.com>
>> Sent: Tuesday, December 21, 2021 05:33
>> 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; 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
>>
>> 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
> 
> I can't find your related fix in two official Linux drivers:

There's no submission from me on the hotplug issue for the mainline,
because the issue isn't present in Linux, only in DPDK.

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

It's the board covered by Intel Doc # 574437.

> I recommend you submit the fix to kernel firstly, you will get more
> experts' reviews and fully test:

Since patch 3 isn't directly related to the hotplug issue should I pull
it from the series for v3 to keep the hotplug fixes moving forward here,
and in parallel submit just that one to Linux?

Thanks,
Steve

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