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

Arınç ÜNAL
2024-01-10T17:16:09Z

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ç