powerpc/fadump: Add timeout to RTAS busy-wait loops

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.

Track the accumulated delay in a total_wait counter and bail out with -ETIMEDOUT if it reaches RTAS_FADUMP_MAX_WAIT_MS (60 seconds) before firmware signals completion. This follows the bounded busy-wait pattern used in rtas-rtc.c.

Signed-off-by Adriano Vero <[email protected]>

Quoted 6 lines · Adriano Vero

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.

Was there an issue which you encountered? Can you share the details of the same please?

Quoted 7 lines

Track the accumulated delay in a total_wait counter and bail out with -ETIMEDOUT if it reaches RTAS_FADUMP_MAX_WAIT_MS (60 seconds) before firmware signals completion. This follows the bounded busy-wait pattern used in rtas-rtc.c.

Signed-off-by Adriano Vero <[email protected]>

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.

Track the accumulated delay in a total_wait counter and bail out with -ETIMEDOUT if it reaches RTAS_FADUMP_MAX_WAIT_MS (60 seconds)

before
firmware signals completion. This follows the bounded busy-wait pattern used in rtas-rtc.c.

Signed-off-by Adriano Vero <[email protected]>

---
  arch/powerpc/platforms/pseries/rtas-fadump.c | 37 ++++++++++++++------
  arch/powerpc/platforms/pseries/rtas-fadump.h |  6 ++++
  2 files changed, 33 insertions(+), 10 deletions(-)

arch/powerpc/platforms/pseries/rtas-fadump.c +20 -9

index eceb32893..b165f165c 100644
@@ -181,7 +181,7 @@ static u64 rtas_fadump_get_bootmem_min(void)
 
  static int rtas_fadump_register(struct fw_dump *fadump_conf)
  {
-	unsigned int wait_time, fdm_size;
+	unsigned int wait_time, total_wait, fdm_size;
  	int rc, err = -EIO;
 
  	/*

@@ -192,15 +192,20 @@ 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 */
+	total_wait = 0;
  	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)
+		if (wait_time) {
+			if (total_wait >= RTAS_FADUMP_MAX_WAIT_MS) {
+				pr_err("Timed out waiting for firmware to register fadump\n");
+				return -ETIMEDOUT;
+			}
+			total_wait += wait_time;
  			mdelay(wait_time);
-
+		}
  	} while (wait_time);
 
  	switch (rc) {

@@ -234,18 +239,24 @@ static int rtas_fadump_register(struct fw_dump *fadump_conf)
 
  static int rtas_fadump_unregister(struct fw_dump *fadump_conf)
  {
-	unsigned int wait_time;
+	unsigned int wait_time, total_wait;
  	int rc;
 
-	/* TODO: Add upper time limit for the delay */
+	total_wait = 0;
  	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)
+		if (wait_time) {
+			if (total_wait >= RTAS_FADUMP_MAX_WAIT_MS) {
+				pr_err("Timed out waiting for firmware to unregister fadump\n");
+				return -ETIMEDOUT;
+			}
+			total_wait += wait_time;
  			mdelay(wait_time);
+		}
  	} while (wait_time);
 
  	if (rc) {

@@ -259,18 +270,24 @@ static int rtas_fadump_unregister(struct fw_dump *fadump_conf)
 
  static int rtas_fadump_invalidate(struct fw_dump *fadump_conf)
  {
-	unsigned int wait_time;
+	unsigned int wait_time, total_wait;
  	int rc;
 
-	/* TODO: Add upper time limit for the delay */
+	total_wait = 0;
  	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);

arch/powerpc/platforms/pseries/rtas-fadump.c +0 -0

  	if (rc) {

arch/powerpc/platforms/pseries/rtas-fadump.h +6 -0

index c109abf6b..65fdab7b5 100644
@@ -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;

Quoted 1 line

What is the rationale behind choosing a 60-second limit?

The 60-second value was chosen somewhat arbitrarily. Looking at

Quoted 1 line

Would it make sense to introduce a helper function to wrap the

rtas_call, along with handling the wait time and timeout?

Absolutely, I will introduce a helper in v2. The three sites are

Will send v2 shortly from my new email [email protected]

Adriano

On Mon, Apr 13, 2026, 15: 50 Sourabh Jain <sourabhjain@ linux. ibm. com> wrote: > What is the rationale behind choosing a 60-second limit? The 60-second value was chosen somewhat arbitrarily. Looking at rtas_busy_delay_time(), the maximum

Quoted 1 line · On Mon, Apr 13, 2026, 15:50 Sourabh Jain <[email protected]> wrote:

What is the rationale behind choosing a 60-second limit?

The 60-second value was chosen somewhat arbitrarily. Looking at

rtas_busy_delay_time(), the maximum single delay returned is 100000ms

(status 9905). In practice, legitimate busy delays are expected to be

much shorter. I was following the spirit of rtas-rtc.c which uses a

5-second limit for simpler operations; fadump is a heavier firmware

operation, so I went higher. I am open to suggestions on a more

principled value.

Quoted 4 lines

Would it make sense to introduce a helper function to wrap the

rtas_call, along with handling the wait time and timeout?

Absolutely, I will introduce a helper in v2. The three sites are

indeed identical except for the FADUMP_REGISTER/UNREGISTER/INVALIDATE

argument and the struct pointer/size. I will factor out the common

busy-wait loop into a static helper.

Will send v2 shortly from my new email [email protected]

Adriano

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 <[email protected]>

---
  arch/powerpc/platforms/pseries/rtas-fadump.c | 80 ++++++++++++--------
  arch/powerpc/platforms/pseries/rtas-fadump.h |  6 ++
  2 files changed, 53 insertions(+), 33 deletions(-)

arch/powerpc/platforms/pseries/rtas-fadump.c +47 -33

index eceb32893..3bb4ac2ab 100644
@@ -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);

arch/powerpc/platforms/pseries/rtas-fadump.h +0 -0

index c109abf6b..65fdab7b5 100644
@@ -41,6 +41,12 @@
  #define MAX_SECTIONS				10
  #define RTAS_FADUMP_MAX_BOOT_MEM_REGS		7

Quoted 145 lines · Adriano Vero

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 <[email protected]>

---
  arch/powerpc/platforms/pseries/rtas-fadump.c | 80 ++++++++++++--------
  arch/powerpc/platforms/pseries/rtas-fadump.h |  6 ++
  2 files changed, 53 insertions(+), 33 deletions(-)

arch/powerpc/platforms/pseries/rtas-fadump.c +47 -33

index eceb32893..3bb4ac2ab 100644
@@ -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);

