On Wed, Aug 28, 2024 at 11:51:02AM +0200, Maxime Chevallier wrote:
> fs_enet is a quite old but still used Ethernet driver found on some NXP
> devices. It has support for 10/100 Mbps ethernet, with half and full
> duplex. Some variants of it can use RMII, while other integrations are
> MII-only.
>
> Add phylink support, thus removing custom fixed-link hanldling.
>
> This also allows removing some internal flags such as the use_rmii flag.
>
> Signed-off-by: Maxime Chevallier <maxime.chevall...@bootlin.com>
Hi Maxime,
Some minor issues from my side: not a full review by any stretch of
the imagination.
...
> @@ -911,7 +894,7 @@ static int fs_enet_probe(struct platform_device *ofdev)
> if (!IS_ERR(clk)) {
> ret = clk_prepare_enable(clk);
> if (ret)
> - goto out_deregister_fixed_link;
> + goto out_phylink;
>
> fpi->clk_per = clk;
> }
This goto will result in a dereference of fep.
But fep is not initialised until the following line,
which appears a little below this hunk.
fep = netdev_priv(ndev);
This goto will also result in the function returning without
releasing clk.
Both flagged by Smatch.
> @@ -936,6 +919,26 @@ static int fs_enet_probe(struct platform_device *ofdev)
> fep->fpi = fpi;
> fep->ops = ops;
>
> + fep->phylink_config.dev = &ndev->dev;
> + fep->phylink_config.type = PHYLINK_NETDEV;
> + fep->phylink_config.mac_capabilities = MAC_10 | MAC_100;
> +
> + __set_bit(PHY_INTERFACE_MODE_MII,
> + fep->phylink_config.supported_interfaces);
> +
> + if (of_device_is_compatible(ofdev->dev.of_node, "fsl,mpc5125-fec"))
> + __set_bit(PHY_INTERFACE_MODE_RMII,
> + fep->phylink_config.supported_interfaces);
> +
> + phylink = phylink_create(&fep->phylink_config, dev_fwnode(fep->dev),
> + phy_mode, &fs_enet_phylink_mac_ops);
> + if (IS_ERR(phylink)) {
> + ret = PTR_ERR(phylink);
> + goto out_free_fpi;
This also appears to leak clk, as well as ndev.
I didn't look for other cases.
> + }
> +
> + fep->phylink = phylink;
> +
> ret = fep->ops->setup_data(ndev);
> if (ret)
> goto out_free_dev;
> @@ -968,8 +971,6 @@ static int fs_enet_probe(struct platform_device *ofdev)
>
> ndev->ethtool_ops = &fs_ethtool_ops;
>
> - netif_carrier_off(ndev);
> -
> ndev->features |= NETIF_F_SG;
>
> ret = register_netdev(ndev);
> @@ -988,10 +989,8 @@ static int fs_enet_probe(struct platform_device *ofdev)
> free_netdev(ndev);
> out_put:
> clk_disable_unprepare(fpi->clk_per);
> -out_deregister_fixed_link:
> - of_node_put(fpi->phy_node);
> - if (of_phy_is_fixed_link(ofdev->dev.of_node))
> - of_phy_deregister_fixed_link(ofdev->dev.of_node);
> +out_phylink:
> + phylink_destroy(fep->phylink);
> out_free_fpi:
> kfree(fpi);
> return ret;
...
