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: <3692ac8a-a992-653d-920b-20efe795b22f@alliedtelesis.co.nz>
Date:   Thu, 1 Dec 2022 02:02:07 +0000
From:   Chris Packham <Chris.Packham@...iedtelesis.co.nz>
To:     Hariprasad Kelam <hkelam@...vell.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "cjacob@...vell.com" <cjacob@...vell.com>
CC:     "kuba@...nel.org" <kuba@...nel.org>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "willemdebruijn.kernel@...il.com" <willemdebruijn.kernel@...il.com>,
        "andrew@...n.ch" <andrew@...n.ch>,
        "sgoutham@...vell.com" <sgoutham@...vell.com>,
        "lcherian@...vell.com" <lcherian@...vell.com>,
        "gakula@...vell.com" <gakula@...vell.com>,
        "jerinj@...vell.com" <jerinj@...vell.com>,
        "sbhatta@...vell.com" <sbhatta@...vell.com>,
        Richard Laing <Richard.Laing@...iedtelesis.co.nz>
Subject: Re: [Patch v3 net-next 5/7] octeontx2-af: advertised link modes
 support on cgx

Hi Hariprasad, Christina,

Sorry for poking an old thread but Richard and I were looking at some 
code and noticed something odd about it.

On 1/02/21 18:24, Hariprasad Kelam wrote:
> From: Christina Jacob <cjacob@...vell.com>
>
> CGX supports setting advertised link modes on physical link.
> This patch adds support to derive cgx mode from ethtool
> link mode and pass it to firmware to configure the same.
>
> Signed-off-by: Christina Jacob <cjacob@...vell.com>
> Signed-off-by: Sunil Goutham <sgoutham@...vell.com>
> Signed-off-by: Hariprasad Kelam <hkelam@...vell.com>
> ---
>   drivers/net/ethernet/marvell/octeontx2/af/cgx.c    | 114 ++++++++++++++++++++-
>   .../net/ethernet/marvell/octeontx2/af/cgx_fw_if.h  |  32 +++++-
>   drivers/net/ethernet/marvell/octeontx2/af/mbox.h   |   3 +-
>   3 files changed, 146 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/cgx.c b/drivers/net/ethernet/marvell/octeontx2/af/cgx.c
> index 5b7d858..9c62129 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/cgx.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/cgx.c
> @@ -14,6 +14,7 @@
>   #include <linux/pci.h>
>   #include <linux/netdevice.h>
>   #include <linux/etherdevice.h>
> +#include <linux/ethtool.h>
>   #include <linux/phy.h>
>   #include <linux/of.h>
>   #include <linux/of_mdio.h>
> @@ -646,6 +647,7 @@ static inline void cgx_link_usertable_init(void)
>   	cgx_speed_mbps[CGX_LINK_25G] = 25000;
>   	cgx_speed_mbps[CGX_LINK_40G] = 40000;
>   	cgx_speed_mbps[CGX_LINK_50G] = 50000;
> +	cgx_speed_mbps[CGX_LINK_80G] = 80000;
>   	cgx_speed_mbps[CGX_LINK_100G] = 100000;
>   
>   	cgx_lmactype_string[LMAC_MODE_SGMII] = "SGMII";
> @@ -693,6 +695,110 @@ static int cgx_link_usertable_index_map(int speed)
>   	return CGX_LINK_NONE;
>   }
>   
> +static void set_mod_args(struct cgx_set_link_mode_args *args,
> +			 u32 speed, u8 duplex, u8 autoneg, u64 mode)
> +{
> +	/* Fill default values incase of user did not pass
> +	 * valid parameters
> +	 */
> +	if (args->duplex == DUPLEX_UNKNOWN)
> +		args->duplex = duplex;
> +	if (args->speed == SPEED_UNKNOWN)
> +		args->speed = speed;
> +	if (args->an == AUTONEG_UNKNOWN)
> +		args->an = autoneg;
> +	args->mode = mode;
> +	args->ports = 0;
> +}
> +
> +static void otx2_map_ethtool_link_modes(u64 bitmask,
> +					struct cgx_set_link_mode_args *args)
> +{
> +	switch (bitmask) {
> +	case ETHTOOL_LINK_MODE_10baseT_Half_BIT:
> +		set_mod_args(args, 10, 1, 1, BIT_ULL(CGX_MODE_SGMII));
> +		break;
> +	case  ETHTOOL_LINK_MODE_10baseT_Full_BIT:
> +		set_mod_args(args, 10, 0, 1, BIT_ULL(CGX_MODE_SGMII));
> +		break;
> +	case  ETHTOOL_LINK_MODE_100baseT_Half_BIT:
> +		set_mod_args(args, 100, 1, 1, BIT_ULL(CGX_MODE_SGMII));
> +		break;
> +	case  ETHTOOL_LINK_MODE_100baseT_Full_BIT:
> +		set_mod_args(args, 100, 0, 1, BIT_ULL(CGX_MODE_SGMII));
> +		break;
> +	case  ETHTOOL_LINK_MODE_1000baseT_Half_BIT:
> +		set_mod_args(args, 1000, 1, 1, BIT_ULL(CGX_MODE_SGMII));
> +		break;
> +	case  ETHTOOL_LINK_MODE_1000baseT_Full_BIT:
> +		set_mod_args(args, 1000, 0, 1, BIT_ULL(CGX_MODE_SGMII));
> +		break;
> +	case  ETHTOOL_LINK_MODE_1000baseX_Full_BIT:
> +		set_mod_args(args, 1000, 0, 0, BIT_ULL(CGX_MODE_1000_BASEX));
> +		break;
> +	case  ETHTOOL_LINK_MODE_10000baseT_Full_BIT:
> +		set_mod_args(args, 1000, 0, 1, BIT_ULL(CGX_MODE_QSGMII));

Here the speed is set to 1000 but it looks like this should be 10000. I 
would have sent a patch to fix that but the mode is also a bit 
confusing. Normally I'd expect QSGMII to be used for Quad SGMII (i.e. 
4x1G Ethernet ports multiplexed over a single SERDES). I don't see any 
other mode in enum CGX_MODE_ that is obviously for 10GBase-T so I 
thought I'd bring this to your attention. I'd still be happy to whip up 
a patch if you could confirm what the correct mode should be.

> +		break;
> +	case  ETHTOOL_LINK_MODE_10000baseSR_Full_BIT:
> +		set_mod_args(args, 10000, 0, 0, BIT_ULL(CGX_MODE_10G_C2C));
> +		break;
> +	case  ETHTOOL_LINK_MODE_10000baseLR_Full_BIT:
> +		set_mod_args(args, 10000, 0, 0, BIT_ULL(CGX_MODE_10G_C2M));
> +		break;
> +	case  ETHTOOL_LINK_MODE_10000baseKR_Full_BIT:
> +		set_mod_args(args, 10000, 0, 1, BIT_ULL(CGX_MODE_10G_KR));
> +		break;
> +	case  ETHTOOL_LINK_MODE_25000baseSR_Full_BIT:
> +		set_mod_args(args, 25000, 0, 0, BIT_ULL(CGX_MODE_25G_C2C));
> +		break;
> +	case  ETHTOOL_LINK_MODE_25000baseCR_Full_BIT:
> +		set_mod_args(args, 25000, 0, 1, BIT_ULL(CGX_MODE_25G_CR));
> +		break;
> +	case  ETHTOOL_LINK_MODE_25000baseKR_Full_BIT:
> +		set_mod_args(args, 25000, 0, 1, BIT_ULL(CGX_MODE_25G_KR));
> +		break;
> +	case  ETHTOOL_LINK_MODE_40000baseSR4_Full_BIT:
> +		set_mod_args(args, 40000, 0, 0, BIT_ULL(CGX_MODE_40G_C2C));
> +		break;
> +	case  ETHTOOL_LINK_MODE_40000baseLR4_Full_BIT:
> +		set_mod_args(args, 40000, 0, 0, BIT_ULL(CGX_MODE_40G_C2M));
> +		break;
> +	case  ETHTOOL_LINK_MODE_40000baseCR4_Full_BIT:
> +		set_mod_args(args, 40000, 0, 1, BIT_ULL(CGX_MODE_40G_CR4));
> +		break;
> +	case  ETHTOOL_LINK_MODE_40000baseKR4_Full_BIT:
> +		set_mod_args(args, 40000, 0, 1, BIT_ULL(CGX_MODE_40G_KR4));
> +		break;
> +	case  ETHTOOL_LINK_MODE_50000baseSR_Full_BIT:
> +		set_mod_args(args, 50000, 0, 0, BIT_ULL(CGX_MODE_50G_C2C));
> +		break;
> +	case  ETHTOOL_LINK_MODE_50000baseLR_ER_FR_Full_BIT:
> +		set_mod_args(args, 50000, 0, 0, BIT_ULL(CGX_MODE_50G_C2M));
> +		break;
> +	case  ETHTOOL_LINK_MODE_50000baseCR_Full_BIT:
> +		set_mod_args(args, 50000, 0, 1, BIT_ULL(CGX_MODE_50G_CR));
> +		break;
> +	case  ETHTOOL_LINK_MODE_50000baseKR_Full_BIT:
> +		set_mod_args(args, 50000, 0, 1, BIT_ULL(CGX_MODE_50G_KR));
> +		break;
> +	case  ETHTOOL_LINK_MODE_100000baseSR4_Full_BIT:
> +		set_mod_args(args, 100000, 0, 0, BIT_ULL(CGX_MODE_100G_C2C));
> +		break;
> +	case  ETHTOOL_LINK_MODE_100000baseLR4_ER4_Full_BIT:
> +		set_mod_args(args, 100000, 0, 0, BIT_ULL(CGX_MODE_100G_C2M));
> +		break;
> +	case  ETHTOOL_LINK_MODE_100000baseCR4_Full_BIT:
> +		set_mod_args(args, 100000, 0, 1, BIT_ULL(CGX_MODE_100G_CR4));
> +		break;
> +	case  ETHTOOL_LINK_MODE_100000baseKR4_Full_BIT:
> +		set_mod_args(args, 100000, 0, 1, BIT_ULL(CGX_MODE_100G_KR4));
> +		break;
> +	default:
> +		set_mod_args(args, 0, 1, 0, BIT_ULL(CGX_MODE_MAX));
> +		break;
> +	}
> +}
> +
>   static inline void link_status_user_format(u64 lstat,
>   					   struct cgx_link_user_info *linfo,
>   					   struct cgx *cgx, u8 lmac_id)
> @@ -887,13 +993,19 @@ int cgx_set_link_mode(void *cgxd, struct cgx_set_link_mode_args args,
>   	if (!cgx)
>   		return -ENODEV;
>   
> +	if (args.mode)
> +		otx2_map_ethtool_link_modes(args.mode, &args);
> +	if (!args.speed && args.duplex && !args.an)
> +		return -EINVAL;
> +
>   	req = FIELD_SET(CMDREG_ID, CGX_CMD_MODE_CHANGE, req);
>   	req = FIELD_SET(CMDMODECHANGE_SPEED,
>   			cgx_link_usertable_index_map(args.speed), req);
>   	req = FIELD_SET(CMDMODECHANGE_DUPLEX, args.duplex, req);
>   	req = FIELD_SET(CMDMODECHANGE_AN, args.an, req);
>   	req = FIELD_SET(CMDMODECHANGE_PORT, args.ports, req);
> -	req = FIELD_SET(CMDMODECHANGE_FLAGS, args.flags, req);
> +	req = FIELD_SET(CMDMODECHANGE_FLAGS, args.mode, req);
> +
>   	return cgx_fwi_cmd_generic(req, &resp, cgx, lmac_id);
>   }
>   int cgx_set_fec(u64 fec, int cgx_id, int lmac_id)
> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/cgx_fw_if.h b/drivers/net/ethernet/marvell/octeontx2/af/cgx_fw_if.h
> index 70610e7..dde2bd0 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/cgx_fw_if.h
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/cgx_fw_if.h
> @@ -70,6 +70,36 @@ enum cgx_link_speed {
>   	CGX_LINK_SPEED_MAX,
>   };
>   
> +enum CGX_MODE_ {
> +	CGX_MODE_SGMII,
> +	CGX_MODE_1000_BASEX,
> +	CGX_MODE_QSGMII,
> +	CGX_MODE_10G_C2C,
> +	CGX_MODE_10G_C2M,
> +	CGX_MODE_10G_KR,
> +	CGX_MODE_20G_C2C,
> +	CGX_MODE_25G_C2C,
> +	CGX_MODE_25G_C2M,
> +	CGX_MODE_25G_2_C2C,
> +	CGX_MODE_25G_CR,
> +	CGX_MODE_25G_KR,
> +	CGX_MODE_40G_C2C,
> +	CGX_MODE_40G_C2M,
> +	CGX_MODE_40G_CR4,
> +	CGX_MODE_40G_KR4,
> +	CGX_MODE_40GAUI_C2C,
> +	CGX_MODE_50G_C2C,
> +	CGX_MODE_50G_C2M,
> +	CGX_MODE_50G_4_C2C,
> +	CGX_MODE_50G_CR,
> +	CGX_MODE_50G_KR,
> +	CGX_MODE_80GAUI_C2C,
> +	CGX_MODE_100G_C2C,
> +	CGX_MODE_100G_C2M,
> +	CGX_MODE_100G_CR4,
> +	CGX_MODE_100G_KR4,
> +	CGX_MODE_MAX /* = 29 */
> +};
>   /* REQUEST ID types. Input to firmware */
>   enum cgx_cmd_id {
>   	CGX_CMD_NONE,
> @@ -231,6 +261,6 @@ struct cgx_lnk_sts {
>   #define CMDMODECHANGE_DUPLEX		GENMASK_ULL(12, 12)
>   #define CMDMODECHANGE_AN		GENMASK_ULL(13, 13)
>   #define CMDMODECHANGE_PORT		GENMASK_ULL(21, 14)
> -#define CMDMODECHANGE_FLAGS		GENMASK_ULL(29, 22)
> +#define CMDMODECHANGE_FLAGS		GENMASK_ULL(63, 22)
>   
>   #endif /* __CGX_FW_INTF_H__ */
> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/mbox.h b/drivers/net/ethernet/marvell/octeontx2/af/mbox.h
> index a050902..05a6da2 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/mbox.h
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/mbox.h
> @@ -462,10 +462,11 @@ struct cgx_set_link_mode_args {
>   	u8 duplex;
>   	u8 an;
>   	u8 ports;
> -	u8 flags;
> +	u64 mode;
>   };
>   
>   struct cgx_set_link_mode_req {
> +#define AUTONEG_UNKNOWN		0xff
>   	struct mbox_msghdr hdr;
>   	struct cgx_set_link_mode_args args;
>   };

Thanks,
Chris

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