From: Thomas Kupper <hidden> Date: 2022-11-11 08:48:14
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.
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(-)
From: Thomas Kupper <hidden> Date: 2022-11-11 13:12:28
Am 11.11.22 um 09:46 schrieb Thomas Kupper:
quoted hunk
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.
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(-)
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))
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
The second try (from a different email address) to submit the patch did
fail again. I finally found the reason: setting the sending format to
'Only Plain Text' in 'Options' in Thunderbird is not enough. N00b error
I assume. Terrible sorry for the noise created.
I'll submit a v3 using git send-email later this day.
From: Tom Lendacky <thomas.lendacky@amd.com> Date: 2022-11-11 14:22:54
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.
quoted hunk
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(-)
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.
Thanks,
Tom
From: Thomas Kupper <hidden> Date: 2022-11-11 16:00:29
On 11/11/22 15:18, Tom Lendacky wrote:
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.
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(-)
}
/* 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
From: Tom Lendacky <thomas.lendacky@amd.com> Date: 2022-11-11 17:53:09
On 11/11/22 10:00, Thomas Kupper wrote:
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?
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(-)
@@ -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...
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
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
From: Thomas Kupper <hidden> Date: 2022-11-12 19:12:33
On 11/11/22 17:00, Thomas Kupper wrote:
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?
/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.
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(-)
}
/* 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
From: Thomas Kupper <hidden> Date: 2022-11-12 19:25:43
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?
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
From: Tom Lendacky <thomas.lendacky@amd.com> Date: 2022-11-14 17:39:51
On 11/12/22 13:12, Thomas Kupper wrote:
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.
@@ -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{+}elseif(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;elseif(sfp_base[XGBE_SFP_BASE_10GBE_CC]&XGBE_SFP_BASE_10GBE_CC_SR)
/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(-)
@@ -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
From: Thomas Kupper <hidden> Date: 2022-11-14 19:20:59
On 11/14/22 18:39, Tom Lendacky wrote:
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.
/* 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
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(-)
????? }
????? /* 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
From: Tom Lendacky <thomas.lendacky@amd.com> Date: 2022-11-14 20:51:22
On 11/14/22 13:20, Thomas Kupper wrote:
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
/* 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
@@ -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{+}elseif(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;elseif(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(-)
@@ -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
From: Thomas Kupper <hidden> Date: 2022-12-12 04:36:57
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
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(-)
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