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

[Andrew Lunn <andrew@lunn.ch>]

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