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:   Thu, 24 Feb 2022 16:57:36 -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 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.

> > +static void fun_get_pauseparam(struct net_device *netdev,
> > +                            struct ethtool_pauseparam *pause)
> > +{
> > +     const struct funeth_priv *fp = netdev_priv(netdev);
> > +     u8 active_pause = fp->active_fc;
> > +
> > +     pause->rx_pause = !!(active_pause & FUN_PORT_CAP_RX_PAUSE);
> > +     pause->tx_pause = !!(active_pause & FUN_PORT_CAP_TX_PAUSE);
> > +     pause->autoneg = !!(fp->advertising & FUN_PORT_CAP_AUTONEG);
>
> pause->autoneg is if you are negotiating pause via autneg, not if you
> are doing autoneg in general. The user can set pause autoneg to false, via

Indeed, overall AN and pause AN are separate controls. But because of current
FW limitation they need to have the same value and pause AN == overall AN.

> ethtool -A|--pause devname [autoneg on|off]
>
> but the link can still negotiate speed, duplex etc. But then it gets
> more confusing with the following code:
>
> > +}
> > +
> > +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.
When AN is enabled FW doesn't allow forcing pause hence this combination is
rejected. I changed it to be this way after the discussion we had last time.

> > +     if (pause->autoneg && !(fp->advertising & FUN_PORT_CAP_AUTONEG))
> > +             return -EINVAL;
>
> So it should be, you have the capability to advertise pause, not that
> you have the ability to advertise advertising. And it sounds like the
> ability to advertise pause is hard coded on.

It's just the AN on/off state, it's not trying to advertise
advertising. When it's on
pause AN is indeed forced on.

> > +static void fun_get_ethtool_stats(struct net_device *netdev,
> > +                               struct ethtool_stats *stats, u64 *data)
> > +{
> > +     const struct funeth_priv *fp = netdev_priv(netdev);
> > +     struct funeth_txq_stats txs;
> > +     struct funeth_rxq_stats rxs;
> > +     struct funeth_txq **xdpqs;
> > +     struct funeth_rxq **rxqs;
> > +     unsigned int i, start;
> > +     u64 *totals, *tot;
> > +
> > +     if (!netif_running(netdev))
> > +             return;
>
> Why this limitation? I don't expect the counters to increment, but
> they should still indicate the state when the interface was configured
> down.

Most of the counters are queue counters and the queues are deleted when the
net device is down. That was there to prevent attempting to walk non-existing
queues. It's not needed, the condition is handled by another if further down.
I've removed this, at least you get the last MAC stats now.

>
>         Andrew