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

[Hariprasad Kelam <hkelam@marvell.com>]

Hi Andrew Lunn,


> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Wednesday, January 27, 2021 7:22 PM
> To: Hariprasad Kelam <hkelam@marvell.com>
> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
> davem@davemloft.net; kuba@kernel.org; Sunil Kovvuri Goutham
> <sgoutham@marvell.com>; Linu Cherian <lcherian@marvell.com>;
> Geethasowjanya Akula <gakula@marvell.com>; Jerin Jacob Kollanukkaran
> <jerinj@marvell.com>; Subbaraya Sundeep Bhatta <sbhatta@marvell.com>;
> Christina Jacob <cjacob@marvell.com>
> Subject: [EXT] 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.
>

I will fix this in next version.
 
> 
> > +	}
> > +}
> > +
> > +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