DSA: phy polling

DSA: phy polling

[Russell King - ARM Linux]

Andrew,

I think you're the current maintainer of the Marvell DSA code, as being
the most recent author of changes to it. :)

I've noticed in my testing that the Marvell DSA code seems to poll the
internal phy link status in mv88e6xxx_poll_link(), and set the network
device carrier status according to the results.

However, the internal phys are created using phylib, which also polls
the phys for their link status, and controls the associated netdev
carrier status.

The side effect of this is that I see duplicated link status messages in
the kernel log when connecting or disconnecting cables from the switch,
caused by the code in mv88e6xxx_poll_link() racing with the phylib code.
>From what I can see, the code in mv88e6xxx_poll_link() is entirely
redundant as the phylib layer will take care of any phy attached to the
switch.

To prove this, I have the following code in my tree, which disables the
polling on a port where we have a phy attached (either an internal or
external phy).  The result is that the per-port network devices are still
updated with the link status even though this code is disabled - thanks
to the phylib polling.

I'm left wondering whether the DSA specific phy polling does anything
useful, or whether the entire polling code both in mv88e6xxx.c and
net/dsa can be removed (mv88e6xxx.c seems to be its only user.)

diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
index 26ec2fbfaa89..4c324eafeef2 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -400,6 +400,13 @@ void mv88e6xxx_poll_link(struct dsa_switch *ds)
 		if (dev == NULL)
 			continue;
 
