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@xxxxxxxxxx>
> Suggested-by: Phil Edworthy <phil.edworthy@xxxxxxxxxxx>
> Signed-off-by: Clément Léger <clement.leger@xxxxxxxxxxx>
> ---
> +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", ®)) {
> + 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);
> +}
