[PATCH 1/2][ethtool] ethtool: sync ethtool-copy.h: adds support for EDPD

[PATCH 1/2][ethtool] ethtool: sync ethtool-copy.h: adds support for EDPD

[Alexandru Ardelean]

This change syncs the `ethtool-copy.h` file with Linux net-next to add
support for Energy Detect Powerdown control via phy tunable.

net-next commit/sync-point:
commit 1bab8d4c488be22d57f9dd09968c90a0ddc413bf
Merge: 990925fad5c2 00b368502d18
Author: David S. Miller <davem@davemloft.net>
Date:   Tue Sep 17 23:51:10 2019 +0200

    Merge ra.kernel.org:/pub/scm/linux/kernel/git/netdev/net

    Pull in bug fixes from 'net' tree for the merge window.

    Signed-off-by: David S. Miller <davem@davemloft.net>

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 ethtool-copy.h | 30 ++++++++++++++++++++++++++----
 1 file changed, 26 insertions(+), 4 deletions(-)

diff --git a/ethtool-copy.h b/ethtool-copy.h
index ad16e8f..9afd2e6 100644
--- a/ethtool-copy.h
+++ b/ethtool-copy.h
@@ -257,10 +257,32 @@ struct ethtool_tunable {
 #define ETHTOOL_PHY_FAST_LINK_DOWN_ON	0
 #define ETHTOOL_PHY_FAST_LINK_DOWN_OFF	0xff
 
+/* Energy Detect Power Down (EDPD) is a feature supported by some PHYs, where
+ * the PHY's RX & TX blocks are put into a low-power mode when there is no
+ * link detected (typically cable is un-plugged). For RX, only a minimal
+ * link-detection is available, and for TX the PHY wakes up to send link pulses
+ * to avoid any lock-ups in case the peer PHY may also be running in EDPD mode.
+ *
+ * Some PHYs may support configuration of the wake-up interval for TX pulses,
+ * and some PHYs may support only disabling TX pulses entirely. For the latter
+ * a special value is required (ETHTOOL_PHY_EDPD_NO_TX) so that this can be
+ * configured from userspace (should the user want it).
+ *
+ * The interval units for TX wake-up are in milliseconds, since this should
+ * cover a reasonable range of intervals:
+ *  - from 1 millisecond, which does not sound like much of a power-saver
+ *  - to ~65 seconds which is quite a lot to wait for a link to come up when
+ *    plugging a cable
+ */
+#define ETHTOOL_PHY_EDPD_DFLT_TX_MSECS		0xffff
+#define ETHTOOL_PHY_EDPD_NO_TX			0xfffe
+#define ETHTOOL_PHY_EDPD_DISABLE		0
+
 enum phy_tunable_id {
 	ETHTOOL_PHY_ID_UNSPEC,
 	ETHTOOL_PHY_DOWNSHIFT,
 	ETHTOOL_PHY_FAST_LINK_DOWN,
+	ETHTOOL_PHY_EDPD,
 	/*
 	 * Add your fresh new phy tunable attribute above and remember to update
 	 * phy_tunable_strings[] in net/core/ethtool.c
@@ -1481,8 +1503,8 @@ enum ethtool_link_mode_bit_indices {
 	ETHTOOL_LINK_MODE_200000baseLR4_ER4_FR4_Full_BIT = 64,
 	ETHTOOL_LINK_MODE_200000baseDR4_Full_BIT	 = 65,
 	ETHTOOL_LINK_MODE_200000baseCR4_Full_BIT	 = 66,
-	ETHTOOL_LINK_MODE_100baseT1_Full_BIT             = 67,
-	ETHTOOL_LINK_MODE_1000baseT1_Full_BIT            = 68,
+	ETHTOOL_LINK_MODE_100baseT1_Full_BIT		 = 67,
+	ETHTOOL_LINK_MODE_1000baseT1_Full_BIT		 = 68,
 
 	/* must be last entry */
 	__ETHTOOL_LINK_MODE_MASK_NBITS
@@ -1712,8 +1734,8 @@ static __inline__ int ethtool_validate_duplex(__u8 duplex)
 #define ETH_MODULE_SFF_8436		0x4
 #define ETH_MODULE_SFF_8436_LEN		256
 
-#define ETH_MODULE_SFF_8636_MAX_LEN	640
-#define ETH_MODULE_SFF_8436_MAX_LEN	640
+#define ETH_MODULE_SFF_8636_MAX_LEN     640
+#define ETH_MODULE_SFF_8436_MAX_LEN     640
 
 /* Reset flags */
 /* The reset() operation must clear the flags for the components which
-- 
2.20.1

[PATCH 2/2][ethtool] ethtool: implement support for Energy Detect Power Down

[Alexandru Ardelean]

This change adds control for enabling/disabling Energy Detect Power Down
mode, as well as configuring wake-up intervals for TX pulses, via the new
ETHTOOL_PHY_EDPD control added in PHY tunable, in the kernel.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 ethtool.8.in | 28 +++++++++++++++++
 ethtool.c    | 87 +++++++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 111 insertions(+), 4 deletions(-)

diff --git a/ethtool.8.in b/ethtool.8.in
index cd3be91..609a05a 100644
--- a/ethtool.8.in
+++ b/ethtool.8.in
@@ -362,11 +362,17 @@ ethtool \- query or control network driver and hardware settings
 .A1 on off
 .BN msecs
 .RB ]
+.RB [
+.B energy\-detect\-power\-down
+.A1 on off
+.BN msecs
+.RB ]
 .HP
 .B ethtool \-\-get\-phy\-tunable
 .I devname
 .RB [ downshift ]
 .RB [ fast-link-down ]
+.RB [ energy-detect-power-down ]
 .HP
 .B ethtool \-\-reset
 .I devname
@@ -1100,6 +1106,24 @@ lB	l.
 	Sets the period after which the link is reported as down. Note that the PHY may choose
 	the closest supported value. Only on reading back the tunable do you get the actual value.
 .TE
+.TP
+.A2 energy-detect-power-down on off
+Specifies whether Energy Detect Power Down (EDPD) should be enabled (if supported).
+This will put the RX and TX circuit blocks into a low power mode, and the PHY will
+wake up periodically to send link pulses to avoid any lock-up situation with a peer
+PHY that may also have EDPD enabled. By default, this setting will also enable the
+periodic transmission of TX pulses.
+.TS
+nokeep;
+lB	l.
+.BI msecs \ N
+	Some PHYs support configuration of the wake-up interval to send TX pulses.
+	This setting allows the control of this interval, and 0 disables TX pulses
+	if the PHY supports this. Disabling TX pulses can create a lock-up situation
+	where neither of the PHYs wakes the other one. If unspecified the default
+	value (in milliseconds) will be used by the PHY.
+.TE
+.TP
 .PD
 .RE
 .TP
@@ -1122,6 +1146,10 @@ Some PHYs support a Fast Link Down Feature and may allow configuration of the de
 before a broken link is reported as being down.
 
 Gets the PHY Fast Link Down status / period.
+.TP
+.B energy\-detect\-power\-down
+Gets the current configured setting for Energy Detect Power Down (if supported).
+
 .RE
 .TP
 .B \-\-reset
diff --git a/ethtool.c b/ethtool.c
index c0e2903..45b5478 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -4897,6 +4897,30 @@ static int do_get_phy_tunable(struct cmd_context *ctx)
 		else
 			fprintf(stdout, "Fast Link Down enabled, %d msecs\n",
 				cont.msecs);
+	} else if (!strcmp(argp[0], "energy-detect-power-down")) {
+		struct {
+			struct ethtool_tunable ds;
+			u16 msecs;
+		} cont;
+
+		cont.ds.cmd = ETHTOOL_PHY_GTUNABLE;
+		cont.ds.id = ETHTOOL_PHY_EDPD;
+		cont.ds.type_id = ETHTOOL_TUNABLE_U16;
+		cont.ds.len = 2;
+		if (send_ioctl(ctx, &cont.ds) < 0) {
+			perror("Cannot Get PHY Energy Detect Power Down value");
+			return 87;
+		}
+
+		if (cont.msecs == ETHTOOL_PHY_EDPD_DISABLE)
+			fprintf(stdout, "Energy Detect Power Down: disabled\n");
+		else if (cont.msecs == ETHTOOL_PHY_EDPD_NO_TX)
+			fprintf(stdout,
+				"Energy Detect Power Down: enabled, TX disabled\n");
+		else
+			fprintf(stdout,
+				"Energy Detect Power Down: enabled, TX %u msecs\n",
+				cont.msecs);
 	} else {
 		exit_bad_args();
 	}
@@ -5018,7 +5042,8 @@ static int parse_named_bool(struct cmd_context *ctx, const char *name, u8 *on)
 	return 1;
 }
 
-static int parse_named_u8(struct cmd_context *ctx, const char *name, u8 *val)
+static int parse_named_uint(struct cmd_context *ctx, const char *name,
+			    void *val, enum tunable_type_id type_id)
 {
 	if (ctx->argc < 2)
 		return 0;
@@ -5026,7 +5051,16 @@ static int parse_named_u8(struct cmd_context *ctx, const char *name, u8 *val)
 	if (strcmp(*ctx->argp, name))
 		return 0;
 
-	*val = get_uint_range(*(ctx->argp + 1), 0, 0xff);
+	switch (type_id) {
+	case ETHTOOL_TUNABLE_U8:
+		*(u8 *)val = get_uint_range(*(ctx->argp + 1), 0, 0xff);
+		break;
+	case ETHTOOL_TUNABLE_U16:
+		*(u16 *)val = get_uint_range(*(ctx->argp + 1), 0, 0xffff);
+		break;
+	default:
+		return 0;
+	}
 
 	ctx->argc -= 2;
 	ctx->argp += 2;
@@ -5034,6 +5068,16 @@ static int parse_named_u8(struct cmd_context *ctx, const char *name, u8 *val)
 	return 1;
 }
 
+static int parse_named_u8(struct cmd_context *ctx, const char *name, u8 *val)
+{
+	return parse_named_uint(ctx, name, val, ETHTOOL_TUNABLE_U8);
+}
+
+static int parse_named_u16(struct cmd_context *ctx, const char *name, u16 *val)
+{
+	return parse_named_uint(ctx, name, val, ETHTOOL_TUNABLE_U16);
+}
+
 static int do_set_phy_tunable(struct cmd_context *ctx)
 {
 	int err = 0;
@@ -5041,6 +5085,8 @@ static int do_set_phy_tunable(struct cmd_context *ctx)
 	u8 ds_changed = 0, ds_has_cnt = 0, ds_enable = 0;
 	u8 fld_changed = 0, fld_enable = 0;
 	u8 fld_msecs = ETHTOOL_PHY_FAST_LINK_DOWN_ON;
+	u8 edpd_changed = 0, edpd_enable = 0;
+	u16 edpd_tx_interval = ETHTOOL_PHY_EDPD_DFLT_TX_MSECS;
 
 	/* Parse arguments */
 	if (parse_named_bool(ctx, "downshift", &ds_enable)) {
@@ -5050,6 +5096,11 @@ static int do_set_phy_tunable(struct cmd_context *ctx)
 		fld_changed = 1;
 		if (fld_enable)
 			parse_named_u8(ctx, "msecs", &fld_msecs);
+	} else if (parse_named_bool(ctx, "energy-detect-power-down",
+				    &edpd_enable)) {
+		edpd_changed = 1;
+		if (edpd_enable)
+			parse_named_u16(ctx, "msecs", &edpd_tx_interval);
 	} else {
 		exit_bad_args();
 	}
@@ -5074,6 +5125,16 @@ static int do_set_phy_tunable(struct cmd_context *ctx)
 			fld_msecs = ETHTOOL_PHY_FAST_LINK_DOWN_OFF;
 		else if (fld_msecs == ETHTOOL_PHY_FAST_LINK_DOWN_OFF)
 			exit_bad_args();
+	} else if (edpd_changed) {
+		if (!edpd_enable)
+			edpd_tx_interval = ETHTOOL_PHY_EDPD_DISABLE;
+		else if (edpd_tx_interval == 0)
+			edpd_tx_interval = ETHTOOL_PHY_EDPD_NO_TX;
+		else if (edpd_tx_interval > ETHTOOL_PHY_EDPD_NO_TX) {
+			fprintf(stderr, "'msecs' max value is %d.\n",
+				(ETHTOOL_PHY_EDPD_NO_TX - 1));
+			exit_bad_args();
+		}
 	}
 
 	/* Do it */
@@ -5109,6 +5170,22 @@ static int do_set_phy_tunable(struct cmd_context *ctx)
 			perror("Cannot Set PHY Fast Link Down value");
 			err = 87;
 		}
