Message-ID: <20220724202839.klkwevxaqxnvbfha@skbuf>
Date:   Sun, 24 Jul 2022 20:28:40 +0000
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     Florian Fainelli <f.fainelli@...il.com>
CC:     Andrew Lunn <andrew@...n.ch>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Oleksij Rempel <linux@...pel-privat.de>,
        Christian Marangi <ansuelsmth@...il.com>,
        John Crispin <john@...ozen.org>,
        Kurt Kanzenbach <kurt@...utronix.de>,
        Mans Rullgard <mans@...sr.com>,
        Arun Ramadoss <arun.ramadoss@...rochip.com>,
        Woojung Huh <woojung.huh@...rochip.com>,
        "UNGLinuxDriver@...rochip.com" <UNGLinuxDriver@...rochip.com>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        George McCollister <george.mccollister@...il.com>,
        DENG Qingfang <dqfext@...il.com>,
        Sean Wang <sean.wang@...iatek.com>,
        Landen Chao <Landen.Chao@...iatek.com>,
        Matthias Brugger <matthias.bgg@...il.com>,
        Hauke Mehrtens <hauke@...ke-m.de>,
        Martin Blumenstingl <martin.blumenstingl@...glemail.com>,
        Aleksander Jan Bajkowski <olek2@...pl>,
        Alvin Šipraga <alsi@...g-olufsen.dk>,
        Luiz Angelo Daros de Luca <luizluca@...il.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        Pawel Dembicki <paweldembicki@...il.com>,
        Clément Léger <clement.leger@...tlin.com>,
        Geert Uytterhoeven <geert+renesas@...der.be>,
        Russell King <rmk+kernel@...linux.org.uk>
Subject: Re: [PATCH net-next] net: dsa: validate that DT nodes of shared ports
 have the properties they need

