Re: [PATCH net-next 6/6] net: ethernet: fs_enet: phylink conversion
From: Simon Horman <horms@kernel.org>
Date: 2024-08-28 16:32:29
Also in:
lkml, netdev
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.chevallier@bootlin.com>
Hi Maxime, Some minor issues from my side: not a full review by any stretch of the imagination. ...
quoted hunk ↗ jump to hunk
@@ -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.
quoted hunk ↗ jump to hunk
@@ -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.
quoted hunk ↗ jump to hunk
+ } + + 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;
...