Date:   Mon, 12 Dec 2022 05:36:47 +0100
From:   Thomas Kupper <thomas@...per.org>
To:     Tom Lendacky <thomas.lendacky@....com>
Cc:     netdev@...r.kernel.org
Subject: 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:
>> On 11/14/22 18:39, Tom Lendacky wrote:
>>> 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.
>>
>> 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
>>
>>>
>>>
>>> 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
>>>>>>>
Morning Tom,

The patches you supplied work well for me, thanks.

Can you cancel/abort/close my submitted patch?

/Thomas

Powered by blists - more mailing lists