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

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] [day] [month] [year] [list]

Message-ID: <AM0PR04MB49944F03886D8E147485947394A10@AM0PR04MB4994.eurprd04.prod.outlook.com>
Date:   Mon, 26 Aug 2019 13:00:22 +0000
From:   Ioana Ciocoi Radulescu <ruxandra.radulescu@....com>
To:     Andrew Lunn <andrew@...n.ch>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "davem@...emloft.net" <davem@...emloft.net>,
        Ioana Ciornei <ioana.ciornei@....com>
Subject: RE: [PATCH net-next] dpaa2-eth: Add pause frame support

> -----Original Message-----
> From: Andrew Lunn <andrew@...n.ch>
> Sent: Friday, August 23, 2019 7:31 PM
> To: Ioana Ciocoi Radulescu <ruxandra.radulescu@....com>
> Cc: netdev@...r.kernel.org; davem@...emloft.net; Ioana Ciornei
> <ioana.ciornei@....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