2021-01-22 21:59:13

by Sergej Bauer

Subject: [PATCH v2] lan743x: add virtual PHY for PHY-less devices

From: [email protected]

v1->v2:
switch to using of fixed_phy as was suggested by Andrew and Florian
also features-related parts are removed

Previous versions can be found at:
v1:
initial version
https://lkml.org/lkml/2020/9/17/1272

Signed-off-by: Sergej Bauer <[email protected]>

diff --git a/MAINTAINERS b/MAINTAINERS
index 650deb973913..86304efd7691 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11699,6 +11699,12 @@ L: [email protected]
S: Maintained
F: drivers/net/ethernet/microchip/lan743x_*

+MICROCHIP LAN743X VIRTUAL PHY
+M: Sergej Bauer <[email protected]>
+L: [email protected]
+S: Maintained
+F: drivers/net/ethernet/microchip/lan743x_virtual_phy.*
+
MICROCHIP LCDFB DRIVER
M: Nicolas Ferre <[email protected]>
L: [email protected]
diff --git a/drivers/net/ethernet/microchip/Kconfig b/drivers/net/ethernet/microchip/Kconfig
index d0f6dfe0dcf3..fbc94cf115bd 100644
--- a/drivers/net/ethernet/microchip/Kconfig
+++ b/drivers/net/ethernet/microchip/Kconfig
@@ -48,6 +48,7 @@ config LAN743X
select PHYLIB
select CRC16
select CRC32
+ select FIXED_PHY
help
Support for the Microchip LAN743x PCI Express Gigabit Ethernet chip

diff --git a/drivers/net/ethernet/microchip/Makefile b/drivers/net/ethernet/microchip/Makefile
index da603540ca57..dc3a2d66c286 100644
--- a/drivers/net/ethernet/microchip/Makefile
+++ b/drivers/net/ethernet/microchip/Makefile
@@ -7,4 +7,4 @@ obj-$(CONFIG_ENC28J60) += enc28j60.o
obj-$(CONFIG_ENCX24J600) += encx24j600.o encx24j600-regmap.o
obj-$(CONFIG_LAN743X) += lan743x.o

-lan743x-objs := lan743x_main.o lan743x_ethtool.o lan743x_ptp.o
+lan743x-objs := lan743x_main.o lan743x_ethtool.o lan743x_ptp.o lan743x_virtual_phy.o
diff --git a/drivers/net/ethernet/microchip/lan743x_ethtool.c b/drivers/net/ethernet/microchip/lan743x_ethtool.c
index c5de8f46cdd3..22636366d0db 100644
--- a/drivers/net/ethernet/microchip/lan743x_ethtool.c
+++ b/drivers/net/ethernet/microchip/lan743x_ethtool.c
@@ -816,6 +816,17 @@ static int lan743x_ethtool_set_wol(struct net_device *netdev,
}
#endif /* CONFIG_PM */

+int lan743x_set_link_ksettings(struct net_device *netdev,
+ const struct ethtool_link_ksettings *cmd)
+{
+ if (!netdev->phydev)
+ return -ENETDOWN;
+
+ return phy_is_pseudo_fixed_link(netdev->phydev) ?
+ lan743x_set_virtual_link_ksettings(netdev, cmd)
+ : phy_ethtool_set_link_ksettings(netdev, cmd);
+}
+
const struct ethtool_ops lan743x_ethtool_ops = {
.get_drvinfo = lan743x_ethtool_get_drvinfo,
.get_msglevel = lan743x_ethtool_get_msglevel,
@@ -839,7 +850,7 @@ const struct ethtool_ops lan743x_ethtool_ops = {
.get_eee = lan743x_ethtool_get_eee,
.set_eee = lan743x_ethtool_set_eee,
.get_link_ksettings = phy_ethtool_get_link_ksettings,
- .set_link_ksettings = phy_ethtool_set_link_ksettings,
+ .set_link_ksettings = lan743x_set_link_ksettings,
#ifdef CONFIG_PM
.get_wol = lan743x_ethtool_get_wol,
.set_wol = lan743x_ethtool_set_wol,
diff --git a/drivers/net/ethernet/microchip/lan743x_ethtool.h b/drivers/net/ethernet/microchip/lan743x_ethtool.h
index d0d11a777a58..7287474d4a5c 100644
--- a/drivers/net/ethernet/microchip/lan743x_ethtool.h
+++ b/drivers/net/ethernet/microchip/lan743x_ethtool.h
@@ -8,4 +8,7 @@

extern const struct ethtool_ops lan743x_ethtool_ops;

+int lan743x_set_virtual_link_ksettings(struct net_device *netdev,
+ const struct ethtool_link_ksettings *cmd);
+
#endif /* _LAN743X_ETHTOOL_H */
diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c
index 3804310c853a..d8c00fa298d0 100644
--- a/drivers/net/ethernet/microchip/lan743x_main.c
+++ b/drivers/net/ethernet/microchip/lan743x_main.c
@@ -17,6 +17,11 @@
#include <linux/crc16.h>
#include "lan743x_main.h"
#include "lan743x_ethtool.h"
+#include "lan743x_virtual_phy.h"
+
+static char *mii_regs[32];
+static int mii_regs_count;
+module_param_array(mii_regs, charp, &mii_regs_count, 0644);

static void lan743x_pci_cleanup(struct lan743x_adapter *adapter)
{
@@ -821,7 +826,7 @@ static int lan743x_mac_init(struct lan743x_adapter *adapter)
return 0;
}

-static int lan743x_mac_open(struct lan743x_adapter *adapter)
+int lan743x_mac_open(struct lan743x_adapter *adapter)
{
u32 temp;

@@ -832,7 +837,7 @@ static int lan743x_mac_open(struct lan743x_adapter *adapter)
return 0;
}

-static void lan743x_mac_close(struct lan743x_adapter *adapter)
+void lan743x_mac_close(struct lan743x_adapter *adapter)
{
u32 temp;

@@ -1000,8 +1005,10 @@ static void lan743x_phy_close(struct lan743x_adapter *adapter)
struct net_device *netdev = adapter->netdev;

phy_stop(netdev->phydev);
- phy_disconnect(netdev->phydev);
- netdev->phydev = NULL;
+ if (phy_is_pseudo_fixed_link(netdev->phydev))
+ lan743x_virtual_phy_disconnect(netdev->phydev);
+ else
+ phy_disconnect(netdev->phydev);
}

static int lan743x_phy_open(struct lan743x_adapter *adapter)
@@ -1019,11 +1026,22 @@ static int lan743x_phy_open(struct lan743x_adapter *adapter)
/* try internal phy */
phydev = phy_find_first(adapter->mdiobus);
if (!phydev)
+ phydev = lan743x_virtual_phy(adapter, mii_regs,
+ mii_regs_count);
+
+ if (!phydev || IS_ERR(phydev))
goto return_error;

- ret = phy_connect_direct(netdev, phydev,
- lan743x_phy_link_status_change,
- PHY_INTERFACE_MODE_GMII);
+ if (phy_is_pseudo_fixed_link(phydev)) {
+ ret = phy_connect_direct(netdev, phydev,
+ lan743x_virtual_phy_status_change,
+ PHY_INTERFACE_MODE_MII);
+ } else {
+ ret = phy_connect_direct(netdev, phydev,
+ lan743x_phy_link_status_change,
+ PHY_INTERFACE_MODE_GMII);
+ }
+
if (ret)
goto return_error;
}
@@ -1031,6 +1049,15 @@ static int lan743x_phy_open(struct lan743x_adapter *adapter)
/* MAC doesn't support 1000T Half */
phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_1000baseT_Half_BIT);

