stop OProfile calling arch_exit when arch_init fails

The OProfile driver no longer calls oprofile_arch_exit when oprofile_arch_init return failure.

This patch fixes the x86 implementation of oprofile_arch_init to ensure that op_nmi_exit is called if necessary.

Cc Robert Richter <[email protected]>

Cc Matt Fleming <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Ingo Molnar <[email protected]>

Signed-off-by Will Deacon <[email protected]>

---
 arch/x86/oprofile/init.c |   26 +++++++++++++++-----------
 1 files changed, 15 insertions(+), 11 deletions(-)

int __init oprofile_arch_init(struct oprofile_operations *ops) {

arch/x86/oprofile/init.c +1 -5

 	int ret;
 
-	ret = -ENODEV;
-
-#ifdef CONFIG_X86_LOCAL_APIC
 	ret = op_nmi_init(ops);
-#endif
-#ifdef CONFIG_X86_IO_APIC
 	if (ret < 0)
+#ifdef CONFIG_X86_IO_APIC

return ret;
}

Quoted 35 lines · Will Deacon

The OProfile driver no longer calls oprofile_arch_exit when oprofile_arch_init return failure.

This patch fixes the x86 implementation of oprofile_arch_init to ensure that op_nmi_exit is called if necessary.

Cc Robert Richter <[email protected]>

Cc Matt Fleming <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Ingo Molnar <[email protected]>

Signed-off-by Will Deacon <[email protected]>

---
 arch/x86/oprofile/init.c |   26 +++++++++++++++-----------
 1 files changed, 15 insertions(+), 11 deletions(-)

int __init oprofile_arch_init(struct oprofile_operations *ops)

 {
      int ret;

arch/x86/oprofile/init.c +4 -5

-     ret = -ENODEV;
-
-#ifdef CONFIG_X86_LOCAL_APIC
      ret = op_nmi_init(ops);
-#endif
-#ifdef CONFIG_X86_IO_APIC
      if (ret < 0)
+#ifdef CONFIG_X86_IO_APIC
              ret = op_nmi_timer_init(ops);
+#else
+             return ret;
 #endif
+
      ops->backtrace = x86_backtrace;

arch/x86/oprofile/init.c line 46

Quoted 3 lines

arch/x86/oprofile/init.c +3 -0

+     if (ret < 0)
+             op_nmi_exit();
+

I don't see why we have to do this. All init functions above clean up properly on failure. If op_nmi_init() succeeds we don't call op_nmi_timer_init(), so we don't need to free it it either.

Do you think it would be better to rework this patch so that the static using_nmi variable is set explicitly by init.c, or shall I just drop this patch altogether (and resubmit the first two)?

Quoted 3 lines · Will Deacon

Do you think it would be better to rework this patch so that the static using_nmi variable is set explicitly by init.c, or shall I just drop this patch altogether (and resubmit the first two)?

For x86 wont change the code (actually I found a bug in the init_sysfs error handler for which I will send a fix). Just wanted to get your confirmation in case I was missing something that x86 is not affected. I will apply the first 2 patches, no need to resubmit.

Ok, great. The commit comment might need tweaking for the first patch now that the x86 code remains unchanged.

Quoted 4 lines

For x86 wont change the code (actually I found a bug in the init_sysfs error handler for which I will send a fix). Just wanted to get your confirmation in case I was missing something that x86 is not affected. I will apply the first 2 patches, no need to resubmit.

Quoted 4 lines · Will Deacon

Quoted 4 lines

For x86 wont change the code (actually I found a bug in the init_sysfs error handler for which I will send a fix). Just wanted to get your confirmation in case I was missing something that x86 is not affected. I will apply the first 2 patches, no need to resubmit.

This is the patch, applied to:

git://git.kernel.org/pub/scm/linux/kernel/git/rric/oprofile.git urgent

-Robert

--

Quoted 1 line

From 10f0412f57f2a76a90eff4376f59cbb0a39e4e18 Mon Sep 17 00:00:00 2001

From: Robert Richter <[email protected]>
Date: Mon, 30 Aug 2010 10:56:18 +0200
Subject: [PATCH] oprofile, x86: fix init_sysfs error handling

On failure init_sysfs() might not properly free resources. The error code of the function is not checked. And, when reinitializing the exit function might be called twice. This patch fixes all this.

Cc stable at kernel.org

Signed-off-by Robert Richter <[email protected]>

---
 arch/x86/oprofile/nmi_int.c |   16 +++++++++++++---
 1 files changed, 13 insertions(+), 3 deletions(-)

arch/x86/oprofile/nmi_int.c +9 -2

index f6b48f6..73a41d3 100644
@@ -568,8 +568,13 @@ static int __init init_sysfs(void)
 	int error;
 
 	error = sysdev_class_register(&oprofile_sysclass);
-	if (!error)
-		error = sysdev_register(&device_oprofile);
+	if (error)
+		return error;
+
+	error = sysdev_register(&device_oprofile);
+	if (error)
+		sysdev_class_unregister(&oprofile_sysclass);
+
 	return error;
 }
 

@@ -695,6 +700,8 @@ int __init op_nmi_init(struct oprofile_operations *ops)
 	char *cpu_type = NULL;
 	int ret = 0;
 
+	using_nmi = 0;
+
 	if (!cpu_has_apic)
 		return -ENODEV;
 

Review int __init op_nmi_init(struct oprofile_operations *ops) · line 784

Quoted 6 lines

(quoted diff) +1 -1

@@ -774,7 +781,10 @@ int __init op_nmi_init(struct oprofile_operations *ops)
 
 	mux_init(ops);
 
-	init_sysfs();
+	ret = init_sysfs();

FYI, this causes a build error if CONFIG_PM is off:

arch/x86/oprofile/nmi_int.c:784: error: expected expression before ?do?

Due to this assymetric form of the wrapper:

  #define init_sysfs() do { } while (0)

The wrapper should be changed to return 0 i suspect, via something like this:

static inline int init_sysfs(void) { return 0; }

(untested)

These patches remove the oprofile_arch_exit call from oprofile_init, allowing architectures that perform memory allocation in their init functions to be simplified. This requires some changes to the ARM and x86 OProfile backends to ensure that their init functions clean up after themselves if they fail.

This is required for Matt's combined OProfile/Perf driver which will be shared between all architectures.

Patches taken against tip/master.

Cc Robert Richter <[email protected]>

Cc Matt Fleming <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Ingo Molnar <[email protected]>

Will Deacon (3):
oprofile: don't call arch exit code from init code on failure
ARM: oprofile: fix and simplify init/exit functions
x86: oprofile: fix oprofile_arch_init behaviour on failure

 arch/arm/oprofile/common.c |   47 +++++++++++++++++++++++--------------------
 arch/x86/oprofile/init.c   |   26 ++++++++++++++----------
 drivers/oprofile/oprof.c   |   11 +--------
 3 files changed, 42 insertions(+), 42 deletions(-)