netdev - Re: [PATCH v2] net: dsa: microchip: call phy_remove_link

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200717131814.GA1336433@lunn.ch>
Date:   Fri, 17 Jul 2020 15:18:14 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Helmut Grohne <helmut.grohne@...enta.de>
Cc:     Florian Fainelli <f.fainelli@...il.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Russell King <linux@...linux.org.uk>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Woojung Huh <woojung.huh@...rochip.com>,
        Microchip Linux Driver Support <UNGLinuxDriver@...rochip.com>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Tristram Ha <Tristram.Ha@...rochip.com>
Subject: Re: [PATCH v2] net: dsa: microchip: call phy_remove_link_mode during
 probe

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