[RFC] Support for unidirectional ethernet links

[RFC] Support for unidirectional ethernet links

[Kyle Moffett]

Hello,

I'm in the process of dusting off some old unidirectional fiber link
patches as part of support for a new custom hardware chassis.

Specifically, a critical piece of the hardware is unidirectional fiber
connections between independent computers, with data transmission
performed using UDP to a static ARP entry.

These first 2 RFC patches provide support for off-the-shelf gigabit
sky2 boards.  I also have another fairly trivial patch to "ethtool"
which enables it to configure the new ethtool values.

If these patches are acceptable, I'm going to polish up and refine a
more invasive series which cleans up and extends some of the PHY
ethtool handling to allow PHYs to handle the unidirectional modes in
a driver-specific way.

Cheers,
Kyle Moffett

[PATCH 1/2] ethtool.h: Add #defines for unidirectional ethernet duplex

[Kyle Moffett]

A large variety of fiber ethernet PHYs and some copper ethernet PHYs
support forced "unidirectional link" modes.  Some signalling modes are
designed for last-mile ethernet plants while others are designed for
strict security isolation (fiber with no return-path).

In order to configure those kinds of forced modes from userspace, we
need to add additional options to the "ethtool" interface.  As such
"unidirectional" ethernet modes most closely resemble ethernet "duplex",
we add two additional DUPLEX_* defines to the ethtool interface:

  #define DUPLEX_TXONLY 0x02
  #define DUPLEX_RXONLY 0x03

Most ethernet PHYs will still need to be configured internally in a mode
with autoneg off and duplex full, except for a few model-specific PHY
register adjustments.

Most PHYs can simply use a regular forced-mode for rx-only configuration,
but it may be useful in the ethernet driver to completely disable the TX
queues or similar.