+	} else if (edpd_changed) {
+		struct {
+			struct ethtool_tunable fld;
+			u16 msecs;
+		} cont;
+
+		cont.fld.cmd = ETHTOOL_PHY_STUNABLE;
+		cont.fld.id = ETHTOOL_PHY_EDPD;
+		cont.fld.type_id = ETHTOOL_TUNABLE_U16;
+		cont.fld.len = 2;
+		cont.msecs = edpd_tx_interval;
+		err = send_ioctl(ctx, &cont.fld);
+		if (err < 0) {
+			perror("Cannot Set PHY Energy Detect Power Down");
+			err = 87;
+		}
 	}
 
 	return err;
@@ -5361,10 +5438,12 @@ static const struct option {
 	  "		[ tx-timer %d ]\n"},
 	{ "--set-phy-tunable", 1, do_set_phy_tunable, "Set PHY tunable",
 	  "		[ downshift on|off [count N] ]\n"
-	  "		[ fast-link-down on|off [msecs N] ]\n"},
+	  "		[ fast-link-down on|off [msecs N] ]\n"
+	  "		[ energy-detect-power-down on|off [msecs N] ]\n"},
 	{ "--get-phy-tunable", 1, do_get_phy_tunable, "Get PHY tunable",
 	  "		[ downshift ]\n"
-	  "		[ fast-link-down ]\n"},
+	  "		[ fast-link-down ]\n"
+	  "		[ energy-detect-power-down ]\n"},
 	{ "--reset", 1, do_reset, "Reset components",
 	  "		[ flags %x ]\n"
 	  "		[ mgmt ]\n"
-- 
2.20.1

Re: [PATCH 2/2][ethtool] ethtool: implement support for Energy Detect Power Down

[Andrew Lunn]

> -static int parse_named_u8(struct cmd_context *ctx, const char *name, u8 *val)
> +static int parse_named_uint(struct cmd_context *ctx, const char *name,
> +			    void *val, enum tunable_type_id type_id)
>  {
>  	if (ctx->argc < 2)
>  		return 0;
> @@ -5026,7 +5051,16 @@ static int parse_named_u8(struct cmd_context *ctx, const char *name, u8 *val)
>  	if (strcmp(*ctx->argp, name))
>  		return 0;
>  
> -	*val = get_uint_range(*(ctx->argp + 1), 0, 0xff);
> +	switch (type_id) {
> +	case ETHTOOL_TUNABLE_U8:
> +		*(u8 *)val = get_uint_range(*(ctx->argp + 1), 0, 0xff);
> +		break;
> +	case ETHTOOL_TUNABLE_U16:
> +		*(u16 *)val = get_uint_range(*(ctx->argp + 1), 0, 0xffff);

I personally don't like these casts. Could you refactor this code in
some other way to avoid them. Make the parse_named_u8()
parse_named_u16() a bit fatter, and the shared code a bit slimmer?

Thanks
	Andrew

Re: [PATCH 2/2][ethtool] ethtool: implement support for Energy Detect Power Down

[Ardelean, Alexandru]

On Thu, 2019-09-19 at 16:00 +0200, Andrew Lunn wrote:
> [External]
> 
> > -static int parse_named_u8(struct cmd_context *ctx, const char *name,
> > u8 *val)
> > +static int parse_named_uint(struct cmd_context *ctx, const char *name,
> > +			    void *val, enum tunable_type_id type_id)
> >  {
> >  	if (ctx->argc < 2)
> >  		return 0;
> > @@ -5026,7 +5051,16 @@ static int parse_named_u8(struct cmd_context
> > *ctx, const char *name, u8 *val)
> >  	if (strcmp(*ctx->argp, name))
> >  		return 0;
> >  
> > -	*val = get_uint_range(*(ctx->argp + 1), 0, 0xff);
> > +	switch (type_id) {
> > +	case ETHTOOL_TUNABLE_U8:
> > +		*(u8 *)val = get_uint_range(*(ctx->argp + 1), 0, 0xff);
> > +		break;
> > +	case ETHTOOL_TUNABLE_U16:
> > +		*(u16 *)val = get_uint_range(*(ctx->argp + 1), 0, 0xffff);
> 
> I personally don't like these casts. Could you refactor this code in
> some other way to avoid them. Make the parse_named_u8()
> parse_named_u16() a bit fatter, and the shared code a bit slimmer?
> 

Sure thing.
V2 coming shortly.

> Thanks
> 	Andrew