+ if (phy_is_pseudo_fixed_link(phydev)) {
+ phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_TP_BIT);
+ linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Full_BIT,
+ phydev->supported);
+ linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT,
+ phydev->supported);
+ phy_advertise_supported(phydev);
+ }
+
/* support both flow controls */
phy_support_asym_pause(phydev);
phy->fc_request_control = (FLOW_CTRL_RX | FLOW_CTRL_TX);
@@ -2580,11 +2607,20 @@ static netdev_tx_t lan743x_netdev_xmit_frame(struct sk_buff *skb,
static int lan743x_netdev_ioctl(struct net_device *netdev,
struct ifreq *ifr, int cmd)
{
+ int ret;
+
if (!netif_running(netdev))
return -EINVAL;
+
if (cmd == SIOCSHWTSTAMP)
return lan743x_ptp_ioctl(netdev, ifr, cmd);
- return phy_mii_ioctl(netdev->phydev, ifr, cmd);
+
+ if (phy_is_pseudo_fixed_link(netdev->phydev))
+ ret = lan743x_virtual_phy_mii_ioctl(netdev->phydev, ifr, cmd);
+ else
+ ret = phy_mii_ioctl(netdev->phydev, ifr, cmd);
+
+ return ret;
}

static void lan743x_netdev_set_multicast(struct net_device *netdev)
diff --git a/drivers/net/ethernet/microchip/lan743x_main.h b/drivers/net/ethernet/microchip/lan743x_main.h
index 404af3f4635e..8e16fd0f166a 100644
--- a/drivers/net/ethernet/microchip/lan743x_main.h
+++ b/drivers/net/ethernet/microchip/lan743x_main.h
@@ -109,6 +109,7 @@
#define MAC_CR_EEE_EN_ BIT(17)
#define MAC_CR_ADD_ BIT(12)
#define MAC_CR_ASD_ BIT(11)
+#define MAC_CR_INT_LOOPBACK_ BIT(10)
#define MAC_CR_CNTR_RST_ BIT(5)
#define MAC_CR_DPX_ BIT(3)
#define MAC_CR_CFG_H_ BIT(2)
diff --git a/drivers/net/ethernet/microchip/lan743x_virtual_phy.c b/drivers/net/ethernet/microchip/lan743x_virtual_phy.c
new file mode 100644
index 000000000000..b6bf7c016448
--- /dev/null
+++ b/drivers/net/ethernet/microchip/lan743x_virtual_phy.c
@@ -0,0 +1,335 @@
+// SPDX-License-Identifier: GPL-2.0+
+// Copyright (C) 2021 Sergej Bauer
+#include <linux/netdevice.h>
+#include <linux/phy.h>
+#include <linux/phy_fixed.h>
+#include <linux/ethtool.h>
+#include <linux/rtnetlink.h>
+#include "lan743x_main.h"
+#include "lan743x_ethtool.h"
+#include "lan743x_virtual_phy.h"
+
+static u16 regs802p3[32];
+
+int lan743x_virtual_phy_mii_ioctl(struct phy_device *phydev, struct ifreq *ifr,
+ int cmd)
+{
+ struct ethtool_link_ksettings ksettings;
+ struct net_device *netdev = phydev->attached_dev;
+ struct mii_ioctl_data *mii_data = if_mii(ifr);
+ int ret = 0;
+ u16 val = mii_data->val_in;
+ u16 reg_num = mii_data->reg_num;
+
+ if (reg_num > 0x1f)
+ return -ERANGE;
+
+ memset(&ksettings, 0, sizeof(ksettings));
+ phy_ethtool_get_link_ksettings(netdev, &ksettings);
+ ksettings.base.autoneg = AUTONEG_ENABLE;
+
+ switch (cmd) {
+ case SIOCGMIIPHY:
+ break;
+ case SIOCGMIIREG:
+ mii_data->val_out = regs802p3[reg_num];
+ break;
+ case SIOCSMIIREG:
+ switch (reg_num) {
+ case MII_BMCR:
+ val &= ~BMCR_RESET;
+ if (!(val & BMCR_FULLDPLX))
+ val |= BMCR_FULLDPLX;
+ val &= ~0x1f;
+
+ if (val & BMCR_ANRESTART)
+ val &= ~BMCR_ANRESTART;
+
+ if ((val & BMCR_SPEED1000) && (val & BMCR_SPEED100)) {
+ ret = -EINVAL;
+ netdev_err(netdev, "invalid bits 0.6 and 0.13");
+ break;
+ }
+
+ ksettings.base.duplex = DUPLEX_FULL;
+
+ if (val & BMCR_SPEED1000)
+ ksettings.base.speed = SPEED_1000;
+ else if (val & BMCR_SPEED100)
+ ksettings.base.speed = SPEED_100;
+ else
+ ksettings.base.speed = SPEED_10;
+
+ break;
+
+ case 1:
+ case 2:
+ case 3:
+ case 15:
+ default:
+ regs802p3[reg_num] = val;
+ break;
+ }
+ ret = lan743x_set_virtual_link_ksettings(netdev, &ksettings);
+ if (ret == 0)
+ regs802p3[reg_num] = val;
+
+ break;
+ default:
+ netdev_err(netdev, "not supported ioctl %X\n", cmd);
+ ret = -EOPNOTSUPP;
+ };
+
+ return ret;
+}
+
+void lan743x_virtual_phy_disconnect(struct phy_device *phydev)
+{
+ struct net_device *attached_dev = phydev->attached_dev;
+
+ if (phydev->sysfs_links) {
+ if (phydev->attached_dev)
+ sysfs_remove_link(&attached_dev->dev.kobj,
+ "phydev");
+ sysfs_remove_link(&phydev->mdio.dev.kobj,
+ "attached_dev");
+ }
+
+ fixed_phy_unregister(phydev);
+}
+
+void lan743x_virtual_phy_status_change(struct net_device *netdev)
+{
+ struct lan743x_adapter *adapter = netdev_priv(netdev);
+ struct phy_device *phydev = netdev->phydev;
+ u32 data;
+
+ if (!netdev->phydev)
+ return;
+
+ if (phydev->state == PHY_RUNNING) {
+ data = lan743x_csr_read(adapter, MAC_CR);
+ data |= MAC_CR_INT_LOOPBACK_;
+ data &= ~MAC_CR_MII_EN_;
+ data |= MAC_CR_DPX_;
+
+ switch (phydev->speed) {
+ case SPEED_10:
+ data &= ~MAC_CR_CFG_H_;
+ data &= ~MAC_CR_CFG_L_;
+ break;
+ case SPEED_100:
+ data &= ~MAC_CR_CFG_H_;
+ data |= MAC_CR_CFG_L_;
+ break;
+ case SPEED_1000:
+ data |= MAC_CR_CFG_H_;
+ data &= ~MAC_CR_CFG_L_;
+ break;
+ }
+ lan743x_csr_write(adapter, MAC_CR, data);
+ }
+ phy_print_status(phydev);
+}
+
+struct phy_device *lan743x_virtual_phy(struct lan743x_adapter *adapter,
+ char *mii_regs[], int mii_regs_count)
+{
+ struct phy_device *phydev = NULL;
+ struct net_device *netdev = adapter->netdev;
+ struct mii_ioctl_data mii_data;
+ struct ifreq ifr;
+ long res;
+ int ret, i;
+
+ struct fixed_phy_status status = {
+ .link = 1,
+ .speed = SPEED_1000,
+ .duplex = DUPLEX_FULL,
+ .asym_pause = 1,
+ };
+
+ phydev = fixed_phy_register(PHY_POLL, &status, NULL);
+ if (!phydev || IS_ERR(phydev)) {
+ netif_err(adapter, ifup, adapter->netdev,
+ "failed to regiter fixed_phy\n");
+ goto out;
+ }
+ phydev->attached_dev = netdev;
+ netdev->phydev = phydev;
+
+ linkmode_clear_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT,
+ phydev->supported);
+ linkmode_clear_bit(ETHTOOL_LINK_MODE_100baseT_Half_BIT,
+ phydev->supported);
+ linkmode_clear_bit(ETHTOOL_LINK_MODE_1000baseT_Half_BIT,
+ phydev->supported);
+
+ if (mii_regs_count > 0) {
+ for (i = 0; i < mii_regs_count; i++) {
+ mii_data.reg_num = i;
+ ret = kstrtol(mii_regs[i], 16, &res);
+
+ if (ret == 0)
+ mii_data.val_in = res;
+ else
+ mii_data.val_in = 0xffff;
+
+ memcpy(&ifr.ifr_ifru.ifru_data, &mii_data,
+ sizeof(mii_data));
+ ret = lan743x_virtual_phy_mii_ioctl(phydev, &ifr,
+ SIOCSMIIREG);
+ if (ret < 0)
+ return NULL;
+ }
+ } else {
+ mii_data.reg_num = MII_BMSR;
+ mii_data.val_in = BMSR_100FULL | BMSR_10FULL | BMSR_ESTATEN |
+ BMSR_ANEGCOMPLETE | BMSR_ANEGCAPABLE | BMSR_LSTATUS |
+ BMSR_ERCAP;
+ memcpy(&ifr.ifr_ifru.ifru_data, &mii_data, sizeof(mii_data));
+ lan743x_virtual_phy_mii_ioctl(phydev, &ifr, SIOCSMIIREG);
+
+ mii_data.reg_num = MII_PHYSID1;
+ mii_data.val_in = (adapter->csr.id_rev & ID_REV_ID_MASK_) >> 16;
+ memcpy(&ifr.ifr_ifru.ifru_data, &mii_data, sizeof(mii_data));
+ lan743x_virtual_phy_mii_ioctl(phydev, &ifr, SIOCSMIIREG);
+
+ mii_data.reg_num = MII_PHYSID2;
+ mii_data.val_in = adapter->csr.id_rev & ID_REV_CHIP_REV_MASK_;
+ memcpy(&ifr.ifr_ifru.ifru_data, &mii_data, sizeof(mii_data));
+ lan743x_virtual_phy_mii_ioctl(phydev, &ifr, SIOCSMIIREG);
+
+ mii_data.reg_num = MII_ADVERTISE;
+ mii_data.val_in = ADVERTISE_100FULL | ADVERTISE_10FULL |
+ ADVERTISE_LPACK;
+ memcpy(&ifr.ifr_ifru.ifru_data, &mii_data, sizeof(mii_data));
+ lan743x_virtual_phy_mii_ioctl(phydev, &ifr, SIOCSMIIREG);
+
+ mii_data.reg_num = MII_LPA;
+ mii_data.val_in = ADVERTISE_100FULL | ADVERTISE_10FULL |
+ ADVERTISE_LPACK;
+ memcpy(&ifr.ifr_ifru.ifru_data, &mii_data, sizeof(mii_data));
+ lan743x_virtual_phy_mii_ioctl(phydev, &ifr, SIOCSMIIREG);
+
+ mii_data.reg_num = MII_EXPANSION;
+ mii_data.val_in = EXPANSION_NWAY | EXPANSION_ENABLENPAGE |
+ EXPANSION_NPCAPABLE;
+ memcpy(&ifr.ifr_ifru.ifru_data, &mii_data, sizeof(mii_data));
+ lan743x_virtual_phy_mii_ioctl(phydev, &ifr, SIOCSMIIREG);
+
+ mii_data.reg_num = MII_CTRL1000;
+ mii_data.val_in = ADVERTISE_1000FULL;
+ memcpy(&ifr.ifr_ifru.ifru_data, &mii_data, sizeof(mii_data));
+ lan743x_virtual_phy_mii_ioctl(phydev, &ifr, SIOCSMIIREG);
+
+ mii_data.reg_num = MII_STAT1000;
+ mii_data.val_in = LPA_1000FULL | LPA_1000REMRXOK |
+ LPA_1000LOCALRXOK | LPA_1000MSRES;
+ memcpy(&ifr.ifr_ifru.ifru_data, &mii_data, sizeof(mii_data));
+ lan743x_virtual_phy_mii_ioctl(phydev, &ifr, SIOCSMIIREG);
+
+ mii_data.reg_num = MII_ESTATUS;
+ mii_data.val_in = ESTATUS_1000_TFULL;
+ memcpy(&ifr.ifr_ifru.ifru_data, &mii_data, sizeof(mii_data));
+ lan743x_virtual_phy_mii_ioctl(phydev, &ifr, SIOCSMIIREG);
+
+ mii_data.reg_num = MII_BMCR;
+ mii_data.val_in = BMCR_SPEED1000 | BMCR_FULLDPLX |
+ BMCR_LOOPBACK | BMCR_ANENABLE;
+ memcpy(&ifr.ifr_ifru.ifru_data, &mii_data, sizeof(mii_data));
+ lan743x_virtual_phy_mii_ioctl(phydev, &ifr, SIOCSMIIREG);
+ }
+
+ phydev->attached_dev = NULL;
+out:
+ return phydev;
+}
+
+int lan743x_set_virtual_link_ksettings(struct net_device *netdev,
+ const struct ethtool_link_ksettings *cmd)
+{
+ struct ethtool_link_ksettings *cmd_ref =
+ (struct ethtool_link_ksettings *)cmd;
+ struct lan743x_adapter *adapter = netdev_priv(netdev);
+ u32 speed = cmd->base.speed;
+ u8 duplex = cmd->base.duplex;
+ u16 v = 0;
+ u32 data;
+
+ if (!netdev->phydev)
+ return -ENETDOWN;
+ if (speed &&
+ speed != SPEED_10 && speed != SPEED_100 && speed != SPEED_1000) {
+ netdev_err(netdev, "%s: unknown speed %d\n", netdev->name,
+ speed);
+ return -EINVAL;
+ }
+
+ v = regs802p3[0];
+ v &= ~(BMCR_SPEED1000 | BMCR_SPEED100);
+ data = lan743x_csr_read(adapter, MAC_CR);
+ duplex = DUPLEX_FULL;
+
+ if (speed != adapter->netdev->phydev->speed) {
+ lan743x_mac_close(adapter);
+ switch (speed) {
+ case 0:
+ break;
+ case SPEED_10:
+ data &= ~(MAC_CR_CFG_H_ | MAC_CR_CFG_L_);
+ lan743x_csr_write(adapter, MAC_CR, data);
+
+ regs802p3[MII_ADVERTISE] = ADVERTISE_10FULL |
+ ADVERTISE_100FULL |
+ ADVERTISE_LPACK;
+ regs802p3[MII_LPA] = LPA_10FULL | LPA_LPACK;
+ regs802p3[MII_EXPANSION] = ESTATUS_1000_TFULL;
+ regs802p3[MII_CTRL1000] = ADVERTISE_1000FULL;
+ regs802p3[MII_STAT1000] = 0;
+ break;
+ case SPEED_100:
+ v |= BMCR_SPEED100;
+ data &= ~MAC_CR_CFG_H_;
+ data |= MAC_CR_CFG_L_;
+ lan743x_csr_write(adapter, MAC_CR, data);
+
+ regs802p3[MII_ADVERTISE] = ADVERTISE_10FULL |
+ ADVERTISE_100FULL |
+ ADVERTISE_LPACK;
+ regs802p3[MII_LPA] = LPA_10FULL | LPA_100FULL |
+ LPA_LPACK;
+ regs802p3[MII_EXPANSION] = ESTATUS_1000_TFULL;
+ regs802p3[MII_CTRL1000] = ADVERTISE_1000FULL;
+ regs802p3[MII_STAT1000] = 0;
+ break;
+ case SPEED_1000:
+ v |= BMCR_SPEED1000;
+ data |= MAC_CR_CFG_H_;
+ data &= ~MAC_CR_CFG_L_;
+ lan743x_csr_write(adapter, MAC_CR, data);
+
+ regs802p3[MII_ADVERTISE] = ADVERTISE_10FULL |
+ ADVERTISE_100FULL |
+ ADVERTISE_LPACK;
+ regs802p3[MII_LPA] = LPA_10FULL | LPA_100FULL |
+ LPA_LPACK;
+ regs802p3[MII_EXPANSION] = ESTATUS_1000_TFULL;
+ regs802p3[MII_CTRL1000] = ADVERTISE_1000FULL;
+ regs802p3[MII_STAT1000] = LPA_1000FULL |
+ LPA_1000REMRXOK | LPA_1000LOCALRXOK |
+ LPA_1000MSRES;
+ break;
+ };
+ lan743x_mac_open(adapter);
+
+ v |= BMCR_LOOPBACK | BMCR_FULLDPLX | BMCR_ANENABLE;
+ regs802p3[0] = v;
+ }
+
+ cmd_ref->base.duplex = duplex;
+ cmd_ref->base.autoneg = AUTONEG_ENABLE;
+
+ return phy_ethtool_set_link_ksettings(netdev, cmd_ref);
+}
+
diff --git a/drivers/net/ethernet/microchip/lan743x_virtual_phy.h b/drivers/net/ethernet/microchip/lan743x_virtual_phy.h
new file mode 100644
index 000000000000..caef07d73d74
--- /dev/null
+++ b/drivers/net/ethernet/microchip/lan743x_virtual_phy.h
@@ -0,0 +1,20 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/* Copyright (C) 2021 Sergej Bauer */
+#ifndef LAN743X_FAKE_PHY_H
+#define LAN743X_FAKE_PHY_H
+
+#include "lan743x_main.h"
+
+struct phy_device *lan743x_virtual_phy(struct lan743x_adapter *adapter,
+ char *mii_regs[], int mii_regs_count);
+void lan743x_virtual_phy_disconnect(struct phy_device *phydev);
+int lan743x_virtual_phy_mii_ioctl(struct phy_device *phydev, struct ifreq *ifr,
+ int cmd);
+void lan743x_virtual_phy_status_change(struct net_device *netdev);
+int lan743x_set_virtual_link_ksettings(struct net_device *netdev,
+ const struct ethtool_link_ksettings *cmd);
+
+int lan743x_mac_open(struct lan743x_adapter *adapter);
+void lan743x_mac_close(struct lan743x_adapter *adapter);
+
+#endif

