Messages in this thread | | | Date | Sun, 14 Nov 2021 22:19:29 +0300 | Subject | Re: [PATCH 2/2] net: ethernet: Add driver for Sunplus SP7021 | From | Pavel Skripkin <> |
| |
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
| |