netdev - Re: [PATCH net-next 08/30] net: dsa: mt7530: change p{5,6}_interface to p{5,6}

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: <b47311f8-315d-46d9-bd5b-757141708a3f@arinc9.com>
Date: Wed, 10 Jan 2024 20:15:20 +0300
From: Arınç ÜNAL <arinc.unal@...nc9.com>
To: Vladimir Oltean <olteanv@...il.com>
Cc: "Russell King (Oracle)" <linux@...linux.org.uk>,
 Sean Wang <sean.wang@...iatek.com>, Landen Chao <Landen.Chao@...iatek.com>,
 DENG Qingfang <dqfext@...il.com>, Daniel Golle <daniel@...rotopia.org>,
 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>,
 Richard van Schagen <richard@...terhints.com>,
 Richard van Schagen <vschagen@...com>,
 Frank Wunderlich <frank-w@...lic-files.de>,
 Bartel Eerdekens <bartel.eerdekens@...stell8.be>, erkin.bozoglu@...ont.com,
 mithat.guner@...ont.com, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-mediatek@...ts.infradead.org
Subject: Re: [PATCH net-next 08/30] net: dsa: mt7530: change p{5,6}_interface
 to p{5,6}_configured

On 10.01.2024 17:27, Vladimir Oltean wrote:
> On Wed, Jan 10, 2024 at 02:15:57PM +0300, Arınç ÜNAL wrote:
>> I have finally tested this.
> 
> Replying to a question from 6 months ago is nice of you, like replying
> to any question is. But everybody's short memory is by now hit like a
> cold cache, everything has been forgotten. I don't even have this thread
> in my inbox anymore, it's in the "seen" folder.
> 
> There's something to be said about having to re-read a long thread and
> the code for 30 minutes, just to reply "Ok".
> 
> I think you need to develop a better feeling for when to let go of past
> discussions when they become stale, summarize the essence in the commit
> description of a patch, and then just resubmit that new patch. People
> will have to open the code and make a fresh analysis anyway, so just
> help them skip reading past discussions and just focus on the conclusion.

I agree, thanks for bearing with me here.

> 
> FYI, you can prefix your prints with something like this to make the log
> easier to follow in terms of code paths taken.
> 
> 	"%s called from %pS <- %pS: ...\n",
> 		__func__, __builtin_return_address(0), __builtin_return_address(1)

__builtin_return_address(1) doesn't seem to work. I'm running this on
arm64. Apart from that, it works well. Thank you.

