Re: [PATCH 2/2] net: ethernet: Add driver for Sunplus SP7021

[Pavel Skripkin <paskripkin@gmail.com>]

Hi, Wells!

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

[code snip]

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

(*)

> +				// 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.

> +				// 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]

> +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 :)

> +	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`.

> +	return 0;
> +}


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



With regards,
Pavel Skripkin