[PATCH] net: phy: phy_remove_link_mode should not advertise new modes

[PATCH] net: phy: phy_remove_link_mode should not advertise new modes

[Helmut Grohne]

When doing "ip link set dev ... up" for a ksz9477 backed link,
ksz9477_phy_setup is called and it calls phy_remove_link_mode to remove
1000baseT HDX. During phy_remove_link_mode, phy_advertise_supported is
called.

If one wants to advertise fewer modes than the supported ones, one
usually reduces the advertised link modes before upping the link (e.g.
by passing an appropriate .link file to udev).  However upping
overrwrites the advertised link modes due to the call to
phy_advertise_supported reverting to the supported link modes.

It seems unintentional to have phy_remove_link_mode enable advertising
bits and it does not match its description in any way. Instead of
calling phy_advertise_supported, we should simply clear the link mode to
be removed from both supported and advertising.

Signed-off-by: Helmut Grohne <helmut.grohne@intenta.de>
Fixes: 41124fa64d4b29 ("net: ethernet: Add helper to remove a supported link mode")
---
 drivers/net/phy/phy_device.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index b4978c5fb2ca..74d06dc8fddb 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -2509,7 +2509,7 @@ EXPORT_SYMBOL(genphy_loopback);
 void phy_remove_link_mode(struct phy_device *phydev, u32 link_mode)
 {
 	linkmode_clear_bit(link_mode, phydev->supported);
-	phy_advertise_supported(phydev);
+	linkmode_clear_bit(link_mode, phydev->advertising);
 }
 EXPORT_SYMBOL(phy_remove_link_mode);
 
-- 
2.20.1

Re: [PATCH] net: phy: phy_remove_link_mode should not advertise new modes

[David Miller]

From: Helmut Grohne <helmut.grohne@intenta.de>
Date: Tue, 14 Jul 2020 10:25:42 +0200

> When doing "ip link set dev ... up" for a ksz9477 backed link,
> ksz9477_phy_setup is called and it calls phy_remove_link_mode to remove
> 1000baseT HDX. During phy_remove_link_mode, phy_advertise_supported is
> called.
> 
> If one wants to advertise fewer modes than the supported ones, one
> usually reduces the advertised link modes before upping the link (e.g.
> by passing an appropriate .link file to udev).  However upping
> overrwrites the advertised link modes due to the call to
> phy_advertise_supported reverting to the supported link modes.
> 
> It seems unintentional to have phy_remove_link_mode enable advertising
> bits and it does not match its description in any way. Instead of
> calling phy_advertise_supported, we should simply clear the link mode to
> be removed from both supported and advertising.
> 
> Signed-off-by: Helmut Grohne <helmut.grohne@intenta.de>
> Fixes: 41124fa64d4b29 ("net: ethernet: Add helper to remove a supported link mode")

The problem is that we can't allow the advertised setting to exceed
what is in the supported list.

That's why this helper is coded this way from day one.

Re: [PATCH] net: phy: phy_remove_link_mode should not advertise new modes

[Helmut Grohne]

On Tue, Jul 14, 2020 at 11:07:10PM +0200, David Miller wrote:
> From: Helmut Grohne <helmut.grohne@intenta.de>
> Date: Tue, 14 Jul 2020 10:25:42 +0200
> 
> > When doing "ip link set dev ... up" for a ksz9477 backed link,
> > ksz9477_phy_setup is called and it calls phy_remove_link_mode to remove
> > 1000baseT HDX. During phy_remove_link_mode, phy_advertise_supported is
> > called.
> > 
> > If one wants to advertise fewer modes than the supported ones, one
> > usually reduces the advertised link modes before upping the link (e.g.
> > by passing an appropriate .link file to udev).  However upping
> > overrwrites the advertised link modes due to the call to
> > phy_advertise_supported reverting to the supported link modes.
> > 
> > It seems unintentional to have phy_remove_link_mode enable advertising
> > bits and it does not match its description in any way. Instead of
> > calling phy_advertise_supported, we should simply clear the link mode to
> > be removed from both supported and advertising.
> 
> The problem is that we can't allow the advertised setting to exceed
> what is in the supported list.
> 
> That's why this helper is coded this way from day one.

Would you mind going into a little more detail here?

I think you have essentially two possible cases with respect to that
assertion.

Case A: advertised does not exceed supported before the call to
        phy_remove_link_mode.

    In this case, the relevant link mode is removed from both supported
    and advertised after my patch and therefore the requested invariant
    is still ok.

Case B: advertised exceeds supported prior to the call to
        phy_remove_link_mode.

    You said that we cannot allow this to happen. So it would seem to be
    a bug somewhere else. Do you see phy_remove_link_mode as a tool to
    fix up a violated invariant?

It also is not true that the current code ensures your assertion.
Specifically, phy_advertise_supported copies the pause bits from the old
advertised to the new one regardless of whether they're set in
supported. I believe this is expected, but it means that your invariant
needs to be:

    We cannot allow advertised to exceed the supported list for
    non-pause bits.

In any case, having a helper called "phy_remove_link_mode" enable bits
in the advertised bit field is fairly unexpected. Do you disagree with
this being a bug?

Helmut

Re: [PATCH] net: phy: phy_remove_link_mode should not advertise new modes

[Jakub Kicinski]

On Wed, 15 Jul 2020 09:03:45 +0200 Helmut Grohne wrote:
> On Tue, Jul 14, 2020 at 11:07:10PM +0200, David Miller wrote:
> > From: Helmut Grohne <helmut.grohne@intenta.de>
> > Date: Tue, 14 Jul 2020 10:25:42 +0200
> >   
> > > When doing "ip link set dev ... up" for a ksz9477 backed link,
> > > ksz9477_phy_setup is called and it calls phy_remove_link_mode to remove
> > > 1000baseT HDX. During phy_remove_link_mode, phy_advertise_supported is
> > > called.
> > > 
> > > If one wants to advertise fewer modes than the supported ones, one
> > > usually reduces the advertised link modes before upping the link (e.g.
> > > by passing an appropriate .link file to udev).  However upping
> > > overrwrites the advertised link modes due to the call to
> > > phy_advertise_supported reverting to the supported link modes.
> > > 
> > > It seems unintentional to have phy_remove_link_mode enable advertising
> > > bits and it does not match its description in any way. Instead of
> > > calling phy_advertise_supported, we should simply clear the link mode to
> > > be removed from both supported and advertising.  
> > 
> > The problem is that we can't allow the advertised setting to exceed
> > what is in the supported list.
> > 
> > That's why this helper is coded this way from day one.  
> 
> Would you mind going into a little more detail here?
> 
> I think you have essentially two possible cases with respect to that
> assertion.
> 
> Case A: advertised does not exceed supported before the call to
>         phy_remove_link_mode.
> 
>     In this case, the relevant link mode is removed from both supported
>     and advertised after my patch and therefore the requested invariant
>     is still ok.
> 
> Case B: advertised exceeds supported prior to the call to
>         phy_remove_link_mode.
> 
>     You said that we cannot allow this to happen. So it would seem to be
>     a bug somewhere else. Do you see phy_remove_link_mode as a tool to
>     fix up a violated invariant?