arch/powerpc/platforms/pseries/rtas-fadump.h +0 -0

index c109abf6b..65fdab7b5 100644
@@ -41,6 +41,12 @@
  #define MAX_SECTIONS                                10
  #define RTAS_FADUMP_MAX_BOOT_MEM_REGS               7

arch/powerpc/platforms/pseries/rtas-fadump.h line 52

Quoted 10 lines

arch/powerpc/platforms/pseries/rtas-fadump.h +6 -0

 
+/*
+ * 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 <[email protected]>

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

Hi Sourabh,

Thank you for the review. v3 with your Reviewed-by tag has been sent.

Adriano

Quoted 172 lines · Sourabh Jain

Quoted 145 lines · Adriano Vero

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 <[email protected]>

---
   arch/powerpc/platforms/pseries/rtas-fadump.c | 80 ++++++++++++--------
   arch/powerpc/platforms/pseries/rtas-fadump.h |  6 ++
   2 files changed, 53 insertions(+), 33 deletions(-)

arch/powerpc/platforms/pseries/rtas-fadump.c +47 -33

index eceb32893..3bb4ac2ab 100644
@@ -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);

arch/powerpc/platforms/pseries/rtas-fadump.h +0 -0

index c109abf6b..65fdab7b5 100644
@@ -41,6 +41,12 @@
   #define MAX_SECTIONS                                10
   #define RTAS_FADUMP_MAX_BOOT_MEM_REGS               7

arch/powerpc/platforms/pseries/rtas-fadump.h line 52

Quoted 10 lines

arch/powerpc/platforms/pseries/rtas-fadump.h +6 -0

 
+/*
+ * 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 <[email protected]>

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

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 <[email protected]>

Reviewed-by Sourabh Jain <[email protected]>

---
  arch/powerpc/platforms/pseries/rtas-fadump.c | 80 ++++++++++++--------
  arch/powerpc/platforms/pseries/rtas-fadump.h |  6 ++
  2 files changed, 53 insertions(+), 33 deletions(-)

arch/powerpc/platforms/pseries/rtas-fadump.c +47 -33

index eceb3289383e..3bb4ac2ab6cc 100644
@@ -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);

arch/powerpc/platforms/pseries/rtas-fadump.h +6 -0

index c109abf6befd..65fdab7b5b8d 100644
@@ -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;

Hello Adriano,

Quoted 14 lines · Adriano Vero

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.

Quoted 3 lines

Signed-off-by Adriano Vero <[email protected]>

Reviewed-by Sourabh Jain <[email protected]>

New line between tags is unnecessary.

Including a Changelog in subsequent versions of the same patch is helpful for the maintainer and reviewers to understand what has changed in this version compared to the previous version.

For example:
https://lore.kernel.org/all/[email protected]/

Thanks,
Sourabh Jain

Quoted 140 lines

---
  arch/powerpc/platforms/pseries/rtas-fadump.c | 80 ++++++++++++--------
  arch/powerpc/platforms/pseries/rtas-fadump.h |  6 ++
  2 files changed, 53 insertions(+), 33 deletions(-)

arch/powerpc/platforms/pseries/rtas-fadump.c +47 -33

index eceb3289383e..3bb4ac2ab6cc 100644
@@ -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);

arch/powerpc/platforms/pseries/rtas-fadump.h +6 -0

index c109abf6befd..65fdab7b5b8d 100644
@@ -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;

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.

[...]