| 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
| ||
|
Message-ID: <e2865f69-3254-4768-7a7a-bb84d76a85eb@arinc9.com>
Date: Mon, 5 Jun 2023 17:36:56 +0300
From: Arınç ÜNAL <arinc.unal@...nc9.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
Cc: Vladimir Oltean <olteanv@...il.com>,
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 4.06.2023 18:13, Russell King (Oracle) wrote:
> On Sun, Jun 04, 2023 at 04:14:31PM +0300, Arınç ÜNAL wrote:
>> On 4.06.2023 16:07, Russell King (Oracle) wrote:
>>> On Sun, Jun 04, 2023 at 03:55:17PM +0300, Vladimir Oltean wrote:
>>>> On Sun, Jun 04, 2023 at 01:18:04PM +0100, Russell King (Oracle) wrote:
>>>>> I don't remember whether Vladimir's firmware validator will fail for
>>>>> mt753x if CPU ports are not fully described, but that would be well
>>>>> worth checking. If it does, then we can be confident that phylink
>>>>> will always be used, and those bypassing calls should not be necessary.
>>>>
>>>> It does, I've just retested this:
>>>>
>>>> [ 8.469152] mscc_felix 0000:00:00.5: OF node /soc/pcie@...000000/ethernet-switch@0,5/ports/port@4 of CPU port 4 lacks the required "phy-handle", "fixed-link" or "managed" properties
>>>> [ 8.494571] mscc_felix 0000:00:00.5: error -EINVAL: Failed to register DSA switch
>>>> [ 8.502151] mscc_felix: probe of 0000:00:00.5 failed with error -22
>>>
>>> ... which isn't listed in dsa_switches_apply_workarounds[], and
>>> neither is mt753x. Thanks.
>>>
>>> So, that should be sufficient to know that the CPU port will always
>>> properly described, and thus bypassing phylink in mt753x for the CPU
>>> port should not be necessary.
>>
>> Perfect! If I understand correctly, there's this code - specific to MT7531
>> and MT7988 ports being used as CPU ports - which runs in addition to what's
>> in mt753x_phylink_mac_config():
>>
>> mt7530_write(priv, MT7530_PMCR_P(port),
>> PMCR_CPU_PORT_SETTING(priv->id));
>>
>> This should be put on mt753x_phylink_mac_config(), under priv->id ==
>> ID_MT7531, priv->id == ID_MT7988, and dsa_is_cpu_port(ds, port) checks?
>
> Please remember that I have very little knowledge of MT753x, so in
> order to answer this question, I've read through the mt7530 driver
> code.
>
> Looking at mt7530.h:
>
> #define PMCR_CPU_PORT_SETTING(id) (PMCR_FORCE_MODE_ID((id)) | \
> PMCR_IFG_XMIT(1) | PMCR_MAC_MODE | \
> PMCR_BACKOFF_EN | PMCR_BACKPR_EN | \
> PMCR_TX_EN | PMCR_RX_EN | \
> PMCR_TX_FC_EN | PMCR_RX_FC_EN | \
> PMCR_FORCE_SPEED_1000 | \
> PMCR_FORCE_FDX | PMCR_FORCE_LNK)
>
> This seems to be some kind of port control register that sets amongst
> other things parameters such as whether flow control is enabled, the
> port speed, the duplex setting, whether link is forced up, etc.
>
> Looking at what mt753x_phylink_mac_link_up() does:
>
> 1. it sets PMCR_RX_EN | PMCR_TX_EN | PMCR_FORCE_LNK.
> 2. it sets PMCR_FORCE_SPEED_1000 if speed was 1000Mbps, or if using
> an internal, TRGMII, 1000base-X or 2500base-X phy interface mode.
> 3. it sets PMCR_FORCE_FDX if full duplex was requested.
> 4. it sets PMCR_TX_FC_EN if full duplex was requested with tx pause.
> 5. it sets PMCR_RX_FC_EN if full duplex was requested with rx pause.
>
> So, provided this is called with the appropriate parameters, for a
> fixed link, that will leave the following:
>
> PMCR_FORCE_MODE_ID(id)
> PMCR_IFG_XMIT(1)
> PMCR_MAC_MODE
> PMCR_BACKOFF_EN
> PMCR_BACKPR_EN
>
> If we now look at mt753x_phylink_mac_config(), this sets
> PMCR_IFG_XMIT(1), PMCR_MAC_MODE, PMCR_BACKOFF_EN, PMCR_BACKPR_EN,
> and PMCR_FORCE_MODE_ID(priv->id), which I believe is everything that
> PMCR_CPU_PORT_SETTING(priv->id) is doing.
>
> So, Wouldn't a fixed-link description indicating 1Gbps, full duplex
> with pause cause phylink to call both mt753x_phylink_mac_config() and
> mt753x_phylink_mac_link_up() with appropriate arguments to set all
> of these parameters in PMCR?
>
> Now, I'm going to analyse something else. mt7531_cpu_port_config()
> is called from mt753x_cpu_port_enable(), which is itself called from
> mt7531_setup_common(). That is ultimately called from the DSA switch
> ops .setup() method.
>
> This method is called from dsa_switch_setup() for each switch in the
> DSA tree. dsa_tree_setup_switches() calls this, and is called from
> dsa_tree_setup(). Once dsa_tree_setup_switches() finishes
> successfully, dsa_tree_setup_ports() will be called. This will then
> setup DSA and CPU ports, which will then setup a phylink instance
> for these ports. phylink will parse the firmware description for
> the port. DSA will then call dsa_port_enable().
>
> dsa_port_enable() will then call any port_enable() method in the
> mt7530.c driver, which will be mt7530_port_enable(). This then...
>
> mt7530_clear(priv, MT7530_PMCR_P(port), PMCR_LINK_SETTINGS_MASK);
>
> which is:
>
> #define PMCR_LINK_SETTINGS_MASK (PMCR_TX_EN | PMCR_FORCE_SPEED_1000 | \
> PMCR_RX_EN | PMCR_FORCE_SPEED_100 | \
> PMCR_TX_FC_EN | PMCR_RX_FC_EN | \
> PMCR_FORCE_FDX | PMCR_FORCE_LNK | \
> PMCR_FORCE_EEE1G | PMCR_FORCE_EEE100)
>
> So it wipes out all the PMCR settings that mt7531_cpu_port_config()
> performed - undoing *everything* below that switch() statement in
> mt7531_cpu_port_config()!
>
> Once the port_enable() method returns, DSA will then call
> phylink_start(), which will trigger phylink to bring up the link
> according to the settings it has, which will mean phylink calls
> the mac_config(), pcs_config(), pcs_link_up() and mac_link_up()
> with the appropriate parameters for the firmware described link.
I'm slowly learning how DSA and phylink works, this is the full code
path I could make up for the MT7530 DSA subdriver:
mt7530_probe() & mt7988_probe()
-> mt7530_probe_common()
-> dsa_register_switch()
-> dsa_switch_probe()
-> dsa_tree_setup()
-> dsa_tree_setup_switches()
-> dsa_switch_setup()
-> ds->ops->setup(): mt753x_setup()
-> mt7530_setup()
-> mt753x_cpu_port_enable()
-> mt7531_setup()
-> mt7531_setup_common()
-> mt753x_cpu_port_enable()
-> priv->info->cpu_port_config():
mt7531_cpu_port_config()
-> mt7988_setup()
-> mt7531_setup_common()
-> mt753x_cpu_port_enable()
-> priv->info->cpu_port_config():
mt7988_cpu_port_config()
-> dsa_tree_setup_ports()
-> dsa_port_setup()
-> dsa_shared_port_link_register_of()
-> dsa_shared_port_link_register_of()
-> dsa_shared_port_phylink_register()
-> dsa_port_phylink_create()
-> ds->ops->phylink_get_caps():
mt753x_phylink_get_caps()
-> phylink_create()
-> INIT_WORK(&pl->resolve, phylink_resolve)
-> dsa_port_enable()
-> dsa_port_enable_rt()
-> ds->ops->port_enable():
mt7530_port_enable()
-> phylink_start()
-> phylink_mac_initial_config()
-> phylink_major_config()
-> phylink_mac_config()
-> pl->mac_ops->mac_config():
dsa_port_phylink_mac_config()
-> ds->ops->phylink_mac_config():
mt753x_phylink_mac_config()
-> pl->pcs->ops->pcs_config():
mt753x_pcs_config()
-> phylink_enable_and_run_resolve()
-> phylink_run_resolve()
-> queue_work(system_power_efficient_wq, &pl->resolve)
-> phylink_link_up()
-> pl->pcs->ops->pcs_link_up():
mtk_pcs_lynxi_link_up()
-> pl->mac_ops->mac_link_up():
dsa_port_phylink_mac_link_up()
-> ds->ops->phylink_mac_link_up():
mt753x_phylink_mac_link_up()
>
> So I think I have the answer to my initial thought: do the calls in
> mt7531_cpu_port_config() to the phylink methods have any use what so
> ever? The answer is no, they are entirely useless. The same goes for
> the other cpu_port_config() methods that do something similar. The
> same goes for the PMCR register write that's changing any bits
> included in PMCR_LINK_SETTINGS_MASK.
>
> What that means is that mt7988_cpu_port_config() can be entirely
> removed, it serves no useful purpose what so ever. For
> mt7531_cpu_port_config(), it only needs to set priv->p[56]_interface
> which, as far as I can see, probably only avoids mac_config() doing
> any pad setup (that's a guess.)
This is what I also believe and the reason why I made this patch to
simplify it. Looks like I'll just remove priv->info->cpu_port_config()
instead.
Arınç
Powered by blists - more mailing lists