On Mon, 2015-11-30 at 14:05 -0800, David Decotigny wrote:

From: David Decotigny <redacted>

This patch defines a new ETHTOOL_GSETTINGS/SSETTINGS API, handled by
the new get_ksettings/set_ksettings callbacks. This API provides
support for most legacy ethtool_cmd fields, adds support for larger
link mode masks (up to 4064 bits, variable length), and removes
ethtool_cmd deprecated fields (transceiver/maxrxpkt/maxtxpkt).

As you have to introduce new commands and a new structure, please
include the word 'link' in their names.

[...]

'indice'?  Shoudn't this be 'index' or 'mode'?

+/* number of link mode bits handled internally by kernel */
+#define __ETHTOOL_LINK_MODE_MASK_NBITS (__ETHTOOL_LINK_MODE_LAST+1)

Spaces around the + sign.

+typedef struct {
+	unsigned long mask[BITS_TO_LONGS(__ETHTOOL_LINK_MODE_MASK_NBITS)];
+} ethtool_link_mode_mask_t;

checkpatch claims you shouldn't introduce such typedefs.

[...]

+/**
+ * struct ethtool_settings - link control and status
+ * This structure deprecates struct %ethtool_cmd.

We do the deprecating; the structures are purely passive.

[...]

+ * Deprecated fields should be ignored by both users and drivers.

Delete this last paragraph.

[...]

Please delete this TODO and all the similar ones; we don't remove
userland APIs just because they're deprecated.

[...]

+/* number of 32-bit words to store the user's link mode bitmaps */
+#define __ETHTOOL_LINK_MODE_MASK_NU32			\
+	((__ETHTOOL_LINK_MODE_MASK_NBITS + 31) / 32)

Should use DIV_ROUND_UP().