Is 

Case C: driver does not initialize advertised at all and depends on
        phy_remove_link_mode() to do it

possible?

> It also is not true that the current code ensures your assertion.
> Specifically, phy_advertise_supported copies the pause bits from the old
> advertised to the new one regardless of whether they're set in
> supported. I believe this is expected, but it means that your invariant
> needs to be:
> 
>     We cannot allow advertised to exceed the supported list for
>     non-pause bits.
> 
> In any case, having a helper called "phy_remove_link_mode" enable bits
> in the advertised bit field is fairly unexpected. Do you disagree with
> this being a bug?

Hm. I think it's clear that the change may uncover other bugs, but
perhaps indeed those should be addressed elsewhere.

Andrew, WDYT?

Re: [PATCH] net: phy: phy_remove_link_mode should not advertise new modes

[Andrew Lunn]

> Is 
> 
> Case C: driver does not initialize advertised at all and depends on
>         phy_remove_link_mode() to do it
> 
> possible?

No. phylib initializes advertise to supported as part of probing the
PHY. So the PHY by default advertises everything it supports, except
the oddities of Pause.

    Andrew

Re: [PATCH] net: phy: phy_remove_link_mode should not advertise new modes

[Andrew Lunn]

> It also is not true that the current code ensures your assertion.
> Specifically, phy_advertise_supported copies the pause bits from the old
> advertised to the new one regardless of whether they're set in
> supported.

This is an oddity of Pause. The PHY should not sets Pause in
supported, because the PHY is not the device which implements
Pause. The MAC needs to indicate to PHYLIB it implements Pause, and
then the PHY will advertise Pause.

I will address the other points in a separate email.

  Andrew

Re: [PATCH] net: phy: phy_remove_link_mode should not advertise new modes

[Andrew Lunn]

On Tue, Jul 14, 2020 at 10:25:42AM +0200, Helmut Grohne wrote:
> When doing "ip link set dev ... up" for a ksz9477 backed link,
> ksz9477_phy_setup is called and it calls phy_remove_link_mode to remove
> 1000baseT HDX. During phy_remove_link_mode, phy_advertise_supported is
> called.
> 
> If one wants to advertise fewer modes than the supported ones, one
> usually reduces the advertised link modes before upping the link (e.g.
> by passing an appropriate .link file to udev).  However upping
> overrwrites the advertised link modes due to the call to
> phy_advertise_supported reverting to the supported link modes.
> 
> It seems unintentional to have phy_remove_link_mode enable advertising
> bits and it does not match its description in any way. Instead of
> calling phy_advertise_supported, we should simply clear the link mode to
> be removed from both supported and advertising.

We have two different reasons for removing link modes.

1) The PHY cannot support a link mode. E.g.

static int bcm84881_get_features(struct phy_device *phydev)
{
        int ret;

        ret = genphy_c45_pma_read_abilities(phydev);
        if (ret)
                return ret;

        /* Although the PHY sets bit 1.11.8, it does not support 10M modes */
        linkmode_clear_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT,
                           phydev->supported);
        linkmode_clear_bit(ETHTOOL_LINK_MODE_10baseT_Full_BIT,
                           phydev->supported);

        return 0;
}

This is done very early on, as part of probing the PHY. This is done
before supported is copied into advertised towards the end of the PHYs
probe.

2) The MAC does not support a link mode. It uses
phy_remove_link_mode() to remove a link mode. There are two different
times this can be done:

a) As part of open(), the PHY is connected to the MAC. Since the PHY
is not connected to the MAC until you open it, you cannot use ethtool
to change the advertised modes until you have opened it. Hence user
space cannot of removed anything and you don't need to worry about
this copy.

b) As part of the MAC drivers probe, the PHY is connected to the MAC.
In this case, ethtool can be used by userspace to remove link
modes. But the MAC driver should of already removed the modes it does
not support, directly after connecting the PHY to the MAC in its probe
function. So advertising and supported at the same already.

The key point here is ksz9477_phy_setup(), and how it breaks this
model. It is called from ksz_enable_port(). That is called via
dsa_port_enable() in dsa_slave_open(). But the PHY was connected to
the MAC during probe of the MAC. So we have a bad mix of a) and b),
which is leading to your problem. You need to fix the switch driver so
it cleanly does b), removes the link mode early on before the user has
chance to use ethtool.

       Andrew

[PATCH v2] net: dsa: microchip: call phy_remove_link_mode during probe

[Helmut Grohne]

When doing "ip link set dev ... up" for a ksz9477 backed link,
ksz9477_phy_setup is called and it calls phy_remove_link_mode to remove
1000baseT HDX. During phy_remove_link_mode, phy_advertise_supported is
called. Doing so reverts any previous change to advertised link modes
e.g. using a udevd .link file.

phy_remove_link_mode is not meant to be used while opening a link and
should be called during phy probe when the link is not yet available to
userspace.

Therefore move the phy_remove_link_mode calls into ksz9477_setup. This
is called during dsa_switch_register and thus comes after
ksz9477_switch_detect, which initializes dev->features.

Remove phy_setup from ksz_dev_ops as no users remain.

Link: https://lore.kernel.org/netdev/20200715192722.GD1256692@lunn.ch/
Fixes: 42fc6a4c613019 ("net: dsa: microchip: prepare PHY for proper advertisement")
Signed-off-by: Helmut Grohne <helmut.grohne@intenta.de>
---
 drivers/net/dsa/microchip/ksz9477.c    | 31 ++++++++++----------------
 drivers/net/dsa/microchip/ksz_common.c |  2 --
 drivers/net/dsa/microchip/ksz_common.h |  2 --
 3 files changed, 12 insertions(+), 23 deletions(-)

