netdev - Re: [PATCH net-next 06/15] net: dsa: mt7530: do not set priv->p5_interface on mt7530_setup

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]

Date: Thu, 7 Dec 2023 20:18:58 +0200
From: Vladimir Oltean <olteanv@...il.com>
To: Arınç ÜNAL <arinc.unal@...nc9.com>
Cc: Daniel Golle <daniel@...rotopia.org>,
	Landen Chao <Landen.Chao@...iatek.com>,
	DENG Qingfang <dqfext@...il.com>,
	Sean Wang <sean.wang@...iatek.com>, Andrew Lunn <andrew@...n.ch>,
	Florian Fainelli <f.fainelli@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Matthias Brugger <matthias.bgg@...il.com>,
	AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
	Russell King <linux@...linux.org.uk>, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	linux-mediatek@...ts.infradead.org,
	Frank Wunderlich <frank-w@...lic-files.de>,
	Bartel Eerdekens <bartel.eerdekens@...stell8.be>,
	mithat.guner@...ont.com, erkin.bozoglu@...ont.com
Subject: Re: [PATCH net-next 06/15] net: dsa: mt7530: do not set
 priv->p5_interface on mt7530_setup_port5()

On Sat, Nov 18, 2023 at 03:31:56PM +0300, Arınç ÜNAL wrote:
> Do not set priv->p5_interface on mt7530_setup_port5(). There isn't a case
> where mt753x_phylink_mac_config() runs after mt7530_setup_port5() which
> setting priv->p5_interface would prevent mt7530_setup_port5() from running
> more than once.
> 
> Signed-off-by: Arınç ÜNAL <arinc.unal@...nc9.com>
> ---
>  drivers/net/dsa/mt7530.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 069b3dfca6fa..fc87ec817672 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -978,8 +978,6 @@ static void mt7530_setup_port5(struct dsa_switch *ds, phy_interface_t interface)
>  	dev_dbg(ds->dev, "Setup P5, HWTRAP=0x%x, intf_sel=%s, phy-mode=%s\n",
>  		val, p5_intf_modes(priv->p5_intf_sel), phy_modes(interface));
>  
> -	priv->p5_interface = interface;
> -
>  unlock_exit:
>  	mutex_unlock(&priv->reg_mutex);
>  }
> -- 
> 2.40.1
> 

Purely as a matter of theory, mt753x_phylink_mac_config() itself could
be called more than once, at any time there is a phylink_major_config() -
first during initial one, then upon any change of the phy_interface_t.
With the port being fixed and internal, maybe that is unlikely.

Destroying and re-creating the phylink instance could also make the
driver see more than 1 mt753x_phylink_mac_config() call. During periods
of -EPROBE_DEFER, maybe this could even happen.

I think what we need to see is a description of what the priv->p5_interface
test is really protecting against, because it certainly is uncommon in other
drivers to have this much logic to avoid mt753x_mac_config() being
called twice.

If there's nothing wrong with calling it twice and it's just an optimization,
I think that's enough of a justification for removing that extra protection.
At the same time, the optimization (and simplification) that we should
pursue is that we should remove the overlap between phylink and the
initial port setup made by the driver.