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

[Florian Fainelli <f.fainelli@gmail.com>]

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