| Message ID | 20200716125723.GA19500@laureti-dev |
|---|---|
| State | Deferred |
| Delegated to: | David Miller |
| Headers | show |
| Series | [v2] net: dsa: microchip: call phy_remove_link_mode during probe | expand |
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
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------
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
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);
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