Re: [PATCH net-next] dpaa2-eth: Add pause frame support

[Andrew Lunn <andrew@lunn.ch>]

> --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-ethtool.c
> +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-ethtool.c
> @@ -78,29 +78,20 @@ static int
>  dpaa2_eth_get_link_ksettings(struct net_device *net_dev,
>  			     struct ethtool_link_ksettings *link_settings)
>  {
> -	struct dpni_link_state state = {0};
> -	int err = 0;
>  	struct dpaa2_eth_priv *priv = netdev_priv(net_dev);
>  
> -	err = dpni_get_link_state(priv->mc_io, 0, priv->mc_token, &state);
> -	if (err) {
> -		netdev_err(net_dev, "ERROR %d getting link state\n", err);
> -		goto out;
> -	}
> -
>  	/* At the moment, we have no way of interrogating the DPMAC
>  	 * from the DPNI side - and for that matter there may exist
>  	 * no DPMAC at all. So for now we just don't report anything
>  	 * beyond the DPNI attributes.
>  	 */
> -	if (state.options & DPNI_LINK_OPT_AUTONEG)
> +	if (priv->link_state.options & DPNI_LINK_OPT_AUTONEG)
>  		link_settings->base.autoneg = AUTONEG_ENABLE;
> -	if (!(state.options & DPNI_LINK_OPT_HALF_DUPLEX))
> +	if (!(priv->link_state.options & DPNI_LINK_OPT_HALF_DUPLEX))
>  		link_settings->base.duplex = DUPLEX_FULL;
> -	link_settings->base.speed = state.rate;
> +	link_settings->base.speed = priv->link_state.rate;
>  
> -out:
> -	return err;
> +	return 0;

Hi Ioana

I think this patch can be broken up a bit, to help review.

It looks like this change to report state via priv->link_state should
be a separate patch. I think this change can be made without the pause
changes. That then makes the pause changes themselves simpler.

> +static void dpaa2_eth_get_pauseparam(struct net_device *net_dev,
> +				     struct ethtool_pauseparam *pause)
> +{
> +	struct dpaa2_eth_priv *priv = netdev_priv(net_dev);
> +	u64 link_options = priv->link_state.options;
> +
> +	pause->rx_pause = !!(link_options & DPNI_LINK_OPT_PAUSE);
> +	pause->tx_pause = pause->rx_pause ^
> +			  !!(link_options & DPNI_LINK_OPT_ASYM_PAUSE);

Since you don't support auto-neg, you should set pause->autoneg to
false. It probably already is set to false, by a memset, but setting
it explicitly is a form of documentation, this hardware only supports
forced pause configuration.

> +}
> +
> +static int dpaa2_eth_set_pauseparam(struct net_device *net_dev,
> +				    struct ethtool_pauseparam *pause)
> +{
> +	struct dpaa2_eth_priv *priv = netdev_priv(net_dev);
> +	struct dpni_link_cfg cfg = {0};
> +	int err;
> +
> +	if (!dpaa2_eth_has_pause_support(priv)) {
> +		netdev_info(net_dev, "No pause frame support for DPNI version < %d.%d\n",
> +			    DPNI_PAUSE_VER_MAJOR, DPNI_PAUSE_VER_MINOR);
> +		return -EOPNOTSUPP;
> +	}
> +
> +	if (pause->autoneg)
> +		netdev_err(net_dev, "Pause frame autoneg not supported\n");

And here you should return -EOPNOTSUPP. No need for an netdev_err. It
is not an error, you simply don't support it.

There is also the issue of what is the PHY doing? It would not be good
if the MAC is configured one way, but the PHY is advertising something
else. So it appears you have no control over the PHY. But i assume you
know what the PHY is actually doing? Does it advertise pause/asym
pause?

It might be, to keep things consistent, we only accept pause
configuration when auto-neg in general is disabled.

   Andrew