[PATCH net-next] net: phy: introduce phydev->port

[PATCH net-next] net: phy: introduce phydev->port

[Michael Walle]

At the moment, PORT_MII is reported in the ethtool ops. This is odd
because it is an interface between the MAC and the PHY and no external
port. Some network card drivers will overwrite the port to twisted pair
or fiber, though. Even worse, the MDI/MDIX setting is only used by
ethtool if the port is twisted pair.

Set the port to PORT_TP by default because most PHY drivers are copper
ones. If there is fibre support and it is enabled, the PHY driver will
set it to PORT_FIBRE.

This will change reporting PORT_MII to either PORT_TP or PORT_FIBRE;
except for the genphy fallback driver.

Suggested-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/net/phy/broadcom.c   |  2 ++
 drivers/net/phy/dp83822.c    |  3 +++
 drivers/net/phy/dp83869.c    |  4 ++++
 drivers/net/phy/lxt.c        |  1 +
 drivers/net/phy/marvell.c    |  1 +
 drivers/net/phy/marvell10g.c |  2 ++
 drivers/net/phy/micrel.c     | 14 +++++++++++---
 drivers/net/phy/phy.c        |  2 +-
 drivers/net/phy/phy_device.c |  9 +++++++++
 include/linux/phy.h          |  2 ++
 10 files changed, 36 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
index 3142ba768313..0472b3470c59 100644
--- a/drivers/net/phy/broadcom.c
+++ b/drivers/net/phy/broadcom.c
@@ -410,6 +410,8 @@ static int bcm54616s_probe(struct phy_device *phydev)
 		 */
 		if (!(val & BCM54616S_100FX_MODE))
 			phydev->dev_flags |= PHY_BCM_FLAGS_MODE_1000BX;
+
+		phydev->port = PORT_FIBRE;
 	}
 
 	return 0;
diff --git a/drivers/net/phy/dp83822.c b/drivers/net/phy/dp83822.c
index fff371ca1086..be1224b4447b 100644
--- a/drivers/net/phy/dp83822.c
+++ b/drivers/net/phy/dp83822.c
@@ -554,6 +554,9 @@ static int dp83822_probe(struct phy_device *phydev)
 
 	dp83822_of_init(phydev);
 
+	if (dp83822->fx_enabled)
+		phydev->port = PORT_FIBRE;
+
 	return 0;
 }
 
diff --git a/drivers/net/phy/dp83869.c b/drivers/net/phy/dp83869.c
index b30bc142d82e..755220c6451f 100644
--- a/drivers/net/phy/dp83869.c
+++ b/drivers/net/phy/dp83869.c
@@ -855,6 +855,10 @@ static int dp83869_probe(struct phy_device *phydev)
 	if (ret)
 		return ret;
 
+	if (dp83869->mode == DP83869_RGMII_100_BASE ||
+	    dp83869->mode == DP83869_RGMII_1000_BASE)
+		phydev->port = PORT_FIBRE;
+
 	return dp83869_config_init(phydev);
 }
 
diff --git a/drivers/net/phy/lxt.c b/drivers/net/phy/lxt.c
index 0ee23d29c0d4..bde3356a2f86 100644
--- a/drivers/net/phy/lxt.c
+++ b/drivers/net/phy/lxt.c
@@ -292,6 +292,7 @@ static int lxt973_probe(struct phy_device *phydev)
 		phy_write(phydev, MII_BMCR, val);
 		/* Remember that the port is in fiber mode. */
 		phydev->priv = lxt973_probe;
+		phydev->port = PORT_FIBRE;
 	} else {
 		phydev->priv = NULL;
 	}
diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index b523aa37ebf0..3238d0fbf437 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -1552,6 +1552,7 @@ static int marvell_read_status_page(struct phy_device *phydev, int page)
 	phydev->asym_pause = 0;
 	phydev->speed = SPEED_UNKNOWN;
 	phydev->duplex = DUPLEX_UNKNOWN;
+	phydev->port = fiber ? PORT_FIBRE : PORT_TP;
 
 	if (phydev->autoneg == AUTONEG_ENABLE)
 		err = marvell_read_status_page_an(phydev, fiber, status);
diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
index 1901ba277413..b1bb9b8e1e4e 100644
--- a/drivers/net/phy/marvell10g.c
+++ b/drivers/net/phy/marvell10g.c
@@ -631,6 +631,7 @@ static int mv3310_read_status_10gbaser(struct phy_device *phydev)
 	phydev->link = 1;
 	phydev->speed = SPEED_10000;
 	phydev->duplex = DUPLEX_FULL;
+	phydev->port = PORT_FIBRE;
 
 	return 0;
 }
@@ -690,6 +691,7 @@ static int mv3310_read_status_copper(struct phy_device *phydev)
 
 	phydev->duplex = cssr1 & MV_PCS_CSSR1_DUPLEX_FULL ?
 			 DUPLEX_FULL : DUPLEX_HALF;
+	phydev->port = PORT_TP;
 	phydev->mdix = cssr1 & MV_PCS_CSSR1_MDIX ?
 		       ETH_TP_MDI_X : ETH_TP_MDI;
 
diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index 494abf608b8f..7ec6f70d6a82 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -341,14 +341,19 @@ static int kszphy_config_init(struct phy_device *phydev)
 	return kszphy_config_reset(phydev);
 }
 
+static int ksz8041_fiber_mode(struct phy_device *phydev)
+{
+	struct device_node *of_node = phydev->mdio.dev.of_node;
+
+	return of_property_read_bool(of_node, "micrel,fiber-mode");
+}
+
 static int ksz8041_config_init(struct phy_device *phydev)
 {
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
 
-	struct device_node *of_node = phydev->mdio.dev.of_node;
-
 	/* Limit supported and advertised modes in fiber mode */
-	if (of_property_read_bool(of_node, "micrel,fiber-mode")) {
+	if (ksz8041_fiber_mode(phydev)) {
 		phydev->dev_flags |= MICREL_PHY_FXEN;
 		linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT, mask);
 		linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Half_BIT, mask);
@@ -1176,6 +1181,9 @@ static int kszphy_probe(struct phy_device *phydev)
 		}
 	}
 
