lkml.org 
[lkml]   [2021]   [Nov]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH 2/2] net: ethernet: Add driver for Sunplus SP7021
From
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

\
 
 \ /
  Last update: 2021-11-14 20:20    [W:0.307 / U:0.064 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site