[    1.863034] mt7530-mdio mdio-bus:00: mt753x_cpu_port_enable called from mt7531_setup_common+0x32c/0x370 <- 0x0: running cpu_port_config()
[    1.875736] mt7530-mdio mdio-bus:00: mt7531_cpu_port_config called from mt753x_cpu_port_enable+0x64/0x23c <- 0x0: MT7530_PMCR_P5 PMCR_CPU_PORT_SETTING is set
[    1.889922] mt7530-mdio mdio-bus:00: mt753x_phylink_mac_link_up called from mt7531_cpu_port_config+0xe8/0x12c <- 0x0: MT7530_PMCR_P5 PMCR_LINK_SETTINGS_MASK equivalent is set
[    1.905491] mt7530-mdio mdio-bus:00: cpu_port_config() ran
[    1.912336] mt7530-mdio mdio-bus:00: mt753x_cpu_port_enable called from mt7531_setup_common+0x32c/0x370 <- 0x0: running cpu_port_config()
[    1.924777] mt7530-mdio mdio-bus:00: mt7531_cpu_port_config called from mt753x_cpu_port_enable+0x64/0x23c <- 0x0: MT7530_PMCR_P6 PMCR_CPU_PORT_SETTING is set
[    1.938953] mt7530-mdio mdio-bus:00: mt753x_phylink_mac_link_up called from mt7531_cpu_port_config+0xe8/0x12c <- 0x0: MT7530_PMCR_P6 PMCR_LINK_SETTINGS_MASK equivalent is set
[    1.954525] mt7530-mdio mdio-bus:00: cpu_port_config() ran
[    1.985409] mt7530-mdio mdio-bus:00: mt7530_port_enable called from dsa_port_enable_rt+0x2c/0x98 <- 0x0: MT7530_PMCR_P5 PMCR_LINK_SETTINGS_MASK is cleared
[    1.999378] mt7530-mdio mdio-bus:00: configuring for fixed/rgmii link mode
[    2.006347] mt7530-mdio mdio-bus:00: mt753x_phylink_mac_config called from dsa_port_phylink_mac_config+0x30/0x3c <- 0x0: MT7530_PMCR_P5 PMCR_CPU_PORT_SETTING equivalent is set
[    2.022197] mt7530-mdio mdio-bus:00: mt753x_phylink_mac_link_up called from dsa_port_phylink_mac_link_up+0x48/0x74 <- 0x0: MT7530_PMCR_P5 PMCR_LINK_SETTINGS_MASK equivalent is set
[    2.022645] mt7530-mdio mdio-bus:00: mt7530_port_enable called from dsa_port_enable_rt+0x2c/0x98 <- 0x0: MT7530_PMCR_P6 PMCR_LINK_SETTINGS_MASK is cleared
[    2.038203] mt7530-mdio mdio-bus:00: Link is Up - 1Gbps/Full - flow control rx/tx
[    2.052090] mt7530-mdio mdio-bus:00: configuring for fixed/2500base-x link mode
[    2.066894] mt7530-mdio mdio-bus:00: mt753x_phylink_mac_config called from dsa_port_phylink_mac_config+0x30/0x3c <- 0x0: MT7530_PMCR_P6 PMCR_CPU_PORT_SETTING equivalent is set
[    2.084406] mt7530-mdio mdio-bus:00: mt753x_phylink_mac_link_up called from dsa_port_phylink_mac_link_up+0x48/0x74 <- 0x0: MT7530_PMCR_P6 PMCR_LINK_SETTINGS_MASK equivalent is set
[    2.095093] mt7530-mdio mdio-bus:00 wan (uninitialized): PHY [mt7530-0:00] driver [MediaTek MT7531 PHY] (irq=137)
[    2.100427] mt7530-mdio mdio-bus:00: Link is Up - 2.5Gbps/Full - flow control rx/tx

