netdev - Re: [PATCH net-next v7 4/8] net/funeth: ethtool operations

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]

Date:   Fri, 25 Feb 2022 08:52:03 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Dimitris Michailidis <d.michailidis@...gible.com>
Cc:     davem@...emloft.net, Jakub Kicinski <kuba@...nel.org>,
        netdev@...r.kernel.org
Subject: Re: [PATCH net-next v7 4/8] net/funeth: ethtool operations

On Thu, Feb 24, 2022 at 04:57:36PM -0800, Dimitris Michailidis wrote:
> On Thu, Feb 24, 2022 at 12:30 PM Andrew Lunn <andrew@...n.ch> wrote:
> >
> > > +static void fun_link_modes_to_ethtool(u64 modes,
> > > +                                   unsigned long *ethtool_modes_map)
> > > +{
> > > +#define ADD_LINK_MODE(mode) \
> > > +     __set_bit(ETHTOOL_LINK_MODE_ ## mode ## _BIT, ethtool_modes_map)
> > > +
> > > +     if (modes & FUN_PORT_CAP_AUTONEG)
> > > +             ADD_LINK_MODE(Autoneg);
> > > +     if (modes & FUN_PORT_CAP_1000_X)
> > > +             ADD_LINK_MODE(1000baseX_Full);
> > > +     if (modes & FUN_PORT_CAP_10G_R) {
> > > +             ADD_LINK_MODE(10000baseCR_Full);
> > > +             ADD_LINK_MODE(10000baseSR_Full);
> > > +             ADD_LINK_MODE(10000baseLR_Full);
> > > +             ADD_LINK_MODE(10000baseER_Full);
> > > +     }
> >
> > > +static unsigned int fun_port_type(unsigned int xcvr)
> > > +{
> > > +     if (!xcvr)
> > > +             return PORT_NONE;
> > > +
> > > +     switch (xcvr & 7) {
> > > +     case FUN_XCVR_BASET:
> > > +             return PORT_TP;
> >
> > You support twisted pair, so should you also have the BaseT_FULL link
> > modes above?
> 
> I agree with that but FW currently doesn't report BASE-T speeds in its
> port capabilities and the link modes are based on them. Looks simple to fix
> but needs future FW.

Maybe you should drop PORT_TP until you do have the firmware fixed?

> > > +static int fun_set_pauseparam(struct net_device *netdev,
> > > +                           struct ethtool_pauseparam *pause)
> > > +{
> > > +     struct funeth_priv *fp = netdev_priv(netdev);
> > > +     u64 new_advert;
> > > +
> > > +     if (fp->port_caps & FUN_PORT_CAP_VPORT)
> > > +             return -EOPNOTSUPP;
> > > +     /* Forcing PAUSE settings with AN enabled is unsupported. */
> > > +     if (!pause->autoneg && (fp->advertising & FUN_PORT_CAP_AUTONEG))
> > > +             return -EOPNOTSUPP;
> >
> > This seems wrong. You don't advertise you cannot advertise. You simply
> > don't advertise. It could just be you have a bad variable name here?
> 
> advertising & FUN_PORT_CAP_AUTONEG means that AN is enabled, and
> when this bit is off AN is disabled.

So, i was correct, the name of the variable is not so good. Maybe
fp->advertising need splitting into two, fp->cap_enabled for
capabilities of the firmware that are enabled, and fp->advertising for
what is actually been advertised to the link partner?

     Andrew