[PATCH net v2 0/2] Fix phylink unloadable issue

[PATCH net v2 0/2] Fix phylink unloadable issue

[Gan, Yi Fang]

Add module_exit_stub() for phylink module.

Changes since v1:
- Introduce a helper macro for module_exit() boilerplate

This series is the combination of following:
v1 with empty phylink_exit():
https://lore.kernel.org/all/20231127101603.807593-1-yi.fang.gan@intel.com/
v1 of module_exit_stub():
https://lore.kernel.org/all/20231212094352.2263283-1-yi.fang.gan@intel.com/

Gan, Yi Fang (2):
  driver.h: add helper macro for module_exit() boilerplate
  net: phylink: Add module_exit_stub()

 drivers/net/phy/phylink.c     |  1 +
 include/linux/device/driver.h | 14 ++++++++++++++
 2 files changed, 15 insertions(+)

-- 
2.34.1

[PATCH net v2 1/2] driver.h: add helper macro for module_exit() boilerplate

[Gan, Yi Fang]

For the modules need a module_init() but don't need to do
anything special in module_exit() might need to have an empty
module_exit(). This patch add a new macro module_exit_stub() to
replace the empty module_exit(). The macro is useful to remove
the module_exit() boilerplate.

Cc: <stable@vger.kernel.org> # 6.1+
Suggested-by: Lobakin, Aleksander <aleksander.lobakin@intel.com>
Signed-off-by: Gan, Yi Fang <yi.fang.gan@intel.com>
---
 include/linux/device/driver.h | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/include/linux/device/driver.h b/include/linux/device/driver.h
index 7738f458995f..7d322eef501e 100644
--- a/include/linux/device/driver.h
+++ b/include/linux/device/driver.h
@@ -288,4 +288,18 @@ static int __init __driver##_init(void) \
 } \
 device_initcall(__driver##_init);
 
+/**
+ * module_exit_stub() - Helper macro for drivers that have init but don't
+ * do anything in exit. This eliminates some boilerplate.
+ * Each module may only use this macro once, and calling it replaces
+ * module_exit().
+ *
+ * @__driver: driver name
+ */
+#define module_exit_stub(__driver) \
+static void __exit __driver##_exit(void) \
+{ \
+} \
+module_exit(__driver##_exit)
+
 #endif	/* _DEVICE_DRIVER_H_ */
-- 
2.34.1

[PATCH net v2 2/2] net: phylink: Add module_exit_stub()

[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.
Add module_exit_stub() in phylink for the module to be unloadable.

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: Gan, Yi Fang <yi.fang.gan@intel.com>
---
 drivers/net/phy/phylink.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 25c19496a336..823c9b43cd92 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -3725,6 +3725,7 @@ static int __init phylink_init(void)
 }
 
 module_init(phylink_init);
+module_exit_stub(phylink);
 
 MODULE_LICENSE("GPL v2");
 MODULE_DESCRIPTION("phylink models the MAC to optional PHY connection");
-- 
2.34.1

Re: [PATCH net v2 2/2] net: phylink: Add module_exit_stub()

[Andrew Lunn]

On Thu, Dec 21, 2023 at 04:51:09PM +0800, Gan, Yi Fang wrote:
> 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.

You are missing justification it is actually safe to remove
phylink. Maybe Russell King deliberately did not implement
module_exit() because it can explode in interesting ways if it was?

	Andrew

RE: [PATCH net v2 2/2] net: phylink: Add module_exit_stub()

[Gan, Yi Fang]

Hi Andrew,

The function phylink_init() is introduced by others.
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?h=linux-6.6.y&id=eca68a3c7d05b38b4e728cead0c49718f2bc1d5a 
The module_exit() is added by referring to
https://elixir.bootlin.com/linux/latest/source/kernel/module/main.c#L738.
Since the macro function is rejected, I will send a V3.
Let's see if Rusell King has any comment. Thanks.


Best Regards,
Gan Yi Fang

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Thursday, December 21, 2023 5:23 PM
> To: Gan, Yi Fang <yi.fang.gan@intel.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Andrew Halaney
> <ahalaney@redhat.com>; Javier Martinez Canillas <javierm@redhat.com>; John
> Stultz <jstultz@google.com>; Rafael J . Wysocki <rafael@kernel.org>; Jens Axboe
> <axboe@kernel.dk>; 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>; Lai, Peter Jun Ann
> <peter.jun.ann.lai@intel.com>; Choong, Yong Liang
> <yong.liang.choong@intel.com>
> Subject: Re: [PATCH net v2 2/2] net: phylink: Add module_exit_stub()
> 
> On Thu, Dec 21, 2023 at 04:51:09PM +0800, Gan, Yi Fang wrote:
> > 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.
> 
> You are missing justification it is actually safe to remove phylink. Maybe Russell
> King deliberately did not implement
> module_exit() because it can explode in interesting ways if it was?
> 
> 	Andrew

Re: [PATCH net v2 2/2] net: phylink: Add module_exit_stub()

[Andrew Lunn]

On Thu, Jan 04, 2024 at 09:45:47AM +0000, Gan, Yi Fang wrote:
> Hi Andrew,
> 
> The function phylink_init() is introduced by others.
> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?h=linux-6.6.y&id=eca68a3c7d05b38b4e728cead0c49718f2bc1d5a 
> The module_exit() is added by referring to
> https://elixir.bootlin.com/linux/latest/source/kernel/module/main.c#L738.
> Since the macro function is rejected, I will send a V3.
> Let's see if Rusell King has any comment. Thanks.

Please don't top post when using linux kernel mailing lists.

> > You are missing justification it is actually safe to remove phylink. Maybe Russell
> > King deliberately did not implement
> > module_exit() because it can explode in interesting ways if it was?

You still need to explain why it is safe to implement it.

    Andrew

Re: [PATCH net v2 0/2] Fix phylink unloadable issue

[Greg Kroah-Hartman]

On Thu, Dec 21, 2023 at 04:51:07PM +0800, Gan, Yi Fang wrote:
> Add module_exit_stub() for phylink module.
> 
> Changes since v1:
> - Introduce a helper macro for module_exit() boilerplate
> 
> This series is the combination of following:
> v1 with empty phylink_exit():
> https://lore.kernel.org/all/20231127101603.807593-1-yi.fang.gan@intel.com/
> v1 of module_exit_stub():
> https://lore.kernel.org/all/20231212094352.2263283-1-yi.fang.gan@intel.com/

As I said before, no, this isn't ok.  Why just resubmit a patch when
it's already been rejected?

This patch series should NOT be accepted as-is, you know this!

Also, you are not following the documented and REQUIRED rules for Intel
developers to be submitting kernel patches, so on that reason alone
these need to be rejected.

Please work with the Intel internel developers to do this correctly if
you wish to submit this again in the future.

greg k-h