[PATCH net v3 1/1] net: phylink: Add module_exit()

[PATCH net v3 1/1] net: phylink: Add module_exit()

[Gan, Yi Fang]

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

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");
-- 
2.34.1

Re: [PATCH net v3 1/1] net: phylink: Add module_exit()

[Andrew Lunn]

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

RE: [PATCH net v3 1/1] net: phylink: Add module_exit()

[Gan, Yi Fang]

> -----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

Re: [PATCH net v3 1/1] net: phylink: Add module_exit()

[Russell King (Oracle)]

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

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net v3 1/1] net: phylink: Add module_exit()

[Andrew Lunn]

> 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