| Message ID | 20240104101255.3056090-1-yi.fang.gan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [net,v3,1/1] net: phylink: Add module_exit() | expand |
On Thu, Jan 04, 2024 at 06:12:55PM +0800, Gan, Yi Fang wrote: 65;7401;1c> In delete_module(), if mod->init callback is defined but mod->exit callback > is not defined, it will assume the module cannot be removed and return > EBUSY. The module_exit() is missing from current phylink module drive > causing failure while unloading it. This is still missing the explanation why this is safe. Andrew --- pw-bot: cr
> -----Original Message----- > From: Andrew Lunn <andrew@lunn.ch> > Sent: Thursday, January 4, 2024 9:05 PM > To: Gan, Yi Fang <yi.fang.gan@intel.com> > Cc: Russell King <linux@armlinux.org.uk>; Heiner Kallweit > <hkallweit1@gmail.com>; David S . Miller <davem@davemloft.net>; Eric > Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo > Abeni <pabeni@redhat.com>; Marek Behún <kabel@kernel.org>; > netdev@vger.kernel.org; linux-stm32@st-md-mailman.stormreply.com; linux- > arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; Looi, Hong Aun > <hong.aun.looi@intel.com>; Voon, Weifeng <weifeng.voon@intel.com>; Song, > Yoong Siang <yoong.siang.song@intel.com>; Choong, Yong Liang > <yong.liang.choong@intel.com> > Subject: Re: [PATCH net v3 1/1] net: phylink: Add module_exit() > > On Thu, Jan 04, 2024 at 06:12:55PM +0800, Gan, Yi Fang wrote: > 65;7401;1c> In delete_module(), if mod->init callback is defined but mod->exit > callback > > is not defined, it will assume the module cannot be removed and return > > EBUSY. The module_exit() is missing from current phylink module drive > > causing failure while unloading it. > > This is still missing the explanation why this is safe. > > > Andrew > > --- > pw-bot: cr Hi Andrew, Regarding the justification on why it is safe to remove phylink, we had done some memory leak check when unloading the phylink module. root@localhost:~# lsmod | grep "phylink" phylink 73728 0 root@localhost:~# rmmod phylink root@localhost:~# echo scan > /sys/kernel/debug/kmemleak root@localhost:~# cat /sys/kernel/debug/kmemleak root@localhost:~# So far, we didn't observe any memory leaking happened when unloading phylink module. Is it sufficient or do you have any other suggestions to check on whether the module is safe to remove? Best Regards, Gan Yi Fang
On Thu, Jan 11, 2024 at 06:38:58AM +0000, Gan, Yi Fang wrote: > Hi Andrew, > > Regarding the justification on why it is safe to remove phylink, > we had done some memory leak check when unloading the phylink module. > > root@localhost:~# lsmod | grep "phylink" > phylink 73728 0 > root@localhost:~# rmmod phylink > root@localhost:~# echo scan > /sys/kernel/debug/kmemleak > root@localhost:~# cat /sys/kernel/debug/kmemleak > root@localhost:~# > > So far, we didn't observe any memory leaking happened when unloading > phylink module. Is it sufficient or do you have any other suggestions to check > on whether the module is safe to remove? I have no idea why one would think that memory leaks are in some way related to whether a module can be removed or not. To me at least they are two separate issues. I'll continue waiting for the justification...
> Hi Andrew, > > Regarding the justification on why it is safe to remove phylink, > we had done some memory leak check when unloading the phylink module. > > root@localhost:~# lsmod | grep "phylink" > phylink 73728 0 > root@localhost:~# rmmod phylink > root@localhost:~# echo scan > /sys/kernel/debug/kmemleak > root@localhost:~# cat /sys/kernel/debug/kmemleak > root@localhost:~# > > So far, we didn't observe any memory leaking happened when unloading > phylink module. Is it sufficient or do you have any other suggestions to check > on whether the module is safe to remove? In general, leaked memory is safe. Being leaked, nothing is using it. If nothing is using it, how can it cause an opps, corrupt a file system, etc. What you need to do is review all users of phylink, and determine if any of them retains a pointer to anything which phylink manages and will not be freed or uninitialized when it is unloaded. Is all polling of GPIOs cleanly stopped? Are interrupt handlers disabled and removed. Are PCS and MAC drivers cleanly unloaded first? Are hwmon entries cleanly removed, taking into account that user space might have them open? All ethtool ioctl/netlink calls are out of the code before it is removed, etc. Andrew
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c index 25c19496a336..4a05cda74d42 100644 --- a/drivers/net/phy/phylink.c +++ b/drivers/net/phy/phylink.c @@ -3726,5 +3726,11 @@ static int __init phylink_init(void) module_init(phylink_init); +static void __exit phylink_exit(void) +{ +} + +module_exit(phylink_exit); + MODULE_LICENSE("GPL v2"); MODULE_DESCRIPTION("phylink models the MAC to optional PHY connection");
In delete_module(), if mod->init callback is defined but mod->exit callback is not defined, it will assume the module cannot be removed and return EBUSY. The module_exit() is missing from current phylink module drive causing failure while unloading it. This patch introduces phylink_exit() for phylink module removal. Fixes: eca68a3c7d05 ("net: phylink: pass supported host PHY interface modes to phylib for SFP's PHYs") Cc: <stable@vger.kernel.org> # 6.1+ Signed-off-by: Lai Peter Jun Ann <jun.ann.lai@intel.com> Signed-off-by: Gan, Yi Fang <yi.fang.gan@intel.com> --- v1 -> v2: Introduce a macro function to reduce the boilerplate v2 -> v3: Remove the macro function as it is rejected and fix the format issue suggested from v1 drivers/net/phy/phylink.c | 6 ++++++ 1 file changed, 6 insertions(+)