netdev - Re: [PATCH net] net: dsa: fix phylink_start()/phylink

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: <20200229154215.GD6305@lunn.ch>
Date:   Sat, 29 Feb 2020 16:42:15 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Russell King <rmk+kernel@...linux.org.uk>
Cc:     Florian Fainelli <f.fainelli@...il.com>,
        Vivien Didelot <vivien.didelot@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Ioana Ciornei <ioana.ciornei@....com>,
        Vladimir Oltean <olteanv@...il.com>, netdev@...r.kernel.org
Subject: Re: [PATCH net] net: dsa: fix phylink_start()/phylink_stop() calls

> -int dsa_port_enable(struct dsa_port *dp, struct phy_device *phy)
> +int dsa_port_enable_locked(struct dsa_port *dp, struct phy_device *phy)
>  {
>  	struct dsa_switch *ds = dp->ds;
>  	int port = dp->index;
>  	int err;
>  
> +	if (dp->pl)
> +		phylink_start(dp->pl);
> +
>  	if (ds->ops->port_enable) {
>  		err = ds->ops->port_enable(ds, port, phy);
>  		if (err)
> @@ -81,7 +84,18 @@ int dsa_port_enable(struct dsa_port *dp, struct phy_device *phy)
>  	return 0;
>  }

Hi Russell

I'm wondering about the order here. You are starting phylink before
the port is actually enabled in the hardware. Could phylink_start()
result in synchronous calls into the MAC to configure the port? If the
port is disabled, maybe that configuration will not stick?

The current code in dsa_slave_open() first enables the port, then
calls phylink_start(). So maybe we should keep the ordering the same?
  
> +void dsa_port_disable_locked(struct dsa_port *dp)
>  {
>  	struct dsa_switch *ds = dp->ds;
>  	int port = dp->index;
> @@ -91,6 +105,16 @@ void dsa_port_disable(struct dsa_port *dp)
>  
>  	if (ds->ops->port_disable)
>  		ds->ops->port_disable(ds, port);
> +
> +	if (dp->pl)
> +		phylink_stop(dp->pl);
> +}

The current code first stops phylink, then disables the port...

Apart from this ordering issue, the code looks good.

Thanks
    Andrew