> -----Original Message-----
> From: Andrew Lunn <and...@lunn.ch>
> Sent: Friday, August 23, 2019 7:31 PM
> To: Ioana Ciocoi Radulescu <ruxandra.radule...@nxp.com>
> Cc: netdev@vger.kernel.org; da...@davemloft.net; Ioana Ciornei
> <ioana.cior...@nxp.com>
> Subject: Re: [PATCH net-next] dpaa2-eth: Add pause frame support
>
> > --- 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.
Agree, will change in v2
>
> > +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.
>
Ok.
> > +}
> > +
> > +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.
Ok
>
> 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.
Ah, this is an area in our driver that's a bit messy and complicated.
Like you said, we don't control the PHY, actually we only support
fixed-link PHYs for now. General autoneg should really be reported as
always off.
We're working to add proper support in this area, but until we manage
to do it I think it's best we just remove the possibility of users
changing the link settings via ethtool - it's code that shouldn't be
there (and firmware doesn't allow it anyway). I'll include this cleanup
in the next version.
Thanks a lot for the feedback,
Ioana