On Sun, Jul 24, 2022 at 09:59:14AM -0700, Florian Fainelli wrote:
> Le 24/07/2022 à 07:21, Vladimir Oltean a écrit :
> > On Sat, Jul 23, 2022 at 08:09:34PM +0200, Andrew Lunn wrote:
> > > Hi Vladimir
> > > 
> > > I think this is a first good step.
> > > 
> > > > +static const char * const dsa_switches_skipping_validation[] = {
> > > 
> > > One thing to consider is do we want to go one step further and make
> > > this dsa_switches_dont_enforce_validation
> > > 
> > > Meaning always run the validation, and report what is not valid, but
> > > don't enforce with an -EINVAL for switches on the list?
> > 
> > Can do. The question is what are our prospects of eventually getting rid
> > of incompletely specified DT blobs. If they're likely to stay around
> > forever, maybe printing with dev_err() doesn't sound like such a great
> > idea?
> > 
> > I know what's said in Documentation/devicetree/bindings/net/dsa/marvell.txt
> > about not putting a DT blob somewhere where you can't update it, but
> > will that be the case for everyone? Florian, I think some bcm_sf2 device
> > trees are pretty much permanent, based on some of your past commits?
> 
> The Device Tree blob is provided and runtime populated by the bootloader, so
> we can definitively make changes and missing properties are always easy to
> add as long as we can keep a reasonable window of time (2 to 3 years seems
> to be about the right window) for people to update their systems. FWIW, all
> of the bcm_sf2 based systems have had a fixed-link property for the CPU
> port.

Ok, so does this mean I can remove these from dsa_switches_dont_enforce_validation?

#if IS_ENABLED(CONFIG_NET_DSA_BCM_SF2)
	"brcm,bcm7445-switch-v4.0",
	"brcm,bcm7278-switch-v4.0",
	"brcm,bcm7278-switch-v4.8",
#endif

> > > Maybe it is too early for that, we first need to submit patches to the
> > > mainline DT files to fixes those we can?
> > > 
> > > Looking at the mv88e6xxx instances, adding fixed-links should not be
> > > too hard. What might be harder is the phy-mode, in particular, what
> > > RGMII delay should be specified.
> > 
> > Since DT blobs and kernels have essentially separate lifetimes, I'm
> > thinking it doesn't matter too much if we first fix the mainline DT
> > blobs or not; it's not like that would avoid users seeing errors.
> > 
> > Anyway I'm thinking it would be useful in general to verbally resolve
> > some of the incomplete DT descriptions I've pointed out here. This would
> > be a good indication whether we can add automatic logic that comes to
> > the same resolution at least for all known cases.
> 
> Agreed.

Ok, so let's start with b53?

Correct me if I'm wrong, I'm just looking at code and it's been a while
since I've transitioned my drivers from adjust_link.

Essentially since b53 still implements b53_adjust_link, this makes
dsa_port_link_register_of() (what is called for DSA_PORT_TYPE_CPU and
DSA_PORT_TYPE_DSA) take the following route:

int dsa_port_link_register_of(struct dsa_port *dp)
{
	struct dsa_switch *ds = dp->ds;
	struct device_node *phy_np;
	int port = dp->index;

	if (!ds->ops->adjust_link) { // this is false, b53 has adjust_link
		phy_np = of_parse_phandle(dp->dn, "phy-handle", 0);
		if (of_phy_is_fixed_link(dp->dn) || phy_np) {
			if (ds->ops->phylink_mac_link_down)
				ds->ops->phylink_mac_link_down(ds, port,
					MLO_AN_FIXED, PHY_INTERFACE_MODE_NA);
			of_node_put(phy_np);
			return dsa_port_phylink_register(dp);
		}
		of_node_put(phy_np);
		return 0;
	} // as a result, we never register with phylink for CPU/DSA ports, Russell's logic is avoided

	dev_warn(ds->dev,
		 "Using legacy PHYLIB callbacks. Please migrate to PHYLINK!\n"); // you do see this warning

	if (of_phy_is_fixed_link(dp->dn))
		return dsa_port_fixed_link_register_of(dp);
	else
		return dsa_port_setup_phy_of(dp, true);
}

So one of dsa_port_fixed_link_register_of() or dsa_port_setup_phy_of()
is going to get called in your case.

If you have a fixed-link in your device tree, dsa_port_fixed_link_register_of()
will fake a call to adjust_link() with the fixed PHY that has its
phydev->interface populated based on phy-mode (if missing, this defaults to NA).
The b53_adjust_link() function cares about phydev->interface only to the
extent of checking for RGMII delays, otherwise it doesn't matter that
the phy-mode is missing (arch/arm/boot/dts/bcm47094-linksys-panamera.dts),
for practical purposes.

If your description is missing a fixed-link (arch/arm/boot/dts/bcm47081-buffalo-wzr-600dhp2.dts),
the other function will get called, dsa_port_setup_phy_of().
Essentially this will call dsa_port_get_phy_device(), which will return
NULL, so we will exit early, do nothing and return 0. Right?

So b53 is going to be unaffected by Russell's changes, due to it still
implementing adjust_link.



Now on to the device trees, let's imagine for a second you'll actually
delete b53_adjust_link:

    arch/arm/boot/dts/bcm47094-linksys-panamera.dts
    - lacks phy-mode

phylink will call b53_srab_phylink_get_caps() to determine the maximum
supported interface. b53_srab_phylink_get_caps() will circularly look at
its current interface, p->mode (which is PHY_INTERFACE_MODE_NA, right?
because we lack a phy-mode), and not populate config->supported_interfaces
with anything.

So what is the expected phy-mode here? phylink couldn't find it :)
I think this is a case where the b53 driver would need to be patched to
report a default_interface for ports 5, 7 and 8 of brcm,bcm53012-srab.

    arch/arm/boot/dts/bcm47189-tenda-ac9.dts
    - lacks phy-mode and fixed-link

If my above logic was correct, things here are even worse, because when
phylink can't determine the supported_interfaces (no phy-mode), it can't
determine the speed for the fixed-link either.

    arch/arm/boot/dts/bcm47081-luxul-xap-1410.dts
    arch/arm/boot/dts/bcm47081-luxul-xwr-1200.dts
    arch/arm/boot/dts/bcm47081-buffalo-wzr-600dhp2.dts
    - lacks phy-mode and fixed-link
    arch/arm/boot/dts/bcm47094-luxul-xbr-4500.dts
    arch/arm/boot/dts/bcm4708-smartrg-sr400ac.dts
    arch/arm/boot/dts/bcm4708-luxul-xap-1510.dts
    arch/arm/boot/dts/bcm953012er.dts
    arch/arm/boot/dts/bcm4708-netgear-r6250.dts
    arch/arm/boot/dts/bcm4708-buffalo-wzr-1166dhp-common.dtsi
    arch/arm/boot/dts/bcm4708-luxul-xwc-1000.dts
    arch/arm/boot/dts/bcm47094-luxul-abr-4500.dts
    - lacks phy-mode and fixed-link
    arch/arm/boot/dts/bcm53016-meraki-mr32.dts
    - lacks phy-mode

I guess that the bottom line is that the b53 driver is safe due to adjust_link.
Beyond that, it's up to you how much you want to polish things up.
It's going to be quite a bit of work to bring everything up to date.

Powered by blists - more mailing lists