2021-01-22 22:13:16

by Andrew Lunn

Subject: Re: [PATCH v2] lan743x: add virtual PHY for PHY-less devices

On Sat, Jan 23, 2021 at 12:42:41AM +0300, Sergej Bauer wrote:
> From: [email protected]
>
> v1->v2:
> switch to using of fixed_phy as was suggested by Andrew and Florian
> also features-related parts are removed

This is not using fixed_phy, at least not in the normal way.

Take a look at bgmac_phy_connect_direct() for example. Call
fixed_phy_register(), and then phy_connect_direct() with the
phydev. End of story. Done.

> +int lan743x_set_link_ksettings(struct net_device *netdev,
> + const struct ethtool_link_ksettings *cmd)
> +{
> + if (!netdev->phydev)
> + return -ENETDOWN;
> +
> + return phy_is_pseudo_fixed_link(netdev->phydev) ?
> + lan743x_set_virtual_link_ksettings(netdev, cmd)
> + : phy_ethtool_set_link_ksettings(netdev, cmd);
> +}

There should not be any need to do something different. The whole
point of fixed_phy is it looks like a PHY. So calling
phy_ethtool_set_link_ksettings() should work, nothing special needed.

> @@ -1000,8 +1005,10 @@ static void lan743x_phy_close(struct lan743x_adapter *adapter)
> struct net_device *netdev = adapter->netdev;
>
> phy_stop(netdev->phydev);
> - phy_disconnect(netdev->phydev);
> - netdev->phydev = NULL;
> + if (phy_is_pseudo_fixed_link(netdev->phydev))
> + lan743x_virtual_phy_disconnect(netdev->phydev);
> + else
> + phy_disconnect(netdev->phydev);

phy_disconnect() should work. You might want to call
fixed_phy_unregister() afterwards, so you do not leak memory.

> + if (phy_is_pseudo_fixed_link(phydev)) {
> + ret = phy_connect_direct(netdev, phydev,
> + lan743x_virtual_phy_status_change,
> + PHY_INTERFACE_MODE_MII);
> + } else {
> + ret = phy_connect_direct(netdev, phydev,
> + lan743x_phy_link_status_change,

There should not be any need for a special link change
callback. lan743x_phy_link_status_change() should work fine, the MAC
should have no idea it is using a fixed_phy.

> + PHY_INTERFACE_MODE_GMII);
> + }
> +
> if (ret)
> goto return_error;
> }
> @@ -1031,6 +1049,15 @@ static int lan743x_phy_open(struct lan743x_adapter *adapter)
> /* MAC doesn't support 1000T Half */
> phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_1000baseT_Half_BIT);
>
> + if (phy_is_pseudo_fixed_link(phydev)) {
> + phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_TP_BIT);
> + linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Full_BIT,
> + phydev->supported);
> + linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT,
> + phydev->supported);
> + phy_advertise_supported(phydev);
> + }

The fixed PHY driver will set these bits depending on the speed it has
been configured for. No need to change them. The MAC should also not
care if it is TP, AUI, Fibre or smoke signals.

Andrew

2021-01-22 23:13:58

by Sergej Bauer

Subject: Re: [PATCH v2] lan743x: add virtual PHY for PHY-less devices

On Saturday, January 23, 2021 1:08:03 AM MSK Andrew Lunn wrote:
> On Sat, Jan 23, 2021 at 12:42:41AM +0300, Sergej Bauer wrote:
> > From: [email protected]
> >
> > v1->v2:
> > switch to using of fixed_phy as was suggested by Andrew and Florian
> > also features-related parts are removed
>
> This is not using fixed_phy, at least not in the normal way.
>
> Take a look at bgmac_phy_connect_direct() for example. Call
> fixed_phy_register(), and then phy_connect_direct() with the
> phydev. End of story. Done.
>
> > +int lan743x_set_link_ksettings(struct net_device *netdev,
> > + const struct ethtool_link_ksettings *cmd)
> > +{
> > + if (!netdev->phydev)
> > + return -ENETDOWN;
> > +
> > + return phy_is_pseudo_fixed_link(netdev->phydev) ?
> > + lan743x_set_virtual_link_ksettings(netdev, cmd)
> > + : phy_ethtool_set_link_ksettings(netdev, cmd);
> > +}
>
> There should not be any need to do something different. The whole
> point of fixed_phy is it looks like a PHY. So calling
> phy_ethtool_set_link_ksettings() should work, nothing special needed.
>
> > @@ -1000,8 +1005,10 @@ static void lan743x_phy_close(struct
> > lan743x_adapter *adapter)>
> > struct net_device *netdev = adapter->netdev;
> >
> > phy_stop(netdev->phydev);
> >
> > - phy_disconnect(netdev->phydev);
> > - netdev->phydev = NULL;
> > + if (phy_is_pseudo_fixed_link(netdev->phydev))
> > + lan743x_virtual_phy_disconnect(netdev->phydev);
> > + else
> > + phy_disconnect(netdev->phydev);
>
> phy_disconnect() should work. You might want to call
Andrew, this is why I was playing with my own _connect/_disconnect
realizations
It should work but it don't.
modprobe lan743x
rmmod lan743x
modprobe lan743x
leads to
"net eth7: could not add device link to fixed-0:06 err -17"
in dmesg (it does not removes corresponding phydev file in /sysfs)
moreover phy_disconnect leads to kernel panic
[82363.976726] BUG: kernel NULL pointer dereference, address: 00000000000003c4
[82363.977402] #PF: supervisor read access in kernel mode
[82363.978077] #PF: error_code(0x0000) - not-present page
[82363.978588] PGD 0 P4D 0
[82363.978588] Oops: 0000 [#1] SMP NOPTI
[82363.978588] CPU: 3 PID: 2634 Comm: ifconfig Tainted: G O
5.11.0-rc4net-next-virtual_phy+ #3
[82363.978588] Hardware name: Gigabyte Technology Co., Ltd. H110-D3/H110-D3-
CF, BIOS F24 04/11/2018
[82363.978588] RIP: 0010:lan743x_phy_close.isra.26+0x28/0x40 [lan743x]
[82363.978588] Code: c3 90 0f 1f 44 00 00 53 48 89 fb 48 8b bf 28 08 00 00 e8
ab 5e 86 ff 48 8b bb 28 08 00 00 e8 4f 92 86 ff 48 8b bb 28 08 00 00 <f6> 87 c4
03 00 00 04 75 02 5b c3 5b e9 f7 35 ea ff 0f 1f 80 00 00
[82363.982448] RSP: 0018:ffffa528c04c7c38 EFLAGS: 00010246
[82363.982448] RAX: 000000000000000f RBX: ffff991a47e38000 RCX: 0000000000000027
[82363.982448] RDX: 0000000000000000 RSI: ffff991c76d98b30 RDI: 0000000000000000
[82363.982448] RBP: 0000000000000001 R08: 0000000000000000 R09: c0000000ffffefff
[82363.982448] R10: 0000000000000001 R11: ffffa528c04c7a48 R12: ffff991a47e388c0
[82363.986855] R13: ffff991a47e390b8 R14: ffff991a47e39050 R15: ffff991a47e388c0
[82363.986855] FS: 00007f7c3ebd6540(0000) GS:ffff991c76d80000(0000) knlGS:
0000000000000000
[82363.986855] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[82363.986855] CR2: 00000000000003c4 CR3: 000000001b2ac005 CR4:
00000000003706e0
[82363.986855] Call Trace:
[82363.986855] lan743x_netdev_close+0x223/0x250 [lan743x]
...

> fixed_phy_unregister() afterwards, so you do not leak memory.
>
> > + if (phy_is_pseudo_fixed_link(phydev)) {
> > + ret = phy_connect_direct(netdev, phydev,
> > + lan743x_virtual_phy_status_change,
> > + PHY_INTERFACE_MODE_MII);
> > + } else {
> > + ret = phy_connect_direct(netdev, phydev,
> > + lan743x_phy_link_status_change,
>
> There should not be any need for a special link change
> callback. lan743x_phy_link_status_change() should work fine, the MAC
> should have no idea it is using a fixed_phy.
>
> > + PHY_INTERFACE_MODE_GMII);
> > + }
> > +
> >
> > if (ret)
> >
> > goto return_error;
> >
> > }
> >
> > @@ -1031,6 +1049,15 @@ static int lan743x_phy_open(struct lan743x_adapter
> > *adapter)>
> > /* MAC doesn't support 1000T Half */
> > phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_1000baseT_Half_BIT);
> >
> > + if (phy_is_pseudo_fixed_link(phydev)) {
> > + phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_TP_BIT);
> > + linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Full_BIT,
> > + phydev->supported);
> > + linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT,
> > + phydev->supported);
> > + phy_advertise_supported(phydev);
> > + }
>
> The fixed PHY driver will set these bits depending on the speed it has
> been configured for. No need to change them. The MAC should also not
> care if it is TP, AUI, Fibre or smoke signals.
It was to make ethtool show full set of supported speeds and MII only in
supported ports (without TP and the no any ports in the bare card).

>
> Andrew

I think I should reproduce the panic with clean version of net-net

--
Regards,
Sergej.

2021-01-22 23:28:08

by Andrew Lunn

Subject: Re: [PATCH v2] lan743x: add virtual PHY for PHY-less devices

> > > @@ -1000,8 +1005,10 @@ static void lan743x_phy_close(struct
> > > lan743x_adapter *adapter)>
> > > struct net_device *netdev = adapter->netdev;
> > >
> > > phy_stop(netdev->phydev);
> > >
> > > - phy_disconnect(netdev->phydev);
> > > - netdev->phydev = NULL;
> > > + if (phy_is_pseudo_fixed_link(netdev->phydev))
> > > + lan743x_virtual_phy_disconnect(netdev->phydev);
> > > + else
> > > + phy_disconnect(netdev->phydev);
> >
> > phy_disconnect() should work. You might want to call

There are drivers which call phy_disconnect() on a fixed_link. e.g.

https://elixir.bootlin.com/linux/v5.11-rc4/source/drivers/net/usb/lan78xx.c#L3555.

It could be your missing call to fixed_phy_unregister() is leaving
behind bad state.

> It was to make ethtool show full set of supported speeds and MII only in
> supported ports (without TP and the no any ports in the bare card).

