Hi Andrew Lunn,
> -----Original Message-----
> From: Andrew Lunn <and...@lunn.ch>
> Sent: Wednesday, January 27, 2021 7:22 PM
> To: Hariprasad Kelam <hke...@marvell.com>
> Cc: netdev@vger.kernel.org; linux-ker...@vger.kernel.org;
> da...@davemloft.net; k...@kernel.org; Sunil Kovvuri Goutham
> <sgout...@marvell.com>; Linu Cherian <lcher...@marvell.com>;
> Geethasowjanya Akula <gak...@marvell.com>; Jerin Jacob Kollanukkaran
> <jer...@marvell.com>; Subbaraya Sundeep Bhatta <sbha...@marvell.com>;
> Christina Jacob <cja...@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
