netdev - Re: [PATCH net 3/3] amd-xgbe: fix the SFP compliance codes check for DAC cables

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d377d924-d205-cd25-e3d0-7521ed8a3ca1@amd.com>
Date:   Thu, 6 Oct 2022 10:00:27 -0500
From:   Tom Lendacky <thomas.lendacky@....com>
To:     Raju Rangoju <Raju.Rangoju@....com>, Shyam-sundar.S-k@....com,
        davem@...emloft.net, kuba@...nel.org
Cc:     netdev@...r.kernel.org, rrangoju@....com
Subject: Re: [PATCH net 3/3] amd-xgbe: fix the SFP compliance codes check for
 DAC cables

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@....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: