Hello Simon,

On Wed, 28 Aug 2024 17:32:24 +0100
Simon Horman [off-list ref] wrote:

On Wed, Aug 28, 2024 at 11:51:02AM +0200, Maxime Chevallier wrote:

quoted

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

@@ -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);

Nice catch, the goto should rather go to out_free_fpi.

This goto will also result in the function returning without
releasing clk.

ah yes, it's never disabled_unprepared, the phylink cleanup label is at
the wrong spot. I'll include a patch in the next iteration to make use
of devm_clk_get_enabled(), that should simplify all of that.

Both flagged by Smatch.

quoted

@@ -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.

Thanks, will be addressed in V2.

I didn't look for other cases.

I'll go over the cleanup path, thanks for checking this !

Thanks,

Maxime