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

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: <YBFv5ZqTDMyhEIgP@lunn.ch>
Date:   Wed, 27 Jan 2021 14:51:33 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Hariprasad Kelam <hkelam@...vell.com>
Cc:     netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        davem@...emloft.net, kuba@...nel.org, sgoutham@...vell.com,
        lcherian@...vell.com, gakula@...vell.com, jerinj@...vell.com,
        sbhatta@...vell.com, Christina Jacob <cjacob@...vell.com>
Subject: Re: [Patch v2 net-next 6/7] octeontx2-pf: ethtool physical link
 status

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