Re: [PATCH net-next v4 06/12] net: dsa: rzn1-a5psw: add Renesas RZ/N1 advanced 5 port switch driver

[Vladimir Oltean <olteanv@gmail.com>]

On Mon, May 09, 2022 at 03:18:54PM +0200, Clément Léger wrote:
> Add Renesas RZ/N1 advanced 5 port switch driver. This switch handles 5
> ports including 1 CPU management port. A MDIO bus is also exposed by
> this switch and allows to communicate with PHYs connected to the ports.
> Each switch port (except for the CPU management ports) is connected to
> the MII converter.
> 
> This driver includes basic bridging support, more support will be added
> later (vlan, etc).
> 
> Suggested-by: Jean-Pierre Geslin <jean-pierre.geslin@non.se.com>
> Suggested-by: Phil Edworthy <phil.edworthy@renesas.com>
> Signed-off-by: Clément Léger <clement.leger@bootlin.com>
> ---
> +static int a5psw_port_bridge_join(struct dsa_switch *ds, int port,
> +				  struct dsa_bridge bridge,
> +				  bool *tx_fwd_offload,
> +				  struct netlink_ext_ack *extack)
> +{
> +	struct a5psw *a5psw = ds->priv;
> +
> +	/* We only support 1 bridge device */
> +	if (a5psw->br_dev && bridge.dev != a5psw->br_dev) {
> +		NL_SET_ERR_MSG_MOD(extack,
> +				   "Forwarding offload supported for a single bridge");

I don't think I saw the dsa_slave_changeupper() patch that avoids
overwriting the extack when dsa_port_bridge_join() returns -EOPNOTSUPP.

> +		return -EOPNOTSUPP;
> +	}
> +
> +	a5psw->br_dev = bridge.dev;
> +	a5psw_flooding_set_resolution(a5psw, port, true);
> +	a5psw_port_mgmtfwd_set(a5psw, port, false);
> +
> +	return 0;

By the way, does this switch pass tools/testing/selftests/drivers/net/dsa/no_forwarding.sh?

> +}
> +
> +static void a5psw_port_bridge_leave(struct dsa_switch *ds, int port,
> +				    struct dsa_bridge bridge)
> +{
> +	struct a5psw *a5psw = ds->priv;
> +
> +	a5psw_flooding_set_resolution(a5psw, port, false);
> +	a5psw_port_mgmtfwd_set(a5psw, port, true);
> +
> +	/* No more ports bridged */
> +	if (a5psw->bridged_ports == BIT(A5PSW_CPU_PORT))
> +		a5psw->br_dev = NULL;
> +}
> +
> +static int a5psw_pcs_get(struct a5psw *a5psw)
> +{
> +	struct device_node *ports, *port, *pcs_node;
> +	struct phylink_pcs *pcs;
> +	int ret;
> +	u32 reg;
> +
> +	ports = of_get_child_by_name(a5psw->dev->of_node, "ethernet-ports");
> +	if (!ports)
> +		ports = of_get_child_by_name(a5psw->dev->of_node, "ports");
> +
> +	if (!ports)
> +		return -EINVAL;
> +
> +	for_each_available_child_of_node(ports, port) {
> +		pcs_node = of_parse_phandle(port, "pcs-handle", 0);
> +		if (!pcs_node)
> +			continue;
> +
> +		if (of_property_read_u32(port, "reg", &reg)) {
> +			ret = -EINVAL;
> +			goto free_pcs;
> +		}
> +
> +		if (reg >= ARRAY_SIZE(a5psw->pcs)) {
> +			ret = -ENODEV;
> +			goto free_pcs;
> +		}
> +
> +		pcs = miic_create(pcs_node);
> +		if (IS_ERR(pcs)) {
> +			dev_err(a5psw->dev, "Failed to create PCS for port %d\n",
> +				reg);
> +			ret = PTR_ERR(pcs);
> +			goto free_pcs;
> +		}
> +
> +		a5psw->pcs[reg] = pcs;
> +	}
> +	of_node_put(ports);
> +
> +	return 0;
> +
> +free_pcs:

The error path is missing of_node_put(ports).

> +	a5psw_pcs_free(a5psw);
> +
> +	return ret;
> +}
> +
> +static int a5psw_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *mdio;
> +	struct dsa_switch *ds;
> +	struct a5psw *a5psw;
> +	int ret;
> +
> +	a5psw = devm_kzalloc(dev, sizeof(*a5psw), GFP_KERNEL);
> +	if (!a5psw)
> +		return -ENOMEM;
> +
> +	a5psw->dev = dev;
> +	spin_lock_init(&a5psw->lk_lock);
> +	spin_lock_init(&a5psw->reg_lock);
> +	a5psw->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (!a5psw->base)
> +		return -EINVAL;
> +
> +	ret = a5psw_pcs_get(a5psw);
> +	if (ret)
> +		return ret;
> +
> +	a5psw->hclk = devm_clk_get(dev, "hclk");
> +	if (IS_ERR(a5psw->hclk)) {
> +		dev_err(dev, "failed get hclk clock\n");
> +		ret = PTR_ERR(a5psw->hclk);
> +		goto free_pcs;
> +	}
> +
> +	a5psw->clk = devm_clk_get(dev, "clk");
> +	if (IS_ERR(a5psw->clk)) {
> +		dev_err(dev, "failed get clk_switch clock\n");
> +		ret = PTR_ERR(a5psw->clk);
> +		goto free_pcs;
> +	}
> +
> +	ret = clk_prepare_enable(a5psw->clk);
> +	if (ret)
> +		goto free_pcs;
> +
> +	ret = clk_prepare_enable(a5psw->hclk);
> +	if (ret)
> +		goto clk_disable;
> +
> +	mdio = of_get_child_by_name(dev->of_node, "mdio");
> +	if (of_device_is_available(mdio)) {
> +		ret = a5psw_probe_mdio(a5psw, mdio);
> +		if (ret) {
> +			of_node_put(mdio);
> +			dev_err(&pdev->dev, "Failed to register MDIO: %d\n",
> +				ret);
> +			goto hclk_disable;
> +		}

Missing an of_node_put(mdio) if the device is available and ret == 0.

> +	} else {
> +		of_node_put(mdio);
> +	}
> +
> +	ds = &a5psw->ds;
> +	ds->dev = &pdev->dev;
> +	ds->num_ports = A5PSW_PORTS_NUM;
> +	ds->ops = &a5psw_switch_ops;
> +	ds->priv = a5psw;
> +
> +	ret = dsa_register_switch(ds);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to register DSA switch: %d\n", ret);
> +		goto hclk_disable;
> +	}
> +
> +	return 0;
> +
> +hclk_disable:
> +	clk_disable_unprepare(a5psw->hclk);
> +clk_disable:
> +	clk_disable_unprepare(a5psw->clk);
> +free_pcs:
> +	a5psw_pcs_free(a5psw);
> +
> +	return ret;
> +}
> +
> +static int a5psw_remove(struct platform_device *pdev)
> +{
> +	struct a5psw *a5psw = platform_get_drvdata(pdev);

I hate to repeat myself, but drivers in general can be removed after the device
was shut down. If that happens, you will dereference a platform_get_drvdata()
which is NULL, as set by yourself in a5psw_shutdown().

> +
> +	dsa_unregister_switch(&a5psw->ds);
> +	a5psw_pcs_free(a5psw);
> +	clk_disable_unprepare(a5psw->hclk);
> +	clk_disable_unprepare(a5psw->clk);
> +
> +	return 0;
> +}
> +
> +static void a5psw_shutdown(struct platform_device *pdev)
> +{
> +	struct a5psw *a5psw = platform_get_drvdata(pdev);
> +
> +	if (!a5psw)
> +		return;
> +
> +	dsa_switch_shutdown(&a5psw->ds);
> +
> +	platform_set_drvdata(pdev, NULL);
> +}