Messages in this thread | | | Subject | Re: [PATCH v2 4/4] net: ethoc: implement ethtool operations | From | Ben Hutchings <> | Date | Thu, 30 Jan 2014 01:59:36 +0000 |
| |
On Wed, 2014-01-29 at 10:00 +0400, Max Filippov wrote: > The following methods are implemented: > - get/set settings; > - get registers length/registers; > - get link state (standard implementation); > - get/set ring parameters; > - get timestamping info (standard implementation). [...] > --- a/drivers/net/ethernet/ethoc.c > +++ b/drivers/net/ethernet/ethoc.c > [...] > +static int ethoc_get_settings(struct net_device *dev, struct ethtool_cmd *cmd) > +{ > + struct ethoc *priv = netdev_priv(dev); > + struct phy_device *phydev = priv->phy; > + > + if (!phydev) > + return -ENODEV; > + > + return phy_ethtool_gset(phydev, cmd); > +} > + > +static int ethoc_set_settings(struct net_device *dev, struct ethtool_cmd *cmd) > +{ > + struct ethoc *priv = netdev_priv(dev); > + struct phy_device *phydev = priv->phy; > + > + if (!phydev) > + return -ENODEV; > + > + return phy_ethtool_sset(phydev, cmd); > +}
I think these should return -EOPNOTSUPP in the PHY-less case, just as if the set_settings pointer was not set.
[...] > +static void ethoc_get_ringparam(struct net_device *dev, > + struct ethtool_ringparam *ring) > +{ > + struct ethoc *priv = netdev_priv(dev); > + > + ring->rx_max_pending = priv->num_bd; > + ring->rx_mini_max_pending = 0; > + ring->rx_jumbo_max_pending = 0; > + ring->tx_max_pending = priv->num_bd; > + > + ring->rx_pending = priv->num_rx; > + ring->rx_mini_pending = 0; > + ring->rx_jumbo_pending = 0; > + ring->tx_pending = priv->num_tx; > +} > + > +static int ethoc_set_ringparam(struct net_device *dev, > + struct ethtool_ringparam *ring) > +{ > + struct ethoc *priv = netdev_priv(dev); > + > + if (netif_running(dev)) > + return -EBUSY;
This is unhelpful. Most implementations of this operation will restart the interface if it's running.
> + priv->num_tx = rounddown_pow_of_two(ring->tx_pending);
Range check?
> + priv->num_rx = priv->num_bd - priv->num_tx; > + if (priv->num_rx > ring->rx_pending) > + priv->num_rx = ring->rx_pending;
So the RX ring may only ever be shrunk?! Did you mean to compare with priv->num_bd instead?
> + return 0; > +} > + > +const struct ethtool_ops ethoc_ethtool_ops = {
static
> + .get_settings = ethoc_get_settings, > + .set_settings = ethoc_set_settings, > + .get_regs_len = ethoc_get_regs_len, > + .get_regs = ethoc_get_regs, > + .get_link = ethtool_op_get_link, > + .get_ringparam = ethoc_get_ringparam, > + .set_ringparam = ethoc_set_ringparam, > + .get_ts_info = ethtool_op_get_ts_info, > +}; [...]
-- Ben Hutchings It is a miracle that curiosity survives formal education. - Albert Einstein [unhandled content-type:application/pgp-signature] | |