Re: [PATCH] sh: hp6xx: unset apm_get_power_status on unload

On 2026-04-18 21:43, John Paul Adrian Glaubitz wrote:
> Hi Artur,
> 
> On Sat, 2026-04-18 at 21:20 +0200, Artur Rojek wrote:
>> On 2026-04-12 09:30, John Paul Adrian Glaubitz wrote:
>> > Hi Ahelenia,
>> >
>> > On Fri, 2025-10-17 at 00:05 +0200, Ahelenia Ziemiańska wrote:
>> > > The API for apm_get_power_status is "call it if it isn't NULL".
>> > > If the module is unloaded and it's not unset,
>> > > reading /proc/apm will jump into unloaded kernel memory.
>> > >
>> > > The first commit that added this incompletely refactored
>> > > the assigned-to variable in __exit,
>> > > the second deleted it instead of fixing it.
>> > >
>> > > Unset it on unload like drivers/macintosh/apm_emu.c.
>> > >
>> > > Fixes: 0a9b0db19262 ("[APM] SH: Convert to use shared APM emulation.")
>> > > Fixes: 8c8ee8254767 ("sh: hp6xx: APM build fix and new battery
>> > > values.")
>> > > Signed-off-by: Ahelenia Ziemiańska <[email protected]>
>> > > ---
>> > >  arch/sh/boards/mach-hp6xx/hp6xx_apm.c | 3 +++
>> > >  1 file changed, 3 insertions(+)
>> > >
>> > > diff --git a/arch/sh/boards/mach-hp6xx/hp6xx_apm.c
>> > > b/arch/sh/boards/mach-hp6xx/hp6xx_apm.c
>> > > index e5c4c7d34139..089eca39c4e6 100644
>> > > --- a/arch/sh/boards/mach-hp6xx/hp6xx_apm.c
>> > > +++ b/arch/sh/boards/mach-hp6xx/hp6xx_apm.c
>> > > @@ -98,6 +98,9 @@ static int __init hp6x0_apm_init(void)
>> > >
>> > >  static void __exit hp6x0_apm_exit(void)
>> > >  {
>> > > +	if (apm_get_power_status == hp6x0_apm_get_power_status)
>> > > +		apm_get_power_status = NULL;
>> > > +
>> > >  	free_irq(HP680_BTN_IRQ, 0);
>> > >  }
>> > >
>> >
>> > Let's CC Artur Rojek who has worked on the hp6xx code before and should
>> > give his ACK.
>> 
>> This driver can't be built as a module, so this code is never called.
>> But since we can't get rid of module_exit(apm_exit):
> 
> Isn't hp6x0_apm_exit() called when the device is being shutdown even 
> when
> it's not built as a module?

I went by suggestion found in the kernel docs [1], and after a quick
glance over include/linux/module.h:
> /**
>  * module_exit() - driver exit entry point
>  * @x: function to be run when driver is removed
>  *
>  * module_exit() will wrap the driver clean-up code
>  * with cleanup_module() when used with rmmod when
>  * the driver is a module.  If the driver is statically
>  * compiled into the kernel, module_exit() has no effect.
>  * There can only be one per module.
>  */
> #define module_exit(x)	__exitcall(x);

The symbol is certainly not eliminated at compilation time:
> $ readelf -sW vmlinux | grep hp6x0_apm_exit
>  2586: 8d36b000    36 FUNC    LOCAL  DEFAULT   14 hp6x0_apm_exit

However, the section which should hold it does not exit:
> #define __exitcall(fn)						\
> 	static exitcall_t __exitcall_##fn __exit_call = fn
> (...)
> #define __exit_call	__used __section(".exitcall.exit")

> $ readelf -S vmlinux | grep "exitcall\.exit"
> $ echo $?
> 1

I don't know what to make of it, but nevertheless, I am ok with merging
the patch in question.

Cheers,
Artur

[1] 
https://www.kernel.org/doc/html/latest/driver-api/basics.html#c.module_exit

> 
> Adrian