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]

Message-ID: <Yhfq1N7ce/adhmN9@lunn.ch>
Date:   Thu, 24 Feb 2022 21:30:12 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Dimitris Michailidis <d.michailidis@...gible.com>
Cc:     davem@...emloft.net, kuba@...nel.org, netdev@...r.kernel.org
Subject: Re: [PATCH net-next v7 4/8] net/funeth: ethtool operations

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

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

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?

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

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

	Andrew