Re: [PATCH v2 net 1/1] amd-xgbe: fix active cable

On 11/11/22 10:00, Thomas Kupper wrote:

quoted

On 11/11/22 15:18, Tom Lendacky wrote:

quoted

On 11/11/22 02:46, Thomas Kupper wrote:

quoted

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.

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.

That looks like a fiber cable with a dedicated SFP+. Can you supply the 
output of an "ethtool -m XXX" command and a "ethtool -m XXX hex on" command?

(Sorry, didn't see your email, seems it went to the mailing list only)

ethtool -m enp6s0f2
	Identifier                                : 0x03 (SFP)
	Extended identifier                       : 0x04 (GBIC/SFP defined by 2-wire interface ID)
	Connector                                 : 0x21 (Copper pigtail)
	Transceiver codes                         : 0x00 0x00 0x00 0x00 0x00 0x08 0x00 0x00 0x00
	Transceiver type                          : Active Cable
	Encoding                                  : 0x06 (64B/66B)
	BR, Nominal                               : 10300MBd
	Rate identifier                           : 0x00 (unspecified)
	Length (SMF,km)                           : 0km
	Length (SMF)                              : 0m
	Length (50um)                             : 0m
	Length (62.5um)                           : 0m
	Length (Copper)                           : 5m
	Length (OM3)                              : 0m
	Active Cu cmplnce.                        : 0x0c (unknown) [SFF-8472 rev10.4 only]
	Vendor name                               : MikroTik
	Vendor OUI                                : 00:00:00
	Vendor PN                                 : S+AO0005
	Vendor rev                                : 1.0
	Option values                             : 0x00 0x12
	Option                                    : RX_LOS implemented
	Option                                    : TX_DISABLE implemented
	BR margin, max                            : 0%
	BR margin, min                            : 0%
	Vendor SN                                 : STST050B1900001
	Date code                                 : 210515

ethtool -m enp6s0f2 hex on
Offset		Values
------		------
0x0000:		03 04 21 00 00 00 00 00 08 00 00 06 67 00 00 00
0x0010:		00 00 05 00 4d 69 6b 72 6f 54 69 6b 20 20 20 20
0x0020:		20 20 20 20 00 00 00 00 53 2b 41 4f 30 30 30 35
0x0030:		20 20 20 20 20 20 20 20 31 2e 30 20 0c 00 00 5a
0x0040:		00 12 00 00 53 54 53 54 30 35 30 42 31 39 30 30
0x0050:		30 30 31 20 32 31 30 35 31 35 20 20 00 00 05 25
0x0060:		00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x0070:		00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x0080:		31 31 35 35 38 38 36 32 ff ff ff ff ff ff ff ff
0x0090:		32 31 30 34 32 38 30 31 32 ff ff ff ff ff ff ff
0x00a0:		ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
0x00b0:		32 31 30 35 30 34 30 30 31 ff ff ff ff ff ff ff
0x00c0:		ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
0x00d0:		32 31 30 35 30 34 30 30 32 ff ff ff ff ff ff ff
0x00e0:		37 37 31 ff ff ff ff ff ff ff ff ff ff ff ff ff
0x00f0:		31 31 35 35 38 38 36 32 ff ff ff ff ff ff ff ff
0x0100:		55 00 f6 00 50 00 fb 00 8c a0 6d 60 88 b8 71 48
0x0110:		1d 4c 00 fa 17 70 01 f4 31 2d 04 ea 27 10 06 30
0x0120:		31 2d 01 3c 27 10 01 8e 00 00 00 00 00 00 00 00
0x0130:		00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x0140:		00 00 00 00 3f 80 00 00 00 00 00 00 01 00 00 00
0x0150:		01 00 00 00 01 00 00 00 01 00 00 00 00 00 00 44
0x0160:		20 e6 7f 00 0c b0 1e 54 0d e4 00 00 00 00 30 00
0x0170:		00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x0180:		00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x0190:		00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x01a0:		00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x01b0:		00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x01c0:		00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x01d0:		00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x01e0:		00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x01f0:		00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

Thanks
/Thomas

quoted

quoted

quoted

Fixes: abf0a1c2b26a ("amd-xgbe: Add support for SFP+ modules")
Signed-off-by: Thomas Kupper <redacted>
---
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. 

Adding Shyam back to see what the status is...

quoted

A quirk seems a good option.

The quirk may be that the parsing code calls a function that updates the 
eeprom data in memory based on the SFP identifier.

Thanks,
Tom

quoted

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

quoted

Thanks,
Tom

quoted

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