Signed-off-by: Kyle Moffett <Kyle.D.Moffett@boeing.com>
---
 drivers/net/phy/phy.c   |   23 ++++++++++++++++-------
 include/linux/ethtool.h |    4 +++-
 2 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 64be466..1103a80 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -44,14 +44,23 @@
  */
 void phy_print_status(struct phy_device *phydev)
 {
-	pr_info("PHY: %s - Link is %s", dev_name(&phydev->dev),
-			phydev->link ? "Up" : "Down");
-	if (phydev->link)
-		printk(" - %d/%s", phydev->speed,
-				DUPLEX_FULL == phydev->duplex ?
-				"Full" : "Half");
-
-	printk("\n");
+	const char *strduplex = "Unknown";
+
+	/* Not much to show if the link is down */
+	if (!phydev->link) {
+		pr_info("PHY: %s - Link is Down\n", dev_name(&phydev->dev));
+		return;
+	}
+
+	/* Otherwise we need to print out speed and duplex */
+	switch (phydev->duplex) {
+		case DUPLEX_HALF:   strduplex = "Half";    break;
+		case DUPLEX_FULL:   strduplex = "Full";    break;
+		case DUPLEX_TXONLY: strduplex = "TX-Only"; break;
+		case DUPLEX_RXONLY: strduplex = "RX-Only"; break;
+	}
+	pr_info("PHY: %s - Link is Up - %d/%s\n", dev_name(&phydev->dev),
+			phydev->speed, strduplex);
 }
 EXPORT_SYMBOL(phy_print_status);
 
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index b4207ca..ccf2e32 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -703,9 +703,11 @@ struct ethtool_ops {
 #define SPEED_2500		2500
 #define SPEED_10000		10000
 
-/* Duplex, half or full. */
+/* Duplex: half/full or unidirectional communication */
 #define DUPLEX_HALF		0x00
 #define DUPLEX_FULL		0x01
+#define DUPLEX_TXONLY		0x02
+#define DUPLEX_RXONLY		0x03
 
 /* Which connector port. */
 #define PORT_TP		0x00
-- 
1.7.1

Re: [PATCH 1/2] ethtool.h: Add #defines for unidirectional ethernet duplex

[David Miller]

From: Kyle Moffett <Kyle.D.Moffett@boeing.com>
Date: Fri, 27 Aug 2010 15:42:17 -0400

> A large variety of fiber ethernet PHYs and some copper ethernet PHYs
> support forced "unidirectional link" modes.  Some signalling modes are
> designed for last-mile ethernet plants while others are designed for
> strict security isolation (fiber with no return-path).
> 
> In order to configure those kinds of forced modes from userspace, we
> need to add additional options to the "ethtool" interface.  As such
> "unidirectional" ethernet modes most closely resemble ethernet "duplex",
> we add two additional DUPLEX_* defines to the ethtool interface:
> 
>   #define DUPLEX_TXONLY 0x02
>   #define DUPLEX_RXONLY 0x03
> 
> Most ethernet PHYs will still need to be configured internally in a mode
> with autoneg off and duplex full, except for a few model-specific PHY
> register adjustments.
> 
> Most PHYs can simply use a regular forced-mode for rx-only configuration,
> but it may be useful in the ethernet driver to completely disable the TX
> queues or similar.
> 
> Signed-off-by: Kyle Moffett <Kyle.D.Moffett@boeing.com>

A fine idea, but you're really going to have to audit all of the networking
drivers to clean up the existing uses that assume this thing is a bitmap
and that there are only essentially two values ('0' and '1').  For example:

drivers/net/e1000/e1000_main.c:	case SPEED_10 + DUPLEX_HALF:
drivers/net/e1000/e1000_main.c:	case SPEED_100 + DUPLEX_HALF:
drivers/net/e1000/e1000_main.c:	case SPEED_1000 + DUPLEX_HALF: /* not supported */
drivers/net/e1000e/ethtool.c:	case SPEED_10 + DUPLEX_HALF:
drivers/net/e1000e/ethtool.c:	case SPEED_100 + DUPLEX_HALF:
drivers/net/e1000e/ethtool.c:	case SPEED_1000 + DUPLEX_HALF: /* not supported */
drivers/net/igb/igb_main.c:	case SPEED_10 + DUPLEX_HALF:
drivers/net/igb/igb_main.c:	case SPEED_100 + DUPLEX_HALF:
drivers/net/igb/igb_main.c:	case SPEED_1000 + DUPLEX_HALF: /* not supported */
drivers/net/sungem_phy.c:		phy->duplex |=  DUPLEX_HALF;
drivers/net/sungem_phy.c:		phy->duplex |=  DUPLEX_HALF;

Also, drivers/net/mii.c does stuff like:

			ecmd->duplex = !!(nego & ADVERTISED_1000baseT_Full);

It also (probably correctly, I don't know, you'll have to decide)
rejects anything other than the existing values.

	if (ecmd->duplex != DUPLEX_HALF && ecmd->duplex != DUPLEX_FULL)
		return -EINVAL;

Finally, phy_ethtool_sset() does this too, so with your patch applied even
phylib would reject the new settings:

	if (cmd->autoneg == AUTONEG_DISABLE &&
	    ((cmd->speed != SPEED_1000 &&
	      cmd->speed != SPEED_100 &&
	      cmd->speed != SPEED_10) ||
	     (cmd->duplex != DUPLEX_HALF &&
	      cmd->duplex != DUPLEX_FULL)))
		return -EINVAL;

making it impossible to add support for TX-only or RX-only in a phylib
using driver.

There are probably other similar dragons hiding in various drivers, you'll
really have to do a full audit of how every driver stores and makes use of
duplex settings before we can seriously apply this patch.

Thanks!

Re: [PATCH 1/2] ethtool.h: Add #defines for unidirectional ethernet duplex

[Kyle Moffett]

On Sat, Aug 28, 2010 at 19:03, David Miller <davem@davemloft.net> wrote:
> From: Kyle Moffett <Kyle.D.Moffett@boeing.com>
> Date: Fri, 27 Aug 2010 15:42:17 -0400
>> In order to configure those kinds of forced modes from userspace, we
>> need to add additional options to the "ethtool" interface.  As such
>> "unidirectional" ethernet modes most closely resemble ethernet "duplex",
>> we add two additional DUPLEX_* defines to the ethtool interface:
>>
>>   #define DUPLEX_TXONLY 0x02
>>   #define DUPLEX_RXONLY 0x03
>
> A fine idea, but you're really going to have to audit all of the networking
> drivers to clean up the existing uses that assume this thing is a bitmap
> and that there are only essentially two values ('0' and '1').  For example:

Well... basically every driver would require specific tweaks to enable
unidirectional link support even in the best case *anyways*.

For example, several SOC chipsets have errata which require a receive
clock in order to reset the chip.  When they are in the "link down"
state, they provide their own fake PHY receive clock copied from an
onboard fixed clock, and when they are in the "link up" state they use
the incoming clock.  Furthermore, they require the RX and TX clocks to
be running at the same frequency (if the RX clock is available)
because of a PLL linking them, so in order to make them work with
unidirectional links, you have to significantly fudge with the fake RX
clock in order to keep the chip operational.

> Finally, phy_ethtool_sset() does this too, so with your patch applied even
> phylib would reject the new settings:
>
>        if (cmd->autoneg == AUTONEG_DISABLE &&
>            ((cmd->speed != SPEED_1000 &&
>              cmd->speed != SPEED_100 &&
>              cmd->speed != SPEED_10) ||
>             (cmd->duplex != DUPLEX_HALF &&
>              cmd->duplex != DUPLEX_FULL)))
>                return -EINVAL;
>
> making it impossible to add support for TX-only or RX-only in a phylib
> using driver.

As you noted, phylib does not have support for the txonly/rxonly
duplex, so drivers using that will automatically return EINVAL as they
would for any other unsupported ethtool options.  I have a very
involved patch series for phylib that reworks some of the ethtool
operations to be more easily customized by individual PHYs but it's
not quite done yet, let alone tested.

Due to the complexity of the phylib patches and because sky2 does not
use phylib yet, I'd like to start with just the simplistic tested sky2
patch.  Since all other drivers would just return -EINVAL, there is no
possibility for ABI/API breakage outside of the sky2 driver.


> There are probably other similar dragons hiding in various drivers, you'll
> really have to do a full audit of how every driver stores and makes use of
> duplex settings before we can seriously apply this patch.

If everyone thinks this particular choice of interface is appropriate
for configuring unidirectional links then I think these two patches
should be mergeable.  I'd especially like to get the first one (which
defines the constants) merged, as that reasonably defines the ABI and
provides a reference for the minor patches to the "ethtool" program.
If you really want I can try to post some of the sample phylib patches
but as I said they're still in very rough shape, as I wanted to get
the ABI extension reviewed and committed before I invested too much
time in them.

In general, my plan for phylib drivers is to change ethtool_sset() and
ethtool_gset() to be phy_driver function pointers.  When the ethernet
driver's mydriver_ethtool_sset() function is called, it will perform
any of its own validation on the settings first, then call static
inline phy_ethtool_sset(), which will look up the function pointer and
call that (by default genphy_ethtool_sset(), based on the current
global version).  Any PHY which has appropriate bits to support
unidirectional operation would customize that function to allow
additional modes and program the PHY regs accordingly.  Please let me
know if this makes sense to you or if you have any other questions.

Thanks again for your comments!

Cheers,
Kyle Moffett

[PATCH 2/2] sky2: Add unidirectional fiber link support

[Kyle Moffett]

Some sky2 fiber hardware seems to support configuring unidirectional
fiber links (using the phy bit PHY_M_FIB_FORCE_LNK).  There are three
parts to enabling this hardware support:

  (1) Allow DUPLEX_TXONLY/DUPLEX_RXONLY to be passed in via ethtool
  (2) Correctly configure the PHY using the new duplex values
  (3) Make sure the initial PHY link-up interrupt does not get lost

It may be possible to use the special DUPLEX_RXONLY operation mode to
disable the fiber transmitter entirely, but in practice we can safely
leave the chipset in regular full-duplex forced-link mode.

Signed-off-by: Kyle Moffett <Kyle.D.Moffett@boeing.com>
---
 drivers/net/sky2.c |   80 ++++++++++++++++++++++++++++++++++-----------------
 drivers/net/sky2.h |    2 +-
 2 files changed, 54 insertions(+), 28 deletions(-)

diff --git a/drivers/net/sky2.c b/drivers/net/sky2.c
index 7985165..6fc915b 100644
--- a/drivers/net/sky2.c
+++ b/drivers/net/sky2.c
@@ -387,6 +387,10 @@ static void sky2_phy_init(struct sky2_hw *hw, unsigned port)
 		/* disable Automatic Crossover */
 
 		ctrl &= ~PHY_M_PC_MDIX_MSK;
+
+		/* If we have a transmit-only link then force the fiber on */
+		if (sky2->duplex == DUPLEX_TXONLY)
+			ctrl |= PHY_M_FIB_FORCE_LNK;
 	}
 
 	gm_phy_write(hw, port, PHY_MARV_PHY_CTRL, ctrl);
@@ -462,7 +466,7 @@ static void sky2_phy_init(struct sky2_hw *hw, unsigned port)
 			break;
 		}
 
-		if (sky2->duplex == DUPLEX_FULL) {
+		if (sky2->duplex != DUPLEX_HALF) {
 			reg |= GM_GPCR_DUP_FULL;
 			ctrl |= PHY_CT_DUP_MD;
 		} else if (sky2->speed < SPEED_1000)
@@ -1667,6 +1671,15 @@ static int sky2_up(struct net_device *dev)
 
 	netif_info(sky2, ifup, dev, "enabling interface\n");
 
+	/* 
+	 * Once interrupts are reenabled, reset the PHY again to make sure
+	 * that we didn't miss a link-up interrupt.  This is especially
+	 * likely to occur if we're in fiber-txonly mode, as a link-up
+	 * interrupt is generated almost immediately after we finish
+	 * programming the PHY.
+	 */
+	sky2_phy_reinit(sky2);
+
 	return 0;
 
 err_out:
@@ -2053,12 +2066,18 @@ static void sky2_link_up(struct sky2_port *sky2)
 {
 	struct sky2_hw *hw = sky2->hw;
 	unsigned port = sky2->port;
-	static const char *fc_name[] = {
+	static const char const *fc_name[] = {
 		[FC_NONE]	= "none",
 		[FC_TX]		= "tx",
 		[FC_RX]		= "rx",
 		[FC_BOTH]	= "both",
 	};
+	static const char const *duplex_name[] = {
+		[DUPLEX_HALF]   = "half",
+		[DUPLEX_FULL]   = "full",
+		[DUPLEX_TXONLY] = "txonly",
+		[DUPLEX_RXONLY] = "rxonly",
+	};
 
 	sky2_enable_rx_tx(sky2);
 
@@ -2073,10 +2092,10 @@ static void sky2_link_up(struct sky2_port *sky2)
 		    LINKLED_ON | LINKLED_BLINK_OFF | LINKLED_LINKSYNC_OFF);
 
 	netif_info(sky2, link, sky2->netdev,
-		   "Link is up at %d Mbps, %s duplex, flow control %s\n",
-		   sky2->speed,
-		   sky2->duplex == DUPLEX_FULL ? "full" : "half",
-		   fc_name[sky2->flow_status]);
+		"Link is up at %d Mbps, %s duplex, flow control %s\n",
+		sky2->speed,
+		duplex_name[sky2->duplex],
+		fc_name[sky2->flow_status]);
 }
 
 static void sky2_link_down(struct sky2_port *sky2)
