[PATCH] fix register_sysrq() in 2.4.9++

[PATCH] fix register_sysrq() in 2.4.9++

[Randy.Dunlap]

[-- Attachment #1: Type: text/plain, Size: 452 bytes --]

Linus, Alan-

register_sysrq_key() is typed #ifdef CONFIG_MAGIC_SYSRQ,
but untyped if it's not defined, causing problems for callers
when CONFIG_MAGIC_SYSRQ is not defined.
This patch fixes that problem.

arch/i386/kernel/apm.c calls [un]register_sysrq_key() with
a pointer to a sysrq_poweroff_op data structure that is
only declared #ifdef CONFIG_MAGIC_SYSRQ.
This patch also wraps the register/unregister calls with
#ifdef CONFIG_MAGIC_SYSRQ.

~Randy

[-- Attachment #2: sysrq-if.patch --]
[-- Type: text/plain, Size: 1235 bytes --]

--- linux/include/linux/sysrq.h.org	Mon Sep 17 10:21:07 2001
+++ linux/include/linux/sysrq.h	Thu Sep 20 08:29:15 2001
@@ -87,8 +87,17 @@
 }
 
 #else
-#define register_sysrq_key(a,b)		do {} while(0)
-#define unregister_sysrq_key(a,b)	do {} while(0)
+
+static inline int register_sysrq_key(int key, struct sysrq_key_op *op_p)
+{
+	return -1;
+}
+
+static inline int unregister_sysrq_key(int key, struct sysrq_key_op *op_p)
+{
+	return 0;
+}
+
 #endif
 
 /* Deferred actions */
--- linux/arch/i386/kernel/apm.c.org	Mon Sep 17 10:15:45 2001
+++ linux/arch/i386/kernel/apm.c	Thu Sep 20 08:34:44 2001
@@ -1564,7 +1564,10 @@
 	/* Install our power off handler.. */
 	if (power_off)
 		pm_power_off = apm_power_off;
-	register_sysrq_key('o',&sysrq_poweroff_op);
+#ifdef CONFIG_MAGIC_SYSRQ
+	if (register_sysrq_key('o',&sysrq_poweroff_op))
+		printk (KERN_ERR "Error: cannot register APM PowerOff SysRq key\n");
+#endif
 
 	if (smp_num_cpus == 1) {
 #if defined(CONFIG_APM_DISPLAY_BLANK) && defined(CONFIG_VT)
@@ -1780,7 +1783,9 @@
 	}
 	misc_deregister(&apm_device);
 	remove_proc_entry("apm", NULL);
+#ifdef CONFIG_MAGIC_SYSRQ
 	unregister_sysrq_key('o',&sysrq_poweroff_op);
+#endif
 	if (power_off)
 		pm_power_off = NULL;
 	exit_kapmd = 1;

Re: [PATCH] fix register_sysrq() in 2.4.9++

[Alan Cox]

u> +
> +static inline int register_sysrq_key(int key, struct sysrq_key_op *op_p)
> +{
> +	return -1;
> +}

make it report ok as other non compiled in stuff does - then you can avoid
masses of ifdefs

Re: [PATCH] fix register_sysrq() in 2.4.9++

[Randy.Dunlap]

Alan Cox wrote:
> 
> u> +
> > +static inline int register_sysrq_key(int key, struct sysrq_key_op *op_p)
> > +{
> > +     return -1;
> > +}
> 
> make it report ok as other non compiled in stuff does - then you can avoid
> masses of ifdefs

Yeah, I considered that, and it doesn't matter to me whether it
reports 0 or -1, but it's the data pointer that (mostly) requires
the #ifdefs, unless the data is always present or a dummy data pointer
is used.... ?

~Randy

Re: [PATCH] fix register_sysrq() in 2.4.9++

[Alan Cox]

> Yeah, I considered that, and it doesn't matter to me whether it
> reports 0 or -1, but it's the data pointer that (mostly) requires
> the #ifdefs, unless the data is always present or a dummy data pointer
> is used.... ?

#define it to an inline without some arguments ?

Alan

[PATCH:v2] fix register_sysrq() in 2.4.9++

[Randy.Dunlap]

[-- Attachment #1: Type: text/plain, Size: 595 bytes --]

Alan Cox wrote:
> 
> > Yeah, I considered that, and it doesn't matter to me whether it
> > reports 0 or -1, but it's the data pointer that (mostly) requires
> > the #ifdefs, unless the data is always present or a dummy data pointer
> > is used.... ?
> 
> #define it to an inline without some arguments ?
~~~~~~~~~~~~~~~~~~
I can't get that to work, but someone else may be able to...

Here's another version for you to consider.

The [un]register_sysrq_key() calls return 0 when CONFIG_MAGIC_SYSRQ
is not defined/configured.
However, it sacrifices one small data structure of 3 pointers.

~Randy

[-- Attachment #2: sysrq-if2.patch --]
[-- Type: text/plain, Size: 734 bytes --]

--- linux/arch/i386/kernel/apm.c.org	Mon Sep 17 10:15:45 2001
+++ linux/arch/i386/kernel/apm.c	Thu Sep 20 11:51:25 2001
@@ -703,6 +703,8 @@
 	help_msg:       "Off",
 	action_msg:     "Power Off\n"
 };
+#else
+struct sysrq_key_op sysrq_poweroff_op;
 #endif
 
 
--- linux/include/linux/sysrq.h.org	Mon Sep 17 10:21:07 2001
+++ linux/include/linux/sysrq.h	Thu Sep 20 11:42:15 2001
@@ -87,8 +87,17 @@
 }
 
 #else
-#define register_sysrq_key(a,b)		do {} while(0)
-#define unregister_sysrq_key(a,b)	do {} while(0)
+
+static inline int register_sysrq_key(int key, struct sysrq_key_op *op_p)
+{
+	return 0;
+}
+
+static inline int unregister_sysrq_key(int key, struct sysrq_key_op *op_p)
+{
+	return 0;
+}
+
 #endif
 
 /* Deferred actions */

Re: [PATCH:v2] fix register_sysrq() in 2.4.9++

[Crutcher Dunnavant]

I'm not sure if this is sufficient. The low level interfaces need to be
exposed, and if we are not expecting modules to pay attention to the
CONFIG_MAGIC_SYSRQ setting, then the all of these interfaces need to be
overridden.

However, do we even need this #ifdef CONFIG_MAGIC_SYSRQ block at all?
What does it matter if modules register or unregister events, if they
cannot be called?

The old code only zaped the enable if sysrq was not defined, and that is
what I'm doing in the table. Some real changes would be neccessary to
actually drop out the whole system.

There is also no real reason to try and no-op these functions for speed,
as they are trivial and FAR outside of the main call path.

So the way to go I see here is:
 a) allow the registration functions to always be defined.
