Re: [PATCH v2 RFC 2/5] ethtool: switch back from ethtool_keee to ethtool_eee for ioctl

On 07.01.2024 17:23, Andrew Lunn wrote:

quoted

 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.

quoted

-
+	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