Re: [Patch v2 net-next 6/7] octeontx2-pf: ethtool physical link status

Hi Andrew Lunn,

-----Original Message-----
From: Andrew Lunn <andrew@lunn.ch>
Sent: Wednesday, January 27, 2021 7:22 PM
To: Hariprasad Kelam <redacted>
Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
davem@davemloft.net; kuba@kernel.org; Sunil Kovvuri Goutham
[off-list ref]; Linu Cherian [off-list ref];
Geethasowjanya Akula [off-list ref]; Jerin Jacob Kollanukkaran
[off-list ref]; Subbaraya Sundeep Bhatta [off-list ref];
Christina Jacob [off-list ref]
Subject: [EXT] Re: [Patch v2 net-next 6/7] octeontx2-pf: ethtool physical link
status

quoted

+static void otx2_get_link_mode_info(u64 index, int mode,
+				    struct ethtool_link_ksettings
+				    *link_ksettings)
+{
+	u64 ethtool_link_mode = 0;
+	int bit_position = 0;
+	u64 link_modes = 0;
+
+	/* CGX link modes to Ethtool link mode mapping */
+	const int cgx_link_mode[29] = {0, /* SGMII  Mode */
+		ETHTOOL_LINK_MODE_1000baseX_Full_BIT,
+		ETHTOOL_LINK_MODE_10000baseT_Full_BIT,
+		ETHTOOL_LINK_MODE_10000baseSR_Full_BIT,
+		ETHTOOL_LINK_MODE_10000baseLR_Full_BIT,
+		ETHTOOL_LINK_MODE_10000baseKR_Full_BIT,
+		OTX2_RESERVED_ETHTOOL_LINK_MODE,
+		ETHTOOL_LINK_MODE_25000baseSR_Full_BIT,
+		OTX2_RESERVED_ETHTOOL_LINK_MODE,
+		OTX2_RESERVED_ETHTOOL_LINK_MODE,
+		ETHTOOL_LINK_MODE_25000baseCR_Full_BIT,
+		ETHTOOL_LINK_MODE_25000baseKR_Full_BIT,
+		ETHTOOL_LINK_MODE_40000baseSR4_Full_BIT,
+		ETHTOOL_LINK_MODE_40000baseLR4_Full_BIT,
+		ETHTOOL_LINK_MODE_40000baseCR4_Full_BIT,
+		ETHTOOL_LINK_MODE_40000baseKR4_Full_BIT,
+		OTX2_RESERVED_ETHTOOL_LINK_MODE,
+		ETHTOOL_LINK_MODE_50000baseSR_Full_BIT,
+		OTX2_RESERVED_ETHTOOL_LINK_MODE,
+		ETHTOOL_LINK_MODE_50000baseLR_ER_FR_Full_BIT,
+		ETHTOOL_LINK_MODE_50000baseCR_Full_BIT,
+		ETHTOOL_LINK_MODE_50000baseKR_Full_BIT,
+		OTX2_RESERVED_ETHTOOL_LINK_MODE,
+		ETHTOOL_LINK_MODE_100000baseSR4_Full_BIT,
+		ETHTOOL_LINK_MODE_100000baseLR4_ER4_Full_BIT,
+		ETHTOOL_LINK_MODE_100000baseCR4_Full_BIT,
+		ETHTOOL_LINK_MODE_100000baseKR4_Full_BIT
+	};
+
+	link_modes = index & OTX2_ETHTOOL_SUPPORTED_MODES;
+
+	for (bit_position = 0; link_modes; bit_position++, link_modes >>= 1) {
+		if (!(link_modes & 1))
+			continue;
+
+		if (bit_position ==  0)
+			ethtool_link_mode = 0x3F;
+
+		if (cgx_link_mode[bit_position])
+			ethtool_link_mode |= 1ULL <<

cgx_link_mode[bit_position];

quoted

+
+		if (mode)
+			*link_ksettings->link_modes.advertising |=
+							ethtool_link_mode;
+		else
+			*link_ksettings->link_modes.supported |=
+							ethtool_link_mode;

You should not be derefererncing these bitmask like this. Use the helpers,
ethtool_link_ksettings_add_link_mode(). You cannot assume these a ULL,
they are not.

Please review all the patches. There are too many levels of obfustication for
me to easily follow the code, bit it looks like you have other bitwise
operations which might be operating on kernel bitmaps, and you are not
using the helpers.

I will fix this in next version.
 

quoted

+	}
+}
+
+static int otx2_get_link_ksettings(struct net_device *netdev,
+				   struct ethtool_link_ksettings *cmd) {
+	struct otx2_nic *pfvf = netdev_priv(netdev);
+	struct cgx_fw_data *rsp = NULL;
+	u32 supported = 0;
+
+	cmd->base.duplex  = pfvf->linfo.full_duplex;
+	cmd->base.speed   = pfvf->linfo.speed;
+	cmd->base.autoneg = pfvf->linfo.an;
+
+	rsp = otx2_get_fwdata(pfvf);
+	if (IS_ERR(rsp))
+		return PTR_ERR(rsp);
+
+	if (rsp->fwdata.supported_an)
+		supported |= SUPPORTED_Autoneg;
+	ethtool_convert_legacy_u32_to_link_mode(cmd-
link_modes.supported,
+						supported);

Why use the legacy stuff when you can directly set the bit using the helpers.
Don't the word legacy actually suggest you should not be using it in new
code?

	Andrew

Agreed.  Will fix this in next version.

Thanks,
Hariprasad k