and either:
 b) handle the return failure in the __sysrq_XXX functions themselves,
 c) or not.

++ 20/09/01 11:57 -0700 - Randy.Dunlap:
> Alan Cox wrote:
> > 
> > > Yeah, I considered that, and it doesn't matter to me whether it
> > > reports 0 or -1, but it's the data pointer that (mostly) requires
> > > the #ifdefs, unless the data is always present or a dummy data pointer
> > > is used.... ?
> > 
> > #define it to an inline without some arguments ?
> ~~~~~~~~~~~~~~~~~~
> I can't get that to work, but someone else may be able to...
> 
> Here's another version for you to consider.
> 
> The [un]register_sysrq_key() calls return 0 when CONFIG_MAGIC_SYSRQ
> is not defined/configured.
> However, it sacrifices one small data structure of 3 pointers.
> 
> ~Randy
> --- linux/arch/i386/kernel/apm.c.org	Mon Sep 17 10:15:45 2001
> +++ linux/arch/i386/kernel/apm.c	Thu Sep 20 11:51:25 2001
> @@ -703,6 +703,8 @@
>  	help_msg:       "Off",
>  	action_msg:     "Power Off\n"
>  };
> +#else
> +struct sysrq_key_op sysrq_poweroff_op;
>  #endif
>  
>  
> --- linux/include/linux/sysrq.h.org	Mon Sep 17 10:21:07 2001
> +++ linux/include/linux/sysrq.h	Thu Sep 20 11:42:15 2001
> @@ -87,8 +87,17 @@
>  }
>  
>  #else
> -#define register_sysrq_key(a,b)		do {} while(0)
> -#define unregister_sysrq_key(a,b)	do {} while(0)
> +
> +static inline int register_sysrq_key(int key, struct sysrq_key_op *op_p)
> +{
> +	return 0;
> +}
> +
> +static inline int unregister_sysrq_key(int key, struct sysrq_key_op *op_p)
> +{
> +	return 0;
> +}
> +
>  #endif
>  
>  /* Deferred actions */


