| Message ID | ba3105df-74ae-4883-b9e9-d517036a73b3@gmail.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ethtool: switch EEE netlink interface to use EEE linkmode bitmaps | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
> static int ethtool_get_eee(struct net_device *dev, char __user *useraddr) > { > - struct ethtool_keee edata; > + struct ethtool_keee keee; > + struct ethtool_eee eee; > int rc; > > if (!dev->ethtool_ops->get_eee) > return -EOPNOTSUPP; > > - memset(&edata, 0, sizeof(struct ethtool_keee)); > - edata.cmd = ETHTOOL_GEEE; > - rc = dev->ethtool_ops->get_eee(dev, &edata); With the old code, the edata passed to the driver has edata.cmd set to ETHTOOL_GEEE. > - > + memset(&keee, 0, sizeof(keee)); > + rc = dev->ethtool_ops->get_eee(dev, &keee); Here, its not set. I don't know if it makes a difference, if any driver actually looks at it. If you reviewed all the drivers and think this is O.K, i would suggest a comment in the commit message explaining this. Andrew
> +static void eee_to_keee(struct ethtool_keee *keee, > + const struct ethtool_eee *eee) > +{ > + memset(keee, 0, sizeof(*keee)); > + > + keee->supported = eee->supported; > + keee->advertised = eee->advertised; > + keee->lp_advertised = eee->lp_advertised; > + keee->eee_active = eee->eee_active; > + keee->eee_enabled = eee->eee_enabled; > + keee->tx_lpi_enabled = eee->tx_lpi_enabled; > + keee->tx_lpi_timer = eee->tx_lpi_timer; Just to avoid surprises, i would also copy keee->cmd to eee->cmd. > +} > + > +static void keee_to_eee(struct ethtool_eee *eee, > + const struct ethtool_keee *keee) > +{ > + memset(eee, 0, sizeof(*eee)); > + > + eee->supported = keee->supported; > + eee->advertised = keee->advertised; > + eee->lp_advertised = keee->lp_advertised; > + eee->eee_active = keee->eee_active; > + eee->eee_enabled = keee->eee_enabled; > + eee->tx_lpi_enabled = keee->tx_lpi_enabled; > + eee->tx_lpi_timer = keee->tx_lpi_timer; Same here. Since reserved is not supposed to be used, not copying that is O.K. Andrew
On 07.01.2024 17:23, Andrew Lunn wrote: >> static int ethtool_get_eee(struct net_device *dev, char __user *useraddr) >> { >> - struct ethtool_keee edata; >> + struct ethtool_keee keee; >> + struct ethtool_eee eee; >> int rc; >> >> if (!dev->ethtool_ops->get_eee) >> return -EOPNOTSUPP; >> >> - memset(&edata, 0, sizeof(struct ethtool_keee)); >> - edata.cmd = ETHTOOL_GEEE; >> - rc = dev->ethtool_ops->get_eee(dev, &edata); > > With the old code, the edata passed to the driver has edata.cmd set to > ETHTOOL_GEEE. > >> - >> + memset(&keee, 0, sizeof(keee)); >> + rc = dev->ethtool_ops->get_eee(dev, &keee); > > Here, its not set. I don't know if it makes a difference, if any > driver actually looks at it. If you reviewed all the drivers and think > this is O.K, i would suggest a comment in the commit message > explaining this. > I saw your comment on patch 3, just to explain this a little bit more: The cmd field is set for ioctl only (by userspace ethtool), it's not populated for netlink. Therefore no driver should use it. Also it wouldn't make sense. If get_eee() is called, then cmd must have been set to ETHTOOL_GEEE (for ioctl). See __dev_ethtool(). We could even remove setting cmd here. Userspace ethtool isn't interested in the cmd field of the GEEE response. - if (copy_to_user(useraddr, &edata, sizeof(edata))) + keee_to_eee(&eee, &keee); + eee.cmd = ETHTOOL_GEEE; + if (copy_to_user(useraddr, &eee, sizeof(eee))) return -EFAULT; > Andrew Heiner
diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c index 099d02e7d..9222fbeeb 100644 --- a/net/ethtool/ioctl.c +++ b/net/ethtool/ioctl.c @@ -1505,22 +1505,51 @@ static int ethtool_set_wol(struct net_device *dev, char __user *useraddr) return 0; } +static void eee_to_keee(struct ethtool_keee *keee, + const struct ethtool_eee *eee) +{ + memset(keee, 0, sizeof(*keee)); + + keee->supported = eee->supported; + keee->advertised = eee->advertised; + keee->lp_advertised = eee->lp_advertised; + keee->eee_active = eee->eee_active; + keee->eee_enabled = eee->eee_enabled; + keee->tx_lpi_enabled = eee->tx_lpi_enabled; + keee->tx_lpi_timer = eee->tx_lpi_timer; +} + +static void keee_to_eee(struct ethtool_eee *eee, + const struct ethtool_keee *keee) +{ + memset(eee, 0, sizeof(*eee)); + + eee->supported = keee->supported; + eee->advertised = keee->advertised; + eee->lp_advertised = keee->lp_advertised; + eee->eee_active = keee->eee_active; + eee->eee_enabled = keee->eee_enabled; + eee->tx_lpi_enabled = keee->tx_lpi_enabled; + eee->tx_lpi_timer = keee->tx_lpi_timer; +} + static int ethtool_get_eee(struct net_device *dev, char __user *useraddr) { - struct ethtool_keee edata; + struct ethtool_keee keee; + struct ethtool_eee eee; int rc; if (!dev->ethtool_ops->get_eee) return -EOPNOTSUPP; - memset(&edata, 0, sizeof(struct ethtool_keee)); - edata.cmd = ETHTOOL_GEEE; - rc = dev->ethtool_ops->get_eee(dev, &edata); - + memset(&keee, 0, sizeof(keee)); + rc = dev->ethtool_ops->get_eee(dev, &keee); if (rc) return rc; - if (copy_to_user(useraddr, &edata, sizeof(edata))) + keee_to_eee(&eee, &keee); + eee.cmd = ETHTOOL_GEEE; + if (copy_to_user(useraddr, &eee, sizeof(eee))) return -EFAULT; return 0; @@ -1528,16 +1557,18 @@ static int ethtool_get_eee(struct net_device *dev, char __user *useraddr) static int ethtool_set_eee(struct net_device *dev, char __user *useraddr) { - struct ethtool_keee edata; + struct ethtool_keee keee; + struct ethtool_eee eee; int ret; if (!dev->ethtool_ops->set_eee) return -EOPNOTSUPP; - if (copy_from_user(&edata, useraddr, sizeof(edata))) + if (copy_from_user(&eee, useraddr, sizeof(eee))) return -EFAULT; - ret = dev->ethtool_ops->set_eee(dev, &edata); + eee_to_keee(&keee, &eee); + ret = dev->ethtool_ops->set_eee(dev, &keee); if (!ret) ethtool_notify(dev, ETHTOOL_MSG_EEE_NTF, NULL); return ret;
In order to later extend struct ethtool_keee, we have to decouple it from the userspace format represented by struct ethtool_eee. Therefore switch back to struct ethtool_eee, representing the userspace format, and add conversion between ethtool_eee and ethtool_keee. Struct ethtool_keee will be changed in follow-up patches, therefore don't do a *keee = *eee here. Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- net/ethtool/ioctl.c | 49 ++++++++++++++++++++++++++++++++++++--------- 1 file changed, 40 insertions(+), 9 deletions(-)