linux-kernel - Re: [RFC PATCH 2/3] net: sparx5: Add Sparx5 switchdev driver

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: <20201128222828.GQ1551@shell.armlinux.org.uk>
Date:   Sat, 28 Nov 2020 22:28:28 +0000
From:   Russell King - ARM Linux admin <linux@...linux.org.uk>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     Steen Hegelund <steen.hegelund@...rochip.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Masahiro Yamada <masahiroy@...nel.org>,
        Lars Povlsen <lars.povlsen@...rochip.com>,
        Bjarni Jonasson <bjarni.jonasson@...rochip.com>,
        Microchip Linux Driver Support <UNGLinuxDriver@...rochip.com>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Microsemi List <microsemi@...ts.bootlin.com>,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 2/3] net: sparx5: Add Sparx5 switchdev driver

On Sat, Nov 28, 2020 at 08:06:16PM +0100, Andrew Lunn wrote:
> > +static void sparx5_phylink_mac_config(struct phylink_config *config,
> > +				      unsigned int mode,
> > +				      const struct phylink_link_state *state)
> > +{
> > +	struct sparx5_port *port = netdev_priv(to_net_dev(config->dev));
> > +	struct sparx5_port_config conf;
> > +	int err = 0;
> > +
> > +	conf = port->conf;
> > +	conf.autoneg = state->an_enabled;
> > +	conf.pause = state->pause;
> > +	conf.duplex = state->duplex;
> > +	conf.power_down = false;
> > +	conf.portmode = state->interface;
> > +
> > +	if (state->speed == SPEED_UNKNOWN) {
> > +		/* When a SFP is plugged in we use capabilities to
> > +		 * default to the highest supported speed
> > +		 */
> 
> This looks suspicious.

Yes, it looks highly suspicious. The fact that
sparx5_phylink_mac_link_up() is empty, and sparx5_phylink_mac_config()
does all the work suggests that this was developed before the phylink
re-organisation, and this code hasn't been updated for it.

Any new code for the kernel really ought to be updated for the new
phylink methodology before it is accepted.

Looking at sparx5_port_config(), it also seems to use
PHY_INTERFACE_MODE_1000BASEX for both 1000BASE-X and 2500BASE-X. All
very well for the driver to do that internally, but it's confusing
when it comes to reviewing this stuff, especially when people outside
of the driver (such as myself) reviewing it need to understand what's
going on with the configuration.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!