changes v3:
- provide separate field for config and state.
- make state rejected on set
- add validation
changes v2:
- change names. Use MASTER_PREFERRED instead of MULTIPORT
- configure master/slave only on request. Default configuration can be
provided by PHY or eeprom
- status and configuration to the user space.
Oleksij Rempel (2):
ethtool: provide UAPI for PHY master/slave configuration.
net: phy: tja11xx: add support for master-slave configuration
Documentation/networking/ethtool-netlink.rst | 3 +
drivers/net/phy/nxp-tja11xx.c | 58 ++++++++++++-
drivers/net/phy/phy.c | 7 +-
drivers/net/phy/phy_device.c | 89 ++++++++++++++++++++
include/linux/phy.h | 3 +
include/uapi/linux/ethtool.h | 29 ++++++-
include/uapi/linux/ethtool_netlink.h | 2 +
include/uapi/linux/mii.h | 2 +
net/ethtool/linkmodes.c | 15 +++-
9 files changed, 204 insertions(+), 4 deletions(-)
--
2.26.2
This UAPI is needed for BroadR-Reach 100BASE-T1 devices. Due to lack of
auto-negotiation support, we needed to be able to configure the
MASTER-SLAVE role of the port manually or from an application in user
space.
The same UAPI can be used for 1000BASE-T or MultiGBASE-T devices to
force MASTER or SLAVE role. See IEEE 802.3-2018:
22.2.4.3.7 MASTER-SLAVE control register (Register 9)
22.2.4.3.8 MASTER-SLAVE status register (Register 10)
40.5.2 MASTER-SLAVE configuration resolution
45.2.1.185.1 MASTER-SLAVE config value (1.2100.14)
45.2.7.10 MultiGBASE-T AN control 1 register (Register 7.32)
The MASTER-SLAVE role affects the clock configuration:
-------------------------------------------------------------------------------
When the PHY is configured as MASTER, the PMA Transmit function shall
source TX_TCLK from a local clock source. When configured as SLAVE, the
PMA Transmit function shall source TX_TCLK from the clock recovered from
data stream provided by MASTER.
iMX6Q KSZ9031 XXX
------\ /-----------\ /------------\
| | | | |
MAC |<----RGMII----->| PHY Slave |<------>| PHY Master |
|<--- 125 MHz ---+-<------/ | | \ |
------/ \-----------/ \------------/
^
\-TX_TCLK
-------------------------------------------------------------------------------
Since some clock or link related issues are only reproducible in a
specific MASTER-SLAVE-role, MAC and PHY configuration, it is beneficial
to provide generic (not 100BASE-T1 specific) interface to the user space
for configuration flexibility and trouble shooting.
Signed-off-by: Oleksij Rempel <[email protected]>
---
Documentation/networking/ethtool-netlink.rst | 3 +
drivers/net/phy/phy.c | 7 +-
drivers/net/phy/phy_device.c | 89 ++++++++++++++++++++
include/linux/phy.h | 3 +
include/uapi/linux/ethtool.h | 29 ++++++-
include/uapi/linux/ethtool_netlink.h | 2 +
include/uapi/linux/mii.h | 2 +
net/ethtool/linkmodes.c | 15 +++-
8 files changed, 147 insertions(+), 3 deletions(-)
diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
index 567326491f80b..c32b109ebe1b3 100644
--- a/Documentation/networking/ethtool-netlink.rst
+++ b/Documentation/networking/ethtool-netlink.rst
@@ -399,6 +399,8 @@ Kernel response contents:
``ETHTOOL_A_LINKMODES_PEER`` bitset partner link modes
``ETHTOOL_A_LINKMODES_SPEED`` u32 link speed (Mb/s)
``ETHTOOL_A_LINKMODES_DUPLEX`` u8 duplex mode
+ ``ETHTOOL_A_LINKMODES_MASTER_SLAVE_CFG`` u8 Master/slave port mode
+ ``ETHTOOL_A_LINKMODES_MASTER_SLAVE_STATE`` u8 Master/slave port mode
==================================== ====== ==========================
For ``ETHTOOL_A_LINKMODES_OURS``, value represents advertised modes and mask
@@ -421,6 +423,7 @@ Request contents:
``ETHTOOL_A_LINKMODES_PEER`` bitset partner link modes
``ETHTOOL_A_LINKMODES_SPEED`` u32 link speed (Mb/s)
``ETHTOOL_A_LINKMODES_DUPLEX`` u8 duplex mode
+ ``ETHTOOL_A_LINKMODES_MASTER_SLAVE_CFG`` u8 Master/slave port mode
==================================== ====== ==========================
``ETHTOOL_A_LINKMODES_OURS`` bit set allows setting advertised link modes. If
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 72c69a9c8a98a..a6a774beb2f90 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -285,6 +285,9 @@ int phy_ethtool_ksettings_set(struct phy_device *phydev,
duplex != DUPLEX_FULL)))
return -EINVAL;
+ if (!ethtool_validate_master_slave_cfg(cmd->base.master_slave_cfg))
+ return -EINVAL;
+
phydev->autoneg = autoneg;
phydev->speed = speed;
@@ -295,7 +298,7 @@ int phy_ethtool_ksettings_set(struct phy_device *phydev,
phydev->advertising, autoneg == AUTONEG_ENABLE);
phydev->duplex = duplex;
-
+ phydev->master_slave_set = cmd->base.master_slave_cfg;
phydev->mdix_ctrl = cmd->base.eth_tp_mdix_ctrl;
/* Restart the PHY */
@@ -314,6 +317,8 @@ void phy_ethtool_ksettings_get(struct phy_device *phydev,
cmd->base.speed = phydev->speed;
cmd->base.duplex = phydev->duplex;
+ cmd->base.master_slave_cfg = phydev->master_slave_get;
+ cmd->base.master_slave_state = phydev->master_slave_state;
if (phydev->interface == PHY_INTERFACE_MODE_MOCA)
cmd->base.port = PORT_BNC;
else
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index ac2784192472f..756c3642d0f8e 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -9,6 +9,7 @@
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+#include <linux/bitfield.h>
#include <linux/kernel.h>
#include <linux/string.h>
#include <linux/errno.h>
@@ -1768,6 +1769,84 @@ int genphy_setup_forced(struct phy_device *phydev)
}
EXPORT_SYMBOL(genphy_setup_forced);
+static int genphy_setup_master_slave(struct phy_device *phydev)
+{
+ u16 ctl = 0;
+
+ if (!phydev->is_gigabit_capable)
+ return 0;
+
+ switch (phydev->master_slave_set) {
+ case PORT_MODE_CFG_MASTER_PREFERRED:
+ ctl |= CTL1000_PREFER_MASTER;
+ break;
+ case PORT_MODE_CFG_SLAVE_PREFERRED:
+ break;
+ case PORT_MODE_CFG_MASTER_FORCE:
+ ctl |= CTL1000_AS_MASTER;
+ /* fallthrough */
+ case PORT_MODE_CFG_SLAVE_FORCE:
+ ctl |= CTL1000_ENABLE_MASTER;
+ break;
+ case PORT_MODE_CFG_UNKNOWN:
+ return 0;
+ default:
+ phydev_warn(phydev, "Unsupported Master/Slave mode\n");
+ return 0;
+ }
+
+ return phy_modify_changed(phydev, MII_CTRL1000,
+ (CTL1000_ENABLE_MASTER | CTL1000_AS_MASTER |
+ CTL1000_PREFER_MASTER), ctl);
+}
+
+static int genphy_read_master_slave(struct phy_device *phydev)
+{
+ int cfg, state = 0;
+ u16 val;
+
+ phydev->master_slave_get = 0;
+ phydev->master_slave_state = 0;
+
+ if (!phydev->is_gigabit_capable)
+ return 0;
+
+ val = phy_read(phydev, MII_CTRL1000);
+ if (val < 0)
+ return val;
+
+ if (val & CTL1000_ENABLE_MASTER) {
+ if (val & CTL1000_AS_MASTER)
+ cfg = PORT_MODE_CFG_MASTER_FORCE;
+ else
+ cfg = PORT_MODE_CFG_SLAVE_FORCE;
+ } else {
+ if (val & CTL1000_PREFER_MASTER)
+ cfg = PORT_MODE_CFG_MASTER_PREFERRED;
+ else
+ cfg = PORT_MODE_CFG_SLAVE_PREFERRED;
+ }
+
+ val = phy_read(phydev, MII_STAT1000);
+ if (val < 0)
+ return val;
+
+ if (val & LPA_1000MSFAIL) {
+ state = PORT_MODE_STATE_ERR;
+ } else if (phydev->link) {
+ /* this bits are valid only for active link */
+ if (val & LPA_1000MSRES)
+ state = PORT_MODE_STATE_MASTER;
+ else
+ state = PORT_MODE_STATE_SLAVE;
+ }
+
+ phydev->master_slave_get = cfg;
+ phydev->master_slave_state = state;
+
+ return 0;
+}
+
/**
* genphy_restart_aneg - Enable and Restart Autonegotiation
* @phydev: target phy_device struct
@@ -1826,6 +1905,12 @@ int __genphy_config_aneg(struct phy_device *phydev, bool changed)
if (genphy_config_eee_advert(phydev))
changed = true;
+ err = genphy_setup_master_slave(phydev);
+ if (err < 0)
+ return err;
+ else if (err)
+ changed = true;
+
if (AUTONEG_ENABLE != phydev->autoneg)
return genphy_setup_forced(phydev);
@@ -2060,6 +2145,10 @@ int genphy_read_status(struct phy_device *phydev)
phydev->pause = 0;
phydev->asym_pause = 0;
+ err = genphy_read_master_slave(phydev);
+ if (err < 0)
+ return err;
+
err = genphy_read_lpa(phydev);
if (err < 0)
return err;
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 2432ca463ddc0..19cd4fe6efbf1 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -431,6 +431,9 @@ struct phy_device {
int duplex;
int pause;
int asym_pause;
+ u8 master_slave_get;
+ u8 master_slave_set;
+ u8 master_slave_state;
/* Union of PHY and Attached devices' supported link modes */
/* See ethtool.h for more info */
diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index 92f737f101178..eb680e3d6bda5 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -1666,6 +1666,31 @@ static inline int ethtool_validate_duplex(__u8 duplex)
return 0;
}
+/* Port mode */
+#define PORT_MODE_CFG_UNKNOWN 0
+#define PORT_MODE_CFG_MASTER_PREFERRED 1
+#define PORT_MODE_CFG_SLAVE_PREFERRED 2
+#define PORT_MODE_CFG_MASTER_FORCE 3
+#define PORT_MODE_CFG_SLAVE_FORCE 4
+#define PORT_MODE_STATE_UNKNOWN 0
+#define PORT_MODE_STATE_MASTER 1
+#define PORT_MODE_STATE_SLAVE 2
+#define PORT_MODE_STATE_ERR 3
+
+static inline int ethtool_validate_master_slave_cfg(__u8 cfg)
+{
+ switch (cfg) {
+ case PORT_MODE_CFG_MASTER_PREFERRED:
+ case PORT_MODE_CFG_SLAVE_PREFERRED:
+ case PORT_MODE_CFG_MASTER_FORCE:
+ case PORT_MODE_CFG_SLAVE_FORCE:
+ case PORT_MODE_CFG_UNKNOWN:
+ return 1;
+ }
+
+ return 0;
+}
+
/* Which connector port. */
#define PORT_TP 0x00
#define PORT_AUI 0x01
@@ -1904,7 +1929,9 @@ struct ethtool_link_settings {
__u8 eth_tp_mdix_ctrl;
__s8 link_mode_masks_nwords;
__u8 transceiver;
- __u8 reserved1[3];
+ __u8 master_slave_cfg;
+ __u8 master_slave_state;
+ __u8 reserved1[1];
__u32 reserved[7];
__u32 link_mode_masks[0];
/* layout of link_mode_masks fields:
diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
index 7fde76366ba46..bf1d310e20bc6 100644
--- a/include/uapi/linux/ethtool_netlink.h
+++ b/include/uapi/linux/ethtool_netlink.h
@@ -216,6 +216,8 @@ enum {
ETHTOOL_A_LINKMODES_PEER, /* bitset */
ETHTOOL_A_LINKMODES_SPEED, /* u32 */
ETHTOOL_A_LINKMODES_DUPLEX, /* u8 */
+ ETHTOOL_A_LINKMODES_MASTER_SLAVE_CFG, /* u8 */
+ ETHTOOL_A_LINKMODES_MASTER_SLAVE_STATE, /* u8 */
/* add new constants above here */
__ETHTOOL_A_LINKMODES_CNT,
diff --git a/include/uapi/linux/mii.h b/include/uapi/linux/mii.h
index 90f9b4e1ba277..39f7c44baf535 100644
--- a/include/uapi/linux/mii.h
+++ b/include/uapi/linux/mii.h
@@ -151,11 +151,13 @@
/* 1000BASE-T Control register */
#define ADVERTISE_1000FULL 0x0200 /* Advertise 1000BASE-T full duplex */
#define ADVERTISE_1000HALF 0x0100 /* Advertise 1000BASE-T half duplex */
+#define CTL1000_PREFER_MASTER 0x0400 /* prefer to operate as master */
#define CTL1000_AS_MASTER 0x0800
#define CTL1000_ENABLE_MASTER 0x1000
/* 1000BASE-T Status register */
#define LPA_1000MSFAIL 0x8000 /* Master/Slave resolution failure */
+#define LPA_1000MSRES 0x4000 /* Master/Slave resolution status */
#define LPA_1000LOCALRXOK 0x2000 /* Link partner local receiver status */
#define LPA_1000REMRXOK 0x1000 /* Link partner remote receiver status */
#define LPA_1000FULL 0x0800 /* Link partner 1000BASE-T full duplex */
diff --git a/net/ethtool/linkmodes.c b/net/ethtool/linkmodes.c
index 452608c6d8562..f67da2f1ba8a1 100644
--- a/net/ethtool/linkmodes.c
+++ b/net/ethtool/linkmodes.c
@@ -27,6 +27,8 @@ linkmodes_get_policy[ETHTOOL_A_LINKMODES_MAX + 1] = {
[ETHTOOL_A_LINKMODES_PEER] = { .type = NLA_REJECT },
[ETHTOOL_A_LINKMODES_SPEED] = { .type = NLA_REJECT },
[ETHTOOL_A_LINKMODES_DUPLEX] = { .type = NLA_REJECT },
+ [ETHTOOL_A_LINKMODES_MASTER_SLAVE_CFG] = { .type = NLA_REJECT },
+ [ETHTOOL_A_LINKMODES_MASTER_SLAVE_STATE] = { .type = NLA_REJECT },
};
static int linkmodes_prepare_data(const struct ethnl_req_info *req_base,
@@ -69,6 +71,8 @@ static int linkmodes_reply_size(const struct ethnl_req_info *req_base,
len = nla_total_size(sizeof(u8)) /* LINKMODES_AUTONEG */
+ nla_total_size(sizeof(u32)) /* LINKMODES_SPEED */
+ nla_total_size(sizeof(u8)) /* LINKMODES_DUPLEX */
+ + nla_total_size(sizeof(u8)) /* LINKMODES_MASTER_SLAVE_CFG */
+ + nla_total_size(sizeof(u8)) /* LINKMODES_MASTER_SLAVE_STATE */
+ 0;
ret = ethnl_bitset_size(ksettings->link_modes.advertising,
ksettings->link_modes.supported,
@@ -119,7 +123,12 @@ static int linkmodes_fill_reply(struct sk_buff *skb,
}
if (nla_put_u32(skb, ETHTOOL_A_LINKMODES_SPEED, lsettings->speed) ||
- nla_put_u8(skb, ETHTOOL_A_LINKMODES_DUPLEX, lsettings->duplex))
+ nla_put_u8(skb, ETHTOOL_A_LINKMODES_DUPLEX, lsettings->duplex) ||
+ nla_put_u8(skb, ETHTOOL_A_LINKMODES_MASTER_SLAVE_CFG,
+ lsettings->master_slave_cfg) ||
+ nla_put_u8(skb, ETHTOOL_A_LINKMODES_MASTER_SLAVE_STATE,
+ lsettings->master_slave_state))
+
return -EMSGSIZE;
return 0;
@@ -249,6 +258,8 @@ linkmodes_set_policy[ETHTOOL_A_LINKMODES_MAX + 1] = {
[ETHTOOL_A_LINKMODES_PEER] = { .type = NLA_REJECT },
[ETHTOOL_A_LINKMODES_SPEED] = { .type = NLA_U32 },
[ETHTOOL_A_LINKMODES_DUPLEX] = { .type = NLA_U8 },
+ [ETHTOOL_A_LINKMODES_MASTER_SLAVE_CFG] = { .type = NLA_U8 },
+ [ETHTOOL_A_LINKMODES_MASTER_SLAVE_STATE] = { .type = NLA_REJECT },
};
/* Set advertised link modes to all supported modes matching requested speed
@@ -311,6 +322,8 @@ static int ethnl_update_linkmodes(struct genl_info *info, struct nlattr **tb,
mod);
ethnl_update_u8(&lsettings->duplex, tb[ETHTOOL_A_LINKMODES_DUPLEX],
mod);
+ ethnl_update_u8(&lsettings->master_slave_cfg,
+ tb[ETHTOOL_A_LINKMODES_MASTER_SLAVE_CFG], mod);
if (!tb[ETHTOOL_A_LINKMODES_OURS] && lsettings->autoneg &&
(req_speed || req_duplex) &&
--
2.26.2
The TJA11xx PHYs have a vendor specific Master/Slave configuration bit,
which is not compatible with IEEE 803.2-2018 spec for 100Base-T1
devices. So, provide a custom config_ange call back to solve this
problem.
Signed-off-by: Oleksij Rempel <[email protected]>
---
drivers/net/phy/nxp-tja11xx.c | 58 ++++++++++++++++++++++++++++++++++-
1 file changed, 57 insertions(+), 1 deletion(-)
diff --git a/drivers/net/phy/nxp-tja11xx.c b/drivers/net/phy/nxp-tja11xx.c
index cc766b2d4136e..c316d22ea7530 100644
--- a/drivers/net/phy/nxp-tja11xx.c
+++ b/drivers/net/phy/nxp-tja11xx.c
@@ -30,6 +30,7 @@
#define MII_ECTRL_WAKE_REQUEST BIT(0)
#define MII_CFG1 18
+#define MII_CFG1_MASTER_SLAVE BIT(15)
#define MII_CFG1_AUTO_OP BIT(14)
#define MII_CFG1_SLEEP_CONFIRM BIT(6)
#define MII_CFG1_LED_MODE_MASK GENMASK(5, 4)
@@ -167,6 +168,33 @@ static int tja11xx_soft_reset(struct phy_device *phydev)
return genphy_soft_reset(phydev);
}
+static int tja11xx_config_aneg(struct phy_device *phydev)
+{
+ u16 ctl = 0;
+ int ret;
+
+ switch (phydev->master_slave_set) {
+ case PORT_MODE_CFG_MASTER_FORCE:
+ case PORT_MODE_CFG_MASTER_PREFERRED:
+ ctl |= MII_CFG1_MASTER_SLAVE;
+ break;
+ case PORT_MODE_CFG_SLAVE_FORCE:
+ case PORT_MODE_CFG_SLAVE_PREFERRED:
+ break;
+ case PORT_MODE_CFG_UNKNOWN:
+ return 0;
+ default:
+ phydev_warn(phydev, "Unsupported Master/Slave mode\n");
+ return -ENOTSUPP;
+ }
+
+ ret = phy_modify_changed(phydev, MII_CFG1, MII_CFG1_MASTER_SLAVE, ctl);
+ if (ret < 0)
+ return ret;
+
+ return __genphy_config_aneg(phydev, ret);
+}
+
static int tja11xx_config_init(struct phy_device *phydev)
{
int ret;
@@ -222,12 +250,24 @@ static int tja11xx_config_init(struct phy_device *phydev)
static int tja11xx_read_status(struct phy_device *phydev)
{
- int ret;
+ int cfg, state = 0;
+ int ret, cfg1;
+
+ phydev->master_slave_get = 0;
ret = genphy_update_link(phydev);
if (ret)
return ret;
+ cfg1 = phy_read(phydev, MII_CFG1);
+ if (cfg1 < 0)
+ return cfg1;
+
+ if (cfg1 & MII_CFG1_MASTER_SLAVE)
+ cfg = PORT_MODE_CFG_MASTER_FORCE;
+ else
+ cfg = PORT_MODE_CFG_SLAVE_FORCE;
+
if (phydev->link) {
ret = phy_read(phydev, MII_COMMSTAT);
if (ret < 0)
@@ -235,8 +275,20 @@ static int tja11xx_read_status(struct phy_device *phydev)
if (!(ret & MII_COMMSTAT_LINK_UP))
phydev->link = 0;
+
+ ret = phy_read(phydev, MII_CFG1);
+ if (ret < 0)
+ return ret;
+
+ if (cfg1 & MII_CFG1_MASTER_SLAVE)
+ state = PORT_MODE_STATE_MASTER;
+ else
+ state = PORT_MODE_STATE_SLAVE;
}
+ phydev->master_slave_get = cfg;
+ phydev->master_slave_state = state;
+
return 0;
}
@@ -504,6 +556,7 @@ static struct phy_driver tja11xx_driver[] = {
.features = PHY_BASIC_T1_FEATURES,
.probe = tja11xx_probe,
.soft_reset = tja11xx_soft_reset,
+ .config_aneg = tja11xx_config_aneg,
.config_init = tja11xx_config_init,
.read_status = tja11xx_read_status,
.suspend = genphy_suspend,
@@ -519,6 +572,7 @@ static struct phy_driver tja11xx_driver[] = {
.features = PHY_BASIC_T1_FEATURES,
.probe = tja11xx_probe,
.soft_reset = tja11xx_soft_reset,
+ .config_aneg = tja11xx_config_aneg,
.config_init = tja11xx_config_init,
.read_status = tja11xx_read_status,
.suspend = genphy_suspend,
@@ -533,6 +587,7 @@ static struct phy_driver tja11xx_driver[] = {
.features = PHY_BASIC_T1_FEATURES,
.probe = tja1102_p0_probe,
.soft_reset = tja11xx_soft_reset,
+ .config_aneg = tja11xx_config_aneg,
.config_init = tja11xx_config_init,
.read_status = tja11xx_read_status,
.match_phy_device = tja1102_p0_match_phy_device,
@@ -551,6 +606,7 @@ static struct phy_driver tja11xx_driver[] = {
.features = PHY_BASIC_T1_FEATURES,
/* currently no probe for Port 1 is need */
.soft_reset = tja11xx_soft_reset,
+ .config_aneg = tja11xx_config_aneg,
.config_init = tja11xx_config_init,
.read_status = tja11xx_read_status,
.match_phy_device = tja1102_p1_match_phy_device,
--
2.26.2
On Tue, Apr 28, 2020 at 09:53:07AM +0200, Oleksij Rempel wrote:
Hi Oleksij
Sorry for taking a while to review this. I was busy fixing the FEC
driver which i broke :-(
> --- a/Documentation/networking/ethtool-netlink.rst
> +++ b/Documentation/networking/ethtool-netlink.rst
> @@ -399,6 +399,8 @@ Kernel response contents:
> ``ETHTOOL_A_LINKMODES_PEER`` bitset partner link modes
> ``ETHTOOL_A_LINKMODES_SPEED`` u32 link speed (Mb/s)
> ``ETHTOOL_A_LINKMODES_DUPLEX`` u8 duplex mode
> + ``ETHTOOL_A_LINKMODES_MASTER_SLAVE_CFG`` u8 Master/slave port mode
> + ``ETHTOOL_A_LINKMODES_MASTER_SLAVE_STATE`` u8 Master/slave port mode
> ==================================== ====== ==========================
I've not used Sphinx for a while. But it used to be, tables had to be
correctly aligned. I think you need to pad the other rows with spaces.
Also, the comments should differ. The first is how we want it
configured, the second is the current state.
>
> For ``ETHTOOL_A_LINKMODES_OURS``, value represents advertised modes and mask
> @@ -421,6 +423,7 @@ Request contents:
> ``ETHTOOL_A_LINKMODES_PEER`` bitset partner link modes
> ``ETHTOOL_A_LINKMODES_SPEED`` u32 link speed (Mb/s)
> ``ETHTOOL_A_LINKMODES_DUPLEX`` u8 duplex mode
> + ``ETHTOOL_A_LINKMODES_MASTER_SLAVE_CFG`` u8 Master/slave port mode
> ==================================== ====== ==========================
Same table cleanup needed here.
> +static int genphy_read_master_slave(struct phy_device *phydev)
> +{
> + int cfg, state = 0;
> + u16 val;
> +
> + phydev->master_slave_get = 0;
> + phydev->master_slave_state = 0;
Could you use the _UNKNOWN #defined here?
> diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
> index 92f737f101178..eb680e3d6bda5 100644
> --- a/include/uapi/linux/ethtool.h
> +++ b/include/uapi/linux/ethtool.h
> @@ -1666,6 +1666,31 @@ static inline int ethtool_validate_duplex(__u8 duplex)
> return 0;
> }
>
> +static inline int ethtool_validate_master_slave_cfg(__u8 cfg)
> +{
> + switch (cfg) {
> + case PORT_MODE_CFG_MASTER_PREFERRED:
> + case PORT_MODE_CFG_SLAVE_PREFERRED:
> + case PORT_MODE_CFG_MASTER_FORCE:
> + case PORT_MODE_CFG_SLAVE_FORCE:
> + case PORT_MODE_CFG_UNKNOWN:
> + return 1;
> + }
> +
> + return 0;
> +}
Does this need to be an inline function?
Andrew
> +static int tja11xx_config_aneg(struct phy_device *phydev)
> +{
> + u16 ctl = 0;
> + int ret;
> +
> + switch (phydev->master_slave_set) {
> + case PORT_MODE_CFG_MASTER_FORCE:
> + case PORT_MODE_CFG_MASTER_PREFERRED:
> + ctl |= MII_CFG1_MASTER_SLAVE;
> + break;
> + case PORT_MODE_CFG_SLAVE_FORCE:
> + case PORT_MODE_CFG_SLAVE_PREFERRED:
> + break;
> + case PORT_MODE_CFG_UNKNOWN:
> + return 0;
> + default:
> + phydev_warn(phydev, "Unsupported Master/Slave mode\n");
> + return -ENOTSUPP;
> + }
Does the hardware actually support PORT_MODE_CFG_SLAVE_PREFERRED and
PORT_MODE_CFG_MASTER_PREFERRED? I thought that required autoneg, which
this PHY does not support? So i would of expected these two values to
return ENOTSUPP?
Andrew
On Tue, Apr 28, 2020 at 09:53:07AM +0200, Oleksij Rempel wrote:
> This UAPI is needed for BroadR-Reach 100BASE-T1 devices. Due to lack of
> auto-negotiation support, we needed to be able to configure the
> MASTER-SLAVE role of the port manually or from an application in user
> space.
>
> The same UAPI can be used for 1000BASE-T or MultiGBASE-T devices to
> force MASTER or SLAVE role. See IEEE 802.3-2018:
> 22.2.4.3.7 MASTER-SLAVE control register (Register 9)
> 22.2.4.3.8 MASTER-SLAVE status register (Register 10)
> 40.5.2 MASTER-SLAVE configuration resolution
> 45.2.1.185.1 MASTER-SLAVE config value (1.2100.14)
> 45.2.7.10 MultiGBASE-T AN control 1 register (Register 7.32)
>
> The MASTER-SLAVE role affects the clock configuration:
>
> -------------------------------------------------------------------------------
> When the PHY is configured as MASTER, the PMA Transmit function shall
> source TX_TCLK from a local clock source. When configured as SLAVE, the
> PMA Transmit function shall source TX_TCLK from the clock recovered from
> data stream provided by MASTER.
>
> iMX6Q KSZ9031 XXX
> ------\ /-----------\ /------------\
> | | | | |
> MAC |<----RGMII----->| PHY Slave |<------>| PHY Master |
> |<--- 125 MHz ---+-<------/ | | \ |
> ------/ \-----------/ \------------/
> ^
> \-TX_TCLK
>
> -------------------------------------------------------------------------------
>
> Since some clock or link related issues are only reproducible in a
> specific MASTER-SLAVE-role, MAC and PHY configuration, it is beneficial
> to provide generic (not 100BASE-T1 specific) interface to the user space
> for configuration flexibility and trouble shooting.
>
> Signed-off-by: Oleksij Rempel <[email protected]>
> ---
[...]
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 72c69a9c8a98a..a6a774beb2f90 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -285,6 +285,9 @@ int phy_ethtool_ksettings_set(struct phy_device *phydev,
> duplex != DUPLEX_FULL)))
> return -EINVAL;
>
> + if (!ethtool_validate_master_slave_cfg(cmd->base.master_slave_cfg))
> + return -EINVAL;
> +
Unless we can/want to pass extack down here, I would prefer to have the
sanity check in ethtool_update_linkmodes() or ethtool_set_linkmodes() so
that we can set meaningful error message and offending attribute in
extack. (It could be even part of the policy.) Also, with the check only
here, drivers/devices not calling phy_ethtool_set_link_ksettings()
(directly or via phy_ethtool_set_link_ksettings()) and not handling the
new members themselves would silently ignore any value from userspace.
> phydev->autoneg = autoneg;
>
> phydev->speed = speed;
[...]
> +static int genphy_setup_master_slave(struct phy_device *phydev)
> +{
> + u16 ctl = 0;
> +
> + if (!phydev->is_gigabit_capable)
> + return 0;
Shouldn't we rather return -EOPNOTSUPP if value different from
CFG_UNKNOWN was requested?
> +
> + switch (phydev->master_slave_set) {
> + case PORT_MODE_CFG_MASTER_PREFERRED:
> + ctl |= CTL1000_PREFER_MASTER;
> + break;
> + case PORT_MODE_CFG_SLAVE_PREFERRED:
> + break;
> + case PORT_MODE_CFG_MASTER_FORCE:
> + ctl |= CTL1000_AS_MASTER;
> + /* fallthrough */
> + case PORT_MODE_CFG_SLAVE_FORCE:
> + ctl |= CTL1000_ENABLE_MASTER;
> + break;
> + case PORT_MODE_CFG_UNKNOWN:
> + return 0;
> + default:
> + phydev_warn(phydev, "Unsupported Master/Slave mode\n");
> + return 0;
> + }
[...]
> diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
> index 92f737f101178..eb680e3d6bda5 100644
> --- a/include/uapi/linux/ethtool.h
> +++ b/include/uapi/linux/ethtool.h
> @@ -1666,6 +1666,31 @@ static inline int ethtool_validate_duplex(__u8 duplex)
> return 0;
> }
>
> +/* Port mode */
> +#define PORT_MODE_CFG_UNKNOWN 0
> +#define PORT_MODE_CFG_MASTER_PREFERRED 1
> +#define PORT_MODE_CFG_SLAVE_PREFERRED 2
> +#define PORT_MODE_CFG_MASTER_FORCE 3
> +#define PORT_MODE_CFG_SLAVE_FORCE 4
> +#define PORT_MODE_STATE_UNKNOWN 0
> +#define PORT_MODE_STATE_MASTER 1
> +#define PORT_MODE_STATE_SLAVE 2
> +#define PORT_MODE_STATE_ERR 3
You have "MASTER_SLAVE" or "master_slave" everywhere but "PORT_MODE" in
these constants which is inconsistent.
> +
> +static inline int ethtool_validate_master_slave_cfg(__u8 cfg)
> +{
> + switch (cfg) {
> + case PORT_MODE_CFG_MASTER_PREFERRED:
> + case PORT_MODE_CFG_SLAVE_PREFERRED:
> + case PORT_MODE_CFG_MASTER_FORCE:
> + case PORT_MODE_CFG_SLAVE_FORCE:
> + case PORT_MODE_CFG_UNKNOWN:
> + return 1;
> + }
> +
> + return 0;
> +}
Should we really allow CFG_UNKNOWN in client requests? As far as I can
see, this value is handled as no-op which should be rather expressed by
absence of the attribute. Allowing the client to request a value,
keeping current one and returning 0 (success) is IMHO wrong.
Also, should this function be in UAPI header?
[...]
> @@ -119,7 +123,12 @@ static int linkmodes_fill_reply(struct sk_buff *skb,
> }
>
> if (nla_put_u32(skb, ETHTOOL_A_LINKMODES_SPEED, lsettings->speed) ||
> - nla_put_u8(skb, ETHTOOL_A_LINKMODES_DUPLEX, lsettings->duplex))
> + nla_put_u8(skb, ETHTOOL_A_LINKMODES_DUPLEX, lsettings->duplex) ||
> + nla_put_u8(skb, ETHTOOL_A_LINKMODES_MASTER_SLAVE_CFG,
> + lsettings->master_slave_cfg) ||
> + nla_put_u8(skb, ETHTOOL_A_LINKMODES_MASTER_SLAVE_STATE,
> + lsettings->master_slave_state))
> +
> return -EMSGSIZE;
From the two handlers you introduced, it seems we only get CFG_UNKNOWN
or STATE_UNKNOWN if driver or device does not support the feature at all
so it would be IMHO more appropriate to omit the attribute in such case.
Michal
>
> return 0;
Hi Andrew,
On Wed, Apr 29, 2020 at 08:16:14PM +0200, Andrew Lunn wrote:
> On Tue, Apr 28, 2020 at 09:53:07AM +0200, Oleksij Rempel wrote:
>
> Hi Oleksij
>
> Sorry for taking a while to review this. I was busy fixing the FEC
> driver which i broke :-(
Not problem.
Interesting, what is wrong with FEC? We use it a lot.
> > --- a/Documentation/networking/ethtool-netlink.rst
> > +++ b/Documentation/networking/ethtool-netlink.rst
> > @@ -399,6 +399,8 @@ Kernel response contents:
> > ``ETHTOOL_A_LINKMODES_PEER`` bitset partner link modes
> > ``ETHTOOL_A_LINKMODES_SPEED`` u32 link speed (Mb/s)
> > ``ETHTOOL_A_LINKMODES_DUPLEX`` u8 duplex mode
> > + ``ETHTOOL_A_LINKMODES_MASTER_SLAVE_CFG`` u8 Master/slave port mode
> > + ``ETHTOOL_A_LINKMODES_MASTER_SLAVE_STATE`` u8 Master/slave port mode
> > ==================================== ====== ==========================
>
> I've not used Sphinx for a while. But it used to be, tables had to be
> correctly aligned. I think you need to pad the other rows with spaces.
>
> Also, the comments should differ. The first is how we want it
> configured, the second is the current state.
ok
> >
> > For ``ETHTOOL_A_LINKMODES_OURS``, value represents advertised modes and mask
> > @@ -421,6 +423,7 @@ Request contents:
> > ``ETHTOOL_A_LINKMODES_PEER`` bitset partner link modes
> > ``ETHTOOL_A_LINKMODES_SPEED`` u32 link speed (Mb/s)
> > ``ETHTOOL_A_LINKMODES_DUPLEX`` u8 duplex mode
> > + ``ETHTOOL_A_LINKMODES_MASTER_SLAVE_CFG`` u8 Master/slave port mode
> > ==================================== ====== ==========================
>
> Same table cleanup needed here.
>
> > +static int genphy_read_master_slave(struct phy_device *phydev)
> > +{
> > + int cfg, state = 0;
> > + u16 val;
> > +
> > + phydev->master_slave_get = 0;
> > + phydev->master_slave_state = 0;
>
> Could you use the _UNKNOWN #defined here?
ok
> > diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
> > index 92f737f101178..eb680e3d6bda5 100644
> > --- a/include/uapi/linux/ethtool.h
> > +++ b/include/uapi/linux/ethtool.h
> > @@ -1666,6 +1666,31 @@ static inline int ethtool_validate_duplex(__u8 duplex)
> > return 0;
> > }
> >
> > +static inline int ethtool_validate_master_slave_cfg(__u8 cfg)
> > +{
> > + switch (cfg) {
> > + case PORT_MODE_CFG_MASTER_PREFERRED:
> > + case PORT_MODE_CFG_SLAVE_PREFERRED:
> > + case PORT_MODE_CFG_MASTER_FORCE:
> > + case PORT_MODE_CFG_SLAVE_FORCE:
> > + case PORT_MODE_CFG_UNKNOWN:
> > + return 1;
> > + }
> > +
> > + return 0;
> > +}
>
> Does this need to be an inline function?
Yes, otherwise we get a lot of "defined but not used " warnings.
Regards,
Oleksij
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Hi Michal,
On Wed, Apr 29, 2020 at 09:52:22PM +0200, Michal Kubecek wrote:
> On Tue, Apr 28, 2020 at 09:53:07AM +0200, Oleksij Rempel wrote:
> > This UAPI is needed for BroadR-Reach 100BASE-T1 devices. Due to lack of
> > auto-negotiation support, we needed to be able to configure the
> > MASTER-SLAVE role of the port manually or from an application in user
> > space.
> >
> > The same UAPI can be used for 1000BASE-T or MultiGBASE-T devices to
> > force MASTER or SLAVE role. See IEEE 802.3-2018:
> > 22.2.4.3.7 MASTER-SLAVE control register (Register 9)
> > 22.2.4.3.8 MASTER-SLAVE status register (Register 10)
> > 40.5.2 MASTER-SLAVE configuration resolution
> > 45.2.1.185.1 MASTER-SLAVE config value (1.2100.14)
> > 45.2.7.10 MultiGBASE-T AN control 1 register (Register 7.32)
> >
> > The MASTER-SLAVE role affects the clock configuration:
> >
> > -------------------------------------------------------------------------------
> > When the PHY is configured as MASTER, the PMA Transmit function shall
> > source TX_TCLK from a local clock source. When configured as SLAVE, the
> > PMA Transmit function shall source TX_TCLK from the clock recovered from
> > data stream provided by MASTER.
> >
> > iMX6Q KSZ9031 XXX
> > ------\ /-----------\ /------------\
> > | | | | |
> > MAC |<----RGMII----->| PHY Slave |<------>| PHY Master |
> > |<--- 125 MHz ---+-<------/ | | \ |
> > ------/ \-----------/ \------------/
> > ^
> > \-TX_TCLK
> >
> > -------------------------------------------------------------------------------
> >
> > Since some clock or link related issues are only reproducible in a
> > specific MASTER-SLAVE-role, MAC and PHY configuration, it is beneficial
> > to provide generic (not 100BASE-T1 specific) interface to the user space
> > for configuration flexibility and trouble shooting.
> >
> > Signed-off-by: Oleksij Rempel <[email protected]>
> > ---
> [...]
> > diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> > index 72c69a9c8a98a..a6a774beb2f90 100644
> > --- a/drivers/net/phy/phy.c
> > +++ b/drivers/net/phy/phy.c
> > @@ -285,6 +285,9 @@ int phy_ethtool_ksettings_set(struct phy_device *phydev,
> > duplex != DUPLEX_FULL)))
> > return -EINVAL;
> >
> > + if (!ethtool_validate_master_slave_cfg(cmd->base.master_slave_cfg))
> > + return -EINVAL;
> > +
>
> Unless we can/want to pass extack down here, I would prefer to have the
> sanity check in ethtool_update_linkmodes() or ethtool_set_linkmodes() so
> that we can set meaningful error message and offending attribute in
> extack. (It could be even part of the policy.) Also, with the check only
> here, drivers/devices not calling phy_ethtool_set_link_ksettings()
> (directly or via phy_ethtool_set_link_ksettings()) and not handling the
> new members themselves would silently ignore any value from userspace.
ok
> > phydev->autoneg = autoneg;
> >
> > phydev->speed = speed;
> [...]
> > +static int genphy_setup_master_slave(struct phy_device *phydev)
> > +{
> > + u16 ctl = 0;
> > +
> > + if (!phydev->is_gigabit_capable)
> > + return 0;
>
> Shouldn't we rather return -EOPNOTSUPP if value different from
> CFG_UNKNOWN was requested?
sounds plausible.
> > +
> > + switch (phydev->master_slave_set) {
> > + case PORT_MODE_CFG_MASTER_PREFERRED:
> > + ctl |= CTL1000_PREFER_MASTER;
> > + break;
> > + case PORT_MODE_CFG_SLAVE_PREFERRED:
> > + break;
> > + case PORT_MODE_CFG_MASTER_FORCE:
> > + ctl |= CTL1000_AS_MASTER;
> > + /* fallthrough */
> > + case PORT_MODE_CFG_SLAVE_FORCE:
> > + ctl |= CTL1000_ENABLE_MASTER;
> > + break;
> > + case PORT_MODE_CFG_UNKNOWN:
> > + return 0;
> > + default:
> > + phydev_warn(phydev, "Unsupported Master/Slave mode\n");
> > + return 0;
> > + }
> [...]
> > diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
> > index 92f737f101178..eb680e3d6bda5 100644
> > --- a/include/uapi/linux/ethtool.h
> > +++ b/include/uapi/linux/ethtool.h
> > @@ -1666,6 +1666,31 @@ static inline int ethtool_validate_duplex(__u8 duplex)
> > return 0;
> > }
> >
> > +/* Port mode */
> > +#define PORT_MODE_CFG_UNKNOWN 0
> > +#define PORT_MODE_CFG_MASTER_PREFERRED 1
> > +#define PORT_MODE_CFG_SLAVE_PREFERRED 2
> > +#define PORT_MODE_CFG_MASTER_FORCE 3
> > +#define PORT_MODE_CFG_SLAVE_FORCE 4
> > +#define PORT_MODE_STATE_UNKNOWN 0
> > +#define PORT_MODE_STATE_MASTER 1
> > +#define PORT_MODE_STATE_SLAVE 2
> > +#define PORT_MODE_STATE_ERR 3
>
> You have "MASTER_SLAVE" or "master_slave" everywhere but "PORT_MODE" in
> these constants which is inconsistent.
What will be preferred name?
> > +
> > +static inline int ethtool_validate_master_slave_cfg(__u8 cfg)
> > +{
> > + switch (cfg) {
> > + case PORT_MODE_CFG_MASTER_PREFERRED:
> > + case PORT_MODE_CFG_SLAVE_PREFERRED:
> > + case PORT_MODE_CFG_MASTER_FORCE:
> > + case PORT_MODE_CFG_SLAVE_FORCE:
> > + case PORT_MODE_CFG_UNKNOWN:
> > + return 1;
> > + }
> > +
> > + return 0;
> > +}
>
> Should we really allow CFG_UNKNOWN in client requests? As far as I can
> see, this value is handled as no-op which should be rather expressed by
> absence of the attribute. Allowing the client to request a value,
> keeping current one and returning 0 (success) is IMHO wrong.
ok
> Also, should this function be in UAPI header?
It is placed together with other validate functions:
ethtool_validate_duplex
ethtool_validate_speed
Doing it in a different place, would be inconsistent.
> [...]
> > @@ -119,7 +123,12 @@ static int linkmodes_fill_reply(struct sk_buff *skb,
> > }
> >
> > if (nla_put_u32(skb, ETHTOOL_A_LINKMODES_SPEED, lsettings->speed) ||
> > - nla_put_u8(skb, ETHTOOL_A_LINKMODES_DUPLEX, lsettings->duplex))
> > + nla_put_u8(skb, ETHTOOL_A_LINKMODES_DUPLEX, lsettings->duplex) ||
> > + nla_put_u8(skb, ETHTOOL_A_LINKMODES_MASTER_SLAVE_CFG,
> > + lsettings->master_slave_cfg) ||
> > + nla_put_u8(skb, ETHTOOL_A_LINKMODES_MASTER_SLAVE_STATE,
> > + lsettings->master_slave_state))
> > +
> > return -EMSGSIZE;
>
> From the two handlers you introduced, it seems we only get CFG_UNKNOWN
> or STATE_UNKNOWN if driver or device does not support the feature at all
> so it would be IMHO more appropriate to omit the attribute in such case.
STATE_UNKNOWN is returned if link is not active.
Regards,
Oleksij
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On Wed, Apr 29, 2020 at 08:20:53PM +0200, Andrew Lunn wrote:
> > +static int tja11xx_config_aneg(struct phy_device *phydev)
> > +{
> > + u16 ctl = 0;
> > + int ret;
> > +
> > + switch (phydev->master_slave_set) {
> > + case PORT_MODE_CFG_MASTER_FORCE:
> > + case PORT_MODE_CFG_MASTER_PREFERRED:
> > + ctl |= MII_CFG1_MASTER_SLAVE;
> > + break;
> > + case PORT_MODE_CFG_SLAVE_FORCE:
> > + case PORT_MODE_CFG_SLAVE_PREFERRED:
> > + break;
> > + case PORT_MODE_CFG_UNKNOWN:
> > + return 0;
> > + default:
> > + phydev_warn(phydev, "Unsupported Master/Slave mode\n");
> > + return -ENOTSUPP;
> > + }
>
> Does the hardware actually support PORT_MODE_CFG_SLAVE_PREFERRED and
> PORT_MODE_CFG_MASTER_PREFERRED? I thought that required autoneg, which
> this PHY does not support? So i would of expected these two values to
> return ENOTSUPP?
I do not have strong opinion here. Will change it.
Regards,
Oleksij
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On Thu, Apr 30, 2020 at 07:00:58AM +0200, Oleksij Rempel wrote:
> > > diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
> > > index 92f737f101178..eb680e3d6bda5 100644
> > > --- a/include/uapi/linux/ethtool.h
> > > +++ b/include/uapi/linux/ethtool.h
> > > @@ -1666,6 +1666,31 @@ static inline int ethtool_validate_duplex(__u8 duplex)
> > > return 0;
> > > }
> > >
> > > +/* Port mode */
> > > +#define PORT_MODE_CFG_UNKNOWN 0
> > > +#define PORT_MODE_CFG_MASTER_PREFERRED 1
> > > +#define PORT_MODE_CFG_SLAVE_PREFERRED 2
> > > +#define PORT_MODE_CFG_MASTER_FORCE 3
> > > +#define PORT_MODE_CFG_SLAVE_FORCE 4
> > > +#define PORT_MODE_STATE_UNKNOWN 0
> > > +#define PORT_MODE_STATE_MASTER 1
> > > +#define PORT_MODE_STATE_SLAVE 2
> > > +#define PORT_MODE_STATE_ERR 3
> >
> > You have "MASTER_SLAVE" or "master_slave" everywhere but "PORT_MODE" in
> > these constants which is inconsistent.
>
> What will be preferred name?
Not sure, that would be rather question for people more familiar with
the hardware. What I wanted to say is that whether we use "master_slave"
or "port_mode", we should use the same everywhere.
> > > +
> > > +static inline int ethtool_validate_master_slave_cfg(__u8 cfg)
> > > +{
> > > + switch (cfg) {
> > > + case PORT_MODE_CFG_MASTER_PREFERRED:
> > > + case PORT_MODE_CFG_SLAVE_PREFERRED:
> > > + case PORT_MODE_CFG_MASTER_FORCE:
> > > + case PORT_MODE_CFG_SLAVE_FORCE:
> > > + case PORT_MODE_CFG_UNKNOWN:
> > > + return 1;
> > > + }
> > > +
> > > + return 0;
> > > +}
> >
> > Should we really allow CFG_UNKNOWN in client requests? As far as I can
> > see, this value is handled as no-op which should be rather expressed by
> > absence of the attribute. Allowing the client to request a value,
> > keeping current one and returning 0 (success) is IMHO wrong.
>
> ok
>
> > Also, should this function be in UAPI header?
>
> It is placed together with other validate functions:
> ethtool_validate_duplex
> ethtool_validate_speed
>
> Doing it in a different place, would be inconsistent.
Those two have been there since "ever". The important question is if we
want this function to be provided to userspace as part of UAPI (which
would also limit our options for future modifications.
>
> > [...]
> > > @@ -119,7 +123,12 @@ static int linkmodes_fill_reply(struct sk_buff *skb,
> > > }
> > >
> > > if (nla_put_u32(skb, ETHTOOL_A_LINKMODES_SPEED, lsettings->speed) ||
> > > - nla_put_u8(skb, ETHTOOL_A_LINKMODES_DUPLEX, lsettings->duplex))
> > > + nla_put_u8(skb, ETHTOOL_A_LINKMODES_DUPLEX, lsettings->duplex) ||
> > > + nla_put_u8(skb, ETHTOOL_A_LINKMODES_MASTER_SLAVE_CFG,
> > > + lsettings->master_slave_cfg) ||
> > > + nla_put_u8(skb, ETHTOOL_A_LINKMODES_MASTER_SLAVE_STATE,
> > > + lsettings->master_slave_state))
> > > +
> > > return -EMSGSIZE;
> >
> > From the two handlers you introduced, it seems we only get CFG_UNKNOWN
> > or STATE_UNKNOWN if driver or device does not support the feature at all
> > so it would be IMHO more appropriate to omit the attribute in such case.
>
> STATE_UNKNOWN is returned if link is not active.
How about distinguishing the two cases then? Omitting both if CFG is
CFG_UNKNOWN (i.e. driver does not support the feature) and sending
STATE=STATE_UNKNOWN to userspace only if we know it's a meaningful value
actually reported by the driver?
Michal
Hi Michal,
On Thu, Apr 30, 2020 at 10:20:20AM +0200, Michal Kubecek wrote:
> On Thu, Apr 30, 2020 at 07:00:58AM +0200, Oleksij Rempel wrote:
> > > > diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
> > > > index 92f737f101178..eb680e3d6bda5 100644
> > > > --- a/include/uapi/linux/ethtool.h
> > > > +++ b/include/uapi/linux/ethtool.h
> > > > @@ -1666,6 +1666,31 @@ static inline int ethtool_validate_duplex(__u8 duplex)
> > > > return 0;
> > > > }
> > > >
> > > > +/* Port mode */
> > > > +#define PORT_MODE_CFG_UNKNOWN 0
> > > > +#define PORT_MODE_CFG_MASTER_PREFERRED 1
> > > > +#define PORT_MODE_CFG_SLAVE_PREFERRED 2
> > > > +#define PORT_MODE_CFG_MASTER_FORCE 3
> > > > +#define PORT_MODE_CFG_SLAVE_FORCE 4
> > > > +#define PORT_MODE_STATE_UNKNOWN 0
> > > > +#define PORT_MODE_STATE_MASTER 1
> > > > +#define PORT_MODE_STATE_SLAVE 2
> > > > +#define PORT_MODE_STATE_ERR 3
> > >
> > > You have "MASTER_SLAVE" or "master_slave" everywhere but "PORT_MODE" in
> > > these constants which is inconsistent.
> >
> > What will be preferred name?
>
> Not sure, that would be rather question for people more familiar with
> the hardware. What I wanted to say is that whether we use "master_slave"
> or "port_mode", we should use the same everywhere.
Ok, I rename it all to MASTER_SLAVE prefix. It is the only common name
across the all documentations.
> > > > +
> > > > +static inline int ethtool_validate_master_slave_cfg(__u8 cfg)
> > > > +{
> > > > + switch (cfg) {
> > > > + case PORT_MODE_CFG_MASTER_PREFERRED:
> > > > + case PORT_MODE_CFG_SLAVE_PREFERRED:
> > > > + case PORT_MODE_CFG_MASTER_FORCE:
> > > > + case PORT_MODE_CFG_SLAVE_FORCE:
> > > > + case PORT_MODE_CFG_UNKNOWN:
> > > > + return 1;
> > > > + }
> > > > +
> > > > + return 0;
> > > > +}
> > >
> > > Should we really allow CFG_UNKNOWN in client requests? As far as I can
> > > see, this value is handled as no-op which should be rather expressed by
> > > absence of the attribute. Allowing the client to request a value,
> > > keeping current one and returning 0 (success) is IMHO wrong.
> >
> > ok
> >
> > > Also, should this function be in UAPI header?
> >
> > It is placed together with other validate functions:
> > ethtool_validate_duplex
> > ethtool_validate_speed
> >
> > Doing it in a different place, would be inconsistent.
>
> Those two have been there since "ever". The important question is if we
> want this function to be provided to userspace as part of UAPI (which
> would also limit our options for future modifications.
good argument. Moved it to other location.
> >
> > > [...]
> > > > @@ -119,7 +123,12 @@ static int linkmodes_fill_reply(struct sk_buff *skb,
> > > > }
> > > >
> > > > if (nla_put_u32(skb, ETHTOOL_A_LINKMODES_SPEED, lsettings->speed) ||
> > > > - nla_put_u8(skb, ETHTOOL_A_LINKMODES_DUPLEX, lsettings->duplex))
> > > > + nla_put_u8(skb, ETHTOOL_A_LINKMODES_DUPLEX, lsettings->duplex) ||
> > > > + nla_put_u8(skb, ETHTOOL_A_LINKMODES_MASTER_SLAVE_CFG,
> > > > + lsettings->master_slave_cfg) ||
> > > > + nla_put_u8(skb, ETHTOOL_A_LINKMODES_MASTER_SLAVE_STATE,
> > > > + lsettings->master_slave_state))
> > > > +
> > > > return -EMSGSIZE;
> > >
> > > From the two handlers you introduced, it seems we only get CFG_UNKNOWN
> > > or STATE_UNKNOWN if driver or device does not support the feature at all
> > > so it would be IMHO more appropriate to omit the attribute in such case.
> >
> > STATE_UNKNOWN is returned if link is not active.
>
> How about distinguishing the two cases then? Omitting both if CFG is
> CFG_UNKNOWN (i.e. driver does not support the feature) and sending
> STATE=STATE_UNKNOWN to userspace only if we know it's a meaningful value
> actually reported by the driver?
Hm... after thinking about it, we should keep state and config
separately. The TJA1102 PHY do not provide actual state. Even no error
related to Master/Master conflict. I reworked the code to have
unsupported and unknown values. In case we know, that state or config
is not supported, it will not be exported to the user space.
Regards,
Oleksij
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On Thu, Apr 30, 2020 at 02:09:45PM +0200, Oleksij Rempel wrote:
> > > > > @@ -119,7 +123,12 @@ static int linkmodes_fill_reply(struct sk_buff *skb,
> > > > > }
> > > > >
> > > > > if (nla_put_u32(skb, ETHTOOL_A_LINKMODES_SPEED, lsettings->speed) ||
> > > > > - nla_put_u8(skb, ETHTOOL_A_LINKMODES_DUPLEX, lsettings->duplex))
> > > > > + nla_put_u8(skb, ETHTOOL_A_LINKMODES_DUPLEX, lsettings->duplex) ||
> > > > > + nla_put_u8(skb, ETHTOOL_A_LINKMODES_MASTER_SLAVE_CFG,
> > > > > + lsettings->master_slave_cfg) ||
> > > > > + nla_put_u8(skb, ETHTOOL_A_LINKMODES_MASTER_SLAVE_STATE,
> > > > > + lsettings->master_slave_state))
> > > > > +
> > > > > return -EMSGSIZE;
> > > >
> > > > From the two handlers you introduced, it seems we only get CFG_UNKNOWN
> > > > or STATE_UNKNOWN if driver or device does not support the feature at all
> > > > so it would be IMHO more appropriate to omit the attribute in such case.
> > >
> > > STATE_UNKNOWN is returned if link is not active.
> >
> > How about distinguishing the two cases then? Omitting both if CFG is
> > CFG_UNKNOWN (i.e. driver does not support the feature) and sending
> > STATE=STATE_UNKNOWN to userspace only if we know it's a meaningful value
> > actually reported by the driver?
>
> Hm... after thinking about it, we should keep state and config
> separately. The TJA1102 PHY do not provide actual state. Even no error
> related to Master/Master conflict. I reworked the code to have
> unsupported and unknown values. In case we know, that state or config
> is not supported, it will not be exported to the user space.
Sounds good to me, splitting "unsupported" and "unknown" states will be
probably the best option.
Michal
On Thu, Apr 30, 2020 at 06:37:51AM +0200, Oleksij Rempel wrote:
> Hi Andrew,
>
> On Wed, Apr 29, 2020 at 08:16:14PM +0200, Andrew Lunn wrote:
> > On Tue, Apr 28, 2020 at 09:53:07AM +0200, Oleksij Rempel wrote:
> >
> > Hi Oleksij
> >
> > Sorry for taking a while to review this. I was busy fixing the FEC
> > driver which i broke :-(
>
> Not problem.
> Interesting, what is wrong with FEC? We use it a lot.
I broke MDIO transactions, when making them faster. Hopefully fixed.
> > > diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
> > > index 92f737f101178..eb680e3d6bda5 100644
> > > --- a/include/uapi/linux/ethtool.h
> > > +++ b/include/uapi/linux/ethtool.h
> > > @@ -1666,6 +1666,31 @@ static inline int ethtool_validate_duplex(__u8 duplex)
> > > return 0;
> > > }
> > >
> > > +static inline int ethtool_validate_master_slave_cfg(__u8 cfg)
> > > +{
> > > + switch (cfg) {
> > > + case PORT_MODE_CFG_MASTER_PREFERRED:
> > > + case PORT_MODE_CFG_SLAVE_PREFERRED:
> > > + case PORT_MODE_CFG_MASTER_FORCE:
> > > + case PORT_MODE_CFG_SLAVE_FORCE:
> > > + case PORT_MODE_CFG_UNKNOWN:
> > > + return 1;
> > > + }
> > > +
> > > + return 0;
> > > +}
> >
> > Does this need to be an inline function?
>
> Yes, otherwise we get a lot of "defined but not used " warnings.
Sorry, was not clear enough. I think there is only one user of this
function? Why not put it in the same compilation unit, as a normal
static function?
Andrew