| 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
| ||
|
Message-ID: <9180965b-fe96-7500-d139-013d1987c498@amd.com>
Date: Mon, 14 Nov 2022 11:39:28 -0600
From: Tom Lendacky <thomas.lendacky@....com>
To: Thomas Kupper <thomas@...per.org>, netdev@...r.kernel.org,
"S-k, Shyam-sundar" <Shyam-sundar.S-k@....com>
Cc: Jakub Kicinski <kuba@...nel.org>,
Raju Rangoju <Raju.Rangoju@....com>
Subject: Re: [PATCH v2 net 1/1] amd-xgbe: fix active cable
On 11/12/22 13:12, Thomas Kupper wrote:
> On 11/11/22 17:00, Thomas Kupper wrote:
>> On 11/11/22 15:18, Tom Lendacky wrote:
>>> On 11/11/22 02:46, Thomas Kupper wrote:
>>>> When determine the type of SFP, active cables were not handled.
>>>>
>>>> Add the check for active cables as an extension to the passive cable check.
>>>
>>> Is this fixing a particular problem? What SFP is this failing for? A more descriptive commit message would be good.
>>>
>>> Also, since an active cable is supposed to be advertising it's capabilities in the eeprom, maybe this gets fixed via a quirk and not a general check this field.
>
> Tom,
>
> are you sure that an active cable has to advertising it's speed? Searching for details about it I read in "SFF-8472 Rev 12.4", 5.4.2, Table 5-5 Transceiver Identification Examples:
>
> Transceiver Type Transceiver Description Byte Byte Byte Byte Byte Byte Byte Byte
> 3 4 5 6 7 8 9 10
> ...
> 10GE Active cable with SFP(3,4) 00h 00h 00h 00h 00h 08h 00h 00h
>
> And footnotes:
> 3) See A0h Bytes 60 and 61 for compliance of these media to industry electrical specifications
> 4) For Ethernet and SONET applications, rate capability of a link is identified in A0h Byte 12 [nominal signaling
> rate identifier]. This is due to no formal IEEE designation for passive and active cable interconnects, and lack
> of corresponding identifiers in Table 5-3.
>
> Wouldn't that suggest that byte 3 to 10 are all zero, except byte 8?
This issue seems to be from my misinterpretation of active vs passive.
IIUC now, active and passive only applies to copper cables with SFP+ end
connectors. In which case the driver likely needs an additional enum cable
type, XGBE_SFP_CABLE_FIBER, as the default cable type and slightly
different logic.
Can you try the below patch? If it works, I'll work with Shyam to do some
testing to ensure it doesn't break anything.
diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c b/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c
index 4064c3e3dd49..868a768f424c 100644
--- a/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c
+++ b/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c
@@ -189,6 +189,7 @@ enum xgbe_sfp_cable {
XGBE_SFP_CABLE_UNKNOWN = 0,
XGBE_SFP_CABLE_ACTIVE,
XGBE_SFP_CABLE_PASSIVE,
+ XGBE_SFP_CABLE_FIBER,
};
enum xgbe_sfp_base {
@@ -1149,16 +1150,18 @@ static void xgbe_phy_sfp_parse_eeprom(struct xgbe_prv_data *pdata)
phy_data->sfp_tx_fault = xgbe_phy_check_sfp_tx_fault(phy_data);
phy_data->sfp_rx_los = xgbe_phy_check_sfp_rx_los(phy_data);
- /* Assume ACTIVE cable unless told it is PASSIVE */
+ /* Assume FIBER cable unless told otherwise */
if (sfp_base[XGBE_SFP_BASE_CABLE] & XGBE_SFP_BASE_CABLE_PASSIVE) {
phy_data->sfp_cable = XGBE_SFP_CABLE_PASSIVE;
phy_data->sfp_cable_len = sfp_base[XGBE_SFP_BASE_CU_CABLE_LEN];
- } else {
+ } else if (sfp_base[XGBE_SFP_BASE_CABLE] & XGBE_SFP_BASE_CABLE_ACTIVE) {
phy_data->sfp_cable = XGBE_SFP_CABLE_ACTIVE;
+ } else {
+ phy_data->sfp_cable = XGBE_SFP_CABLE_FIBER;
}
/* Determine the type of SFP */
- if (phy_data->sfp_cable == XGBE_SFP_CABLE_PASSIVE &&
+ if (phy_data->sfp_cable != XGBE_SFP_CABLE_FIBER &&
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)
>
>
> /Thomas
>
>>
>> It is fixing a problem regarding a Mikrotik S+AO0005 AOC cable (we were in contact back in Feb to May). And your right I should have been more descriptive in the commit message.
>>
>>>>
>>>> Fixes: abf0a1c2b26a ("amd-xgbe: Add support for SFP+ modules")
>>>> Signed-off-by: Thomas Kupper <thomas.kupper@...il.com>
>>>> ---
>>>> ??drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c | 5 +++--
>>>> ??1 file changed, 3 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c b/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c
>>>> index 4064c3e3dd49..1ba550d5c52d 100644
>>>> --- a/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c
>>>> +++ b/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c
>>>> @@ -1158,8 +1158,9 @@ static void xgbe_phy_sfp_parse_eeprom(struct xgbe_prv_data *pdata)
>>>> ????? }
>>>>
>>>> ????? /* Determine the type of SFP */
>>>> -??? if (phy_data->sfp_cable == XGBE_SFP_CABLE_PASSIVE &&
>>>> -??? ??? xgbe_phy_sfp_bit_rate(sfp_eeprom, XGBE_SFP_SPEED_10000))
>>>> +??? if ((phy_data->sfp_cable == XGBE_SFP_CABLE_PASSIVE ||
>>>> +??? ???? phy_data->sfp_cable == XGBE_SFP_CABLE_ACTIVE) &&
>>>> +??? ???? xgbe_phy_sfp_bit_rate(sfp_eeprom, XGBE_SFP_SPEED_10000))
>>>
>>> This is just the same as saying:
>>>
>>> ????if (xgbe_phy_sfp_bit_rate(sfp_eeprom, XGBE_SFP_SPEED_10000))
>>>
>>> since the sfp_cable value is either PASSIVE or ACTIVE.
>>>
>>> I'm not sure I like fixing whatever issue you have in this way, though. If anything, I would prefer this to be a last case scenario and be placed at the end of the if-then-else block. But it may come down to applying a quirk for your situation.
>>
>> I see now that this cable is probably indeed not advertising its capabilities correctly, I didn't understand what Shyam did refer to in his mail from June 6.
>>
>> Unfortunately I haven't hear back from you guys after June 6 so I tried to fix it myself ... but do lack the knowledge in that area.
>>
>> A quirk seems a good option.
>>
>> From my point of view this patch can be cancelled/aborted/deleted.
>> I'll look into how to fix it using a quirk but maybe I'm not the hest suited candidate to do it.
>>
>> /Thomas
>>
>>>
>>> Thanks,
>>> Tom
>>>
>>>> ????? ??? 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;
>>>> --
>>>> 2.34.1
>>>>
Powered by blists - more mailing lists