But fixed link does not support the full set of speed. It is fixed. It
supports only one speed it is configured with. And by setting it
wrongly, you are going to allow the user to do odd things, like use
ethtool force the link speed to a speed which is not actually
supported.

Andrew

2021-01-23 00:02:42

by Sergej Bauer

Subject: Re: [PATCH v2] lan743x: add virtual PHY for PHY-less devices

On Saturday, January 23, 2021 2:23:25 AM MSK Andrew Lunn wrote:
> > > > @@ -1000,8 +1005,10 @@ static void lan743x_phy_close(struct
> > > > lan743x_adapter *adapter)>
> > > >
> > > > struct net_device *netdev = adapter->netdev;
> > > >
> > > > phy_stop(netdev->phydev);
> > > >
> > > > - phy_disconnect(netdev->phydev);
> > > > - netdev->phydev = NULL;
> > > > + if (phy_is_pseudo_fixed_link(netdev->phydev))
> > > > + lan743x_virtual_phy_disconnect(netdev->phydev);
> > > > + else
> > > > + phy_disconnect(netdev->phydev);
> > >
> > > phy_disconnect() should work. You might want to call
>
> There are drivers which call phy_disconnect() on a fixed_link. e.g.
>
> https://elixir.bootlin.com/linux/v5.11-rc4/source/drivers/net/usb/lan78xx.c#
> L3555.
>

> It could be your missing call to fixed_phy_unregister() is leaving
> behind bad state.
>
lan743x_virtual_phy_disconnect removes sysfs-links and calls
fixed_phy_unregister()
and the reason was phydev in sysfs.

> > It was to make ethtool show full set of supported speeds and MII only in
> > supported ports (without TP and the no any ports in the bare card).
>
> But fixed link does not support the full set of speed. It is fixed. It
> supports only one speed it is configured with.
That's why I "re-implemented the fixed PHY driver" as Florian said.
The goal of virtual phy was to make an illusion of real device working in
loopback mode. So I could use ethtool and ioctl's to switch speed of device.

> And by setting it
> wrongly, you are going to allow the user to do odd things, like use
> ethtool force the link speed to a speed which is not actually
> supported.
I have lan743x only and in loopback mode it allows to use speeds
10/100/1000MBps
in full-duplex mode only. But the highest speed I have achived was something
near
752Mbps...
And I can switch speed on the fly, without reloading the module.

May by I should limit the list of acceptable devices?

>
> Andrew

2021-01-23 00:07:47

by Florian Fainelli

Subject: Re: [PATCH v2] lan743x: add virtual PHY for PHY-less devices

On 1/22/2021 3:58 PM, Sergej Bauer wrote:
> On Saturday, January 23, 2021 2:23:25 AM MSK Andrew Lunn wrote:
>>>>> @@ -1000,8 +1005,10 @@ static void lan743x_phy_close(struct
>>>>> lan743x_adapter *adapter)>
>>>>>
>>>>> struct net_device *netdev = adapter->netdev;
>>>>>
>>>>> phy_stop(netdev->phydev);
>>>>>
>>>>> - phy_disconnect(netdev->phydev);
>>>>> - netdev->phydev = NULL;
>>>>> + if (phy_is_pseudo_fixed_link(netdev->phydev))
>>>>> + lan743x_virtual_phy_disconnect(netdev->phydev);
>>>>> + else
>>>>> + phy_disconnect(netdev->phydev);
>>>>
>>>> phy_disconnect() should work. You might want to call
>>
>> There are drivers which call phy_disconnect() on a fixed_link. e.g.
>>
>> https://elixir.bootlin.com/linux/v5.11-rc4/source/drivers/net/usb/lan78xx.c#
>> L3555.
>>
>
>> It could be your missing call to fixed_phy_unregister() is leaving
>> behind bad state.
>>
> lan743x_virtual_phy_disconnect removes sysfs-links and calls
> fixed_phy_unregister()
> and the reason was phydev in sysfs.
>
>>> It was to make ethtool show full set of supported speeds and MII only in
>>> supported ports (without TP and the no any ports in the bare card).
>>
>> But fixed link does not support the full set of speed. It is fixed. It
>> supports only one speed it is configured with.
> That's why I "re-implemented the fixed PHY driver" as Florian said.
> The goal of virtual phy was to make an illusion of real device working in
> loopback mode. So I could use ethtool and ioctl's to switch speed of device.
>
>> And by setting it
>> wrongly, you are going to allow the user to do odd things, like use
>> ethtool force the link speed to a speed which is not actually
>> supported.
> I have lan743x only and in loopback mode it allows to use speeds
> 10/100/1000MBps
> in full-duplex mode only. But the highest speed I have achived was something
> near
> 752Mbps...
> And I can switch speed on the fly, without reloading the module.
>
> May by I should limit the list of acceptable devices?

It is not clear what your use case is so maybe start with explaining it
and we can help you define something that may be acceptable for upstream
inclusion.
--
Florian

2021-01-23 01:06:29

by Sergej Bauer

Subject: Re: [PATCH v2] lan743x: add virtual PHY for PHY-less devices

On Saturday, January 23, 2021 3:01:47 AM MSK Florian Fainelli wrote:
> On 1/22/2021 3:58 PM, Sergej Bauer wrote:
> > On Saturday, January 23, 2021 2:23:25 AM MSK Andrew Lunn wrote:
> >>>>> @@ -1000,8 +1005,10 @@ static void lan743x_phy_close(struct
> >>>>> lan743x_adapter *adapter)>
> >>>>>
> >>>>> struct net_device *netdev = adapter->netdev;
> >>>>>
> >>>>> phy_stop(netdev->phydev);
> >>>>>
> >>>>> - phy_disconnect(netdev->phydev);
> >>>>> - netdev->phydev = NULL;
> >>>>> + if (phy_is_pseudo_fixed_link(netdev->phydev))
> >>>>> + lan743x_virtual_phy_disconnect(netdev->phydev);
> >>>>> + else
> >>>>> + phy_disconnect(netdev->phydev);
> >>>>
> >>>> phy_disconnect() should work. You might want to call
> >>
> >> There are drivers which call phy_disconnect() on a fixed_link. e.g.
> >>
> >> https://elixir.bootlin.com/linux/v5.11-rc4/source/drivers/net/usb/lan78xx
> >> .c# L3555.
> >>
> >>
> >> It could be your missing call to fixed_phy_unregister() is leaving
> >> behind bad state.
> >
> > lan743x_virtual_phy_disconnect removes sysfs-links and calls
> > fixed_phy_unregister()
> > and the reason was phydev in sysfs.
> >
> >>> It was to make ethtool show full set of supported speeds and MII only in
> >>> supported ports (without TP and the no any ports in the bare card).
> >>
> >> But fixed link does not support the full set of speed. It is fixed. It
> >> supports only one speed it is configured with.
> >
> > That's why I "re-implemented the fixed PHY driver" as Florian said.
> > The goal of virtual phy was to make an illusion of real device working in
> > loopback mode. So I could use ethtool and ioctl's to switch speed of
> > device.>
> >> And by setting it
> >> wrongly, you are going to allow the user to do odd things, like use
> >> ethtool force the link speed to a speed which is not actually
> >> supported.
> >
> > I have lan743x only and in loopback mode it allows to use speeds
> > 10/100/1000MBps
> > in full-duplex mode only. But the highest speed I have achived was
> > something near
> > 752Mbps...
> > And I can switch speed on the fly, without reloading the module.
> >
> > May by I should limit the list of acceptable devices?
>
> It is not clear what your use case is so maybe start with explaining it
> and we can help you define something that may be acceptable for upstream
> inclusion.
it migth be helpful for developers work on userspace networking tools with
PHY-less lan743x (the interface even could not be brought up)
of course, there nothing much to do without TP port but the difference is
representative.

sbauer@metamini ~$ sudo ethtool eth7
Settings for eth7:
Cannot get device settings: No such device
Supports Wake-on: pumbag
Wake-on: d
Current message level: 0x00000137 (311)
drv probe link ifdown ifup tx_queued
Link detected: no
sbauer@metamini ~$ sudo ifup eth7
sbauer@metamini ~$ sudo ethtool eth7
Settings for eth7:
Supported ports: [ MII ]
Supported link modes: 10baseT/Full
100baseT/Full
1000baseT/Full
Supported pause frame use: Symmetric Receive-only
Supports auto-negotiation: Yes
Supported FEC modes: Not reported
Advertised link modes: 10baseT/Full
100baseT/Full
1000baseT/Full
Advertised pause frame use: Symmetric Receive-only
Advertised auto-negotiation: Yes
Advertised FEC modes: Not reported
Speed: 1000Mb/s
Duplex: Full
Port: MII
PHYAD: 0
Transceiver: internal
Auto-negotiation: on
Supports Wake-on: pumbag
Wake-on: d
Current message level: 0x00000137 (311)
drv probe link ifdown ifup tx_queued
Link detected: yes
sbauer@metamini ~$ sudo mii-tool -vv eth7
Using SIOCGMIIPHY=0x8947
eth7: negotiated 1000baseT-FD, link ok
registers for MII PHY 0:
5140 512d 7431 0011 4140 4140 000d 0000
0000 0200 7800 0000 0000 0000 0000 2000
0000 0000 0000 0000 0000 0000 0000 0000
0000 0000 0000 0000 0000 0000 0000 0000
product info: vendor 1d:0c:40, model 1 rev 1
basic mode: loopback, autonegotiation enabled
basic status: autonegotiation complete, link ok
capabilities: 1000baseT-FD 100baseTx-FD 10baseT-FD
advertising: 1000baseT-FD 100baseTx-FD 10baseT-FD
link partner: 1000baseT-FD 100baseTx-FD 10baseT-FD

