netdev - Re: [PATCH net 3/3] amd-xgbe: fix the SFP compliance codes check for DAC cables

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [day] [month] [year] [list]
Message-ID: <40b3f874-e7fc-4049-65e2-3ed449b956f8@amd.com>
Date:   Thu, 6 Oct 2022 14:04:10 -0500
From:   Tom Lendacky <thomas.lendacky@....com>
To:     Raju Rangoju <Raju.Rangoju@....com>, Shyam-sundar.S-k@....com,
        davem@...emloft.net, kuba@...nel.org
Cc:     netdev@...r.kernel.org, rrangoju@....com
Subject: Re: [PATCH net 3/3] amd-xgbe: fix the SFP compliance codes check for
 DAC cables

On 10/6/22 12:37, Raju Rangoju wrote:
> On 10/6/2022 8:30 PM, Tom Lendacky wrote:
>> On 10/6/22 08:54, Raju Rangoju wrote:
>>> The current XGBE code assumes that offset 3 and 6 of EEPROM SFP DAC
>>> (passive) cables are NULL. It also assumes the offset 12 is in the
>>> range 0x64 to 0x68. However, some of the cables (the 5 meter and 7 meter
>>> molex passive cables have non-zero data at offset 3 and 6, also a value
>>> 0x78 at offset 12. So, fix the sfp compliance codes check to ignore
>>> those offsets. Also extend the macro XGBE_SFP_BASE_BR_10GBE range to 0x78.
>>
>> So are these cables going against the specification? Should they be 
>> quirks instead of changing the way code is currently operating? How many 
>> different cables have you found that do this?
>>
>> Why would a passive cable be setting any bit other than passive in byte 
>> 3? Why would byte 6 also have a non-zero value?
>>
>> As for the range, 0x78 puts the cable at 12gbps which kind of seems 
>> outside the normal range of what a 10gbps cable should be reporting.
>>
> 
> For the passive cables, the current SFP checks in driver are not expecting 
> any data at offset 3 and 6. Also, the offset 12 is expected to be in the 
> range 0x64 - 0x68. This was holding good for Fiber store cables so far. 
> However, the 5 and 7 meter Molex passive cables have non-zero data at 
> offset 3 and 6, and also a value 0x78 at offset 12.

The 0x64 - 0x68 BR range was holding well for the various passive cables 
that I tested with. What is the BR value for their other length cables?

> 
> Here is the feedback from cable Vendor when asked about the SFP standard 
> for passive cables:
> 
> "For DAC cables –The Ethernet code compliance code standard for passive 
> cabling, Offset 3 is “0x0” other offsets  4 and 5 - none of the standards 
> are applicable .

Ok, so it's not offset 3 that is the issue as none of the bits are set and 
won't trigger on the 10G Ethernet Compliance Codes.

> Offset 6 – refers to 1000base-cx which is supported .

Ok, that makes sense and argues for moving the passive check first, 
although the code doesn't support being able to switch to 1000Base-CX.


> For passive cable , there is no separate bit to define the compliance code 
> for 10G as per the SFF standard. Please modify your SW accordingly.
> "

This doesn't answer the question about the BR range. 0x78 seems excessive 
to me (12,000 mbps) and so I'm not sure what effect increasing the range 
will have in general vs restricting the change to just the vendor/part 
having the issue.

Thanks,
Tom

> 
>> I guess I'm not opposed to the ordering of the SFP checks (moving the 
>> passive check up as the first check), but the reasons seem odd, hence my 
>> question of whether this should be a quirk.
>>
>> Thanks,
>> Tom
>>
>>>
>>> Fixes: abf0a1c2b26a ("amd-xgbe: Add support for SFP+ modules")
>>> Signed-off-by: Raju Rangoju <Raju.Rangoju@....com>
>>> ---
>>>   drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c | 10 +++++-----
>>>   1 file changed, 5 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c 
>>> b/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c
>>> index 23fbd89a29df..0387e691be68 100644
>>> --- a/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c
>>> +++ b/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c
>>> @@ -238,7 +238,7 @@ enum xgbe_sfp_speed {
>>>   #define XGBE_SFP_BASE_BR_1GBE_MIN        0x0a
>>>   #define XGBE_SFP_BASE_BR_1GBE_MAX        0x0d
>>>   #define XGBE_SFP_BASE_BR_10GBE_MIN        0x64
>>> -#define XGBE_SFP_BASE_BR_10GBE_MAX        0x68
>>> +#define XGBE_SFP_BASE_BR_10GBE_MAX        0x78
>>>   #define XGBE_SFP_BASE_CU_CABLE_LEN        18
>>> @@ -1151,7 +1151,10 @@ static void xgbe_phy_sfp_parse_eeprom(struct 
>>> xgbe_prv_data *pdata)
>>>       }
>>>       /* Determine the type of SFP */
>>> -    if (sfp_base[XGBE_SFP_BASE_10GBE_CC] & XGBE_SFP_BASE_10GBE_CC_SR)
>>> +    if (phy_data->sfp_cable == XGBE_SFP_CABLE_PASSIVE &&
>>> +        xgbe_phy_sfp_bit_rate(sfp_eeprom, XGBE_SFP_SPEED_10000))
>>> +        phy_data->sfp_base = XGBE_SFP_BASE_10000_CR;
>>> +    else if (sfp_base[XGBE_SFP_BASE_10GBE_CC] & 
>>> XGBE_SFP_BASE_10GBE_CC_SR)
>>>           phy_data->sfp_base = XGBE_SFP_BASE_10000_SR;
>>>       else if (sfp_base[XGBE_SFP_BASE_10GBE_CC] & 
>>> XGBE_SFP_BASE_10GBE_CC_LR)
>>>           phy_data->sfp_base = XGBE_SFP_BASE_10000_LR;
>>> @@ -1167,9 +1170,6 @@ static void xgbe_phy_sfp_parse_eeprom(struct 
>>> xgbe_prv_data *pdata)
>>>           phy_data->sfp_base = XGBE_SFP_BASE_1000_CX;
>>>       else if (sfp_base[XGBE_SFP_BASE_1GBE_CC] & XGBE_SFP_BASE_1GBE_CC_T)
>>>           phy_data->sfp_base = XGBE_SFP_BASE_1000_T;
>>> -    else if ((phy_data->sfp_cable == XGBE_SFP_CABLE_PASSIVE) &&
>>> -         xgbe_phy_sfp_bit_rate(sfp_eeprom, XGBE_SFP_SPEED_10000))
>>> -        phy_data->sfp_base = XGBE_SFP_BASE_10000_CR;
>>>       switch (phy_data->sfp_base) {
>>>       case XGBE_SFP_BASE_1000_T: