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

Previous thread: [PATCH 1/2] ethtool.h: Add #defines for unidirectional ethernet duplex by Kyle Moffett on Friday, August 27, 2010 - 12:42 pm. (3 messages)

Next thread: [GIT PULL] eCryptfs fixes for 2.6.36-rc3 by Tyler Hicks on Friday, August 27, 2010 - 12:31 pm. (1 message)
From: Kyle Moffett
Date: Friday, August 27, 2010 - 12:42 pm

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

--

From: Kyle Moffett
Date: Friday, August 27, 2010 - 12:42 pm

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.
+	 ...
From: Stephen Hemminger
Date: Friday, August 27, 2010 - 1:38 pm

On Fri, 27 Aug 2010 15:42:18 -0400

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

-- 
--

From: Moffett, Kyle D
Date: Friday, August 27, 2010 - 1:51 pm

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

--

From: Stephen Hemminger
Date: Friday, August 27, 2010 - 2:22 pm

On Fri, 27 Aug 2010 15:51:58 -0500

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

From: Kyle Moffett
Date: Friday, August 27, 2010 - 10:35 pm

On Fri, Aug 27, 2010 at 17:22, Stephen Hemminger

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

Previous thread: [PATCH 1/2] ethtool.h: Add #defines for unidirectional ethernet duplex by Kyle Moffett on Friday, August 27, 2010 - 12:42 pm. (3 messages)

Next thread: [GIT PULL] eCryptfs fixes for 2.6.36-rc3 by Tyler Hicks on Friday, August 27, 2010 - 12:31 pm. (1 message)