| Message ID | 20170527154615.29684-1-msuchanek@suse.de (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 98b8cd7f75643e0a442d7a4c1cef2c9d53b7e92b |
| Headers | show |
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 <mahesh@linux.vnet.ibm.com> writes: > 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 ? 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 <msuchanek@suse.de> With the below change write to /sys/kernel/fadump_registered returns error appropriately. Tested-by: Hari Bathini <hbathini@linux.vnet.ibm.com> > --- > 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;
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 <msuchanek@suse.de> Applied to powerpc next, thanks. https://git.kernel.org/powerpc/c/98b8cd7f75643e0a442d7a4c1cef2c cheers
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;
- 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 <msuchanek@suse.de> --- v2: fix return in register_fadump() --- arch/powerpc/kernel/fadump.c | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-)