-- 
Crutcher        <crutcher@datastacks.com>
GCS d--- s+:>+:- a-- C++++$ UL++++$ L+++$>++++ !E PS+++ PE Y+ PGP+>++++
    R-(+++) !tv(+++) b+(++++) G+ e>++++ h+>++ r* y+>*$

[PATCH] Magic SysRq alternate fix register functions

[Crutcher Dunnavant]

[-- Attachment #1: Type: text/plain, Size: 3205 bytes --]

++ 21/09/01 18:22 -0400 - Crutcher Dunnavant:
> I'm not sure if this is sufficient. The low level interfaces need to be
> exposed, and if we are not expecting modules to pay attention to the
> CONFIG_MAGIC_SYSRQ setting, then the all of these interfaces need to be
> overridden.
> 
> However, do we even need this #ifdef CONFIG_MAGIC_SYSRQ block at all?
> What does it matter if modules register or unregister events, if they
> cannot be called?
> 
> The old code only zaped the enable if sysrq was not defined, and that is
> what I'm doing in the table. Some real changes would be neccessary to
> actually drop out the whole system.
> 
> There is also no real reason to try and no-op these functions for speed,
> as they are trivial and FAR outside of the main call path.
> 
> So the way to go I see here is:
>  a) allow the registration functions to always be defined.
> and either:
>  b) handle the return failure in the __sysrq_XXX functions themselves,
>  c) or not.

A 'dont-close-it' patch is attached.

> 
> ++ 20/09/01 11:57 -0700 - Randy.Dunlap:
> > Alan Cox wrote:
> > > 
> > > > Yeah, I considered that, and it doesn't matter to me whether it
> > > > reports 0 or -1, but it's the data pointer that (mostly) requires
> > > > the #ifdefs, unless the data is always present or a dummy data pointer
> > > > is used.... ?
> > > 
> > > #define it to an inline without some arguments ?
> > ~~~~~~~~~~~~~~~~~~
> > I can't get that to work, but someone else may be able to...
> > 
> > Here's another version for you to consider.
> > 
> > The [un]register_sysrq_key() calls return 0 when CONFIG_MAGIC_SYSRQ
> > is not defined/configured.
> > However, it sacrifices one small data structure of 3 pointers.
> > 
> > ~Randy
> > --- linux/arch/i386/kernel/apm.c.org	Mon Sep 17 10:15:45 2001
> > +++ linux/arch/i386/kernel/apm.c	Thu Sep 20 11:51:25 2001
> > @@ -703,6 +703,8 @@
> >  	help_msg:       "Off",
> >  	action_msg:     "Power Off\n"
> >  };
> > +#else
> > +struct sysrq_key_op sysrq_poweroff_op;
> >  #endif
> >  
> >  
> > --- linux/include/linux/sysrq.h.org	Mon Sep 17 10:21:07 2001
> > +++ linux/include/linux/sysrq.h	Thu Sep 20 11:42:15 2001
> > @@ -87,8 +87,17 @@
> >  }
> >  
> >  #else
> > -#define register_sysrq_key(a,b)		do {} while(0)
> > -#define unregister_sysrq_key(a,b)	do {} while(0)
> > +
> > +static inline int register_sysrq_key(int key, struct sysrq_key_op *op_p)
> > +{
> > +	return 0;
> > +}
> > +
> > +static inline int unregister_sysrq_key(int key, struct sysrq_key_op *op_p)
> > +{
> > +	return 0;
> > +}
> > +
> >  #endif
> >  
> >  /* Deferred actions */
> 
> 
> -- 
> Crutcher        <crutcher@datastacks.com>
> GCS d--- s+:>+:- a-- C++++$ UL++++$ L+++$>++++ !E PS+++ PE Y+ PGP+>++++
>     R-(+++) !tv(+++) b+(++++) G+ e>++++ h+>++ r* y+>*$
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
Crutcher        <crutcher@datastacks.com>
GCS d--- s+:>+:- a-- C++++$ UL++++$ L+++$>++++ !E PS+++ PE Y+ PGP+>++++
    R-(+++) !tv(+++) b+(++++) G+ e>++++ h+>++ r* y+>*$