@@ -3462,39 +3481,46 @@ static int sky2_set_settings(struct net_device *dev, struct ethtool_cmd *ecmd)
 		sky2->duplex = -1;
 		sky2->speed = -1;
 	} else {
-		u32 setting;
+		u32 setting = supported;
 
+		/* Check the specified speed */
 		switch (ecmd->speed) {
 		case SPEED_1000:
-			if (ecmd->duplex == DUPLEX_FULL)
-				setting = SUPPORTED_1000baseT_Full;
-			else if (ecmd->duplex == DUPLEX_HALF)
-				setting = SUPPORTED_1000baseT_Half;
-			else
-				return -EINVAL;
+			setting &= ~(	SUPPORTED_1000baseT_Full |
+					SUPPORTED_1000baseT_Half );
 			break;
 		case SPEED_100:
-			if (ecmd->duplex == DUPLEX_FULL)
-				setting = SUPPORTED_100baseT_Full;
-			else if (ecmd->duplex == DUPLEX_HALF)
-				setting = SUPPORTED_100baseT_Half;
-			else
-				return -EINVAL;
+			setting &= ~(	SUPPORTED_100baseT_Full |
+					SUPPORTED_100baseT_Half );
 			break;
-
 		case SPEED_10:
-			if (ecmd->duplex == DUPLEX_FULL)
-				setting = SUPPORTED_10baseT_Full;
-			else if (ecmd->duplex == DUPLEX_HALF)
-				setting = SUPPORTED_10baseT_Half;
-			else
-				return -EINVAL;
+			setting &= ~(	SUPPORTED_10baseT_Full |
+					SUPPORTED_10baseT_Half );
+			break;
+		default:
+			return -EINVAL;
+		}
+
+		/* Check the specified duplex */
+		switch (ecmd->duplex) {
+		case DUPLEX_HALF:
+			setting &= ~(	SUPPORTED_1000baseT_Half |
+					SUPPORTED_100baseT_Half  |
+					SUPPORTED_10baseT_Half   );
+			break;
+		case DUPLEX_FULL:
+		case DUPLEX_RXONLY:
+		case DUPLEX_TXONLY:
+			setting &= ~(	SUPPORTED_1000baseT_Full |
+					SUPPORTED_100baseT_Full  |
+					SUPPORTED_10baseT_Full   );
 			break;
 		default:
 			return -EINVAL;
 		}
 
-		if ((setting & supported) == 0)
+		/* Make sure we have a valid mode */
+		if (!setting)
 			return -EINVAL;
 
 		sky2->speed = ecmd->speed;
diff --git a/drivers/net/sky2.h b/drivers/net/sky2.h
index 084eff2..81980a4 100644
--- a/drivers/net/sky2.h
+++ b/drivers/net/sky2.h
@@ -2246,7 +2246,7 @@ struct sky2_port {
 	u16		     advertising;	/* ADVERTISED_ bits */
 	u16		     speed;		/* SPEED_1000, SPEED_100, ... */
 	u8		     wol;		/* WAKE_ bits */
-	u8		     duplex;		/* DUPLEX_HALF, DUPLEX_FULL */
+	u8		     duplex;		/* DUPLEX_HALF, DUPLEX_FULL, ... */
 	u16		     flags;
 #define SKY2_FLAG_RX_CHECKSUM		0x0001
 #define SKY2_FLAG_AUTO_SPEED		0x0002
-- 
1.7.1

Re: [PATCH 2/2] sky2: Add unidirectional fiber link support

[Stephen Hemminger]

On Fri, 27 Aug 2010 15:42:18 -0400
Kyle Moffett <Kyle.D.Moffett@boeing.com> wrote:

> +	/* 
> +	 * Once interrupts are reenabled, reset the PHY again to make sure
> +	 * that we didn't miss a link-up interrupt.  This is especially
> +	 * likely to occur if we're in fiber-txonly mode, as a link-up
> +	 * interrupt is generated almost immediately after we finish
> +	 * programming the PHY.
> +	 */
> +	sky2_phy_reinit(sky2);

Won't this cause a renegotiation causing up to 2 second delay?

-- 

Re: [PATCH 2/2] sky2: Add unidirectional fiber link support

[Moffett, Kyle D]

On Aug 27, 2010, at 16:38, Stephen Hemminger wrote:
> On Fri, 27 Aug 2010 15:42:18 -0400 Kyle Moffett <Kyle.D.Moffett@boeing.com> wrote:
>> +	/* 
>> +	 * Once interrupts are reenabled, reset the PHY again to make sure
>> +	 * that we didn't miss a link-up interrupt.  This is especially
>> +	 * likely to occur if we're in fiber-txonly mode, as a link-up
>> +	 * interrupt is generated almost immediately after we finish
>> +	 * programming the PHY.
>> +	 */
>> +	sky2_phy_reinit(sky2);
> 
> Won't this cause a renegotiation causing up to 2 second delay?

Well, I suppose that's possible, but we've only just reset and enabled the PHY moments before, so I don't see where you could get an *extra* 2-second delay.  On the other hand, I suppose it would be much nicer if there was an easy way to fake an extra interrupt there instead of reinitializing the whole PHY.  I'm not all that comfortable with my understanding of the interrupt logic in the sky2 driver, so I figured I would just reuse all the necessary locking from sky2_phy_reinit().

Do you have any comments or criticisms of the particular "duplex" method of configuring the unidirectional link support?

Cheers,
Kyle Moffett

Re: [PATCH 2/2] sky2: Add unidirectional fiber link support

[Stephen Hemminger]

On Fri, 27 Aug 2010 15:51:58 -0500
"Moffett, Kyle D" <Kyle.D.Moffett@boeing.com> wrote:

> On Aug 27, 2010, at 16:38, Stephen Hemminger wrote:
> > On Fri, 27 Aug 2010 15:42:18 -0400 Kyle Moffett <Kyle.D.Moffett@boeing.com> wrote:
> >> +	/* 
> >> +	 * Once interrupts are reenabled, reset the PHY again to make sure
> >> +	 * that we didn't miss a link-up interrupt.  This is especially
> >> +	 * likely to occur if we're in fiber-txonly mode, as a link-up
> >> +	 * interrupt is generated almost immediately after we finish
> >> +	 * programming the PHY.
> >> +	 */
> >> +	sky2_phy_reinit(sky2);
> > 
> > Won't this cause a renegotiation causing up to 2 second delay?
> 
> Well, I suppose that's possible, but we've only just reset and enabled the PHY moments before, so I don't see where you could get an *extra* 2-second delay.  On the other hand, I suppose it would be much nicer if there was an easy way to fake an extra interrupt there instead of reinitializing the whole PHY.  I'm not all that comfortable with my understanding of the interrupt logic in the sky2 driver, so I figured I would just reuse all the necessary locking from sky2_phy_reinit().
> 
> Do you have any comments or criticisms of the particular "duplex" method of configuring the unidirectional link support?

No that is fine, but the FIB doesn't really understand RX only links
so I expect users will do stupid things.

Re: [PATCH 2/2] sky2: Add unidirectional fiber link support

[Kyle Moffett]

On Fri, Aug 27, 2010 at 17:22, Stephen Hemminger
<shemminger@linux-foundation.org> wrote:
> On Fri, 27 Aug 2010 15:51:58 -0500 "Moffett, Kyle D" <Kyle.D.Moffett@boeing.com> wrote:
>> Do you have any comments or criticisms of the particular "duplex" method of configuring the unidirectional link support?
>
> No that is fine, but the FIB doesn't really understand RX only links so I expect users will do stupid things.

As far as the chipset is concerned, an "rxonly" link would just be a
regular full-duplex forced-mode fiber link.  If the user happens to
wire up the "transmit" path on such a link and is then surprised when
the driver is actually able to use that path, that would seem to be
their own problem.  I'm considering perhaps fiddling with ARP or
routing behavior over txonly/rxonly links, so I'd like to preserve the
symmetry even if just for documentation purposes or future
optimizations.

When I get any applicable review commentary sorted out and worked
through, in whose tree should I request inclusion of these 2 patches?
Thanks for your comments!

Cheers,
Kyle Moffett