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------
