| Message ID | 20221006135440.3680563-4-Raju.Rangoju@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | amd-xgbe: Miscellaneous fixes | expand |
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. 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@amd.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:
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@amd.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:
On 10/7/22 07:25, Raju Rangoju wrote: > On 10/7/2022 12:34 AM, Tom Lendacky wrote: >> 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? > > The 1m and 3m cables have the BR value within the range 0x64 - 0x68. That certainly seems like 5 and 7 meter cables have an inconsistent value then, no? A quirk for Molex vendor or the specific part series, e.g. 74752- or such in xgbe_phy_sfp_bit_rate() might be a better solution here than expanding the range for all cable vendors. Not sure if others with more SFP+ experience could respond and indicate if 0x78 is really valid for a 10Gbps cable. > >> >>> >>> 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. > > As per the Transceiver compliance codes, offset 3 bits 4,5,6,7 represent > 10Gbps speed. However, 5m and 7m transceivers have none of those bits set. Right, and passive cables shouldn't. Those bits are: Bit 7: 10G Base-ER Bit 6: 10G Base-LRM Bit 5: 10G Base-LR Bit 4: 10G Base-SR which are all Fiber standards. > On the other hand, offset 6 is set, so the driver incorrectly assumes the > transceiver as a 1Gbps cable. Correct, 1000Base-CX is copper, e.g., passive. So, as stated in my previous email, I think moving the passive check up as the first check is reasonable. Thanks, Tom > > Transceiver details: > EEPROM sfp_base offset 0-12: > 0: 0x3 1: 0x4 2: 0x21 3: 0x1 4: 0x0 5: 0x0 6: 0x4 7: 0x41 > 8: 0x84 9: 0x80 10: 0xd5 11: 0x0 12: 0x78 > > enp7s0f2: vendor: Molex Inc. > enp7s0f2: part number: 74752-1701 > enp7s0f2: revision level: > enp7s0f2: serial number: 726341570 > >>> 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. >> >>
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:
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. Fixes: abf0a1c2b26a ("amd-xgbe: Add support for SFP+ modules") Signed-off-by: Raju Rangoju <Raju.Rangoju@amd.com> --- drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)