netdev - Re: [PATCH net-next 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]

Message-ID: <CAOkoqZk0O0NidoHuAf4Qbp3e35P7jbPKMYXS=56XWgMx1BceYg@mail.gmail.com>
Date:   Thu, 30 Dec 2021 12:27:38 -0800
From:   Dimitris Michailidis <d.michailidis@...gible.com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     davem@...emloft.net, kuba@...nel.org, netdev@...r.kernel.org
Subject: Re: [PATCH net-next 4/8] net/funeth: ethtool operations

On Thu, Dec 30, 2021 at 10:04 AM Andrew Lunn <andrew@...n.ch> wrote:
>
> > +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);
> > +}
> > +
> > +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;
> > +     if (pause->autoneg && !(fp->advertising & FUN_PORT_CAP_AUTONEG))
> > +             return -EINVAL;
> > +     if (pause->tx_pause & !(fp->port_caps & FUN_PORT_CAP_TX_PAUSE))
> > +             return -EINVAL;
> > +     if (pause->rx_pause & !(fp->port_caps & FUN_PORT_CAP_RX_PAUSE))
> > +             return -EINVAL;
> > +
>
> I _think_ this is wrong. pause->autoneg means we are autoneg'ing
> pause, not that we are using auto-neg in general. The user can have
> autoneg turned on, but force pause by setting pause->autoneg to False.
> In that case, the pause->rx_pause and pause->tx_pause are given direct
> to the MAC, not auto negotiated.

Having this mixed mode needs device FW support, which isn't there today.
If you force pause, then the link flaps and negotiates FW will apply the new
negotiated settings. For your scenario you'd want it to support only partial
application. Meanwhile the partner doesn't know we don't obey the negotiated
settings so I am suspicious that all of this would work.

>
> > +static void fun_get_wol(struct net_device *netdev, struct ethtool_wolinfo *wol)
> > +{
> > +     wol->supported = 0;
> > +     wol->wolopts = 0;
> > +}
>
> Not required. If you don't provide the callback, the core will return
> -EOPNOTSUPP.

OK

>
> > +static void fun_get_drvinfo(struct net_device *netdev,
> > +                         struct ethtool_drvinfo *info)
> > +{
> > +     const struct funeth_priv *fp = netdev_priv(netdev);
> > +
> > +     strscpy(info->driver, KBUILD_MODNAME, sizeof(info->driver));
> > +     strcpy(info->fw_version, "N/A");
>
> Don't set it, if you have nothing useful to put in it.

OK

>
>       Andrew