Regards,
Sergej.

2021-01-23 01:35:40

by Andrew Lunn

Subject: Re: [PATCH v2] lan743x: add virtual PHY for PHY-less devices

> it migth be helpful for developers work on userspace networking tools with
> PHY-less lan743x

(the interface even could not be brought up)
> of course, there nothing much to do without TP port but the difference is
> representative.
>
> sbauer@metamini ~$ sudo ethtool eth7
> Settings for eth7:
> Cannot get device settings: No such device
> Supports Wake-on: pumbag
> Wake-on: d
> Current message level: 0x00000137 (311)
> drv probe link ifdown ifup tx_queued
> Link detected: no
> sbauer@metamini ~$ sudo ifup eth7
> sbauer@metamini ~$ sudo ethtool eth7
> Settings for eth7:
> Supported ports: [ MII ]
> Supported link modes: 10baseT/Full
> 100baseT/Full
> 1000baseT/Full
> Supported pause frame use: Symmetric Receive-only
> Supports auto-negotiation: Yes
> Supported FEC modes: Not reported
> Advertised link modes: 10baseT/Full
> 100baseT/Full
> 1000baseT/Full
> Advertised pause frame use: Symmetric Receive-only
> Advertised auto-negotiation: Yes
> Advertised FEC modes: Not reported
> Speed: 1000Mb/s
> Duplex: Full
> Port: MII
> PHYAD: 0
> Transceiver: internal
> Auto-negotiation: on
> Supports Wake-on: pumbag
> Wake-on: d
> Current message level: 0x00000137 (311)
> drv probe link ifdown ifup tx_queued
> Link detected: yes
> sbauer@metamini ~$ sudo mii-tool -vv eth7
> Using SIOCGMIIPHY=0x8947
> eth7: negotiated 1000baseT-FD, link ok
> registers for MII PHY 0:
> 5140 512d 7431 0011 4140 4140 000d 0000
> 0000 0200 7800 0000 0000 0000 0000 2000
> 0000 0000 0000 0000 0000 0000 0000 0000
> 0000 0000 0000 0000 0000 0000 0000 0000
> product info: vendor 1d:0c:40, model 1 rev 1
> basic mode: loopback, autonegotiation enabled
> basic status: autonegotiation complete, link ok
> capabilities: 1000baseT-FD 100baseTx-FD 10baseT-FD
> advertising: 1000baseT-FD 100baseTx-FD 10baseT-FD
> link partner: 1000baseT-FD 100baseTx-FD 10baseT-FD

You have not shown anything i cannot do with the ethernet interfaces i
have in my laptop. And since ethtool is pretty standardized, what
lan743x offers should be pretty much the same as any 1G Ethernet MAC
using most 1G PHYs.

Andrew

2021-01-23 01:42:56

by Florian Fainelli

Subject: Re: [PATCH v2] lan743x: add virtual PHY for PHY-less devices

On 1/22/2021 5:01 PM, Sergej Bauer wrote:
> On Saturday, January 23, 2021 3:01:47 AM MSK Florian Fainelli wrote:
>> On 1/22/2021 3:58 PM, Sergej Bauer wrote:
>>> On Saturday, January 23, 2021 2:23:25 AM MSK Andrew Lunn wrote:
>>>>>>> @@ -1000,8 +1005,10 @@ static void lan743x_phy_close(struct
>>>>>>> lan743x_adapter *adapter)>
>>>>>>>
>>>>>>> struct net_device *netdev = adapter->netdev;
>>>>>>>
>>>>>>> phy_stop(netdev->phydev);
>>>>>>>
>>>>>>> - phy_disconnect(netdev->phydev);
>>>>>>> - netdev->phydev = NULL;
>>>>>>> + if (phy_is_pseudo_fixed_link(netdev->phydev))
>>>>>>> + lan743x_virtual_phy_disconnect(netdev->phydev);
>>>>>>> + else
>>>>>>> + phy_disconnect(netdev->phydev);
>>>>>>
>>>>>> phy_disconnect() should work. You might want to call
>>>>
>>>> There are drivers which call phy_disconnect() on a fixed_link. e.g.
>>>>
>>>> https://elixir.bootlin.com/linux/v5.11-rc4/source/drivers/net/usb/lan78xx
>>>> .c# L3555.
>>>>
>>>>
>>>> It could be your missing call to fixed_phy_unregister() is leaving
>>>> behind bad state.
>>>
>>> lan743x_virtual_phy_disconnect removes sysfs-links and calls
>>> fixed_phy_unregister()
>>> and the reason was phydev in sysfs.
>>>
>>>>> It was to make ethtool show full set of supported speeds and MII only in
>>>>> supported ports (without TP and the no any ports in the bare card).
>>>>
>>>> But fixed link does not support the full set of speed. It is fixed. It
>>>> supports only one speed it is configured with.
>>>
>>> That's why I "re-implemented the fixed PHY driver" as Florian said.
>>> The goal of virtual phy was to make an illusion of real device working in
>>> loopback mode. So I could use ethtool and ioctl's to switch speed of
>>> device.>
>>>> And by setting it
>>>> wrongly, you are going to allow the user to do odd things, like use
>>>> ethtool force the link speed to a speed which is not actually
>>>> supported.
>>>
>>> I have lan743x only and in loopback mode it allows to use speeds
>>> 10/100/1000MBps
>>> in full-duplex mode only. But the highest speed I have achived was
>>> something near
>>> 752Mbps...
>>> And I can switch speed on the fly, without reloading the module.
>>>
>>> May by I should limit the list of acceptable devices?
>>
>> It is not clear what your use case is so maybe start with explaining it
>> and we can help you define something that may be acceptable for upstream
>> inclusion.
> it migth be helpful for developers work on userspace networking tools with
> PHY-less lan743x (the interface even could not be brought up)
> of course, there nothing much to do without TP port but the difference is
> representative.

You are still not explaining why fixed PHY is not a suitable emulation
and what is different, providing the output of ethtool does not tell me
how you are using it.

Literally everyone using Ethernet switches or MAC to MAC Ethernet
connections uses fixed PHY and is reasonably happy with it.
--
Florian

2021-01-23 04:04:40

by Sergej Bauer

Subject: Re: [PATCH v2] lan743x: add virtual PHY for PHY-less devices

resending answer to all:
On Saturday, January 23, 2021 4:32:38 AM MSK Andrew Lunn wrote:
> > it migth be helpful for developers work on userspace networking tools with
> > PHY-less lan743x
>
> (the interface even could not be brought up)
>
> > of course, there nothing much to do without TP port but the difference is
> > representative.
> >
> > sbauer@metamini ~$ sudo ethtool eth7
> > Settings for eth7:
> > Cannot get device settings: No such device
> >
> > Supports Wake-on: pumbag
> > Wake-on: d
> > Current message level: 0x00000137 (311)
> >
> > drv probe link ifdown ifup tx_queued
> >
> > Link detected: no
> >
> > sbauer@metamini ~$ sudo ifup eth7
> > sbauer@metamini ~$ sudo ethtool eth7
> >
> > Settings for eth7:
> > Supported ports: [ MII ]
> > Supported link modes: 10baseT/Full
> >
> > 100baseT/Full
> > 1000baseT/Full
> >
> > Supported pause frame use: Symmetric Receive-only
> > Supports auto-negotiation: Yes
> > Supported FEC modes: Not reported
> > Advertised link modes: 10baseT/Full
> >
> > 100baseT/Full
> > 1000baseT/Full
> >
> > Advertised pause frame use: Symmetric Receive-only
> > Advertised auto-negotiation: Yes
> > Advertised FEC modes: Not reported
> > Speed: 1000Mb/s
> > Duplex: Full
> > Port: MII
> > PHYAD: 0
> > Transceiver: internal
> > Auto-negotiation: on
> > Supports Wake-on: pumbag
> > Wake-on: d
> > Current message level: 0x00000137 (311)
> >
> > drv probe link ifdown ifup tx_queued
> >
> > Link detected: yes
> >
> > sbauer@metamini ~$ sudo mii-tool -vv eth7
> > Using SIOCGMIIPHY=0x8947
> > eth7: negotiated 1000baseT-FD, link ok
> >
> > registers for MII PHY 0:
> > 5140 512d 7431 0011 4140 4140 000d 0000
> > 0000 0200 7800 0000 0000 0000 0000 2000
> > 0000 0000 0000 0000 0000 0000 0000 0000
> > 0000 0000 0000 0000 0000 0000 0000 0000
> >
> > product info: vendor 1d:0c:40, model 1 rev 1
> > basic mode: loopback, autonegotiation enabled
> > basic status: autonegotiation complete, link ok
> > capabilities: 1000baseT-FD 100baseTx-FD 10baseT-FD
> > advertising: 1000baseT-FD 100baseTx-FD 10baseT-FD
> > link partner: 1000baseT-FD 100baseTx-FD 10baseT-FD
>
> You have not shown anything i cannot do with the ethernet interfaces i
> have in my laptop. And since ethtool is pretty standardized, what
> lan743x offers should be pretty much the same as any 1G Ethernet MAC
> using most 1G PHYs.
>
> Andrew

