| Message ID | 1282938138-17844-2-git-send-email-Kyle.D.Moffett@boeing.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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! -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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(-)