On 07/05/26 08:22, Adriano Vero wrote:
Hi Sourabh,

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

Thanks, but a new version just to include tags is not really needed.
Maintainers generally take care of including them.

- Sourabh Jain


Adriano


On Wed, May 6, 2026 at 9:22 PM Sourabh Jain <[email protected]> wrote:


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 <[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(-)

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 <[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


Reply via email to