Andrew, I can reproduce segfault with following changes:
sbauer@metamini ~/devel/kernel-works/net-next.git master$ git diff .
diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/
ethernet/microchip/lan743x_main.c
index 3804310c853a..6e2961f47211 100644
--- a/drivers/net/ethernet/microchip/lan743x_main.c
+++ b/drivers/net/ethernet/microchip/lan743x_main.c
@@ -1001,6 +1001,8 @@ static void lan743x_phy_close(struct lan743x_adapter
*adapter)

phy_stop(netdev->phydev);
phy_disconnect(netdev->phydev);
+ if (phy_is_pseudo_fixed_link(netdev->phydev))
+ fixed_phy_unregister(netdev->phydev);
netdev->phydev = NULL;
}

@@ -1018,9 +1020,21 @@ static int lan743x_phy_open(struct lan743x_adapter
*adapter)
if (!phydev) {
/* try internal phy */
phydev = phy_find_first(adapter->mdiobus);
- if (!phydev)
- goto return_error;
-
+ if (!phydev) {
+ struct fixed_phy_status status = {
+ .link = 1,
+ .speed = SPEED_1000,
+ .duplex = DUPLEX_FULL,
+ .asym_pause = 1,
+ };
+
+ phydev = fixed_phy_register(PHY_POLL, &status, NULL);
+ if (!phydev || IS_ERR(phydev)) {
+ netif_err(adapter, probe, netdev,
+ "Failed to register fixed PHY
device\n");
+ goto return_error;
+ }
+ }
ret = phy_connect_direct(netdev, phydev,
lan743x_phy_link_status_change,
PHY_INTERFACE_MODE_GMII);

sbauer@metamini ~/devel/kernel-works/net-next.git master$ sudo modprobe
lan743x
sbauer@metamini ~/devel/kernel-works/net-next.git master$ sudo ifup eth7
sbauer@metamini ~/devel/kernel-works/net-next.git master$ ethtool eth7
Settings for eth7:
Supported ports: [ TP MII ]
Supported link modes: 1000baseT/Full
Supported pause frame use: Symmetric Receive-only
Supports auto-negotiation: Yes
Supported FEC modes: Not reported
Advertised link modes: 1000baseT/Full
Advertised pause frame use: Symmetric Receive-only
Advertised auto-negotiation: Yes
Advertised FEC modes: Not reported
Speed: 1000Mb/s
Duplex: Full
Port: MII
PHYAD: 0
Transceiver: internal
Auto-negotiation: on
Cannot get wake-on-lan settings: Operation not permitted
Current message level: 0x00000137 (311)
drv probe link ifdown ifup tx_queued
Link detected: yes
sbauer@metamini ~/devel/kernel-works/net-next.git master$ sudo ifdown eth7
Killed
---

dmesg:
[ 365.856455] lan743x 0000:06:00.0 eth7: Link is Down
[ 365.856542] BUG: kernel NULL pointer dereference, address: 00000000000003c4
[ 365.856611] #PF: supervisor read access in kernel mode
[ 365.856705] #PF: error_code(0x0000) - not-present page
[ 365.856772] PGD 0 P4D 0
[ 365.856832] Oops: 0000 [#1] SMP NOPTI
[ 365.856898] CPU: 0 PID: 21779 Comm: ip Tainted: G O 5.11.0-
rc4net-next+ #4
[ 365.857018] Hardware name: Gigabyte Technology Co., Ltd. H110-D3/H110-D3-
CF, BIOS F24 04/11/2018
[ 365.857111] RIP: 0010:lan743x_phy_close.isra.26+0x28/0x50 [lan743x]
[ 365.857208] Code: c3 90 0f 1f 44 00 00 53 48 89 fb 48 8b bf 28 08 00 00 e8
ab 3e da ff 48 8b bb 28 08 00 00 e8 bf 67 da ff 48 8b bb 28 08 00 00 <f6> 87 c4
03 00 00 04 75 0d 48 c7 83 28 08 00 00 00 00 00 00 5b c3
[ 365.857332] RSP: 0018:ffffb6560468b508 EFLAGS: 00010246
[ 365.857397] RAX: 0000000000000003 RBX: ffff8cfacbe30000 RCX: 0000000000000000
[ 365.857466] RDX: ffffffffc00ae2d8 RSI: 0000000000000001 RDI: 0000000000000000
[ 365.857537] RBP: 0000000000000008 R08: 0000000000000000 R09: ffffffffb2d6dee0
[ 365.857632] R10: ffff8cfb45f69bff R11: ffff8cfb057c829b R12: ffff8cfacbe308c0
[ 365.857697] R13: ffff8cfacbe310b8 R14: ffff8cfacbe31050 R15: ffff8cfacbe308c0
[ 365.857762] FS: 00007f4318318e40(0000) GS:ffff8cfbf6c00000(0000) knlGS:
0000000000000000
[ 365.857844] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 365.857907] CR2: 00000000000003c4 CR3: 0000000145a80001 CR4:
00000000003706f0
[ 365.857972] Call Trace:
[ 365.858029] lan743x_netdev_close+0x223/0x250 [lan743x]
[ 365.858094] __dev_close_many+0x96/0x100
[ 365.858170] __dev_change_flags+0xdc/0x210
[ 365.858264] dev_change_flags+0x21/0x60
[ 365.858325] do_setlink+0x347/0x10e0
[ 365.858385] ? __nla_validate_parse+0x5f/0xaf0
[ 365.858447] __rtnl_newlink+0x541/0x8d0
[ 365.858508] ? get_page_from_freelist+0x1194/0x1310
[ 365.858574] ? mutex_spin_on_owner+0x5c/0xb0
[ 365.858635] ? __mutex_lock.isra.9+0x46e/0x4b0
[ 365.858696] ? asm_exc_page_fault+0x1e/0x30
[ 365.858757] rtnl_newlink+0x43/0x60
[ 365.858817] rtnetlink_rcv_msg+0x134/0x380
[ 365.858878] ? _cond_resched+0x15/0x30
[ 365.858937] ? kmem_cache_alloc+0x3cf/0x7d0
[ 365.858997] ? rtnl_calcit.isra.38+0x110/0x110
[ 365.859058] netlink_rcv_skb+0x50/0x100
[ 365.859118] netlink_unicast+0x1a5/0x280
[ 365.859178] netlink_sendmsg+0x23d/0x470
[ 365.859237] sock_sendmsg+0x5b/0x60
[ 365.859298] ____sys_sendmsg+0x1ef/0x260
[ 365.859358] ? copy_msghdr_from_user+0x5c/0x90
[ 365.859418] ? mntput_no_expire+0x47/0x240
[ 365.859478] ___sys_sendmsg+0x7c/0xc0
[ 365.859537] ? tomoyo_path_number_perm+0x68/0x1e0
[ 365.859600] ? __sk_destruct+0x129/0x1b0
[ 365.859660] ? var_wake_function+0x20/0x20
[ 365.859720] ? fsnotify_grab_connector+0x46/0x80
[ 365.859782] __sys_sendmsg+0x57/0xa0
[ 365.859841] do_syscall_64+0x33/0x80
[ 365.859900] entry_SYSCALL_64_after_hwframe+0x44/0xa9
---
(gdb) l *lan743x_netdev_close+0x223
0x1f43 is in lan743x_netdev_close (drivers/net/ethernet/microchip//
lan743x_main.c:2521).
2516
2517 lan743x_ptp_close(adapter);
2518
2519 lan743x_phy_close(adapter);
2520
2521 lan743x_mac_close(adapter);
2522
2523 lan743x_intr_close(adapter);
2524
2525 return 0;
(gdb)

2021-01-25 10:31:53

by Sergej Bauer

Subject: Re: [PATCH v2] lan743x: add virtual PHY for PHY-less devices

On Saturday, January 23, 2021 4:32:38 AM MSK Andrew Lunn wrote:
> > it migth be helpful for developers work on userspace networking tools with
> > PHY-less lan743x
>
> (the interface even could not be brought up)
>
> > of course, there nothing much to do without TP port but the difference is
> > representative.
> >
> > sbauer@metamini ~$ sudo ethtool eth7
> > Settings for eth7:
> > Cannot get device settings: No such device
> >
> > Supports Wake-on: pumbag
> > Wake-on: d
> > Current message level: 0x00000137 (311)
> >
> > drv probe link ifdown ifup tx_queued
> >
> > Link detected: no
> >
> > sbauer@metamini ~$ sudo ifup eth7
> > sbauer@metamini ~$ sudo ethtool eth7
> >
> > Settings for eth7:
> > Supported ports: [ MII ]
> > Supported link modes: 10baseT/Full
> >
> > 100baseT/Full
> > 1000baseT/Full
> >
> > Supported pause frame use: Symmetric Receive-only
> > Supports auto-negotiation: Yes
> > Supported FEC modes: Not reported
> > Advertised link modes: 10baseT/Full
> >
> > 100baseT/Full
> > 1000baseT/Full
> >
> > Advertised pause frame use: Symmetric Receive-only
> > Advertised auto-negotiation: Yes
> > Advertised FEC modes: Not reported
> > Speed: 1000Mb/s
> > Duplex: Full
> > Port: MII
> > PHYAD: 0
> > Transceiver: internal
> > Auto-negotiation: on
> > Supports Wake-on: pumbag
> > Wake-on: d
> > Current message level: 0x00000137 (311)
> >
> > drv probe link ifdown ifup tx_queued
> >
> > Link detected: yes
> >
> > sbauer@metamini ~$ sudo mii-tool -vv eth7
> > Using SIOCGMIIPHY=0x8947
> > eth7: negotiated 1000baseT-FD, link ok
> >
> > registers for MII PHY 0:
> > 5140 512d 7431 0011 4140 4140 000d 0000
> > 0000 0200 7800 0000 0000 0000 0000 2000
> > 0000 0000 0000 0000 0000 0000 0000 0000
> > 0000 0000 0000 0000 0000 0000 0000 0000
> >
> > product info: vendor 1d:0c:40, model 1 rev 1
> > basic mode: loopback, autonegotiation enabled
> > basic status: autonegotiation complete, link ok
> > capabilities: 1000baseT-FD 100baseTx-FD 10baseT-FD
> > advertising: 1000baseT-FD 100baseTx-FD 10baseT-FD
> > link partner: 1000baseT-FD 100baseTx-FD 10baseT-FD
>
> You have not shown anything i cannot do with the ethernet interfaces i
> have in my laptop. And since ethtool is pretty standardized, what
> lan743x offers should be pretty much the same as any 1G Ethernet MAC
> using most 1G PHYs.
>
> Andrew

Andrew, for this moment with lan743x we can get only this:
sbauer@metamini ~/devel/kernel-works/net-next.git master$ sudo ethtool eth7
Settings for eth7:
Cannot get device settings: No such device
Supports Wake-on: pumbag
Wake-on: d
Current message level: 0x00000137 (311)
drv probe link ifdown ifup tx_queued
Link detected: no
sbauer@metamini ~/devel/kernel-works/net-next.git master$ sudo mii-tool -vv
eth7
Using SIOCGMIIPHY=0x8947
SIOCGMIIPHY on 'eth7' failed: Invalid argument


--
Regards,
Sergej.

2021-01-26 21:49:29

by Sergej Bauer

Subject: Re: [PATCH v2] lan743x: add virtual PHY for PHY-less devices

On Saturday, January 23, 2021 2:23:25 AM MSK Andrew Lunn wrote:
> > > > @@ -1000,8 +1005,10 @@ static void lan743x_phy_close(struct
> > > > lan743x_adapter *adapter)>
> > > >
> > > > struct net_device *netdev = adapter->netdev;
> > > >
> > > > phy_stop(netdev->phydev);
> > > >
> > > > - phy_disconnect(netdev->phydev);
> > > > - netdev->phydev = NULL;
> > > > + if (phy_is_pseudo_fixed_link(netdev->phydev))
> > > > + lan743x_virtual_phy_disconnect(netdev->phydev);
> > > > + else
> > > > + phy_disconnect(netdev->phydev);
> > >
> > > phy_disconnect() should work. You might want to call
>
> There are drivers which call phy_disconnect() on a fixed_link. e.g.
>
> https://elixir.bootlin.com/linux/v5.11-rc4/source/drivers/net/usb/lan78xx.c#
> L3555.
>
> It could be your missing call to fixed_phy_unregister() is leaving
> behind bad state.
>
fixed_phy_unregister() were inside of lan743x_virtual_phy_disconnect()

> > It was to make ethtool show full set of supported speeds and MII only in
> > supported ports (without TP and the no any ports in the bare card).
>
> But fixed link does not support the full set of speed. It is fixed. It
> supports only one speed it is configured with. And by setting it
> wrongly, you are going to allow the user to do odd things, like use
> ethtool force the link speed to a speed which is not actually
> supported.
when writing virtual phy i have used "Microchip AN2948 Configuration Registers
of LAN743x" document and the lan743x is designed only for LAN7430 either
LAN7431 as it pointed in the document and in lan743x_pcidev_tbl. which both
support speeds 10/100/1000 Mbps.


--
Regards,
Sergej.

2021-01-27 03:33:46

by Sergej Bauer

Subject: Re: [PATCH v2] lan743x: add virtual PHY for PHY-less devices

On Saturday, January 23, 2021 4:39:47 AM MSK Florian Fainelli wrote:
> On 1/22/2021 5:01 PM, Sergej Bauer wrote:
> > On Saturday, January 23, 2021 3:01:47 AM MSK Florian Fainelli wrote:
> >> On 1/22/2021 3:58 PM, Sergej Bauer wrote:
> >>> On Saturday, January 23, 2021 2:23:25 AM MSK Andrew Lunn wrote:
> >>>>>>> @@ -1000,8 +1005,10 @@ static void lan743x_phy_close(struct
> >>>>>>> lan743x_adapter *adapter)>
> >>>>>>>
> >>>>>>> struct net_device *netdev = adapter->netdev;
> >>>>>>>
> >>>>>>> phy_stop(netdev->phydev);
> >>>>>>>
> >>>>>>> - phy_disconnect(netdev->phydev);
> >>>>>>> - netdev->phydev = NULL;
> >>>>>>> + if (phy_is_pseudo_fixed_link(netdev->phydev))
> >>>>>>> + lan743x_virtual_phy_disconnect(netdev->phydev);
> >>>>>>> + else
> >>>>>>> + phy_disconnect(netdev->phydev);
> >>>>>>
> >>>>>> phy_disconnect() should work. You might want to call
> >>>>
> >>>> There are drivers which call phy_disconnect() on a fixed_link. e.g.
> >>>>
> >>>> https://elixir.bootlin.com/linux/v5.11-rc4/source/drivers/net/usb/lan78
> >>>> xx
> >>>> .c# L3555.
> >>>>
> >>>>
> >>>> It could be your missing call to fixed_phy_unregister() is leaving
> >>>> behind bad state.
> >>>
> >>> lan743x_virtual_phy_disconnect removes sysfs-links and calls
> >>> fixed_phy_unregister()
> >>> and the reason was phydev in sysfs.
> >>>
> >>>>> It was to make ethtool show full set of supported speeds and MII only
> >>>>> in
> >>>>> supported ports (without TP and the no any ports in the bare card).
> >>>>
> >>>> But fixed link does not support the full set of speed. It is fixed. It
> >>>> supports only one speed it is configured with.
> >>>
> >>> That's why I "re-implemented the fixed PHY driver" as Florian said.
> >>> The goal of virtual phy was to make an illusion of real device working
> >>> in
> >>> loopback mode. So I could use ethtool and ioctl's to switch speed of
> >>> device.>
> >>>
> >>>> And by setting it
> >>>> wrongly, you are going to allow the user to do odd things, like use
> >>>> ethtool force the link speed to a speed which is not actually
> >>>> supported.
> >>>
> >>> I have lan743x only and in loopback mode it allows to use speeds
> >>> 10/100/1000MBps
> >>> in full-duplex mode only. But the highest speed I have achived was
> >>> something near
> >>> 752Mbps...
> >>> And I can switch speed on the fly, without reloading the module.
> >>>
> >>> May by I should limit the list of acceptable devices?
> >>
> >> It is not clear what your use case is so maybe start with explaining it
> >> and we can help you define something that may be acceptable for upstream
> >> inclusion.
> >
> > it migth be helpful for developers work on userspace networking tools with
> > PHY-less lan743x (the interface even could not be brought up)
> > of course, there nothing much to do without TP port but the difference is
> > representative.
>
> You are still not explaining why fixed PHY is not a suitable emulation
> and what is different, providing the output of ethtool does not tell me
> how you are using it.
>
> Literally everyone using Ethernet switches or MAC to MAC Ethernet
> connections uses fixed PHY and is reasonably happy with it.

Florian, the key idea is to make virtual phy device which acts as real 802.11
standard phy.

original fixed_phy and swphy are little bit useless as they do not support
write operation. in contrast of them virtual_phy supports write to all
registers. and can change the speed of interface by means of SIOCSMIIREG ioctl
call either ethtool.
changing of appropriate bits in register 0 will change speed reflecting in
ethtool
and vise versa.


--
Regards,
Sergej