Linux kernel mirror (for testing) git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
kernel os linux

Merge branch 'net-ethernet-convert-to-platform-remove-callback-returning-void'

Uwe Kleine-König says:

====================
net: ethernet: Convert to platform remove callback returning void

in (implicit) v1 of this series
(https://lore.kernel.org/netdev/20231117091655.872426-1-u.kleine-koenig@pengutronix.de)
I tried to address the resource leaks in the three cpsw drivers. However
this is hard to get right without being able to test the changes. So
here comes a series that just converts all drivers below
drivers/net/ethernet to use .remove_new() and adds a comment about the
potential leaks for someone else to fix the problem.

See commit 5c5a7680e67b ("platform: Provide a remove callback that
returns no value") for an extended explanation and the eventual goal.
The TL;DR; is to prevent bugs like the three noticed here.

Note this series results in no change of behaviour apart from improving
the error message for the three cpsw drivers from

remove callback returned a non-zero value. This will be ignored.

to

Failed to resume device (-ESOMETHING)
====================

Link: https://lore.kernel.org/r/20231128173823.867512-1-u.kleine-koenig@pengutronix.de
Signed-off-by: Paolo Abeni <pabeni@redhat.com>

+32 -19
+2 -4
drivers/net/ethernet/ezchip/nps_enet.c
··· 633 633 return err; 634 634 } 635 635 636 - static s32 nps_enet_remove(struct platform_device *pdev) 636 + static void nps_enet_remove(struct platform_device *pdev) 637 637 { 638 638 struct net_device *ndev = platform_get_drvdata(pdev); 639 639 struct nps_enet_priv *priv = netdev_priv(ndev); ··· 641 641 unregister_netdev(ndev); 642 642 netif_napi_del(&priv->napi); 643 643 free_netdev(ndev); 644 - 645 - return 0; 646 644 } 647 645 648 646 static const struct of_device_id nps_enet_dt_ids[] = { ··· 651 653 652 654 static struct platform_driver nps_enet_driver = { 653 655 .probe = nps_enet_probe, 654 - .remove = nps_enet_remove, 656 + .remove_new = nps_enet_remove, 655 657 .driver = { 656 658 .name = DRV_NAME, 657 659 .of_match_table = nps_enet_dt_ids,
+10 -5
drivers/net/ethernet/ti/am65-cpsw-nuss.c
··· 3028 3028 return ret; 3029 3029 } 3030 3030 3031 - static int am65_cpsw_nuss_remove(struct platform_device *pdev) 3031 + static void am65_cpsw_nuss_remove(struct platform_device *pdev) 3032 3032 { 3033 3033 struct device *dev = &pdev->dev; 3034 3034 struct am65_cpsw_common *common; ··· 3037 3037 common = dev_get_drvdata(dev); 3038 3038 3039 3039 ret = pm_runtime_resume_and_get(&pdev->dev); 3040 - if (ret < 0) 3041 - return ret; 3040 + if (ret < 0) { 3041 + /* Note, if this error path is taken, we're leaking some 3042 + * resources. 3043 + */ 3044 + dev_err(&pdev->dev, "Failed to resume device (%pe)\n", 3045 + ERR_PTR(ret)); 3046 + return; 3047 + } 3042 3048 3043 3049 am65_cpsw_unregister_devlink(common); 3044 3050 am65_cpsw_unregister_notifiers(common); ··· 3062 3056 3063 3057 pm_runtime_put_sync(&pdev->dev); 3064 3058 pm_runtime_disable(&pdev->dev); 3065 - return 0; 3066 3059 } 3067 3060 3068 3061 static int am65_cpsw_nuss_suspend(struct device *dev) ··· 3161 3156 .pm = &am65_cpsw_nuss_dev_pm_ops, 3162 3157 }, 3163 3158 .probe = am65_cpsw_nuss_probe, 3164 - .remove = am65_cpsw_nuss_remove, 3159 + .remove_new = am65_cpsw_nuss_remove, 3165 3160 }; 3166 3161 3167 3162 module_platform_driver(am65_cpsw_nuss_driver);
+10 -5
drivers/net/ethernet/ti/cpsw.c
··· 1722 1722 return ret; 1723 1723 } 1724 1724 1725 - static int cpsw_remove(struct platform_device *pdev) 1725 + static void cpsw_remove(struct platform_device *pdev) 1726 1726 { 1727 1727 struct cpsw_common *cpsw = platform_get_drvdata(pdev); 1728 1728 int i, ret; 1729 1729 1730 1730 ret = pm_runtime_resume_and_get(&pdev->dev); 1731 - if (ret < 0) 1732 - return ret; 1731 + if (ret < 0) { 1732 + /* Note, if this error path is taken, we're leaking some 1733 + * resources. 1734 + */ 1735 + dev_err(&pdev->dev, "Failed to resume device (%pe)\n", 1736 + ERR_PTR(ret)); 1737 + return; 1738 + } 1733 1739 1734 1740 for (i = 0; i < cpsw->data.slaves; i++) 1735 1741 if (cpsw->slaves[i].ndev) ··· 1746 1740 cpsw_remove_dt(pdev); 1747 1741 pm_runtime_put_sync(&pdev->dev); 1748 1742 pm_runtime_disable(&pdev->dev); 1749 - return 0; 1750 1743 } 1751 1744 1752 1745 #ifdef CONFIG_PM_SLEEP ··· 1800 1795 .of_match_table = cpsw_of_mtable, 1801 1796 }, 1802 1797 .probe = cpsw_probe, 1803 - .remove = cpsw_remove, 1798 + .remove_new = cpsw_remove, 1804 1799 }; 1805 1800 1806 1801 module_platform_driver(cpsw_driver);
+10 -5
drivers/net/ethernet/ti/cpsw_new.c
··· 2037 2037 return ret; 2038 2038 } 2039 2039 2040 - static int cpsw_remove(struct platform_device *pdev) 2040 + static void cpsw_remove(struct platform_device *pdev) 2041 2041 { 2042 2042 struct cpsw_common *cpsw = platform_get_drvdata(pdev); 2043 2043 int ret; 2044 2044 2045 2045 ret = pm_runtime_resume_and_get(&pdev->dev); 2046 - if (ret < 0) 2047 - return ret; 2046 + if (ret < 0) { 2047 + /* Note, if this error path is taken, we're leaking some 2048 + * resources. 2049 + */ 2050 + dev_err(&pdev->dev, "Failed to resume device (%pe)\n", 2051 + ERR_PTR(ret)); 2052 + return; 2053 + } 2048 2054 2049 2055 cpsw_unregister_notifiers(cpsw); 2050 2056 cpsw_unregister_devlink(cpsw); ··· 2061 2055 cpsw_remove_dt(cpsw); 2062 2056 pm_runtime_put_sync(&pdev->dev); 2063 2057 pm_runtime_disable(&pdev->dev); 2064 - return 0; 2065 2058 } 2066 2059 2067 2060 static int __maybe_unused cpsw_suspend(struct device *dev) ··· 2121 2116 .of_match_table = cpsw_of_mtable, 2122 2117 }, 2123 2118 .probe = cpsw_probe, 2124 - .remove = cpsw_remove, 2119 + .remove_new = cpsw_remove, 2125 2120 }; 2126 2121 2127 2122 module_platform_driver(cpsw_driver);