changes since v1:
 * Don't change phy_remove_link_mode. Instead, call it at the right
   time. Thanks to Andrew Lunn for the detailed explanation.

Helmut

diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index 8d15c3016024..d0023916e1e8 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -974,23 +974,6 @@ static void ksz9477_port_mirror_del(struct dsa_switch *ds, int port,
 			     PORT_MIRROR_SNIFFER, false);
 }
 
-static void ksz9477_phy_setup(struct ksz_device *dev, int port,
-			      struct phy_device *phy)
-{
-	/* Only apply to port with PHY. */
-	if (port >= dev->phy_port_cnt)
-		return;
-
-	/* The MAC actually cannot run in 1000 half-duplex mode. */
-	phy_remove_link_mode(phy,
-			     ETHTOOL_LINK_MODE_1000baseT_Half_BIT);
-
-	/* PHY does not support gigabit. */
-	if (!(dev->features & GBIT_SUPPORT))
-		phy_remove_link_mode(phy,
-				     ETHTOOL_LINK_MODE_1000baseT_Full_BIT);
-}
-
 static bool ksz9477_get_gbit(struct ksz_device *dev, u8 data)
 {
 	bool gbit;
@@ -1353,7 +1336,7 @@ static void ksz9477_config_cpu_port(struct dsa_switch *ds)
 static int ksz9477_setup(struct dsa_switch *ds)
 {
 	struct ksz_device *dev = ds->priv;
-	int ret = 0;
+	int ret = 0, i;
 
 	dev->vlan_cache = devm_kcalloc(dev->dev, sizeof(struct vlan_table),
 				       dev->num_vlans, GFP_KERNEL);
@@ -1391,6 +1374,17 @@ static int ksz9477_setup(struct dsa_switch *ds)
 
 	ksz_init_mib_timer(dev);
 
+	for (i = 0; i < dev->phy_port_cnt; ++i) {
+		/* The MAC actually cannot run in 1000 half-duplex mode. */
+		phy_remove_link_mode(&dev->ports[i].phydev,
+				     ETHTOOL_LINK_MODE_1000baseT_Half_BIT);
+
+		/* PHY does not support gigabit. */
+		if (!(dev->features & GBIT_SUPPORT))
+			phy_remove_link_mode(&dev->ports[i].phydev,
+					     ETHTOOL_LINK_MODE_1000baseT_Full_BIT);
+	}
+
 	return 0;
 }
 
@@ -1603,7 +1597,6 @@ static const struct ksz_dev_ops ksz9477_dev_ops = {
 	.get_port_addr = ksz9477_get_port_addr,
 	.cfg_port_member = ksz9477_cfg_port_member,
 	.flush_dyn_mac_table = ksz9477_flush_dyn_mac_table,
-	.phy_setup = ksz9477_phy_setup,
 	.port_setup = ksz9477_port_setup,
 	.r_mib_cnt = ksz9477_r_mib_cnt,
 	.r_mib_pkt = ksz9477_r_mib_pkt,
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index fd1d6676ae4f..7b6c0dce7536 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -358,8 +358,6 @@ int ksz_enable_port(struct dsa_switch *ds, int port, struct phy_device *phy)
 
 	/* setup slave port */
 	dev->dev_ops->port_setup(dev, port, false);
-	if (dev->dev_ops->phy_setup)
-		dev->dev_ops->phy_setup(dev, port, phy);
 
 	/* port_stp_state_set() will be called after to enable the port so
 	 * there is no need to do anything.
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index f2c9bb68fd33..7d11dd32ec0d 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -119,8 +119,6 @@ struct ksz_dev_ops {
 	u32 (*get_port_addr)(int port, int offset);
 	void (*cfg_port_member)(struct ksz_device *dev, int port, u8 member);
 	void (*flush_dyn_mac_table)(struct ksz_device *dev, int port);
-	void (*phy_setup)(struct ksz_device *dev, int port,
-			  struct phy_device *phy);
 	void (*port_cleanup)(struct ksz_device *dev, int port);
 	void (*port_setup)(struct ksz_device *dev, int port, bool cpu_port);
 	void (*r_phy)(struct ksz_device *dev, u16 phy, u16 reg, u16 *val);
-- 
2.20.1

Re: [PATCH v2] net: dsa: microchip: call phy_remove_link_mode during probe

[Andrew Lunn]

On Thu, Jul 16, 2020 at 02:57:24PM +0200, Helmut Grohne wrote:
> When doing "ip link set dev ... up" for a ksz9477 backed link,
> ksz9477_phy_setup is called and it calls phy_remove_link_mode to remove
> 1000baseT HDX. During phy_remove_link_mode, phy_advertise_supported is
> called. Doing so reverts any previous change to advertised link modes
> e.g. using a udevd .link file.
> 
> phy_remove_link_mode is not meant to be used while opening a link and
> should be called during phy probe when the link is not yet available to
> userspace.
> 
> Therefore move the phy_remove_link_mode calls into ksz9477_setup. This
> is called during dsa_switch_register and thus comes after
> ksz9477_switch_detect, which initializes dev->features.
> 
> Remove phy_setup from ksz_dev_ops as no users remain.
> 
> Link: https://lore.kernel.org/netdev/20200715192722.GD1256692@lunn.ch/
> Fixes: 42fc6a4c613019 ("net: dsa: microchip: prepare PHY for proper advertisement")
> Signed-off-by: Helmut Grohne <helmut.grohne@intenta.de>
> ---
>  drivers/net/dsa/microchip/ksz9477.c    | 31 ++++++++++----------------
>  drivers/net/dsa/microchip/ksz_common.c |  2 --
>  drivers/net/dsa/microchip/ksz_common.h |  2 --
>  3 files changed, 12 insertions(+), 23 deletions(-)
> 
> changes since v1:
>  * Don't change phy_remove_link_mode. Instead, call it at the right
>    time. Thanks to Andrew Lunn for the detailed explanation.
> 
> Helmut

Hi Helmut

This change looks better.

However, i'm having trouble understanding how PHYs actually work in
this driver. 

We have:

struct ksz_port {
        u16 member;
        u16 vid_member;
        int stp_state;
        struct phy_device phydev;

with an instance of this structure per port of the switch.

And it is this phydev which you are manipulating.

> +	for (i = 0; i < dev->phy_port_cnt; ++i) {
> +		/* The MAC actually cannot run in 1000 half-duplex mode. */
> +		phy_remove_link_mode(&dev->ports[i].phydev,
> +				     ETHTOOL_LINK_MODE_1000baseT_Half_BIT);
> +
> +		/* PHY does not support gigabit. */
> +		if (!(dev->features & GBIT_SUPPORT))
> +			phy_remove_link_mode(&dev->ports[i].phydev,
> +					     ETHTOOL_LINK_MODE_1000baseT_Full_BIT);
> +	}
> +
>  	return 0;

But how is this phydev associated with the netdev? I don't see how
phylink_connect_phy() is called using this phydev structure?

      Andrew

Re: [PATCH v2] net: dsa: microchip: call phy_remove_link_mode during probe

[Helmut Grohne]

On Thu, Jul 16, 2020 at 04:10:44PM +0200, Andrew Lunn wrote:
> However, i'm having trouble understanding how PHYs actually work in
> this driver. 
> 
> We have:
> 
> struct ksz_port {
>         u16 member;
>         u16 vid_member;
>         int stp_state;
>         struct phy_device phydev;
> 
> with an instance of this structure per port of the switch.
> 
> And it is this phydev which you are manipulating.
> 
> > +	for (i = 0; i < dev->phy_port_cnt; ++i) {
> > +		/* The MAC actually cannot run in 1000 half-duplex mode. */
> > +		phy_remove_link_mode(&dev->ports[i].phydev,
> > +				     ETHTOOL_LINK_MODE_1000baseT_Half_BIT);
> > +
> > +		/* PHY does not support gigabit. */
> > +		if (!(dev->features & GBIT_SUPPORT))
> > +			phy_remove_link_mode(&dev->ports[i].phydev,
> > +					     ETHTOOL_LINK_MODE_1000baseT_Full_BIT);
> > +	}
> > +
> >  	return 0;
> 
> But how is this phydev associated with the netdev? I don't see how
> phylink_connect_phy() is called using this phydev structure?

The ksz* drivers are implemented using the DSA framework. The relevant
phylink_connect_phy call is issued by the DSA infrastructure. We can see
this (and its ordering relative to phy_remove_link_mode after my patch)
using ftrace by adding the following to the kernel command line:

    trace_options=func_stack_trace ftrace=function ftrace_filter=phylink_connect_phy,phy_remove_link_mode

I've added the trace buffer to the end of this mail to avoid cluttering
it. The key takeaways are:
 * phylink_connect_phy is called by dsa_slave_create. A few inlined
   functions later we arrive at dsa_register_switch, which is called
   during driver probe.
 * All phy_remove_link_mode calls now happen before any
   phylink_connect_phy calls as requested.

Helmut

----8<-----8<------
# tracer: function
#
# entries-in-buffer/entries-written: 10/10   #P:2
#
#                              _-----=> irqs-off
#                             / _----=> need-resched
#                            | / _---=> hardirq/softirq
#                            || / _--=> preempt-depth
#                            ||| /     delay
#           TASK-PID   CPU#  ||||    TIMESTAMP  FUNCTION
#              | |       |   ||||       |         |
       swapper/0-1     [000] ....     7.166645: phy_remove_link_mode <-macb_probe
       swapper/0-1     [000] ....     7.166654: <stack trace>
 => macb_probe
 => platform_drv_probe
 => really_probe
 => driver_probe_device
 => device_driver_attach
 => __driver_attach
 => bus_for_each_dev
 => driver_attach
 => bus_add_driver
 => driver_register
 => __platform_driver_register
 => macb_driver_init
 => do_one_initcall
 => kernel_init_freeable
 => kernel_init
 => ret_from_fork
 => 0
     kworker/1:1-28    [001] ....     7.592479: phy_remove_link_mode <-ksz9477_setup
     kworker/1:1-28    [001] ....     7.592489: <stack trace>
 => ksz9477_setup
 => dsa_register_switch
 => ksz_switch_register
 => ksz9477_switch_register
 => ksz9477_i2c_probe
 => i2c_device_probe
 => really_probe
 => driver_probe_device
 => __device_attach_driver
 => bus_for_each_drv
 => __device_attach
 => device_initial_probe
 => bus_probe_device
 => deferred_probe_work_func
 => process_one_work
 => worker_thread
 => kthread
 => ret_from_fork
 => 0
     kworker/1:1-28    [001] ....     7.592494: phy_remove_link_mode <-ksz9477_setup
     kworker/1:1-28    [001] ....     7.592498: <stack trace>
 => ksz9477_setup
 => dsa_register_switch
 => ksz_switch_register
 => ksz9477_switch_register
 => ksz9477_i2c_probe
 => i2c_device_probe
 => really_probe
 => driver_probe_device
 => __device_attach_driver
 => bus_for_each_drv
 => __device_attach
 => device_initial_probe
 => bus_probe_device
 => deferred_probe_work_func
 => process_one_work
 => worker_thread
 => kthread
 => ret_from_fork
 => 0
     kworker/1:1-28    [001] ....     7.604375: phylink_connect_phy <-dsa_slave_create
     kworker/1:1-28    [001] ....     7.604383: <stack trace>
 => dsa_slave_create
 => dsa_register_switch
 => ksz_switch_register
 => ksz9477_switch_register
 => ksz9477_i2c_probe
 => i2c_device_probe
 => really_probe
 => driver_probe_device
 => __device_attach_driver
 => bus_for_each_drv
 => __device_attach
 => device_initial_probe
 => bus_probe_device
 => deferred_probe_work_func
 => process_one_work
 => worker_thread
 => kthread
 => ret_from_fork
 => 0
     kworker/1:1-28    [001] .n..     7.623675: phylink_connect_phy <-dsa_slave_create
     kworker/1:1-28    [001] .n..     7.623685: <stack trace>
 => dsa_slave_create
 => dsa_register_switch
 => ksz_switch_register
 => ksz9477_switch_register
 => ksz9477_i2c_probe
 => i2c_device_probe
 => really_probe
 => driver_probe_device
 => __device_attach_driver
 => bus_for_each_drv
 => __device_attach
 => device_initial_probe
 => bus_probe_device
 => deferred_probe_work_func
 => process_one_work
 => worker_thread
 => kthread
 => ret_from_fork
 => 0
---->8----->8------

Re: [PATCH v2] net: dsa: microchip: call phy_remove_link_mode during probe

[Andrew Lunn]

On Fri, Jul 17, 2020 at 10:18:52AM +0200, Helmut Grohne wrote:
> On Thu, Jul 16, 2020 at 04:10:44PM +0200, Andrew Lunn wrote:
> > However, i'm having trouble understanding how PHYs actually work in
> > this driver. 
> > 
> > We have:
> > 
> > struct ksz_port {
> >         u16 member;
> >         u16 vid_member;
> >         int stp_state;
> >         struct phy_device phydev;
> > 
> > with an instance of this structure per port of the switch.
> > 
> > And it is this phydev which you are manipulating.
> > 
> > > +	for (i = 0; i < dev->phy_port_cnt; ++i) {
> > > +		/* The MAC actually cannot run in 1000 half-duplex mode. */
> > > +		phy_remove_link_mode(&dev->ports[i].phydev,
> > > +				     ETHTOOL_LINK_MODE_1000baseT_Half_BIT);
> > > +
> > > +		/* PHY does not support gigabit. */
> > > +		if (!(dev->features & GBIT_SUPPORT))
> > > +			phy_remove_link_mode(&dev->ports[i].phydev,
> > > +					     ETHTOOL_LINK_MODE_1000baseT_Full_BIT);
> > > +	}
> > > +
> > >  	return 0;
> > 
> > But how is this phydev associated with the netdev? I don't see how
> > phylink_connect_phy() is called using this phydev structure?
> 
> The ksz* drivers are implemented using the DSA framework. The relevant
> phylink_connect_phy call is issued by the DSA infrastructure. We can see
> this (and its ordering relative to phy_remove_link_mode after my patch)
> using ftrace by adding the following to the kernel command line:

Hi Helmut

I'm not questioning the ordering. I'm questioning which phydev
structure is being manipulated.

We have:
        return phylink_connect_phy(dp->pl, slave_dev->phydev);

and your new:

+               phy_remove_link_mode(&dev->ports[i].phydev,
+                                    ETHTOOL_LINK_MODE_1000baseT_Half_BIT);
+

Is slave_dev->phydev == &dev->ports[i].phydev ?

To me, that is not obviously correct. This driver is doing odd things
with PHYs because of how they fit into the register map. And this
oddness it making it hard for me to follow this code and see how these
is true. It could well be true, i just don't see how.

     Andrew

[PATCH v3] net: dsa: microchip: call phy_remove_link_mode during probe

[Helmut Grohne]

When doing "ip link set dev ... up" for a ksz9477 backed link,
ksz9477_phy_setup is called and it calls phy_remove_link_mode to remove
1000baseT HDX. During phy_remove_link_mode, phy_advertise_supported is
called. Doing so reverts any previous change to advertised link modes
e.g. using a udevd .link file.

phy_remove_link_mode is not meant to be used while opening a link and
should be called during phy probe when the link is not yet available to
userspace.

Therefore move the phy_remove_link_mode calls into
ksz9477_switch_register. It indirectly calls dsa_register_switch, which
creates the relevant struct phy_devices and we update the link modes
right after that. At that time dev->features is already initialized by
ksz9477_switch_detect.

Remove phy_setup from ksz_dev_ops as no users remain.

Link: https://lore.kernel.org/netdev/20200715192722.GD1256692@lunn.ch/
Fixes: 42fc6a4c613019 ("net: dsa: microchip: prepare PHY for proper advertisement")
Signed-off-by: Helmut Grohne <helmut.grohne@intenta.de>
---
 drivers/net/dsa/microchip/ksz9477.c    | 39 +++++++++++++-------------
 drivers/net/dsa/microchip/ksz_common.c |  2 --
 drivers/net/dsa/microchip/ksz_common.h |  2 --
 3 files changed, 20 insertions(+), 23 deletions(-)

On Fri, Jul 17, 2020 at 03:18:14PM +0200, Andrew Lunn wrote:
> I'm not questioning the ordering. I'm questioning which phydev
> structure is being manipulated.
...
> Is slave_dev->phydev == &dev->ports[i].phydev ?

You are spot on. I mistakenly assumed this to be the case, but it really
is not. Thank you. Your detailed explanations are much appreciated. This
slipped my testing, because I only checked whether the 1Gbit HDX mode
was correctly removed. It seems like something else does that already,
so I didn't notice that I was operating on the wrong phy_device.

The dev->ports[i].phydev is not actually exposed beyond the driver. The
driver sets the phydev.speed in a few places and even reads it back in
one place. It also sets phydev.duplex, but never reads it back. It
queries phydev.link, which is statically 0 due to using devm_kzalloc.

I think the use of this ksz_port.phydev is very misleading, but I'm
unsure how to fix this. It is not clear to me whether all those updates
should be performed on the connected phydev instead or whether this is
just internal state tracking.

That leaves the question of when to remove the link modes. ksz9477_setup
is called before dsa_register_switch. At the time it runs, the port's
slave device is a NULL pointer. The actual phydev is not linked up and
we cannot access it. The phydev only gets initialized during
dsa_register_switch when dsa_slave_phy_connect is called. Since there is
no suitable hook there, we cannot change the link modes before
phylink_connect_phy is called.

As far as I understand your previous mails, it is not necessary to
remove the link modes before phylink_connect_phy. So the next place
after dsa_register_switch seems to be inside ksz9477_switch_register (as
dsa_register_switch is the final call in ksz_switch_register). I'm
unsure whether this poses a race condition with user space, but on the
system under test, the race is reliably won if there is any.

Beyond resolving the phydev mess, I wish that we could agree on the
order of verbs and nouns in symbols in some way. dsa_register_switch vs.
ksz_switch_register became confusing at a time. I see where either is
coming from, but I think that consistency would be better here.

Helmut

changes since v2:
 * Operate on the correct phydev. Thanks to Andrew Lunn.
changes since v1:
 * Don't change phy_remove_link_mode. Instead, call it at the right
   time. Thanks to Andrew Lunn for the detailed explanation.

diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index 8d15c3016024..368964b09aae 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -974,23 +974,6 @@ static void ksz9477_port_mirror_del(struct dsa_switch *ds, int port,
 			     PORT_MIRROR_SNIFFER, false);
 }
 
-static void ksz9477_phy_setup(struct ksz_device *dev, int port,
-			      struct phy_device *phy)
-{
-	/* Only apply to port with PHY. */
-	if (port >= dev->phy_port_cnt)
-		return;
-
-	/* The MAC actually cannot run in 1000 half-duplex mode. */
-	phy_remove_link_mode(phy,
-			     ETHTOOL_LINK_MODE_1000baseT_Half_BIT);
-
-	/* PHY does not support gigabit. */
-	if (!(dev->features & GBIT_SUPPORT))
-		phy_remove_link_mode(phy,
-				     ETHTOOL_LINK_MODE_1000baseT_Full_BIT);
-}
-
 static bool ksz9477_get_gbit(struct ksz_device *dev, u8 data)
 {
 	bool gbit;
@@ -1603,7 +1586,6 @@ static const struct ksz_dev_ops ksz9477_dev_ops = {
 	.get_port_addr = ksz9477_get_port_addr,
 	.cfg_port_member = ksz9477_cfg_port_member,
 	.flush_dyn_mac_table = ksz9477_flush_dyn_mac_table,
-	.phy_setup = ksz9477_phy_setup,
 	.port_setup = ksz9477_port_setup,
 	.r_mib_cnt = ksz9477_r_mib_cnt,
 	.r_mib_pkt = ksz9477_r_mib_pkt,
@@ -1617,7 +1599,26 @@ static const struct ksz_dev_ops ksz9477_dev_ops = {
 
 int ksz9477_switch_register(struct ksz_device *dev)
 {
-	return ksz_switch_register(dev, &ksz9477_dev_ops);
+	int ret, i;
+	struct phy_device *phydev;
+
+	ret = ksz_switch_register(dev, &ksz9477_dev_ops);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < dev->phy_port_cnt; ++i) {
+		phydev = dsa_to_port(dev->ds, i)->slave->phydev;
+
+		/* The MAC actually cannot run in 1000 half-duplex mode. */
+		phy_remove_link_mode(phydev,
+				     ETHTOOL_LINK_MODE_1000baseT_Half_BIT);
+
+		/* PHY does not support gigabit. */
+		if (!(dev->features & GBIT_SUPPORT))
+			phy_remove_link_mode(phydev,
+					     ETHTOOL_LINK_MODE_1000baseT_Full_BIT);
+	}
+	return ret;
 }
 EXPORT_SYMBOL(ksz9477_switch_register);
 
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index fd1d6676ae4f..7b6c0dce7536 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -358,8 +358,6 @@ int ksz_enable_port(struct dsa_switch *ds, int port, struct phy_device *phy)
 
 	/* setup slave port */
 	dev->dev_ops->port_setup(dev, port, false);
-	if (dev->dev_ops->phy_setup)
-		dev->dev_ops->phy_setup(dev, port, phy);
 
 	/* port_stp_state_set() will be called after to enable the port so
 	 * there is no need to do anything.
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index f2c9bb68fd33..7d11dd32ec0d 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -119,8 +119,6 @@ struct ksz_dev_ops {
 	u32 (*get_port_addr)(int port, int offset);
 	void (*cfg_port_member)(struct ksz_device *dev, int port, u8 member);
 	void (*flush_dyn_mac_table)(struct ksz_device *dev, int port);
-	void (*phy_setup)(struct ksz_device *dev, int port,
-			  struct phy_device *phy);
 	void (*port_cleanup)(struct ksz_device *dev, int port);
 	void (*port_setup)(struct ksz_device *dev, int port, bool cpu_port);
 	void (*r_phy)(struct ksz_device *dev, u16 phy, u16 reg, u16 *val);
-- 
2.20.1

Re: [PATCH v3] net: dsa: microchip: call phy_remove_link_mode during probe

[Andrew Lunn]

Ignoring the part about how to cleanup this internal phydev for the
moment.

>  int ksz9477_switch_register(struct ksz_device *dev)
>  {
> -	return ksz_switch_register(dev, &ksz9477_dev_ops);
> +	int ret, i;
> +	struct phy_device *phydev;
> +
> +	ret = ksz_switch_register(dev, &ksz9477_dev_ops);
> +	if (ret)
> +		return ret;
> +
> +	for (i = 0; i < dev->phy_port_cnt; ++i) {
> +		phydev = dsa_to_port(dev->ds, i)->slave->phydev;

There is no guarantee this phydev actually exists, as far as i
remember. It will only be allocated for user ports. If a port is not
used, i.e. not listed in DT, it won't have a phydev. So you should add
a test:

		if (!dsa_is_user(ds, i))
			continue;

Otherwise, this now seems correct.

Andrew

[PATCH v4] net: dsa: microchip: call phy_remove_link_mode during probe

[Helmut Grohne]

When doing "ip link set dev ... up" for a ksz9477 backed link,
ksz9477_phy_setup is called and it calls phy_remove_link_mode to remove
1000baseT HDX. During phy_remove_link_mode, phy_advertise_supported is
called. Doing so reverts any previous change to advertised link modes
e.g. using a udevd .link file.

phy_remove_link_mode is not meant to be used while opening a link and
should be called during phy probe when the link is not yet available to
userspace.

Therefore move the phy_remove_link_mode calls into
ksz9477_switch_register. It indirectly calls dsa_register_switch, which
creates the relevant struct phy_devices and we update the link modes
right after that. At that time dev->features is already initialized by
ksz9477_switch_detect.

Remove phy_setup from ksz_dev_ops as no users remain.

Link: https://lore.kernel.org/netdev/20200715192722.GD1256692@lunn.ch/
Fixes: 42fc6a4c613019 ("net: dsa: microchip: prepare PHY for proper advertisement")
Signed-off-by: Helmut Grohne <helmut.grohne@intenta.de>
---
 drivers/net/dsa/microchip/ksz9477.c    | 42 ++++++++++++++------------
 drivers/net/dsa/microchip/ksz_common.c |  2 --
 drivers/net/dsa/microchip/ksz_common.h |  2 --
 3 files changed, 23 insertions(+), 23 deletions(-)

As Andrew Lunn indicated, my patch introduces a null pointer dereference
when PHYs are missing from the device tree. I was able to reproduce the
failure mode and this version fixes it.

Helmut

changes since v3:
 * Check dsa_is_user_port. Thanks to Andrew Lunn.
changes since v2:
 * Operate on the correct phydev. Thanks to Andrew Lunn.
changes since v1:
 * Don't change phy_remove_link_mode. Instead, call it at the right
   time. Thanks to Andrew Lunn for the detailed explanation.

diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index 8d15c3016024..4a9239b2c2e4 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -974,23 +974,6 @@ static void ksz9477_port_mirror_del(struct dsa_switch *ds, int port,
 			     PORT_MIRROR_SNIFFER, false);
 }
 
-static void ksz9477_phy_setup(struct ksz_device *dev, int port,
-			      struct phy_device *phy)
-{
-	/* Only apply to port with PHY. */
-	if (port >= dev->phy_port_cnt)
-		return;
-
-	/* The MAC actually cannot run in 1000 half-duplex mode. */
-	phy_remove_link_mode(phy,
-			     ETHTOOL_LINK_MODE_1000baseT_Half_BIT);
-
-	/* PHY does not support gigabit. */
-	if (!(dev->features & GBIT_SUPPORT))
-		phy_remove_link_mode(phy,
-				     ETHTOOL_LINK_MODE_1000baseT_Full_BIT);
-}
-
 static bool ksz9477_get_gbit(struct ksz_device *dev, u8 data)
 {
 	bool gbit;
@@ -1603,7 +1586,6 @@ static const struct ksz_dev_ops ksz9477_dev_ops = {
 	.get_port_addr = ksz9477_get_port_addr,
 	.cfg_port_member = ksz9477_cfg_port_member,
 	.flush_dyn_mac_table = ksz9477_flush_dyn_mac_table,
-	.phy_setup = ksz9477_phy_setup,
 	.port_setup = ksz9477_port_setup,
 	.r_mib_cnt = ksz9477_r_mib_cnt,
 	.r_mib_pkt = ksz9477_r_mib_pkt,
@@ -1617,7 +1599,29 @@ static const struct ksz_dev_ops ksz9477_dev_ops = {
 
 int ksz9477_switch_register(struct ksz_device *dev)
 {
-	return ksz_switch_register(dev, &ksz9477_dev_ops);
+	int ret, i;
+	struct phy_device *phydev;
+
+	ret = ksz_switch_register(dev, &ksz9477_dev_ops);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < dev->phy_port_cnt; ++i) {
+		if (!dsa_is_user_port(dev->ds, i))
+			continue;
+
+		phydev = dsa_to_port(dev->ds, i)->slave->phydev;
+
+		/* The MAC actually cannot run in 1000 half-duplex mode. */
+		phy_remove_link_mode(phydev,
+				     ETHTOOL_LINK_MODE_1000baseT_Half_BIT);
+
+		/* PHY does not support gigabit. */
+		if (!(dev->features & GBIT_SUPPORT))
+			phy_remove_link_mode(phydev,
+					     ETHTOOL_LINK_MODE_1000baseT_Full_BIT);
+	}
+	return ret;
 }
 EXPORT_SYMBOL(ksz9477_switch_register);
 
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index fd1d6676ae4f..7b6c0dce7536 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -358,8 +358,6 @@ int ksz_enable_port(struct dsa_switch *ds, int port, struct phy_device *phy)
 
 	/* setup slave port */
 	dev->dev_ops->port_setup(dev, port, false);
-	if (dev->dev_ops->phy_setup)
-		dev->dev_ops->phy_setup(dev, port, phy);
 
 	/* port_stp_state_set() will be called after to enable the port so
 	 * there is no need to do anything.
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index f2c9bb68fd33..7d11dd32ec0d 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -119,8 +119,6 @@ struct ksz_dev_ops {
 	u32 (*get_port_addr)(int port, int offset);
 	void (*cfg_port_member)(struct ksz_device *dev, int port, u8 member);
 	void (*flush_dyn_mac_table)(struct ksz_device *dev, int port);
-	void (*phy_setup)(struct ksz_device *dev, int port,
-			  struct phy_device *phy);
 	void (*port_cleanup)(struct ksz_device *dev, int port);
 	void (*port_setup)(struct ksz_device *dev, int port, bool cpu_port);
 	void (*r_phy)(struct ksz_device *dev, u16 phy, u16 reg, u16 *val);
-- 
2.20.1

Re: [PATCH v4] net: dsa: microchip: call phy_remove_link_mode during probe

[Andrew Lunn]

On Tue, Jul 21, 2020 at 01:07:39PM +0200, Helmut Grohne wrote:
> When doing "ip link set dev ... up" for a ksz9477 backed link,
> ksz9477_phy_setup is called and it calls phy_remove_link_mode to remove
> 1000baseT HDX. During phy_remove_link_mode, phy_advertise_supported is
> called. Doing so reverts any previous change to advertised link modes
> e.g. using a udevd .link file.
> 
> phy_remove_link_mode is not meant to be used while opening a link and
> should be called during phy probe when the link is not yet available to
> userspace.
> 
> Therefore move the phy_remove_link_mode calls into
> ksz9477_switch_register. It indirectly calls dsa_register_switch, which
> creates the relevant struct phy_devices and we update the link modes
> right after that. At that time dev->features is already initialized by
> ksz9477_switch_detect.
> 
> Remove phy_setup from ksz_dev_ops as no users remain.
> 
> Link: https://lore.kernel.org/netdev/20200715192722.GD1256692@lunn.ch/
> Fixes: 42fc6a4c613019 ("net: dsa: microchip: prepare PHY for proper advertisement")
> Signed-off-by: Helmut Grohne <helmut.grohne@intenta.de>

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

    Andrew

Re: [PATCH v4] net: dsa: microchip: call phy_remove_link_mode during probe

[David Miller]

From: Helmut Grohne <helmut.grohne@intenta.de>
Date: Tue, 21 Jul 2020 13:07:39 +0200

> When doing "ip link set dev ... up" for a ksz9477 backed link,
> ksz9477_phy_setup is called and it calls phy_remove_link_mode to remove
> 1000baseT HDX. During phy_remove_link_mode, phy_advertise_supported is
> called. Doing so reverts any previous change to advertised link modes
> e.g. using a udevd .link file.
> 
> phy_remove_link_mode is not meant to be used while opening a link and
> should be called during phy probe when the link is not yet available to
> userspace.
> 
> Therefore move the phy_remove_link_mode calls into
> ksz9477_switch_register. It indirectly calls dsa_register_switch, which
> creates the relevant struct phy_devices and we update the link modes
> right after that. At that time dev->features is already initialized by
> ksz9477_switch_detect.
> 
> Remove phy_setup from ksz_dev_ops as no users remain.
> 
> Link: https://lore.kernel.org/netdev/20200715192722.GD1256692@lunn.ch/
> Fixes: 42fc6a4c613019 ("net: dsa: microchip: prepare PHY for proper advertisement")
> Signed-off-by: Helmut Grohne <helmut.grohne@intenta.de>

Applied and queued up for -stable, thanks.

Re: [PATCH v3] net: dsa: microchip: call phy_remove_link_mode during probe

[Andrew Lunn]

> The dev->ports[i].phydev is not actually exposed beyond the driver. The
> driver sets the phydev.speed in a few places and even reads it back in
> one place. It also sets phydev.duplex, but never reads it back. It
> queries phydev.link, which is statically 0 due to using devm_kzalloc.
> 
> I think the use of this ksz_port.phydev is very misleading, but I'm
> unsure how to fix this. It is not clear to me whether all those updates
> should be performed on the connected phydev instead or whether this is
> just internal state tracking.

I took a quick look at the code.

For PHY addresses < dev->phy_port_cnt it passes all reads/writes
through to the hardware. So the Linux MDIO/PHY subsystem will be able
to fully drive these PHYs, and the ksz9477 internal phydev is
unneeded.

Where it gets interesting is addr >= dev->phy_port_cnt. Reads of the
PHY registers return hard coded values, or the link speed from the
local phydev. Writes to these registers are just ignored.

If you compare this to other DSA drivers/DSA switches, reads/write for
addresses where there are no internal PHY get passed out to an
external MDIO bus, where an external PHY can be connected. The Linux
MDIO/PHY subsystem will discover these external PHYs and create phydev
instance for them. If there is no external PHY, for example the MAC is
connected to another MAC, no PHY will be detected, and fixed-link is
used in its place.

Do these switches have an external MDIO bus?
How are external PHYs usually managed?

At a minimum, the internal phydev can be replaced with just a speed,
rather than a full phydev, which will reduce confusion. But it would
be nice to go further and remove all the addr >= dev->phy_port_cnt
handling. But we need to understand the implications of that.

	Andrew

Re: [PATCH v3] net: dsa: microchip: call phy_remove_link_mode during probe

[Helmut Grohne]

Hi Andrew,

Your persistence on this matter is much appreciated.

On Mon, Jul 20, 2020 at 11:04:49PM +0200, Andrew Lunn wrote:
> > The dev->ports[i].phydev is not actually exposed beyond the driver. The
> > driver sets the phydev.speed in a few places and even reads it back in
> > one place. It also sets phydev.duplex, but never reads it back. It
> > queries phydev.link, which is statically 0 due to using devm_kzalloc.
> > 
> > I think the use of this ksz_port.phydev is very misleading, but I'm
> > unsure how to fix this. It is not clear to me whether all those updates
> > should be performed on the connected phydev instead or whether this is
> > just internal state tracking.
> 
> I took a quick look at the code.
> 
> For PHY addresses < dev->phy_port_cnt it passes all reads/writes
> through to the hardware. So the Linux MDIO/PHY subsystem will be able
> to fully drive these PHYs, and the ksz9477 internal phydev is
> unneeded.

I do not fully concur here yet. For instance, ksz8795_port_setup and
ksz9477_port_setup branch on the port being a CPU port and evaluate the
phydev.link for non-CPU ports. Given that phydev.link is never assigned,
the branch where dev->live_ports is assigned is dead. Following
live_ports through the code reveals that it is only ever written to, but
no logic ever depends on its value. I'm not yet sure whether all of that
should simply be removed with no replacement or whether it was meant to
be extended some time later.

> Where it gets interesting is addr >= dev->phy_port_cnt. Reads of the
> PHY registers return hard coded values, or the link speed from the
> local phydev. Writes to these registers are just ignored.

This makes somewhat sense to me. It may become clearer below.

> If you compare this to other DSA drivers/DSA switches, reads/write for
> addresses where there are no internal PHY get passed out to an
> external MDIO bus, where an external PHY can be connected. The Linux
> MDIO/PHY subsystem will discover these external PHYs and create phydev
> instance for them. If there is no external PHY, for example the MAC is
> connected to another MAC, no PHY will be detected, and fixed-link is
> used in its place.

These switches all have internal PHYs for addresses < phy_port_cnt.
Beyond this index, the MACs are located. Few devices have multiple MACs
and only one MAC can be connected to the CPU at a time, because the tail
tagging scheme can only be enabled on one MAC port at a time. The driver
requires tail tagging on CPU ports (although this is not required by the
hardware).

> Do these switches have an external MDIO bus?

One has a choice of how one wishes to communicate with these switches.
Depending on configuration straps, they can do SPI or I²C or MDIO,
though the register space on the MDIO bus is too limited to do anything
useful, so the driver does not support MDIO. You can reach all of the
internal PHYs through the chosen bus. If you connect an external PHY to
a MAC, the KSZ is not involved in a management connection such as MDIO.

> How are external PHYs usually managed?

I honestly don't know. I only deal with internal PHYs. The typical use
case for the MAC ports is to establish fixed-links to other MACs (such
as the CPU or other switches).

> At a minimum, the internal phydev can be replaced with just a speed,
> rather than a full phydev, which will reduce confusion. But it would
> be nice to go further and remove all the addr >= dev->phy_port_cnt
> handling. But we need to understand the implications of that.

addr >= dev->phy_port_cnt identifies a MAC. While the KSZ may have a
data connection to the other side, but it does not have a management
connection (e.g. MDIO). The driver presently assumes that all MAC
connections are fixed-links, which is the case when you connect it to
the CPU. A significant fraction of KSZ switches only have one MAC or
have multiple MACs of which you only use one in a particular product
(e.g. because one only support SGMII and othe other only supports
RGMII). So the common case here is that addr >= dev->phy_port_cnt
uniquely identifies the fixed-link CPU port.

This also means that very likely the addr >= dev->phy_port_cnt handling
is not going away.

It also kinda routes us back to another thread of mine. In the followup
to https://lore.kernel.org/netdev/20200714120827.GA7939@laureti-dev/,
you also identified the assumption that any MAC port is the CPU port of
this driver and asked me to build on it. It is unclear whether that
should be lifted. If it isn't, I think it is fairly safe to assume that
any MAC is connected using a fixed-link and that there is no need for
any external PHY management.

Helmut