| Message ID | 1408495538-27480-9-git-send-email-linux@roeck-us.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Guenter, On Tue, Aug 19, 2014 at 5:45 PM, Guenter Roeck <linux@roeck-us.net> wrote: > Implementing a restart handler in a module don't make sense > as there would be no guarantee that the module is loaded when > a restart is needed. Unexport arm_pm_restart to ensure that > no one gets the idea to do it anyway. > > Signed-off-by: Guenter Roeck <linux@roeck-us.net> > Acked-by: Catalin Marinas <catalin.marinas@arm.com> > Acked-by: Heiko Stuebner <heiko@sntech.de> > --- > v7: No change > v6: No change > v5: No change > v4: No change > v3: No change > v2: No change > > arch/arm/kernel/process.c | 1 - > arch/arm64/kernel/process.c | 1 - > 2 files changed, 2 deletions(-) Reviewed-by: Doug Anderson <dianders@chromium.org> Tested-by: Doug Anderson <dianders@chromium.org> (FYI: all patches tested by me were tested on rk3288-evb)
Hi Günther, On Wed, Aug 20, 2014 at 2:45 AM, Guenter Roeck <linux@roeck-us.net> wrote: > Implementing a restart handler in a module don't make sense > as there would be no guarantee that the module is loaded when > a restart is needed. Unexport arm_pm_restart to ensure that > no one gets the idea to do it anyway. Why not? I was just going to do that, but I got greeted by: ERROR: "arm_pm_restart" [drivers/power/reset/rmobile-reset.ko] undefined! So now we have to make sure all reset drivers for a zillion different hardware devices are builtin, and can't be modular? > Signed-off-by: Guenter Roeck <linux@roeck-us.net> > Acked-by: Catalin Marinas <catalin.marinas@arm.com> > Acked-by: Heiko Stuebner <heiko@sntech.de> > --- > v7: No change > v6: No change > v5: No change > v4: No change > v3: No change > v2: No change > > arch/arm/kernel/process.c | 1 - > arch/arm64/kernel/process.c | 1 - > 2 files changed, 2 deletions(-) > > diff --git a/arch/arm/kernel/process.c b/arch/arm/kernel/process.c > index ea279f7..250b6f6 100644 > --- a/arch/arm/kernel/process.c > +++ b/arch/arm/kernel/process.c > @@ -121,7 +121,6 @@ void (*pm_power_off)(void); > EXPORT_SYMBOL(pm_power_off); > > void (*arm_pm_restart)(enum reboot_mode reboot_mode, const char *cmd); > -EXPORT_SYMBOL_GPL(arm_pm_restart); > > /* > * This is our default idle handler. > diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c > index 0d3fb9f..398ab05 100644 > --- a/arch/arm64/kernel/process.c > +++ b/arch/arm64/kernel/process.c > @@ -98,7 +98,6 @@ void (*pm_power_off)(void); > EXPORT_SYMBOL_GPL(pm_power_off); > > void (*arm_pm_restart)(enum reboot_mode reboot_mode, const char *cmd); > -EXPORT_SYMBOL_GPL(arm_pm_restart); > > /* > * This is our default idle handler. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On 12/04/2014 05:36 AM, Geert Uytterhoeven wrote: > Hi Günther, > > On Wed, Aug 20, 2014 at 2:45 AM, Guenter Roeck <linux@roeck-us.net> wrote: >> Implementing a restart handler in a module don't make sense >> as there would be no guarantee that the module is loaded when >> a restart is needed. Unexport arm_pm_restart to ensure that >> no one gets the idea to do it anyway. > > Why not? I was just going to do that, but I got greeted by: > Because you should register a restart handler instead, like the other drivers in the same directory now do. > ERROR: "arm_pm_restart" [drivers/power/reset/rmobile-reset.ko] undefined! > > So now we have to make sure all reset drivers for a zillion different > hardware devices are builtin, and can't be modular? > No. All those drivers need to do is to register a restart handler using the API provided in the patch series. Ultimately all restart handlers should do that and arm_pm_restart should go away entirely. That was the point of the patch series. Guenter
Hi Günther, On Thu, Dec 4, 2014 at 3:26 PM, Guenter Roeck <linux@roeck-us.net> wrote: > On 12/04/2014 05:36 AM, Geert Uytterhoeven wrote: >> On Wed, Aug 20, 2014 at 2:45 AM, Guenter Roeck <linux@roeck-us.net> wrote: >>> Implementing a restart handler in a module don't make sense >>> as there would be no guarantee that the module is loaded when >>> a restart is needed. Unexport arm_pm_restart to ensure that >>> no one gets the idea to do it anyway. >> >> Why not? I was just going to do that, but I got greeted by: > > Because you should register a restart handler instead, like the other > drivers in the same directory now do. That's a different thing. "there would be no guarantee that the module is loaded when a restart is needed" is also valid for restart handlers... >> ERROR: "arm_pm_restart" [drivers/power/reset/rmobile-reset.ko] undefined! >> >> So now we have to make sure all reset drivers for a zillion different >> hardware devices are builtin, and can't be modular? >> > No. All those drivers need to do is to register a restart handler using > the API provided in the patch series. > > Ultimately all restart handlers should do that and arm_pm_restart should > go away entirely. That was the point of the patch series. Good. That's what I'm doing right know ;-) Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On 12/04/2014 06:44 AM, Geert Uytterhoeven wrote: > Hi Günther, > > On Thu, Dec 4, 2014 at 3:26 PM, Guenter Roeck <linux@roeck-us.net> wrote: >> On 12/04/2014 05:36 AM, Geert Uytterhoeven wrote: >>> On Wed, Aug 20, 2014 at 2:45 AM, Guenter Roeck <linux@roeck-us.net> wrote: >>>> Implementing a restart handler in a module don't make sense >>>> as there would be no guarantee that the module is loaded when >>>> a restart is needed. Unexport arm_pm_restart to ensure that >>>> no one gets the idea to do it anyway. >>> >>> Why not? I was just going to do that, but I got greeted by: >> >> Because you should register a restart handler instead, like the other >> drivers in the same directory now do. > > That's a different thing. "there would be no guarantee that the module is > loaded when a restart is needed" is also valid for restart handlers... > Not really, because you are supposed to unregister the restart handler on unload. Sure, you can instead clear arm_pm_reastart and leave the system with no means to restart ... Guenter >>> ERROR: "arm_pm_restart" [drivers/power/reset/rmobile-reset.ko] undefined! >>> >>> So now we have to make sure all reset drivers for a zillion different >>> hardware devices are builtin, and can't be modular? >>> >> No. All those drivers need to do is to register a restart handler using >> the API provided in the patch series. >> >> Ultimately all restart handlers should do that and arm_pm_restart should >> go away entirely. That was the point of the patch series. > > Good. That's what I'm doing right know ;-) > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds >
On Thursday 04 December 2014 06:51:49 Guenter Roeck wrote: > On 12/04/2014 06:44 AM, Geert Uytterhoeven wrote: > > On Thu, Dec 4, 2014 at 3:26 PM, Guenter Roeck <linux@roeck-us.net> wrote: > >> On 12/04/2014 05:36 AM, Geert Uytterhoeven wrote: > >>> On Wed, Aug 20, 2014 at 2:45 AM, Guenter Roeck <linux@roeck-us.net> wrote: > >>>> Implementing a restart handler in a module don't make sense > >>>> as there would be no guarantee that the module is loaded when > >>>> a restart is needed. Unexport arm_pm_restart to ensure that > >>>> no one gets the idea to do it anyway. > >>> > >>> Why not? I was just going to do that, but I got greeted by: > >> > >> Because you should register a restart handler instead, like the other > >> drivers in the same directory now do. > > > > That's a different thing. "there would be no guarantee that the module is > > loaded when a restart is needed" is also valid for restart handlers... > > > > Not really, because you are supposed to unregister the restart handler > on unload. Sure, you can instead clear arm_pm_reastart and leave the system > with no means to restart ... I agree with Geert that your commit message was confusing, it sounds like you were referring to drivers that are not yet loaded, while the problem that you are really address is drivers that have been unloaded later. Arnd
On Thu, Dec 04, 2014 at 04:06:22PM +0100, Arnd Bergmann wrote: > On Thursday 04 December 2014 06:51:49 Guenter Roeck wrote: > > On 12/04/2014 06:44 AM, Geert Uytterhoeven wrote: > > > On Thu, Dec 4, 2014 at 3:26 PM, Guenter Roeck <linux@roeck-us.net> wrote: > > >> On 12/04/2014 05:36 AM, Geert Uytterhoeven wrote: > > >>> On Wed, Aug 20, 2014 at 2:45 AM, Guenter Roeck <linux@roeck-us.net> wrote: > > >>>> Implementing a restart handler in a module don't make sense > > >>>> as there would be no guarantee that the module is loaded when > > >>>> a restart is needed. Unexport arm_pm_restart to ensure that > > >>>> no one gets the idea to do it anyway. > > >>> > > >>> Why not? I was just going to do that, but I got greeted by: > > >> > > >> Because you should register a restart handler instead, like the other > > >> drivers in the same directory now do. > > > > > > That's a different thing. "there would be no guarantee that the module is > > > loaded when a restart is needed" is also valid for restart handlers... > > > > > > > Not really, because you are supposed to unregister the restart handler > > on unload. Sure, you can instead clear arm_pm_reastart and leave the system > > with no means to restart ... > > I agree with Geert that your commit message was confusing, it sounds like > you were referring to drivers that are not yet loaded, while the problem > that you are really address is drivers that have been unloaded later. > Yes, I realize that now. I should probably have added the rationale for the entire series to this commit. I'll try to do better next time. Guenter
diff --git a/arch/arm/kernel/process.c b/arch/arm/kernel/process.c index ea279f7..250b6f6 100644 --- a/arch/arm/kernel/process.c +++ b/arch/arm/kernel/process.c @@ -121,7 +121,6 @@ void (*pm_power_off)(void); EXPORT_SYMBOL(pm_power_off); void (*arm_pm_restart)(enum reboot_mode reboot_mode, const char *cmd); -EXPORT_SYMBOL_GPL(arm_pm_restart); /* * This is our default idle handler. diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c index 0d3fb9f..398ab05 100644 --- a/arch/arm64/kernel/process.c +++ b/arch/arm64/kernel/process.c @@ -98,7 +98,6 @@ void (*pm_power_off)(void); EXPORT_SYMBOL_GPL(pm_power_off); void (*arm_pm_restart)(enum reboot_mode reboot_mode, const char *cmd); -EXPORT_SYMBOL_GPL(arm_pm_restart); /* * This is our default idle handler.