[PATCH v2] powerpc/fadump: return error when fadump registration fails

 - log an error message when registration fails and no error code listed
 in the switch is returned
 - translate the hv error code to posix error code and return it from
 fw_register
 - return the posix error code from fw_register to the process writing
 to sysfs
 - return EEXIST on re-registration
 - return success on deregistration when fadump is not registered
 - return ENODEV when no memory is reserved for fadump

Signed-off-by: Michal Suchanek <redacted>
---
v2: fix return in register_fadump()
---
 arch/powerpc/kernel/fadump.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c
index 2fa582092d42..83421e0a774b 100644
--- a/arch/powerpc/kernel/fadump.c
+++ b/arch/powerpc/kernel/fadump.c

@@ -398,9 +398,9 @@ static int __init early_fadump_param(char *p)
 }
 early_param("fadump", early_fadump_param);
 
-static void register_fw_dump(struct fadump_mem_struct *fdm)
+static int register_fw_dump(struct fadump_mem_struct *fdm)
 {
-	int rc;
+	int rc, err;
 	unsigned int wait_time;
 
 	pr_debug("Registering for firmware-assisted kernel dump...\n");

@@ -417,7 +417,12 @@ static void register_fw_dump(struct fadump_mem_struct *fdm)
 
 	} while (wait_time);
 
+	err = -EIO;
 	switch (rc) {
+	default:
+		printk(KERN_ERR "Failed to register firmware-assisted kernel"
+			" dump. Unknown Error(%d).\n", rc);
+		break;
 	case -1:
 		printk(KERN_ERR "Failed to register firmware-assisted kernel"
 			" dump. Hardware Error(%d).\n", rc);

@@ -425,18 +430,22 @@ static void register_fw_dump(struct fadump_mem_struct *fdm)
 	case -3:
 		printk(KERN_ERR "Failed to register firmware-assisted kernel"
 			" dump. Parameter Error(%d).\n", rc);
+		err = -EINVAL;
 		break;
 	case -9:
 		printk(KERN_ERR "firmware-assisted kernel dump is already "
 			" registered.");
 		fw_dump.dump_registered = 1;
+		err = -EEXIST;
 		break;
 	case 0:
 		printk(KERN_INFO "firmware-assisted kernel dump registration"
 			" is successful\n");
 		fw_dump.dump_registered = 1;
+		err = 0;
 		break;
 	}
+	return err;
 }
 
 void crash_fadump(struct pt_regs *regs, const char *str)

@@ -977,7 +986,7 @@ static unsigned long init_fadump_header(unsigned long addr)
 	return addr;
 }
 
-static void register_fadump(void)
+static int register_fadump(void)
 {
 	unsigned long addr;
 	void *vaddr;

@@ -987,7 +996,7 @@ static void register_fadump(void)
 	 * assisted dump.
 	 */
 	if (!fw_dump.reserve_dump_area_size)
-		return;
+		return -ENODEV;
 
 	fadump_setup_crash_memory_ranges();
 

@@ -1000,7 +1009,7 @@ static void register_fadump(void)
 	fadump_create_elfcore_headers(vaddr);
 
 	/* register the future kernel dump with firmware. */
-	register_fw_dump(&fdm);
+	return register_fw_dump(&fdm);
 }
 
 static int fadump_unregister_dump(struct fadump_mem_struct *fdm)

@@ -1182,7 +1191,6 @@ static ssize_t fadump_register_store(struct kobject *kobj,
 	switch (buf[0]) {
 	case '0':
 		if (fw_dump.dump_registered == 0) {
-			ret = -EINVAL;
 			goto unlock_out;
 		}
 		/* Un-register Firmware-assisted dump */

@@ -1190,11 +1198,11 @@ static ssize_t fadump_register_store(struct kobject *kobj,
 		break;
 	case '1':
 		if (fw_dump.dump_registered == 1) {
-			ret = -EINVAL;
+			ret = -EEXIST;
 			goto unlock_out;
 		}
 		/* Register Firmware-assisted dump */
-		register_fadump();
+		ret = register_fadump();
 		break;
 	default:
 		ret = -EINVAL;

-- 
2.10.2

On 05/27/2017 09:16 PM, Michal Suchanek wrote:

 - log an error message when registration fails and no error code listed
 in the switch is returned
 - translate the hv error code to posix error code and return it from
 fw_register
 - return the posix error code from fw_register to the process writing
 to sysfs
 - return EEXIST on re-registration
 - return success on deregistration when fadump is not registered
 - return ENODEV when no memory is reserved for fadump

Why do we need this ? Userspace can always read back the fadump
registration status from /sys/kernel/fadump_registered (after echo 1 to
it) to find out whether fadump registration succeeded or not.

 /sys/kernel/fadump_registered

    This is used to display the fadump registration status as well
    as to control (start/stop) the fadump registration.
    0 = fadump is not registered.
    1 = fadump is registered and ready to handle system crash.

-Mahesh.

Mahesh Jagannath Salgaonkar [off-list ref] writes:

On 05/27/2017 09:16 PM, Michal Suchanek wrote:

quoted

 - log an error message when registration fails and no error code listed
 in the switch is returned
 - translate the hv error code to posix error code and return it from
 fw_register
 - return the posix error code from fw_register to the process writing
 to sysfs
 - return EEXIST on re-registration
 - return success on deregistration when fadump is not registered
 - return ENODEV when no memory is reserved for fadump

Why do we need this ?

Because that's how we do error handling.

Userspace can always read back the fadump registration status from
/sys/kernel/fadump_registered (after echo 1 to it) to find out
whether fadump registration succeeded or not.

That's a terrible API.

If we followed that example, open() wouldn't return a value, you'd have
to do another syscall to check if it worked.

I'd appreciate if someone could test this and give me a Tested-by.

cheers

On Saturday 27 May 2017 09:16 PM, Michal Suchanek wrote:

  - log an error message when registration fails and no error code listed
  in the switch is returned
  - translate the hv error code to posix error code and return it from
  fw_register
  - return the posix error code from fw_register to the process writing
  to sysfs
  - return EEXIST on re-registration
  - return success on deregistration when fadump is not registered
  - return ENODEV when no memory is reserved for fadump

Signed-off-by: Michal Suchanek <redacted>

With the below change write to /sys/kernel/fadump_registered returns 
error appropriately.

Tested-by: Hari Bathini <redacted>

On Sat, 2017-05-27 at 15:46:15 UTC, Michal Suchanek wrote:

- log an error message when registration fails and no error code listed
 in the switch is returned
 - translate the hv error code to posix error code and return it from
 fw_register
 - return the posix error code from fw_register to the process writing
 to sysfs
 - return EEXIST on re-registration
 - return success on deregistration when fadump is not registered
 - return ENODEV when no memory is reserved for fadump

Signed-off-by: Michal Suchanek <redacted>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/98b8cd7f75643e0a442d7a4c1cef2c

cheers