+		/*
+		 * Ignore ports which have a phy; phylib will take care
+		 * of polling the link status for these.
+		 */
+		if (dsa_slave_has_phy(dev))
+			continue;
+
 		link = 0;
 		if (dev->flags & IFF_UP) {
 			port_status = mv88e6xxx_reg_read(ds, REG_PORT(i),
diff --git a/include/net/dsa.h b/include/net/dsa.h
index fbca63ba8f73..b31e9da43ea7 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -176,6 +176,8 @@ static inline bool dsa_is_port_initialized(struct dsa_switch *ds, int p)
 	return ds->phys_port_mask & (1 << p) && ds->ports[p];
 }
 
+extern bool dsa_slave_has_phy(struct net_device *);
+
 static inline u8 dsa_upstream_port(struct dsa_switch *ds)
 {
 	struct dsa_switch_tree *dst = ds->dst;
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 35c47ddd04f0..a107242816ff 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -10,6 +10,7 @@
 
 #include <linux/list.h>
 #include <linux/etherdevice.h>
+#include <linux/export.h>
 #include <linux/netdevice.h>
 #include <linux/phy.h>
 #include <linux/phy_fixed.h>
@@ -873,6 +874,14 @@ int dsa_slave_resume(struct net_device *slave_dev)
 	return 0;
 }
 
+bool dsa_slave_has_phy(struct net_device *slave_dev)
+{
+	struct dsa_slave_priv *p = netdev_priv(slave_dev);
+
+	return p->phy != NULL;
+}
+EXPORT_SYMBOL_GPL(dsa_slave_has_phy);
+
 int dsa_slave_create(struct dsa_switch *ds, struct device *parent,
 		     int port, char *name)
 {


-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: DSA: phy polling

[Florian Fainelli]

On 14/09/15 03:42, Russell King - ARM Linux wrote:
> Andrew,
> 
> I think you're the current maintainer of the Marvell DSA code, as being
> the most recent author of changes to it. :)
> 
> I've noticed in my testing that the Marvell DSA code seems to poll the
> internal phy link status in mv88e6xxx_poll_link(), and set the network
> device carrier status according to the results.
> 
> However, the internal phys are created using phylib, which also polls
> the phys for their link status, and controls the associated netdev
> carrier status.
> 
> The side effect of this is that I see duplicated link status messages in
> the kernel log when connecting or disconnecting cables from the switch,
> caused by the code in mv88e6xxx_poll_link() racing with the phylib code.
> From what I can see, the code in mv88e6xxx_poll_link() is entirely
> redundant as the phylib layer will take care of any phy attached to the
> switch.
> 
> To prove this, I have the following code in my tree, which disables the
> polling on a port where we have a phy attached (either an internal or
> external phy).  The result is that the per-port network devices are still
> updated with the link status even though this code is disabled - thanks
> to the phylib polling.
> 
> I'm left wondering whether the DSA specific phy polling does anything
> useful, or whether the entire polling code both in mv88e6xxx.c and
> net/dsa can be removed (mv88e6xxx.c seems to be its only user.)

Just my 2 cents here, I suspect the original intention behind this code
was to help utilize the switch's built-in PHY polling unit when
available, and use the HW to collect the state of all PHYs in fewer
register to read, instead of having to do individual (and quite possibly
expensive) MDIO reads towards each individual per-port PHYs (at least
two reads per PHY to latch MII_BMSR).

Now, I do agree there is a duplication of functionality here, and a
potential fix would be to avoid starting the PHY state machine if/when
the switch supports such a feature (not call phy_start*), that should
still get you consistent consistent link partner advertised/status
values, question is, does that really benefit anybody though?

> 
> diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
> index 26ec2fbfaa89..4c324eafeef2 100644
> --- a/drivers/net/dsa/mv88e6xxx.c
> +++ b/drivers/net/dsa/mv88e6xxx.c
> @@ -400,6 +400,13 @@ void mv88e6xxx_poll_link(struct dsa_switch *ds)
>  		if (dev == NULL)
>  			continue;
>  
> +		/*
> +		 * Ignore ports which have a phy; phylib will take care
> +		 * of polling the link status for these.
> +		 */
> +		if (dsa_slave_has_phy(dev))
> +			continue;
> +
>  		link = 0;
>  		if (dev->flags & IFF_UP) {
>  			port_status = mv88e6xxx_reg_read(ds, REG_PORT(i),
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index fbca63ba8f73..b31e9da43ea7 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -176,6 +176,8 @@ static inline bool dsa_is_port_initialized(struct dsa_switch *ds, int p)
>  	return ds->phys_port_mask & (1 << p) && ds->ports[p];
>  }
>  
> +extern bool dsa_slave_has_phy(struct net_device *);
> +
>  static inline u8 dsa_upstream_port(struct dsa_switch *ds)
>  {
>  	struct dsa_switch_tree *dst = ds->dst;
> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> index 35c47ddd04f0..a107242816ff 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -10,6 +10,7 @@
>  
>  #include <linux/list.h>
>  #include <linux/etherdevice.h>
> +#include <linux/export.h>
>  #include <linux/netdevice.h>
>  #include <linux/phy.h>
>  #include <linux/phy_fixed.h>
> @@ -873,6 +874,14 @@ int dsa_slave_resume(struct net_device *slave_dev)
>  	return 0;
>  }
>  
> +bool dsa_slave_has_phy(struct net_device *slave_dev)
> +{
> +	struct dsa_slave_priv *p = netdev_priv(slave_dev);
> +
> +	return p->phy != NULL;
> +}
> +EXPORT_SYMBOL_GPL(dsa_slave_has_phy);
> +
>  int dsa_slave_create(struct dsa_switch *ds, struct device *parent,
>  		     int port, char *name)
>  {
> 
> 


-- 
Florian

Re: DSA: phy polling

[Russell King - ARM Linux]

On Mon, Sep 14, 2015 at 10:28:55AM -0700, Florian Fainelli wrote:
> Just my 2 cents here, I suspect the original intention behind this code
> was to help utilize the switch's built-in PHY polling unit when
> available, and use the HW to collect the state of all PHYs in fewer
> register to read, instead of having to do individual (and quite possibly
> expensive) MDIO reads towards each individual per-port PHYs (at least
> two reads per PHY to latch MII_BMSR).

Does the Marvell phy have such a register?  Looking at the register
dump and plugging/unplugging cables seems not to show a register
reporting whether any particular interface has changed state, and
I haven't noticed there being any combined register in anything I've
seen on these switches.

> Now, I do agree there is a duplication of functionality here, and a
> potential fix would be to avoid starting the PHY state machine if/when
> the switch supports such a feature (not call phy_start*), that should
> still get you consistent consistent link partner advertised/status
> values, question is, does that really benefit anybody though?

I disagree - it's the DSA polling that needs to go.  The DSA polling
only looks at the port status, and derives from it the carrier
state.  The rest of the information is only turned into a printk().
The PHY state machine does a lot more, recording the link speed so
that ethtool works on the interface.

If we do want to go the other way, then the phy code needs a rework so
that it can be properly classed and drivers with non-standard MII
registers supported without needing to build register emulation layers.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: DSA: phy polling

[Florian Fainelli]

On 14/09/15 11:23, Russell King - ARM Linux wrote:
> On Mon, Sep 14, 2015 at 10:28:55AM -0700, Florian Fainelli wrote:
>> Just my 2 cents here, I suspect the original intention behind this code
>> was to help utilize the switch's built-in PHY polling unit when
>> available, and use the HW to collect the state of all PHYs in fewer
>> register to read, instead of having to do individual (and quite possibly
>> expensive) MDIO reads towards each individual per-port PHYs (at least
>> two reads per PHY to latch MII_BMSR).
> 
> Does the Marvell phy have such a register?  Looking at the register
> dump and plugging/unplugging cables seems not to show a register
> reporting whether any particular interface has changed state, and
> I haven't noticed there being any combined register in anything I've
> seen on these switches.

It seemed to me like the PPU was meant to provide that, but I cannot
find any "summary" register which would give you such a status, must
have conflated that with what Broadcom switches support.

> 
>> Now, I do agree there is a duplication of functionality here, and a
>> potential fix would be to avoid starting the PHY state machine if/when
>> the switch supports such a feature (not call phy_start*), that should
>> still get you consistent consistent link partner advertised/status
>> values, question is, does that really benefit anybody though?
> 
> I disagree - it's the DSA polling that needs to go.  The DSA polling
> only looks at the port status, and derives from it the carrier
> state.  The rest of the information is only turned into a printk().
> The PHY state machine does a lot more, recording the link speed so
> that ethtool works on the interface.

I am fine with that approach, it was not exactly clear to me before
reading the code whether the link polling workqueue was doing anything
useful in mv88e6xxx.c, now it is pretty clear to me, this is as
expensive (more actually because of the PPU get/put) and useless since
the PHY library directly polls the individual per-port PHYs.

> 
> If we do want to go the other way, then the phy code needs a rework so
> that it can be properly classed and drivers with non-standard MII
> registers supported without needing to build register emulation layers.
> 

That part is going to be challenging, all the ethtool/PHY library/MII
code is built around the assumption of translating user-configurable
settings into standard MII calls (all the adv_to* etc.), but, then
again, MII is just one possible translation layer here, you could "plug"
another one if your HW supports that (e.g: non-MDIO, but MMIO for
instance which understands basic concepts like speed/link/duplex/pause).

Oh well.
-- 
Florian

Re: DSA: phy polling

[Peter Korsgaard]

>>>>> "Florian" == Florian Fainelli <f.fainelli@gmail.com> writes:

Hi,

 >> Does the Marvell phy have such a register?  Looking at the register
 >> dump and plugging/unplugging cables seems not to show a register
 >> reporting whether any particular interface has changed state, and
 >> I haven't noticed there being any combined register in anything I've
 >> seen on these switches.

 > It seemed to me like the PPU was meant to provide that, but I cannot
 > find any "summary" register which would give you such a status, must
 > have conflated that with what Broadcom switches support.

It's been some years, but I think the PPU was just to automatically
configure the switch MACs to to match the phy autonegotiation results.

I also don't see any summery register though.

-- 
Bye, Peter Korsgaard

Re: DSA: phy polling

[Andrew Lunn]

On Mon, Sep 14, 2015 at 11:42:54AM +0100, Russell King - ARM Linux wrote:
> Andrew,
> 
> I think you're the current maintainer of the Marvell DSA code, as being
> the most recent author of changes to it. :)

Hi Russell

Sorry for the slow reply, i've been on vacation.

Humm, i suppose i might be the defacto Maintainer for Marvell parts,
but i've no NDA with Marvell, so no access to the data sheets etc.
 
> I've noticed in my testing that the Marvell DSA code seems to poll the
> internal phy link status in mv88e6xxx_poll_link(), and set the network
> device carrier status according to the results.

Peter Korsgaard comment might be correct, the switch needs to know
what the PHY has negotiated. Hence the use of the PPU. There are also
comments in the code that the PPU is needed for indirect access to the
PHY.

So we probably need to keep the PPU, but disable it from changing the
networks stacks idea of the link state, etc.

I will add this to my TODO list to play with it.

  Andrew

Re: DSA: phy polling

[Russell King - ARM Linux]

On Tue, Sep 22, 2015 at 04:14:26AM +0200, Andrew Lunn wrote:
> On Mon, Sep 14, 2015 at 11:42:54AM +0100, Russell King - ARM Linux wrote:
> > Andrew,
> > 
> > I think you're the current maintainer of the Marvell DSA code, as being
> > the most recent author of changes to it. :)
> 
> Hi Russell
> 
> Sorry for the slow reply, i've been on vacation.
> 
> Humm, i suppose i might be the defacto Maintainer for Marvell parts,
> but i've no NDA with Marvell, so no access to the data sheets etc.

Like the rest of us :(

> > I've noticed in my testing that the Marvell DSA code seems to poll the
> > internal phy link status in mv88e6xxx_poll_link(), and set the network
> > device carrier status according to the results.
> 
> Peter Korsgaard comment might be correct, the switch needs to know
> what the PHY has negotiated. Hence the use of the PPU. There are also
> comments in the code that the PPU is needed for indirect access to the
> PHY.

I'm not changing the PPU at all - all I'm doing is to remove the extra
layer of software polling which partially duplicates what the phy layer
is already doing with its own polling.

The poll_link code is reading the port PCS control register and the
port status register, and using that to (a) control the netdev carrier
state, and (b) printing the link state.

Meanwhile, the PHY layer is polling the PHY that we created for the
port, decoding its registers, and controlling the same netdev carrier
state, parsing the standard MII register set to get the speed, duplex
and other parameters, and dsa_slave_adjust_link() is the printing the
new link state.  This then goes on to program the switch port via
mv88e6xxx_adjust_link() if necessary.

So, it's the PHY layer which is doing all the real work, the poll_link
code is just a duplication.

My current patch for this looks like this, and it's what I'm running
against the 88E6176 switch.  Link state is detected via the phy layer
and everything appears to work fine.

I think mv88e6060_poll_link() in drivers/net/dsa/mv88e6060.c can go the
same way too (it seems to be mostly the same as the code below, except
it doesn't support gigabit).  That would mean there's no users of the
.poll_link method in drivers/net/dsa - and that probably means the link
polling code in net/dsa can also be removed.

 drivers/net/dsa/mv88e6123_61_65.c |  1 -
 drivers/net/dsa/mv88e6131.c       |  1 -
 drivers/net/dsa/mv88e6171.c       |  1 -
 drivers/net/dsa/mv88e6352.c       |  1 -
 drivers/net/dsa/mv88e6xxx.c       | 67 ---------------------------------------
 drivers/net/dsa/mv88e6xxx.h       |  1 -
 6 files changed, 72 deletions(-)

diff --git a/drivers/net/dsa/mv88e6123_61_65.c b/drivers/net/dsa/mv88e6123_61_65.c
index 3de2a6d73fdc..4bcfd683bbea 100644
--- a/drivers/net/dsa/mv88e6123_61_65.c
+++ b/drivers/net/dsa/mv88e6123_61_65.c
@@ -125,7 +125,6 @@ struct dsa_switch_driver mv88e6123_61_65_switch_driver = {
 	.set_addr		= mv88e6xxx_set_addr_indirect,
 	.phy_read		= mv88e6xxx_phy_read,
 	.phy_write		= mv88e6xxx_phy_write,
-	.poll_link		= mv88e6xxx_poll_link,
 	.get_strings		= mv88e6xxx_get_strings,
 	.get_ethtool_stats	= mv88e6xxx_get_ethtool_stats,
 	.get_sset_count		= mv88e6xxx_get_sset_count,
diff --git a/drivers/net/dsa/mv88e6131.c b/drivers/net/dsa/mv88e6131.c
index 3e8386529965..c73121c8f155 100644
--- a/drivers/net/dsa/mv88e6131.c
+++ b/drivers/net/dsa/mv88e6131.c
@@ -178,7 +178,6 @@ struct dsa_switch_driver mv88e6131_switch_driver = {
 	.set_addr		= mv88e6xxx_set_addr_direct,
 	.phy_read		= mv88e6131_phy_read,
 	.phy_write		= mv88e6131_phy_write,
-	.poll_link		= mv88e6xxx_poll_link,
 	.get_strings		= mv88e6xxx_get_strings,
 	.get_ethtool_stats	= mv88e6xxx_get_ethtool_stats,
 	.get_sset_count		= mv88e6xxx_get_sset_count,
diff --git a/drivers/net/dsa/mv88e6171.c b/drivers/net/dsa/mv88e6171.c
index c2daaf087761..c95cfab56a4f 100644
--- a/drivers/net/dsa/mv88e6171.c
+++ b/drivers/net/dsa/mv88e6171.c
@@ -104,7 +104,6 @@ struct dsa_switch_driver mv88e6171_switch_driver = {
 	.set_addr		= mv88e6xxx_set_addr_indirect,
 	.phy_read		= mv88e6xxx_phy_read_indirect,
 	.phy_write		= mv88e6xxx_phy_write_indirect,
-	.poll_link		= mv88e6xxx_poll_link,
 	.get_strings		= mv88e6xxx_get_strings,
 	.get_ethtool_stats	= mv88e6xxx_get_ethtool_stats,
 	.get_sset_count		= mv88e6xxx_get_sset_count,
diff --git a/drivers/net/dsa/mv88e6352.c b/drivers/net/dsa/mv88e6352.c
index 1f5129c105fb..37367060676f 100644
--- a/drivers/net/dsa/mv88e6352.c
+++ b/drivers/net/dsa/mv88e6352.c
@@ -324,7 +324,6 @@ struct dsa_switch_driver mv88e6352_switch_driver = {
 	.set_addr		= mv88e6xxx_set_addr_indirect,
 	.phy_read		= mv88e6xxx_phy_read_indirect,
 	.phy_write		= mv88e6xxx_phy_write_indirect,
-	.poll_link		= mv88e6xxx_poll_link,
 	.get_strings		= mv88e6xxx_get_strings,
 	.get_ethtool_stats	= mv88e6xxx_get_ethtool_stats,
 	.get_sset_count		= mv88e6xxx_get_sset_count,
diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
index f8baa897d1a0..c6683f65b125 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -388,73 +388,6 @@ int mv88e6xxx_phy_write_ppu(struct dsa_switch *ds, int addr,
 }
 #endif
 
-void mv88e6xxx_poll_link(struct dsa_switch *ds)
-{
-	int i;
-
-	for (i = 0; i < DSA_MAX_PORTS; i++) {
-		struct net_device *dev;
-		int uninitialized_var(port_status);
-		int pcs_ctrl;
-		int link;
-		int speed;
-		int duplex;
-		int fc;
-
-		dev = ds->ports[i];
-		if (dev == NULL)
-			continue;
-
-		pcs_ctrl = mv88e6xxx_reg_read(ds, REG_PORT(i), PORT_PCS_CTRL);
-		if (pcs_ctrl < 0 || pcs_ctrl & PORT_PCS_CTRL_FORCE_LINK)
-			continue;
-
-		link = 0;
-		if (dev->flags & IFF_UP) {
-			port_status = mv88e6xxx_reg_read(ds, REG_PORT(i),
-							 PORT_STATUS);
-			if (port_status < 0)
-				continue;
-
-			link = !!(port_status & PORT_STATUS_LINK);
-		}
-
-		if (!link) {
-			if (netif_carrier_ok(dev)) {
-				netdev_info(dev, "link down\n");
-				netif_carrier_off(dev);
-			}
-			continue;
-		}
-
-		switch (port_status & PORT_STATUS_SPEED_MASK) {
-		case PORT_STATUS_SPEED_10:
-			speed = 10;
-			break;
-		case PORT_STATUS_SPEED_100:
-			speed = 100;
-			break;
-		case PORT_STATUS_SPEED_1000:
-			speed = 1000;
-			break;
-		default:
-			speed = -1;
-			break;
-		}
-		duplex = (port_status & PORT_STATUS_DUPLEX) ? 1 : 0;
-		fc = (port_status & PORT_STATUS_PAUSE_EN) ? 1 : 0;
-
-		if (!netif_carrier_ok(dev)) {
-			netdev_info(dev,
-				    "link up, %d Mb/s, %s duplex, flow control %sabled\n",
-				    speed,
-				    duplex ? "full" : "half",
-				    fc ? "en" : "dis");
-			netif_carrier_on(dev);
-		}
-	}
-}
-
 static bool mv88e6xxx_6065_family(struct dsa_switch *ds)
 {
 	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
diff --git a/drivers/net/dsa/mv88e6xxx.h b/drivers/net/dsa/mv88e6xxx.h
index 9b6f3d9d5ae1..d94a4e134ee2 100644
--- a/drivers/net/dsa/mv88e6xxx.h
+++ b/drivers/net/dsa/mv88e6xxx.h
@@ -442,7 +442,6 @@ void mv88e6xxx_ppu_state_init(struct dsa_switch *ds);
 int mv88e6xxx_phy_read_ppu(struct dsa_switch *ds, int addr, int regnum);
 int mv88e6xxx_phy_write_ppu(struct dsa_switch *ds, int addr,
 			    int regnum, u16 val);
-void mv88e6xxx_poll_link(struct dsa_switch *ds);
 void mv88e6xxx_get_strings(struct dsa_switch *ds, int port, uint8_t *data);
 void mv88e6xxx_get_ethtool_stats(struct dsa_switch *ds, int port,
 				 uint64_t *data);


-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.