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

Tom Lendacky wrote on 14.11.22 21:51:

On 11/14/22 13:20, Thomas Kupper wrote:

quoted

On 11/14/22 18:39, Tom Lendacky wrote:

quoted

On 11/12/22 13:12, Thomas Kupper wrote:

quoted

On 11/11/22 17: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.

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.

Thanks Tom for getting back to me so soon.

Your patch works well for me, with a passive, an active cable and a
GBIC.

But do you think it's a good idea to just check for !=
XGBE_SFP_CABLE_FIBER? That would also be true for
XGBE_SFP_CABLE_UNKNOWN.

Except the if-then-else block above will set the cable type to one of
the three valid values, so this is ok.

I'll work with Shyam to do some internal testing and get a patch sent
up if everything looks ok on our end.

Thanks,
Tom

quoted

      /* 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)

Cheers
Thomas

quoted

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)

quoted

/Thomas

quoted

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.

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.

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

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

Morning Tom,

The patches you supplied work well for me, thanks.

Can you cancel/abort/close my submitted patch?

/Thomas