> 
>> [    1.763066] mt7530-mdio mdio-bus:00: running cpu_port_config()
>> [    1.769237] mt7530-mdio mdio-bus:00: MT7530_PMCR_P5 PMCR_CPU_PORT_SETTING is set
>> [    1.776724] mt7530-mdio mdio-bus:00: MT7530_PMCR_P5 PMCR_LINK_SETTINGS_MASK equivalent is set
>> [    1.785254] mt7530-mdio mdio-bus:00: cpu_port_config() ran
> 
> This is from mt7531_setup_common(), for port 5.
> 
>> [    1.792098] mt7530-mdio mdio-bus:00: running cpu_port_config()
>> [    1.798019] mt7530-mdio mdio-bus:00: MT7530_PMCR_P6 PMCR_CPU_PORT_SETTING is set
>> [    1.805502] mt7530-mdio mdio-bus:00: MT7530_PMCR_P6 PMCR_LINK_SETTINGS_MASK equivalent is set
>> [    1.814023] mt7530-mdio mdio-bus:00: cpu_port_config() ran
> 
> This is from mt7531_setup_common(), for port 6.
> 
>> [    1.844941] mt7530-mdio mdio-bus:00: MT7530_PMCR_P5 PMCR_LINK_SETTINGS_MASK is cleared
> 
> This is from mt7530_port_enable() for port 5, undoing what mt7531_setup_common() has done.
> It also seems bogus BTW, the enable() function is doing the same "clear"
> as mt7530_port_disable() is doing, rather than mt7530_set(). Were it not
> for what's to come [1], this would be a bug with an actual user impact.
> 
>> [    1.852972] mt7530-mdio mdio-bus:00: configuring for fixed/rgmii link mode
>> [    1.859944] mt7530-mdio mdio-bus:00: MT7530_PMCR_P5 PMCR_CPU_PORT_SETTING equivalent is set
> 
> This is from mt753x_phylink_mac_config(), for port 5, partially
> overwriting what mt7531_setup_common() has done.
> 
>> [    1.868658] mt7530-mdio mdio-bus:00: MT7530_PMCR_P5 PMCR_LINK_SETTINGS_MASK equivalent is set
> 
> [1] This is from mt753x_phylink_mac_link_up(), for port 5, overwriting what
> mt7530_port_enable() has done. I suspect that, in addition to Russell's
> analysis, modifying PMCR_LINK_SETTINGS_MASK from the port_enable()/
> port_disable() ops is also something that can be removed.
> 
>> [    1.868913] mt7530-mdio mdio-bus:00: MT7530_PMCR_P6 PMCR_LINK_SETTINGS_MASK is cleared
>> [    1.877190] mt7530-mdio mdio-bus:00: Link is Up - 1Gbps/Full - flow control rx/tx
>> [    1.885179] mt7530-mdio mdio-bus:00: configuring for fixed/2500base-x link mode
>> [    1.899973] mt7530-mdio mdio-bus:00: MT7530_PMCR_P6 PMCR_CPU_PORT_SETTING equivalent is set
>> [    1.910147] mt7530-mdio mdio-bus:00: MT7530_PMCR_P6 PMCR_LINK_SETTINGS_MASK equivalent is set
>> [    1.918681] mt7530-mdio mdio-bus:00: Link is Up - 2.5Gbps/Full - flow control rx/tx
>> [    1.920654] mt7530-mdio mdio-bus:00 wan (uninitialized): PHY [mt7530-0:00] driver [MediaTek MT7531 PHY] (irq=137)
>> [    1.948453] mt7530-mdio mdio-bus:00 lan0 (uninitialized): PHY [mt7530-0:01] driver [MediaTek MT7531 PHY] (irq=138)
>> [    1.970382] mt7530-mdio mdio-bus:00 lan1 (uninitialized): PHY [mt7530-0:02] driver [MediaTek MT7531 PHY] (irq=139)
>> [    1.992423] mt7530-mdio mdio-bus:00 lan2 (uninitialized): PHY [mt7530-0:03] driver [MediaTek MT7531 PHY] (irq=140)
>> [    2.014310] mt7530-mdio mdio-bus:00 lan3 (uninitialized): PHY [mt7530-0:04] driver [MediaTek MT7531 PHY] (irq=141)
>> [    2.025396] mtk_soc_eth 1b100000.ethernet eth1: entered promiscuous mode
>> [    2.032160] mtk_soc_eth 1b100000.ethernet eth0: entered promiscuous mode
>> [    2.038912] DSA: tree 0 setup
>>
>> Arınç
> 
> And this is all the same for port 6.
> 
> So yes, it would be good to consolidate the code to follow a simple principle.
> Any register fields should be modified only by the set of methods that
> they pertain to. In this case, MT7530_PMCR_P appears to only hold link
> control information, so it pertains to phylink's methods. Otherwise,
> the natural consequence is that they will get unexpectedly overwritten.
> 
> It seems outside of the competence of ds->ops->port_enable() and
> ds->ops->port_disable(). Those would be appropriate, for example, to
> control the switching matrix settings between a user port and its
> corresponding CPU port (but not any more switching matrix settings -
> those pertain to port_bridge_join() and port_bridge_leave()).
> 
> I hope this helps.

This is very helpful, thank you very much. This is what I deduct I should
do:

First patch: Get rid of cpu_port_config().

Second patch: Collect port link control register operations from
port_enable/port_disable and phylink_mac_config to
phylink_mac_link_up/phylink_mac_link_down.

Arınç