netdev - Re: [RFC v1 net-next 0/7] add support for ocelot external ports

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: <20230217011155.4ztjsiqnzhvxvf3b@skbuf>
Date:   Fri, 17 Feb 2023 03:11:55 +0200
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     Colin Foster <colin.foster@...advantage.com>
Cc:     "Russell King (Oracle)" <linux@...linux.org.uk>,
        linux-phy@...ts.infradead.org, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Kishon Vijay Abraham I <kishon@...nel.org>,
        Vinod Koul <vkoul@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Eric Dumazet <edumazet@...gle.com>,
        "David S. Miller" <davem@...emloft.net>,
        Florian Fainelli <f.fainelli@...il.com>,
        Andrew Lunn <andrew@...n.ch>, UNGLinuxDriver@...rochip.com,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Lee Jones <lee@...nel.org>
Subject: Re: [RFC v1 net-next 0/7] add support for ocelot external ports

On Thu, Feb 16, 2023 at 04:42:06PM -0800, Colin Foster wrote:
> I believe the main gotcha was that control over the phy itself, by way
> of phy_set_mode_ext(). That needed the 'struct device_node *portnp'

DT parsing in felix_parse_dt() is not the only DT parsing that is done,
and certainly nothing depends on it in the way you describe.

dsa_switch_parse_of() also parses the device tree. felix_parse_dt() only
exists because the SERDES/PCS drivers from NXP LS1028A do not support
dynamic reconfiguration of the SERDES protocol. So we parse the device
tree to set the initial ocelot_port->phy_mode, and then (with the
current phylink API) we populate phylink's config->supported_interfaces
with just that one bit set, to prevent SERDES protocol changes.

Do not get too hung up on this parsing (unless you believe you could
simplify the code by removing it; case in which I'd be interested if you
had patches in this area). Each port's device_node is also available in
struct dsa_port :: dn.

> 
> .... Keeps looking ....
> 
> Ahh, yes. Regmaps and regfields aren't initialized at the time of
> dt parsing in felix. And the MDIO bus isn't allocated until after that.
> That's the reason for patch 6 parse_port_node() - I need the tree node
> to get MDIO access to the phy, which I don't have until I'm done parsing
> the tree...

Nope. Device tree parsing in DSA is done from dsa_register_switch(), and
dsa_switch_ops :: setup() (aka felix_setup()) is the first callback in
which the information is reliably available.

You can *easily* call phy_set_mode_ext() from the "setup()" callback.
In fact, you're already doing that. Not sure what the problem seems to be.
It doesn't seem to be an ordering problem between phy_set_mode_ext() and
phylink_create() either, because DSA calls phylink_create() after both
the dsa_switch_ops :: setup() as well as port_setup() callbacks.
So there should be plenty of opportunity for you to prepare.

> There might be a cleaner way for me to do that. I'm tiptoeing a little
> bit to avoid any regressions with the felix_vsc9959 or seville_vsc9953.

I can test those. I'd much prefer if you made an initial effort to keep
a relatively consistent code structure.