Hi Pavel,

Hi, Wells!

On 11/3/21 14:02, Wells Lu wrote:

[code snip]

quoted

+		if (comm->dual_nic) {
+			struct net_device *net_dev2 = mac->next_netdev;
+
+			if (!netif_running(net_dev2)) {
+				mac_hw_stop(mac);
+
+				mac2 = netdev_priv(net_dev2);
+

(*)

quoted

+				// unregister and free net device.
+				unregister_netdev(net_dev2);
+				free_netdev(net_dev2);
+				mac->next_netdev = NULL;
+				pr_info(" Unregistered and freed net device \"eth1\"!\n");
+
+				comm->dual_nic = 0;
+				mac_switch_mode(mac);
+				rx_mode_set(net_dev);
+				mac_hw_addr_del(mac2);
+

mac2 is net_dev2 private data (*), so it will become freed after
free_netdev() call.

FWIW the latest `smatch` should warn about this type of bugs.

Yes, this is indeed a bug.
But the code paragraph has been removed thoroughly in [PATCH v2].

quoted

+				// If eth0 is up, turn on lan 0 and 1 when
+				// switching to daisy-chain mode.
+				if (comm->enable & 0x1)
+					comm->enable = 0x3;

[code snip]

quoted

+static int l2sw_remove(struct platform_device *pdev) {
+	struct net_device *net_dev;
+	struct net_device *net_dev2;
+	struct l2sw_mac *mac;
+
+	net_dev = platform_get_drvdata(pdev);
+	if (!net_dev)
+		return 0;
+	mac = netdev_priv(net_dev);
+
+	// Unregister and free 2nd net device.
+	net_dev2 = mac->next_netdev;
+	if (net_dev2) {
+		unregister_netdev(net_dev2);
+		free_netdev(net_dev2);
+	}
+

Is it save here to free mac->next_netdev before unregistering "parent"
netdev? I haven't checked the whole code, just asking :)

Yes, I think it is save.
netdev2 should be unregistered and freed before net_dev.
If net_dev is unregistered and freed in advance,
mac->next_netdev becomes danger because 'mac' has been freed.

quoted

+	sysfs_remove_group(&pdev->dev.kobj, &l2sw_attribute_group);
+
+	mac->comm->enable = 0;
+	soc0_stop(mac);
+
+	napi_disable(&mac->comm->rx_napi);
+	netif_napi_del(&mac->comm->rx_napi);
+	napi_disable(&mac->comm->tx_napi);
+	netif_napi_del(&mac->comm->tx_napi);
+
+	mdio_remove(net_dev);
+
+	// Unregister and free 1st net device.
+	unregister_netdev(net_dev);
+	free_netdev(net_dev);
+
+	clk_disable(mac->comm->clk);
+
+	// Free 'common' area.
+	kfree(mac->comm);

Same here with `mac`.

This is indeed a bug.
But the statement, Kfree(mac->comm);, has been removed in [PATCH v2].
In [PATCH v2], structure data 'mac->comm' is allocated by
devm_kzalloc(). No more need to free it here.

quoted

+	return 0;
+}

I haven't read the whole thread, i am sorry if these questions were already discussed.



With regards,
Pavel Skripkin

Thank you very much for your review!

Best regards,
Wells Lu