[PATCH] powerpc/fadump: re-register firmware-assisted dump if already registered

[PATCH] powerpc/fadump: re-register firmware-assisted dump if already registered

[Hari Bathini]

Firmware-Assisted Dump (FADump) needs to be registered again after any
memory hot add/remove operation to update the crash memory ranges. But
currently, the kernel returns '-EEXIST' if we try to register without
uregistering it first. This could expose the system to racing issues
while unregistering and registering FADump from userspace during udev
events. Spare the userspace of this and let it be taken care of in the
kernel space for a simpler interface.

Since this change, running 'echo 1 > /sys/kernel/fadump_registered'
would result in re-regisering (unregistering and registering) FADump,
if it was already registered.

Signed-off-by: Hari Bathini <hbathini@linux.ibm.com>
---
 arch/powerpc/kernel/fadump.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c
index a711d22..761b28b 100644
--- a/arch/powerpc/kernel/fadump.c
+++ b/arch/powerpc/kernel/fadump.c
@@ -1444,8 +1444,8 @@ static ssize_t fadump_register_store(struct kobject *kobj,
 		break;
 	case 1:
 		if (fw_dump.dump_registered == 1) {
-			ret = -EEXIST;
-			goto unlock_out;
+			/* Un-register Firmware-assisted dump */
+			fadump_unregister_dump(&fdm);
 		}
 		/* Register Firmware-assisted dump */
 		ret = register_fadump();

Re: [PATCH] powerpc/fadump: re-register firmware-assisted dump if already registered

[Petr Tesarik]

On Fri, 14 Sep 2018 19:36:02 +0530
Hari Bathini <hbathini@linux.ibm.com> wrote:

> Firmware-Assisted Dump (FADump) needs to be registered again after any
> memory hot add/remove operation to update the crash memory ranges. But
> currently, the kernel returns '-EEXIST' if we try to register without
> uregistering it first. This could expose the system to racing issues
> while unregistering and registering FADump from userspace during udev
> events. Spare the userspace of this and let it be taken care of in the
> kernel space for a simpler interface.
> 
> Since this change, running 'echo 1 > /sys/kernel/fadump_registered'
> would result in re-regisering (unregistering and registering) FADump,
> if it was already registered.

Great improvement to the API!

Any suggestions what should be done in a client which tries to be
compatible with kernels before this change and after this change?

Petr T

> Signed-off-by: Hari Bathini <hbathini@linux.ibm.com>
> ---
>  arch/powerpc/kernel/fadump.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c
> index a711d22..761b28b 100644
> --- a/arch/powerpc/kernel/fadump.c
> +++ b/arch/powerpc/kernel/fadump.c
> @@ -1444,8 +1444,8 @@ static ssize_t fadump_register_store(struct kobject *kobj,
>  		break;
>  	case 1:
>  		if (fw_dump.dump_registered == 1) {
> -			ret = -EEXIST;
> -			goto unlock_out;
> +			/* Un-register Firmware-assisted dump */
> +			fadump_unregister_dump(&fdm);
>  		}
>  		/* Register Firmware-assisted dump */
>  		ret = register_fadump();
> 

Re: [PATCH] powerpc/fadump: re-register firmware-assisted dump if already registered

[Hari Bathini]

On Friday 14 September 2018 07:58 PM, Petr Tesarik wrote:
> On Fri, 14 Sep 2018 19:36:02 +0530
> Hari Bathini <hbathini@linux.ibm.com> wrote:
>
>> Firmware-Assisted Dump (FADump) needs to be registered again after any
>> memory hot add/remove operation to update the crash memory ranges. But
>> currently, the kernel returns '-EEXIST' if we try to register without
>> uregistering it first. This could expose the system to racing issues
>> while unregistering and registering FADump from userspace during udev
>> events. Spare the userspace of this and let it be taken care of in the
>> kernel space for a simpler interface.
>>
>> Since this change, running 'echo 1 > /sys/kernel/fadump_registered'
>> would result in re-regisering (unregistering and registering) FADump,
>> if it was already registered.
> Great improvement to the API!
>
> Any suggestions what should be done in a client which tries to be
> compatible with kernels before this change and after this change?

If `echo 1 > /sys/kernel/fadump_registered` fails, check for the output
of  `cat /sys/kernel/fadump_registered` and if it is still `1`, that 
indicates
old kernel and we are already registered. Treat it as success if being
registered is what we care about or unregister/register (if re-register
is the intention)..

Hope that helps..

Thanks
Hari

Re: [PATCH] powerpc/fadump: re-register firmware-assisted dump if already registered

[Mahesh Jagannath Salgaonkar]

On 09/14/2018 07:36 PM, Hari Bathini wrote:
> Firmware-Assisted Dump (FADump) needs to be registered again after any
> memory hot add/remove operation to update the crash memory ranges. But
> currently, the kernel returns '-EEXIST' if we try to register without
> uregistering it first. This could expose the system to racing issues
> while unregistering and registering FADump from userspace during udev
> events. Spare the userspace of this and let it be taken care of in the
> kernel space for a simpler interface.
> 
> Since this change, running 'echo 1 > /sys/kernel/fadump_registered'
> would result in re-regisering (unregistering and registering) FADump,
> if it was already registered.
> 
> Signed-off-by: Hari Bathini <hbathini@linux.ibm.com>

Looks good to me.

Acked-by: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>

Thanks,
-Mahesh.

> ---
>  arch/powerpc/kernel/fadump.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c
> index a711d22..761b28b 100644
> --- a/arch/powerpc/kernel/fadump.c
> +++ b/arch/powerpc/kernel/fadump.c
> @@ -1444,8 +1444,8 @@ static ssize_t fadump_register_store(struct kobject *kobj,
>  		break;
>  	case 1:
>  		if (fw_dump.dump_registered == 1) {
> -			ret = -EEXIST;
> -			goto unlock_out;
> +			/* Un-register Firmware-assisted dump */
> +			fadump_unregister_dump(&fdm);
>  		}
>  		/* Register Firmware-assisted dump */
>  		ret = register_fadump();
> 

Re: powerpc/fadump: re-register firmware-assisted dump if already registered

[Michael Ellerman]

On Fri, 2018-09-14 at 14:06:02 UTC, Hari Bathini wrote:
> Firmware-Assisted Dump (FADump) needs to be registered again after any
> memory hot add/remove operation to update the crash memory ranges. But
> currently, the kernel returns '-EEXIST' if we try to register without
> uregistering it first. This could expose the system to racing issues
> while unregistering and registering FADump from userspace during udev
> events. Spare the userspace of this and let it be taken care of in the
> kernel space for a simpler interface.
> 
> Since this change, running 'echo 1 > /sys/kernel/fadump_registered'
> would result in re-regisering (unregistering and registering) FADump,
> if it was already registered.
> 
> Signed-off-by: Hari Bathini <hbathini@linux.ibm.com>
> Acked-by: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/0823c68b054bca9dc321adea829af5

cheers