[-- Attachment #2: patch-2.4.10-pre13-sysrq_register --]
[-- Type: text/plain, Size: 1320 bytes --]

--- 2.4.10-pre13.fix_sysrq/include/linux/sysrq.h.fix_sysrq	Fri Sep 21 17:11:20 2001
+++ 2.4.10-pre13.fix_sysrq/include/linux/sysrq.h	Fri Sep 21 18:25:55 2001
@@ -42,7 +42,6 @@
                 struct kbd_struct *, struct tty_struct *);
 
 
-#ifdef CONFIG_MAGIC_SYSRQ
 
 /*
  * Sysrq registration manipulation functions
@@ -55,7 +54,8 @@
 
 extern __inline__ int
 __sysrq_swap_key_ops_nolock(int key, struct sysrq_key_op *insert_op_p,
-				struct sysrq_key_op *remove_op_p) {
+				struct sysrq_key_op *remove_op_p)
+{
 	int retval;
 	if (__sysrq_get_key_op(key) == remove_op_p) {
 		__sysrq_put_key_op(key, insert_op_p);
@@ -86,11 +86,6 @@
 	return __sysrq_swap_key_ops(key, NULL, op_p);
 }
 
-#else
-#define register_sysrq_key(a,b)		do {} while(0)
-#define unregister_sysrq_key(a,b)	do {} while(0)
-#endif
-
 /* Deferred actions */
 
 extern int emergency_sync_scheduled;
--- 2.4.10-pre13.fix_sysrq/arch/i386/kernel/apm.c.fix_sysrq	Fri Sep 21 18:27:06 2001
+++ 2.4.10-pre13.fix_sysrq/arch/i386/kernel/apm.c	Fri Sep 21 18:27:42 2001
@@ -689,7 +689,6 @@
 		(void) apm_set_power_state(APM_STATE_OFF);
 }
 
-#ifdef CONFIG_MAGIC_SYSRQ
 /*
  * Magic sysrq key and handler for the power off function
  */
@@ -703,7 +702,6 @@
 	help_msg:       "Off",
 	action_msg:     "Power Off\n"
 };
-#endif
 
 
 #ifdef CONFIG_APM_DO_ENABLE

Re: [PATCH] Magic SysRq alternate fix register functions

[Randy.Dunlap]

Crutcher Dunnavant wrote:
> 
> ++ 21/09/01 18:22 -0400 - Crutcher Dunnavant:
> > I'm not sure if this is sufficient. The low level interfaces need to be
> > exposed, and if we are not expecting modules to pay attention to the
> > CONFIG_MAGIC_SYSRQ setting, then the all of these interfaces need to be
> > overridden.
> >
> > However, do we even need this #ifdef CONFIG_MAGIC_SYSRQ block at all?
> > What does it matter if modules register or unregister events, if they
> > cannot be called?
> >
> > The old code only zaped the enable if sysrq was not defined, and that is
> > what I'm doing in the table. Some real changes would be neccessary to
> > actually drop out the whole system.
> >
> > There is also no real reason to try and no-op these functions for speed,
> > as they are trivial and FAR outside of the main call path.
> >
> > So the way to go I see here is:
> >  a) allow the registration functions to always be defined.
> > and either:
> >  b) handle the return failure in the __sysrq_XXX functions themselves,
> >  c) or not.
> 
> A 'dont-close-it' patch is attached.
> 
>   ----------------------------------------------------------------------
>  Name: patch-2.4.10-pre13-sysrq_register
>  patch-2.4.10-pre13-sysrq_register       Type: Plain Text (text/plain)
>  Description: patch-2.4.10-pre13-sysrq_register

Yep, that certainly fixes the API when CONFIG_MAGIC_SYSRQ
is not defined, which is what I wanted to see.

~Randy