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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAOkoqZm-jUuYTX_qrZOGS5Fgx=2t2He-7TUzvvn2hMLkFPBEBg@mail.gmail.com>
Date:   Fri, 25 Feb 2022 01:28:46 -0800
From:   Dimitris Michailidis <d.michailidis@...gible.com>
To:     Andrew Lunn <andrew@...n.ch>
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 11:52 PM Andrew Lunn <andrew@...n.ch> wrote:
>
> 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?

FW can report a port as BASET regardless of anything the driver
does, and it needs to be translated into something ethtool understands.
If the driver doesn't use PORT_TP what would it be then?

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

There are two fields currently: port_caps is the RO capabilities field, and
advertising are the settings we are asking FW to use. Some of them may be
used in AN and advertised to the partner, FW will sort that out. The variable
may have different name and there may be more than one but eventually all
the settings need to be passed to FW as one value. It made sense to maintain
them as one value all the way. Keep in mind these settings are not consumed
by a port and need not be sensible for a port. The consumer is FW.
And while there is a close relationship these settings do not fully match what
a link partner receives in case of AN.

>      Andrew