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

Powered by blists - more mailing lists