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

From: Andrew Lunn <andrew@lunn.ch>
To: Hariprasad Kelam <hkelam@marvell.com>
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
	davem@davemloft.net, kuba@kernel.org, sgoutham@marvell.com,
	lcherian@marvell.com, gakula@marvell.com, jerinj@marvell.com,
	sbhatta@marvell.com, Christina Jacob <cjacob@marvell.com>
Subject: Re: [Patch v2 net-next 6/7] octeontx2-pf: ethtool physical link status
Date: Wed, 27 Jan 2021 14:51:33 +0100	[thread overview]
Message-ID: <YBFv5ZqTDMyhEIgP@lunn.ch> (raw)
In-Reply-To: <1611733552-150419-7-git-send-email-hkelam@marvell.com>

> +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];
> +
> +		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.

> +	}
> +}
> +
> +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