+/* layout of the struct passed from/to userland */
+struct ethtool_usettings {
+	struct ethtool_settings parent;
+	struct {
+		__u32 supported[__ETHTOOL_LINK_MODE_MASK_NU32] __aligned(4);
+		__u32 advertising[__ETHTOOL_LINK_MODE_MASK_NU32] __aligned(4);
+		__u32 lp_advertising[
+			__ETHTOOL_LINK_MODE_MASK_NU32] __aligned(4);

Why __aligned(4)?  Do you have any reason to think that some padding
might be added otherwise?

[...]

+#if BITS_PER_LONG == 64
+static unsigned long _shl32(__u32 v)
+{
+	return ((unsigned long)v) << 32;
+}
+#endif
+
+/* convert a user's __u32[] bitmap in user space to a kernel internal
+ * bitmap. return 0 on success, errno on error. this assumes that
+ * link_mode_masks_nwords was already verified
+ */
+static int load_ksettings_from_user(struct ethtool_ksettings *to,
+				    const void __user *from)
+{
+	struct ethtool_usettings usettings;
+#if BITS_PER_LONG != 32
+	unsigned i;
+#endif
+
+	if (copy_from_user(&usettings, from, sizeof(usettings)))
+		return -EFAULT;
+
+	/* make sure we didn't receive garbage between last allowed bit
+	 * and end of last u32 word
+	 */
+	if (__ETHTOOL_LINK_MODE_MASK_NBITS % 32) {
+		const u32 allowed = (
+			1U << (__ETHTOOL_LINK_MODE_MASK_NBITS % 32)) - 1;
+		if (usettings.link_modes.supported[
+			    __ETHTOOL_LINK_MODE_MASK_NU32 - 1] & ~allowed)
+			return -EINVAL;
+		if (usettings.link_modes.advertising[
+			    __ETHTOOL_LINK_MODE_MASK_NU32 - 1] & ~allowed)
+			return -EINVAL;
+		if (usettings.link_modes.lp_advertising[
+			    __ETHTOOL_LINK_MODE_MASK_NU32 - 1] & ~allowed)
+			return -EINVAL;
+	}
+
+#if BITS_PER_LONG == 32
+	compiletime_assert(sizeof(*to) == sizeof(usettings),
+			   "sizeof(ulong) != 32");
+	memcpy(to, &usettings, sizeof(*to));
+#elif BITS_PER_LONG == 64
+	memset(to, 0, sizeof(*to));

This memset() looks redundant.

+	memcpy(&to->parent, &usettings.parent, sizeof(to->parent));
+	for (i = 0 ; i < __ETHTOOL_LINK_MODE_MASK_NU32 ; ++i) {
+		if (0 == (i & 1)) {
+			to->link_modes.supported.mask[i >> 1]
+				= usettings.link_modes.supported[i];
+			to->link_modes.advertising.mask[i >> 1]
+				= usettings.link_modes.advertising[i];
+			to->link_modes.lp_advertising.mask[i >> 1]
+				= usettings.link_modes.lp_advertising[i];
+		} else {
+			to->link_modes.supported.mask[i >> 1] |= _shl32(
+				usettings.link_modes.supported[i]);
+			to->link_modes.advertising.mask[i >> 1] |= _shl32(
+				usettings.link_modes.advertising[i]);
+			to->link_modes.lp_advertising.mask[i >> 1] |= _shl32(
+				usettings.link_modes.lp_advertising[i]);
+		}
+	}
+#else
+# error "unsupported ulong width"
+#endif
+	return 0;
+}

I think the array conversion ought to be a separate function that you
can call 3 times here, rather than repeating it here.  In fact that
could be a general function in lib/bitmap.c.

Similarly for the opposite conversion below.

[...]

 static int ethtool_get_settings(struct net_device *dev, void __user *useraddr)
 {
-	int err;
 	struct ethtool_cmd cmd;
 
-	err = __ethtool_get_settings(dev, &cmd);
-	if (err < 0)
-		return err;
+	ASSERT_RTNL();
+
+	if (dev->ethtool_ops->get_ksettings) {
+		/* First, use ksettings API if it is supported */
+		int err;
+		struct ethtool_ksettings ksettings;
+
+		memset(&ksettings, 0, sizeof(ksettings));
+		err = dev->ethtool_ops->get_ksettings(dev, &ksettings);
+		if (err < 0)
+			return err;
+		if (!convert_ksettings_to_legacy_settings(&cmd, &ksettings)) {
+			static int __warned;
+
+			/* not all bitmaps could be translated
+			 * acurately to legacy API: print warning with
+			 * netdev name, but does still succeed
+			 */
+			if (!__warned)
+				netdev_warn(dev, "please upgrade ethtool\n");

ethtool isn't the only program that uses this API, not by a long way. 
And since it's the program at fault, not the device, it doesn't make a
lot of sense to use netdev_warn().

So I suggest a message more like that in warn_legacy_capability_use()
in kernel/capability.c.

[...]

 static int ethtool_set_settings(struct net_device *dev, void __user *useraddr)
 {
 	struct ethtool_cmd cmd;
 
-	if (!dev->ethtool_ops->set_settings)
-		return -EOPNOTSUPP;
+	ASSERT_RTNL();
 
 	if (copy_from_user(&cmd, useraddr, sizeof(cmd)))
 		return -EFAULT;
 
+	/* first, try new %ethtool_ksettings API. */
+	if (dev->ethtool_ops->set_ksettings) {
+		struct ethtool_ksettings ksettings;
+
+		if (!convert_legacy_settings_to_ksettings(&ksettings, &cmd)) {
+			static int __warned;
+
+			/* rejecting setting deprecated fields
+			 * transceiver/maxtxpkt/maxrxpkt
+			 */
+			if (!__warned)
+				netdev_warn(dev, "please upgrade ethtool");

I don't think this makes sense - it's not as if ethtool will
automatically try to set these without it being explicitly requested by
the user.  Just return -EINVAL without logging anything.

+			__warned = 1;
+			return -EINVAL;
+		}

[...]

Ben.

-- 
Ben Hutchings
Theory and practice are closer in theory than in practice.
                                - John Levine, moderator of comp.compilers