Message-ID: <eabe2419-9e90-28b3-0b2a-7d779f0f1864@gmail.com>
Date:   Mon, 25 Jul 2022 09:37:35 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Vladimir Oltean <vladimir.oltean@....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 7/24/22 13:28, Vladimir Oltean wrote:
> 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

The DTB that is being passed just specifies port 8 with its 'reg' and 'label = "cpu"' and 'ethernet' phandle to the Ethernet controller node.

The Ethernet controller node does specify the correct 'phy-mode' and 'fixed-link' properties, so I suppose we can turn on validation if we do support such an use case of "looking beyond" what the CPU port node provided us and replicating that into the CPU-port local phylink creation. There are actually many DTSes in that situation, now there are a few cases where we do have some interesting combinations:

- internal switches are typically wired up over GMII or some internal fabric, but the end result is that the ports sort of "automatically" work provided that you have the same link parameters on both side, either explicitly or just by having the hardware and or driver default to some sane settings. In that case, omitting 'fixed-link' on the CPU port node seems reasonable provided that we can retrieve that same piece information from looking at the 'ethernet' or 'link' phandle properties and use that to configure the phylink instance on the CPU/DSA port(s)

- external switches do require providing information on either side of the link, especially for 'phy-mode' (especially true with RGMII), however 'fixed-link' could be somehow omitted if we just matched what the CPU/DSA port has

> 
>>>> 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.

Pretty much yes.

> 
> 
> 
> 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.

SRAB means the switch is internal to the SoC, so GMII would be an appropriate PHY interface if we cared so much as to representation *exactly* how the RTL was designed, but that could also be substituted by PHY_INTERFACE_MODE_INTERNAL or PHY_INTERFACE_MODE_NA IMHO, as long as we match the CPU port's 'fixed-link' property.

> 
>     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.

The plan is still to remove adjust_link, the same plan that has been not worked on for years, so do quote me on that and remind me again in 4 years. That said, the bgmac driver does register a fixed link for its CPU port, sort of "on its own" if it does find one. This is largely historical due to the driver having started on MIPS-based chips where there was no DT to describe anything.

I do think that we should however fix these various DTSes to have the necessary properties, and it should not be an issue because those devices do not have any bootloader awareness of DT and the DTB is always shipped with the kernel image that OpenWrt/Buildroot uses do install.
-- 
Florian

Powered by blists - more mailing lists