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>] [day] [month] [year] [list]
Message-ID: <MWHPR18MB1421B53E6EEAC6102D70083FDEB89@MWHPR18MB1421.namprd18.prod.outlook.com>
Date:   Sat, 30 Jan 2021 09:40:43 +0000
From:   Hariprasad Kelam <hkelam@...vell.com>
To:     Andrew Lunn <andrew@...n.ch>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "kuba@...nel.org" <kuba@...nel.org>,
        Sunil Kovvuri Goutham <sgoutham@...vell.com>,
        Linu Cherian <lcherian@...vell.com>,
        Geethasowjanya Akula <gakula@...vell.com>,
        "Jerin Jacob Kollanukkaran" <jerinj@...vell.com>,
        Subbaraya Sundeep Bhatta <sbhatta@...vell.com>,
        Christina Jacob <cjacob@...vell.com>
Subject: Re: [Patch v2 net-next 6/7] octeontx2-pf: ethtool physical link
 status

Hi Andrew Lunn,


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