Re: [PATCH v2] powerpc/fadump: Add timeout to RTAS busy-wait loops

[Sourabh Jain <sourabhjain@linux.ibm.com>]

On 19/04/26 12:20, Adriano Vero wrote:
> The ibm,configure-kernel-dump RTAS call sites in
> rtas_fadump_register(), rtas_fadump_unregister(), and
> rtas_fadump_invalidate() polled indefinitely while firmware returned
> a busy status. A misbehaving or hung firmware could stall these paths
> forever, blocking fadump registration at boot or preventing clean
> teardown.
>
> Introduce rtas_fadump_call(), a helper that wraps the common
> busy-wait pattern shared by all three sites. The helper accumulates
> the total delay and returns -ETIMEDOUT if firmware keeps returning a
> busy status beyond RTAS_FADUMP_MAX_WAIT_MS (60 seconds). A pr_debug()
> message is emitted on each busy iteration to aid diagnosis when the
> timeout is hit.
>
> Signed-off-by: Adriano Vero <adri.vero.dev@gmail.com>
> ---
>   arch/powerpc/platforms/pseries/rtas-fadump.c | 80 ++++++++++++--------
>   arch/powerpc/platforms/pseries/rtas-fadump.h |  6 ++
>   2 files changed, 53 insertions(+), 33 deletions(-)
>
> diff --git a/arch/powerpc/platforms/pseries/rtas-fadump.c b/arch/powerpc/platforms/pseries/rtas-fadump.c
> index eceb32893..3bb4ac2ab 100644
> --- a/arch/powerpc/platforms/pseries/rtas-fadump.c
> +++ b/arch/powerpc/platforms/pseries/rtas-fadump.c
> @@ -179,9 +179,42 @@ static u64 rtas_fadump_get_bootmem_min(void)
>   	return RTAS_FADUMP_MIN_BOOT_MEM;
>   }
>   
> +/*
> + * Helper to make an ibm,configure-kernel-dump RTAS call with a bounded
> + * busy-wait loop. Returns the RTAS return code on completion, or
> + * -ETIMEDOUT if firmware keeps returning a busy status beyond
> + * RTAS_FADUMP_MAX_WAIT_MS milliseconds.
> + */
> +static int rtas_fadump_call(struct fw_dump *fadump_conf, int operation,
> +			    void *fdm_ptr, unsigned int fdm_size,
> +			    const char *op_name)
> +{
> +	unsigned int wait_time, total_wait = 0;
> +	int rc;
> +
> +	do {
> +		rc = rtas_call(fadump_conf->ibm_configure_kernel_dump, 3, 1,
> +			       NULL, operation, fdm_ptr, fdm_size);
> +		wait_time = rtas_busy_delay_time(rc);
> +		if (wait_time) {
> +			pr_debug("Firmware busy during fadump %s, waiting %ums (total %ums)\n",
> +				 op_name, wait_time, total_wait);
> +			if (total_wait >= RTAS_FADUMP_MAX_WAIT_MS) {
> +				pr_err("Timed out waiting for firmware to complete fadump %s\n",
> +				       op_name);
> +				return -ETIMEDOUT;
> +			}
> +			total_wait += wait_time;
> +			mdelay(wait_time);
> +		}
> +	} while (wait_time);
> +
> +	return rc;
> +}
> +
>   static int rtas_fadump_register(struct fw_dump *fadump_conf)
>   {
> -	unsigned int wait_time, fdm_size;
> +	unsigned int fdm_size;
>   	int rc, err = -EIO;
>   
>   	/*
> @@ -192,16 +225,10 @@ static int rtas_fadump_register(struct fw_dump *fadump_conf)
>   	fdm_size = sizeof(struct rtas_fadump_section_header);
>   	fdm_size += be16_to_cpu(fdm.header.dump_num_sections) * sizeof(struct rtas_fadump_section);
>   
> -	/* TODO: Add upper time limit for the delay */
> -	do {
> -		rc =  rtas_call(fadump_conf->ibm_configure_kernel_dump, 3, 1,
> -				NULL, FADUMP_REGISTER, &fdm, fdm_size);
> -
> -		wait_time = rtas_busy_delay_time(rc);
> -		if (wait_time)
> -			mdelay(wait_time);
> -
> -	} while (wait_time);
> +	rc = rtas_fadump_call(fadump_conf, FADUMP_REGISTER, &fdm, fdm_size,
> +			      "register");
> +	if (rc == -ETIMEDOUT)
> +		return -ETIMEDOUT;
>   
>   	switch (rc) {
>   	case 0:
> @@ -234,19 +261,12 @@ static int rtas_fadump_register(struct fw_dump *fadump_conf)
>   
>   static int rtas_fadump_unregister(struct fw_dump *fadump_conf)
>   {
> -	unsigned int wait_time;
>   	int rc;
>   
> -	/* TODO: Add upper time limit for the delay */
> -	do {
> -		rc =  rtas_call(fadump_conf->ibm_configure_kernel_dump, 3, 1,
> -				NULL, FADUMP_UNREGISTER, &fdm,
> -				sizeof(struct rtas_fadump_mem_struct));
> -
> -		wait_time = rtas_busy_delay_time(rc);
> -		if (wait_time)
> -			mdelay(wait_time);
> -	} while (wait_time);
> +	rc = rtas_fadump_call(fadump_conf, FADUMP_UNREGISTER, &fdm,
> +			      sizeof(struct rtas_fadump_mem_struct), "unregister");
> +	if (rc == -ETIMEDOUT)
> +		return -ETIMEDOUT;
>   
>   	if (rc) {
>   		pr_err("Failed to un-register - unexpected error(%d).\n", rc);
> @@ -259,19 +279,13 @@ static int rtas_fadump_unregister(struct fw_dump *fadump_conf)
>   
>   static int rtas_fadump_invalidate(struct fw_dump *fadump_conf)
>   {
> -	unsigned int wait_time;
>   	int rc;
>   
> -	/* TODO: Add upper time limit for the delay */
> -	do {
> -		rc =  rtas_call(fadump_conf->ibm_configure_kernel_dump, 3, 1,
> -				NULL, FADUMP_INVALIDATE, fdm_active,
> -				sizeof(struct rtas_fadump_mem_struct));
> -
> -		wait_time = rtas_busy_delay_time(rc);
> -		if (wait_time)
> -			mdelay(wait_time);
> -	} while (wait_time);
> +	rc = rtas_fadump_call(fadump_conf, FADUMP_INVALIDATE,
> +			      (void *)fdm_active,
> +			      sizeof(struct rtas_fadump_mem_struct), "invalidate");
> +	if (rc == -ETIMEDOUT)
> +		return -ETIMEDOUT;
>   
>   	if (rc) {
>   		pr_err("Failed to invalidate - unexpected error (%d).\n", rc);
> diff --git a/arch/powerpc/platforms/pseries/rtas-fadump.h b/arch/powerpc/platforms/pseries/rtas-fadump.h
> index c109abf6b..65fdab7b5 100644
> --- a/arch/powerpc/platforms/pseries/rtas-fadump.h
> +++ b/arch/powerpc/platforms/pseries/rtas-fadump.h
> @@ -41,6 +41,12 @@
>   #define MAX_SECTIONS				10
>   #define RTAS_FADUMP_MAX_BOOT_MEM_REGS		7
>   
> +/*
> + * Maximum time to wait for firmware to respond to an
> + * ibm,configure-kernel-dump RTAS call before giving up.
> + */
> +#define RTAS_FADUMP_MAX_WAIT_MS			60000U
> +
>   /* Kernel Dump section info */
>   struct rtas_fadump_section {
>   	__be32	request_flag;

Changes look good to me.

Feel free to add:
Reviewed-by: Sourabh Jain <sourabhjain@linux.ibm.com>

While testing this patch I realized that fadump prints below message 
when registered is successfully
<rtas/opal> fadump: Registration is successful

But there is no message when fadump is unregistered successfully. I 
think it is good to
log that event.  I will send a separate patch for this.

- Sourabh Jain