+	if (ksz8041_fiber_mode(phydev))
+		phydev->port = PORT_FIBRE;
+
 	/* Support legacy board-file configuration */
 	if (phydev->dev_flags & MICREL_PHY_50MHZ_CLK) {
 		priv->rmii_ref_clk_sel = true;
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 2e71d65ead54..9c4ee0a2143a 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -308,7 +308,7 @@ void phy_ethtool_ksettings_get(struct phy_device *phydev,
 	if (phydev->interface == PHY_INTERFACE_MODE_MOCA)
 		cmd->base.port = PORT_BNC;
 	else
-		cmd->base.port = PORT_MII;
+		cmd->base.port = phydev->port;
 	cmd->base.transceiver = phy_is_internal(phydev) ?
 				XCVR_INTERNAL : XCVR_EXTERNAL;
 	cmd->base.phy_address = phydev->mdio.addr;
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 8447e56ba572..30a20a29ae05 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -606,6 +606,7 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id,
 	dev->pause = 0;
 	dev->asym_pause = 0;
 	dev->link = 0;
+	dev->port = PORT_TP;
 	dev->interface = PHY_INTERFACE_MODE_GMII;
 
 	dev->autoneg = AUTONEG_ENABLE;
@@ -1403,6 +1404,14 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 
 	phydev->state = PHY_READY;
 
+	/* Port is set to PORT_TP by default and the actual PHY driver will set
+	 * it to different value depending on the PHY configuration. If we have
+	 * the generic PHY driver we can't figure it out, thus set the old
+	 * legacy PORT_MII value.
+	 */
+	if (using_genphy)
+		phydev->port = PORT_MII;
+
 	/* Initial carrier state is off as the phy is about to be
 	 * (re)initialized.
 	 */
diff --git a/include/linux/phy.h b/include/linux/phy.h
index c22aba1bda59..d0e3a94882b1 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -503,6 +503,7 @@ struct macsec_ops;
  *
  * @speed: Current link speed
  * @duplex: Current duplex
+ * @port: Current port
  * @pause: Current pause
  * @asym_pause: Current asymmetric pause
  * @supported: Combined MAC/PHY supported linkmodes
@@ -581,6 +582,7 @@ struct phy_device {
 	 */
 	int speed;
 	int duplex;
+	int port;
 	int pause;
 	int asym_pause;
 	u8 master_slave_get;
-- 
2.20.1

Re: [PATCH net-next] net: phy: introduce phydev->port

[Florian Fainelli]

On 2/9/21 8:38 AM, Michael Walle wrote:
> At the moment, PORT_MII is reported in the ethtool ops. This is odd
> because it is an interface between the MAC and the PHY and no external
> port. Some network card drivers will overwrite the port to twisted pair
> or fiber, though. Even worse, the MDI/MDIX setting is only used by
> ethtool if the port is twisted pair.
> 
> Set the port to PORT_TP by default because most PHY drivers are copper
> ones. If there is fibre support and it is enabled, the PHY driver will
> set it to PORT_FIBRE.
> 
> This will change reporting PORT_MII to either PORT_TP or PORT_FIBRE;
> except for the genphy fallback driver.
> 
> Suggested-by: Andrew Lunn <andrew@lunn.ch>
> Signed-off-by: Michael Walle <michael@walle.cc>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH net-next] net: phy: introduce phydev->port

[Andrew Lunn]

> @@ -1552,6 +1552,7 @@ static int marvell_read_status_page(struct phy_device *phydev, int page)
>  	phydev->asym_pause = 0;
>  	phydev->speed = SPEED_UNKNOWN;
>  	phydev->duplex = DUPLEX_UNKNOWN;
> +	phydev->port = fiber ? PORT_FIBRE : PORT_TP;
>  
>  	if (phydev->autoneg == AUTONEG_ENABLE)
>  		err = marvell_read_status_page_an(phydev, fiber, status);

...

> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -308,7 +308,7 @@ void phy_ethtool_ksettings_get(struct phy_device *phydev,
>  	if (phydev->interface == PHY_INTERFACE_MODE_MOCA)
>  		cmd->base.port = PORT_BNC;
>  	else
> -		cmd->base.port = PORT_MII;
> +		cmd->base.port = phydev->port;
>  	cmd->base.transceiver = phy_is_internal(phydev) ?
>  				XCVR_INTERNAL : XCVR_EXTERNAL;
>  	cmd->base.phy_address = phydev->mdio.addr;

This is a general comment, not a problem specific to this patch.

There is some interesting race conditions here. The marvell driver
first checks the fibre page and gets the status of the fiber port. As
you can see from the hunk above, it clears out pause, duplex, speed,
sets port to PORT_FIBRE, and then reads the PHY registers to set these
values. If link is not detected on the fibre, it swaps page and does
it all again, but for the copper port. So once per second,
phydev->port is going to flip flop PORT_FIBER->PORT_TP, if copper has
link.

Now, the read_status() call into the driver should be performed while
holding the phydev->lock. So to the PHY state machine, this flip/flop
does not matter, it is atomic with respect to the lock. But
phy_ethtool_ksettings_get() is not talking the lock. It could see
speed, duplex, and port while they have _UNKNOWN values, or port is
part way through a flip flop. I think we need to take the lock here.
phy_ethtool_ksettings_set() should also probably take the lock.

     Andrew

Re: [PATCH net-next] net: phy: introduce phydev->port

[Andrew Lunn]

On Tue, Feb 09, 2021 at 05:38:52PM +0100, Michael Walle wrote:
> At the moment, PORT_MII is reported in the ethtool ops. This is odd
> because it is an interface between the MAC and the PHY and no external
> port. Some network card drivers will overwrite the port to twisted pair
> or fiber, though. Even worse, the MDI/MDIX setting is only used by
> ethtool if the port is twisted pair.
> 
> Set the port to PORT_TP by default because most PHY drivers are copper
> ones. If there is fibre support and it is enabled, the PHY driver will
> set it to PORT_FIBRE.
> 
> This will change reporting PORT_MII to either PORT_TP or PORT_FIBRE;
> except for the genphy fallback driver.
> 
> Suggested-by: Andrew Lunn <andrew@lunn.ch>
> Signed-off-by: Michael Walle <michael@walle.cc>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next] net: phy: introduce phydev->port

[Russell King - ARM Linux admin]

On Wed, Feb 10, 2021 at 02:51:34AM +0100, Andrew Lunn wrote:
> This is a general comment, not a problem specific to this patch.
> 
> There is some interesting race conditions here. The marvell driver
> first checks the fibre page and gets the status of the fiber port. As
> you can see from the hunk above, it clears out pause, duplex, speed,
> sets port to PORT_FIBRE, and then reads the PHY registers to set these
> values. If link is not detected on the fibre, it swaps page and does
> it all again, but for the copper port. So once per second,
> phydev->port is going to flip flop PORT_FIBER->PORT_TP, if copper has
> link.
> 
> Now, the read_status() call into the driver should be performed while
> holding the phydev->lock. So to the PHY state machine, this flip/flop
> does not matter, it is atomic with respect to the lock. But
> phy_ethtool_ksettings_get() is not talking the lock. It could see
> speed, duplex, and port while they have _UNKNOWN values, or port is
> part way through a flip flop. I think we need to take the lock here.
> phy_ethtool_ksettings_set() should also probably take the lock.

phy_ethtool_ksettings_get() needs to take the lock, otherwise it could
read the phy_device members in the middle of an update. This is likely
a long-standing phylib bug.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next] net: phy: introduce phydev->port

[Michael Walle]

Am 2021-02-09 17:38, schrieb Michael Walle:
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -308,7 +308,7 @@ void phy_ethtool_ksettings_get(struct phy_device 
> *phydev,
>  	if (phydev->interface == PHY_INTERFACE_MODE_MOCA)
>  		cmd->base.port = PORT_BNC;
>  	else
> -		cmd->base.port = PORT_MII;
> +		cmd->base.port = phydev->port;
>  	cmd->base.transceiver = phy_is_internal(phydev) ?
>  				XCVR_INTERNAL : XCVR_EXTERNAL;
>  	cmd->base.phy_address = phydev->mdio.addr;

Russell, the phylink has a similiar place where PORT_MII is set. I don't 
know
if we'd have to change that, too.

Also, I wanted to look into the PHY_INTERFACE_MODE_MOCA thing and if we 
can
get rid of the special case here and just set phydev->port to PORT_BNC 
in the
driver. Florian, maybe you have a comment on this?

-michael

Re: [PATCH net-next] net: phy: introduce phydev->port

[Russell King - ARM Linux admin]

On Wed, Feb 10, 2021 at 12:20:02PM +0100, Michael Walle wrote:
> 
> Am 2021-02-09 17:38, schrieb Michael Walle:
> > --- a/drivers/net/phy/phy.c
> > +++ b/drivers/net/phy/phy.c
> > @@ -308,7 +308,7 @@ void phy_ethtool_ksettings_get(struct phy_device
> > *phydev,
> >  	if (phydev->interface == PHY_INTERFACE_MODE_MOCA)
> >  		cmd->base.port = PORT_BNC;
> >  	else
> > -		cmd->base.port = PORT_MII;
> > +		cmd->base.port = phydev->port;
> >  	cmd->base.transceiver = phy_is_internal(phydev) ?
> >  				XCVR_INTERNAL : XCVR_EXTERNAL;
> >  	cmd->base.phy_address = phydev->mdio.addr;
> 
> Russell, the phylink has a similiar place where PORT_MII is set. I don't
> know if we'd have to change that, too.

What would we change it to?

If there's no PHY attached and no SFP, what kind of interface do we
have? As we've no idea what's on the media side, assuming that we are
presenting a MII-like interface to stuff outside of what we control is
entirely reasonable.

Claiming the world is TP would be entirely wrong, there may not be a
RJ45 jack. Consider the case where the MAC is connected to a switch.
It's a MII-like link. It's certianly not TP, BNC, fiber, AUI, or
direct attach.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next] net: phy: introduce phydev->port

[Michael Walle]

Am 2021-02-10 12:54, schrieb Russell King - ARM Linux admin:
> On Wed, Feb 10, 2021 at 12:20:02PM +0100, Michael Walle wrote:
>> 
>> Am 2021-02-09 17:38, schrieb Michael Walle:
>> > --- a/drivers/net/phy/phy.c
>> > +++ b/drivers/net/phy/phy.c
>> > @@ -308,7 +308,7 @@ void phy_ethtool_ksettings_get(struct phy_device
>> > *phydev,
>> >  	if (phydev->interface == PHY_INTERFACE_MODE_MOCA)
>> >  		cmd->base.port = PORT_BNC;
>> >  	else
>> > -		cmd->base.port = PORT_MII;
>> > +		cmd->base.port = phydev->port;
>> >  	cmd->base.transceiver = phy_is_internal(phydev) ?
>> >  				XCVR_INTERNAL : XCVR_EXTERNAL;
>> >  	cmd->base.phy_address = phydev->mdio.addr;
>> 
>> Russell, the phylink has a similiar place where PORT_MII is set. I 
>> don't
>> know if we'd have to change that, too.
> 
> What would we change it to?

It was just a question whether we have to change it and I take from your
answer we do not. I was just curious because there is the same edge case
for the PORT_BNC like in the phy core.

> If there's no PHY attached and no SFP, what kind of interface do we
> have? As we've no idea what's on the media side, assuming that we are
> presenting a MII-like interface to stuff outside of what we control is
> entirely reasonable.
> 
> Claiming the world is TP would be entirely wrong, there may not be a
> RJ45 jack. Consider the case where the MAC is connected to a switch.
> It's a MII-like link. It's certianly not TP, BNC, fiber, AUI, or
> direct attach.

Yes, I get your point.

-michael

Re: [PATCH net-next] net: phy: introduce phydev->port

[Florian Fainelli]

On 2/10/21 3:20 AM, Michael Walle wrote:
> 
> Am 2021-02-09 17:38, schrieb Michael Walle:
>> --- a/drivers/net/phy/phy.c
>> +++ b/drivers/net/phy/phy.c
>> @@ -308,7 +308,7 @@ void phy_ethtool_ksettings_get(struct phy_device
>> *phydev,
>>      if (phydev->interface == PHY_INTERFACE_MODE_MOCA)
>>          cmd->base.port = PORT_BNC;
>>      else
>> -        cmd->base.port = PORT_MII;
>> +        cmd->base.port = phydev->port;
>>      cmd->base.transceiver = phy_is_internal(phydev) ?
>>                  XCVR_INTERNAL : XCVR_EXTERNAL;
>>      cmd->base.phy_address = phydev->mdio.addr;
> 
> Russell, the phylink has a similiar place where PORT_MII is set. I don't
> know
> if we'd have to change that, too.
> 
> Also, I wanted to look into the PHY_INTERFACE_MODE_MOCA thing and if we can
> get rid of the special case here and just set phydev->port to PORT_BNC
> in the
> driver. Florian, maybe you have a comment on this?

For GENET, it's simple because we can do this:

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index fcca023f22e5..34cbd008a3af 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -777,6 +777,8 @@ static int bcmgenet_get_link_ksettings(struct
net_device *dev,
                return -ENODEV;

        phy_ethtool_ksettings_get(dev->phydev, cmd);
+       if (dev->phydev->interface == PHY_INTERFACE_MODE_MOCA)
+               cmd->base.port = PORT_BNC;

        return 0;
 }

but for bcm_sf2.c, we would need to add plumbing between the DSA core
and the DSA driver in order to override the cmd structure with the
desired port and that would be most likely the only driver needing that,
should we really bother? There is also potentially a 3rd driver coming
down the road (bgmac) which would need to report MoCA/BNC.

I don't see this scaling very well nor being such a big issue to have
that in the PHYLIB and PHYLINK.
-- 
Florian