| Message ID | 1390975218-13863-5-git-send-email-jcmvbkbc@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
Le 28/01/2014 22:00, Max Filippov a écrit : > 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). Ideally you should have one patch per ethtool callback that you implement just in case something happens to break, only the specific patch can reverted/referenced. > > Signed-off-by: Max Filippov <jcmvbkbc@gmail.com> > --- > Changes v1->v2: > - new patch. > > drivers/net/ethernet/ethoc.c | 85 ++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 85 insertions(+) > > diff --git a/drivers/net/ethernet/ethoc.c b/drivers/net/ethernet/ethoc.c > index 5854d41..2f8b174 100644 > --- a/drivers/net/ethernet/ethoc.c > +++ b/drivers/net/ethernet/ethoc.c > @@ -52,6 +52,7 @@ MODULE_PARM_DESC(buffer_size, "DMA buffer allocation size"); > #define ETH_HASH0 0x48 > #define ETH_HASH1 0x4c > #define ETH_TXCTRL 0x50 > +#define ETH_END 0x54 > > /* mode register */ > #define MODER_RXEN (1 << 0) /* receive enable */ > @@ -180,6 +181,7 @@ MODULE_PARM_DESC(buffer_size, "DMA buffer allocation size"); > * @membase: pointer to buffer memory region > * @dma_alloc: dma allocated buffer size > * @io_region_size: I/O memory region size > + * @num_bd: number of buffer descriptors > * @num_tx: number of send buffers > * @cur_tx: last send buffer written > * @dty_tx: last buffer actually sent > @@ -200,6 +202,7 @@ struct ethoc { > int dma_alloc; > resource_size_t io_region_size; > > + unsigned int num_bd; > unsigned int num_tx; > unsigned int cur_tx; > unsigned int dty_tx; > @@ -900,6 +903,86 @@ out: > return NETDEV_TX_OK; > } > > +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); > +} > + > +static int ethoc_get_regs_len(struct net_device *netdev) > +{ > + return ETH_END; > +} > + > +static void ethoc_get_regs(struct net_device *dev, struct ethtool_regs *regs, > + void *p) > +{ > + struct ethoc *priv = netdev_priv(dev); > + u32 *regs_buff = p; > + unsigned i; > + > + regs->version = 0; > + for (i = 0; i < ETH_END / sizeof(u32); ++i) > + regs_buff[i] = ethoc_read(priv, i * sizeof(u32)); > +} > + > +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; > + priv->num_tx = rounddown_pow_of_two(ring->tx_pending); > + priv->num_rx = priv->num_bd - priv->num_tx; > + if (priv->num_rx > ring->rx_pending) > + priv->num_rx = ring->rx_pending; > + return 0; > +} > + > +const struct ethtool_ops ethoc_ethtool_ops = { > + .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, > +}; > + > static const struct net_device_ops ethoc_netdev_ops = { > .ndo_open = ethoc_open, > .ndo_stop = ethoc_stop, > @@ -1028,6 +1111,7 @@ static int ethoc_probe(struct platform_device *pdev) > ret = -ENODEV; > goto error; > } > + priv->num_bd = num_bd; > /* num_tx must be a power of two */ > priv->num_tx = rounddown_pow_of_two(num_bd >> 1); > priv->num_rx = num_bd - priv->num_tx; > @@ -1148,6 +1232,7 @@ static int ethoc_probe(struct platform_device *pdev) > netdev->netdev_ops = ðoc_netdev_ops; > netdev->watchdog_timeo = ETHOC_TIMEOUT; > netdev->features |= 0; > + netdev->ethtool_ops = ðoc_ethtool_ops; > > /* setup NAPI */ > netif_napi_add(netdev, &priv->napi, ethoc_poll, 64); > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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, > +}; [...]
On Thu, Jan 30, 2014 at 5:59 AM, Ben Hutchings <ben@decadent.org.uk> wrote: > 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. Ok > [...] >> +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. Ok >> + priv->num_tx = rounddown_pow_of_two(ring->tx_pending); > > Range check? May there be requested more than ring->tx_max_pending that we indicated in the get_ringparam? >> + 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? First all non-TX descriptors are made RX, and if that's more than user requested I trim it. >> + return 0; >> +} >> + >> +const struct ethtool_ops ethoc_ethtool_ops = { > > static Ok
On Thu, 2014-01-30 at 07:04 +0400, Max Filippov wrote: > On Thu, Jan 30, 2014 at 5:59 AM, Ben Hutchings <ben@decadent.org.uk> wrote: > > On Wed, 2014-01-29 at 10:00 +0400, Max Filippov wrote: [...] > >> + priv->num_tx = rounddown_pow_of_two(ring->tx_pending); > > > > Range check? > > May there be requested more than ring->tx_max_pending that we > indicated in the get_ringparam? Yes, the ethtool core doesn't check that for you. > >> + 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? > > First all non-TX descriptors are made RX, and if that's more than user > requested I trim it. [...] OK, I get it. But it would be clearer if you used min(). Ben.
diff --git a/drivers/net/ethernet/ethoc.c b/drivers/net/ethernet/ethoc.c index 5854d41..2f8b174 100644 --- a/drivers/net/ethernet/ethoc.c +++ b/drivers/net/ethernet/ethoc.c @@ -52,6 +52,7 @@ MODULE_PARM_DESC(buffer_size, "DMA buffer allocation size"); #define ETH_HASH0 0x48 #define ETH_HASH1 0x4c #define ETH_TXCTRL 0x50 +#define ETH_END 0x54 /* mode register */ #define MODER_RXEN (1 << 0) /* receive enable */ @@ -180,6 +181,7 @@ MODULE_PARM_DESC(buffer_size, "DMA buffer allocation size"); * @membase: pointer to buffer memory region * @dma_alloc: dma allocated buffer size * @io_region_size: I/O memory region size + * @num_bd: number of buffer descriptors * @num_tx: number of send buffers * @cur_tx: last send buffer written * @dty_tx: last buffer actually sent @@ -200,6 +202,7 @@ struct ethoc { int dma_alloc; resource_size_t io_region_size; + unsigned int num_bd; unsigned int num_tx; unsigned int cur_tx; unsigned int dty_tx; @@ -900,6 +903,86 @@ out: return NETDEV_TX_OK; } +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); +} + +static int ethoc_get_regs_len(struct net_device *netdev) +{ + return ETH_END; +} + +static void ethoc_get_regs(struct net_device *dev, struct ethtool_regs *regs, + void *p) +{ + struct ethoc *priv = netdev_priv(dev); + u32 *regs_buff = p; + unsigned i; + + regs->version = 0; + for (i = 0; i < ETH_END / sizeof(u32); ++i) + regs_buff[i] = ethoc_read(priv, i * sizeof(u32)); +} + +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; + priv->num_tx = rounddown_pow_of_two(ring->tx_pending); + priv->num_rx = priv->num_bd - priv->num_tx; + if (priv->num_rx > ring->rx_pending) + priv->num_rx = ring->rx_pending; + return 0; +} + +const struct ethtool_ops ethoc_ethtool_ops = { + .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, +}; + static const struct net_device_ops ethoc_netdev_ops = { .ndo_open = ethoc_open, .ndo_stop = ethoc_stop, @@ -1028,6 +1111,7 @@ static int ethoc_probe(struct platform_device *pdev) ret = -ENODEV; goto error; } + priv->num_bd = num_bd; /* num_tx must be a power of two */ priv->num_tx = rounddown_pow_of_two(num_bd >> 1); priv->num_rx = num_bd - priv->num_tx; @@ -1148,6 +1232,7 @@ static int ethoc_probe(struct platform_device *pdev) netdev->netdev_ops = ðoc_netdev_ops; netdev->watchdog_timeo = ETHOC_TIMEOUT; netdev->features |= 0; + netdev->ethtool_ops = ðoc_ethtool_ops; /* setup NAPI */ netif_napi_add(netdev, &priv->napi, ethoc_poll, 64);
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). Signed-off-by: Max Filippov <jcmvbkbc@gmail.com> --- Changes v1->v2: - new patch. drivers/net/ethernet/ethoc.c | 85 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 85 insertions(+)