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

[Andrew Lunn <andrew@lunn.ch>]

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