netdev - Re: [PATCH net-next] net: dsa: validate that DT nodes of shared ports have the properties they need

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: <41ef4895-0450-c0ae-558c-45cdd4f8739b@gmail.com>
Date:   Mon, 25 Jul 2022 09:27:42 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Vladimir Oltean <vladimir.oltean@....com>, netdev@...r.kernel.org
Cc:     "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, Andrew Lunn <andrew@...n.ch>,
        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,
        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/23/22 09:46, Vladimir Oltean wrote:

[snip]

> +static int dsa_shared_port_validate_of_node(struct dsa_port *dp,
> +					    const char *description)
> +{
> +	struct device_node *dn = dp->dn, *phy_np;
> +	struct dsa_switch *ds = dp->ds;
> +	phy_interface_t mode;
> +
> +	/* Suppress validation if using platform data */
> +	if (!dn)
> +		return 0;
> +
> +	if (of_device_compatible_match(ds->dev->of_node,
> +				       dsa_switches_skipping_validation))
> +		return 0;
> +
> +	if (of_get_phy_mode(dn, &mode)) {
> +		dev_err(ds->dev,
> +			"%s port %d lacks the required \"phy-mode\" property\n",
> +			description, dp->index);
> +		return -EINVAL;
> +	}
> +
> +	phy_np = of_parse_phandle(dn, "phy-handle", 0);
> +	if (phy_np) {
> +		of_node_put(phy_np);
> +		return 0;
> +	}
> +
> +	/* Note: of_phy_is_fixed_link() also returns true for
> +	 * managed = "in-band-status"
> +	 */
> +	if (of_phy_is_fixed_link(dn))
> +		return 0;

To echo back my reply from the other email here, if we look beyond the rabbit hole and also attempt to parse these properties from the device_node pointed to us by the "ethernet" property, then I think we can trim down the list, and we still have some assurance that we can use phylink and a fixed link property, except we have to replicate the information from the CPU-port connected Ethernet controller.

> +
> +	/* TODO support SFP cages on DSA/CPU ports,
> +	 * here and in dsa_port_link_register_of()
> +	 */
> +	dev_err(ds->dev,
> +		"%s port %d lacks the required \"phy-handle\", \"fixed-link\" or \"managed\" properties\n",
> +		description, dp->index);
> +
> +	return -EINVAL;
> +}
> +
>  static int dsa_port_parse_user(struct dsa_port *dp, const char *name)
>  {
>  	if (!name)
> @@ -1373,6 +1534,12 @@ static int dsa_port_parse_user(struct dsa_port *dp, const char *name)
>  
>  static int dsa_port_parse_dsa(struct dsa_port *dp)
>  {
> +	int err;
> +
> +	err = dsa_shared_port_validate_of_node(dp, "DSA");
> +	if (err)
> +		return err;
> +
>  	dp->type = DSA_PORT_TYPE_DSA;

Move up the assignment so you can just read the type from dsa_shared_port_validate_of_node()?

>  
>  	return 0;
> @@ -1411,6 +1578,11 @@ static int dsa_port_parse_cpu(struct dsa_port *dp, struct net_device *master,
>  	struct dsa_switch_tree *dst = ds->dst;
>  	const struct dsa_device_ops *tag_ops;
>  	enum dsa_tag_protocol default_proto;
> +	int err;
> +
> +	err = dsa_shared_port_validate_of_node(dp, "CPU");
> +	if (err)
> +		return err;

Likewise, I don't think there are adverse effects of moving up the dp->type assignment all the way to the top?

>  
>  	/* Find out which protocol the switch would prefer. */
>  	default_proto = dsa_get_tag_protocol(dp, master);


-- 
Florian