public inbox for linuxppc-dev@ozlabs.org 
 help / color / mirror / Atom feed

* [PATCH] powerpc/fadump: Add timeout to RTAS busy-wait loops
@ 2026-04-06  6:15 Adriano Vero
  2026-04-07  4:06 ` Ritesh Harjani
                   ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: Adriano Vero @ 2026-04-06  6:15 UTC (permalink / raw)
  To: maddy, mpe; +Cc: npiggin, chleroy, linuxppc-dev, linux-kernel, 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.

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 <litaliano00.contact@gmail•com>
---
 arch/powerpc/platforms/pseries/rtas-fadump.c | 37 ++++++++++++++------
 arch/powerpc/platforms/pseries/rtas-fadump.h |  6 ++++
 2 files changed, 33 insertions(+), 10 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/rtas-fadump.c b/arch/powerpc/platforms/pseries/rtas-fadump.c
index eceb32893..b165f165c 100644
--- a/arch/powerpc/platforms/pseries/rtas-fadump.c
+++ b/arch/powerpc/platforms/pseries/rtas-fadump.c
@@ -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);
-		if (wait_time)
+		if (wait_time) {
+			if (total_wait >= RTAS_FADUMP_MAX_WAIT_MS) {
+				pr_err("Timed out waiting for firmware to invalidate fadump\n");
+				return -ETIMEDOUT;
+			}
+			total_wait += wait_time;
 			mdelay(wait_time);
+		}
 	} while (wait_time);
 
 	if (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;
-- 
2.53.0



^ permalink raw reply related	[flat|nested] 13+ messages in thread

• * Re: [PATCH] powerpc/fadump: Add timeout to RTAS busy-wait loops
    2026-04-06  6:15 [PATCH] powerpc/fadump: Add timeout to RTAS busy-wait loops Adriano Vero
  @ 2026-04-07  4:06 ` Ritesh Harjani
    2026-04-07  6:28   ` litaliano00
    2026-04-13 13:50 ` Sourabh Jain
    2026-04-19  6:50 ` [PATCH v2] " Adriano Vero
    2 siblings, 1 reply; 13+ messages in thread
  From: Ritesh Harjani @ 2026-04-07  4:06 UTC (permalink / raw)
    To: Adriano Vero, maddy, mpe
    Cc: npiggin, chleroy, linuxppc-dev, linux-kernel, Adriano Vero
  
  Adriano Vero <litaliano00.contact@gmail•com> writes:
  
  > 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?
  
  
  >
  > 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 <litaliano00.contact@gmail•com>
  
  
  ^ permalink raw reply	[flat|nested] 13+ messages in thread

• • * Re: [PATCH] powerpc/fadump: Add timeout to RTAS busy-wait loops
      2026-04-07  4:06 ` Ritesh Harjani
    @ 2026-04-07  6:28   ` litaliano00
      0 siblings, 0 replies; 13+ messages in thread
    From: litaliano00 @ 2026-04-07  6:28 UTC (permalink / raw)
      To: Ritesh Harjani; +Cc: maddy, mpe, npiggin, chleroy, linuxppc-dev, linux-kernel
    
    [-- Attachment #1: Type: text/plain, Size: 1834 bytes --]
    
    Hi Ritesh,
    
    No, I haven't encountered a specific hang on live hardware in production.
    
    This was a proactive fix that originated from an audit of the RTAS call
    sites. I noticed the explicit `/* TODO: Add upper time limit for the delay
    */` comments in rtas-fadump.c that had not yet been implemented.
    
    When cross-referencing this with other parts of the pSeries code (such as
    the bounded busy-wait pattern used in rtas-rtc.c), it seemed that adding a
    timeout is the standard defensive programming approach for these hypervisor
    calls.
    
    Since these fadump registration and teardown paths happen during critical
    boot and state transition phases, a misbehaving hypervisor or firmware
    anomaly could lead to a hard-to-debug, silent system stall. I implemented
    the 60-second timeout to resolve the pending TODOs and ensure the kernel
    remains resilient in those specific edge cases.
    
    Thanks,
    Adriano
    
    On Tue, Apr 7, 2026 at 6:07 AM Ritesh Harjani <ritesh.list@gmail•com> wrote:
    
    > Adriano Vero <litaliano00.contact@gmail•com> writes:
    >
    > > 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?
    >
    >
    > >
    > > 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 <litaliano00.contact@gmail•com>
    >
    
    [-- Attachment #2: Type: text/html, Size: 2390 bytes --]
    
    ^ permalink raw reply	[flat|nested] 13+ messages in thread

• * Re: [PATCH] powerpc/fadump: Add timeout to RTAS busy-wait loops
    2026-04-06  6:15 [PATCH] powerpc/fadump: Add timeout to RTAS busy-wait loops Adriano Vero
    2026-04-07  4:06 ` Ritesh Harjani
  @ 2026-04-13 13:50 ` Sourabh Jain
    2026-04-13 21:37   ` litaliano00
    2026-04-19  6:50 ` [PATCH v2] " Adriano Vero
    2 siblings, 1 reply; 13+ messages in thread
  From: Sourabh Jain @ 2026-04-13 13:50 UTC (permalink / raw)
    To: Adriano Vero, maddy, mpe; +Cc: npiggin, chleroy, linuxppc-dev, linux-kernel
  
  Hello Adriano
  
  On 06/04/26 11:45, 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.
  
  I agree that it is a good idea to avoid calling rtas_call for
  fadump operations indefinitely. However, so far I have not come
  across a case where the kernel gets stuck during fadump
  registration, unregistration, or invalidation due to phyp/RTAS
  continuously returning a wait time on an LPAR.
  
  That said, since fadump support has recently been extended to
  QEMU, this change might possibly prove useful in that environment.
  
  >
  > Track the accumulated delay in a total_wait counter and bail out with
  > -ETIMEDOUT if it reaches RTAS_FADUMP_MAX_WAIT_MS (60 seconds)
  
  What is the rationale behind choosing a 60-second limit?
  
  
  > before
  > firmware signals completion. This follows the bounded busy-wait pattern
  > used in rtas-rtc.c.
  >
  > Signed-off-by: Adriano Vero <litaliano00.contact@gmail•com>
  > ---
  >   arch/powerpc/platforms/pseries/rtas-fadump.c | 37 ++++++++++++++------
  >   arch/powerpc/platforms/pseries/rtas-fadump.h |  6 ++++
  >   2 files changed, 33 insertions(+), 10 deletions(-)
  >
  > diff --git a/arch/powerpc/platforms/pseries/rtas-fadump.c b/arch/powerpc/platforms/pseries/rtas-fadump.c
  > index eceb32893..b165f165c 100644
  > --- a/arch/powerpc/platforms/pseries/rtas-fadump.c
  > +++ b/arch/powerpc/platforms/pseries/rtas-fadump.c
  > @@ -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);
  > -		if (wait_time)
  > +		if (wait_time) {
  > +			if (total_wait >= RTAS_FADUMP_MAX_WAIT_MS) {
  > +				pr_err("Timed out waiting for firmware to invalidate fadump\n");
  > +				return -ETIMEDOUT;
  > +			}
  > +			total_wait += wait_time;
  >   			mdelay(wait_time);
  > +		}
  >   	} while (wait_time);
  
  This do...while loop is almost identical in all three places.
  Would it make sense to introduce a helper function to wrap the
  rtas_call, along with handling the wait time and timeout?
  
  - Sourabh Jain
  
  >   	if (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;
  
  
  
  
  ^ permalink raw reply	[flat|nested] 13+ messages in thread

• • * Re: [PATCH] powerpc/fadump: Add timeout to RTAS busy-wait loops
      2026-04-13 13:50 ` Sourabh Jain
    @ 2026-04-13 21:37   ` litaliano00
      2026-04-14 10:29     ` Sourabh Jain
      0 siblings, 1 reply; 13+ messages in thread
    From: litaliano00 @ 2026-04-13 21:37 UTC (permalink / raw)
      To: Sourabh Jain; +Cc: maddy, mpe, npiggin, chleroy, linuxppc-dev, linux-kernel
    
    [-- Attachment #1: Type: text/plain, Size: 1025 bytes --]
    
    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 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.
    
    
    > > 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 adri.vero.dev@gmail•com
    
    
    > Adriano
    
    [-- Attachment #2: Type: text/html, Size: 3699 bytes --]
    
    ^ permalink raw reply	[flat|nested] 13+ messages in thread

  • • * Re: [PATCH] powerpc/fadump: Add timeout to RTAS busy-wait loops
        2026-04-13 21:37   ` litaliano00
      @ 2026-04-14 10:29     ` Sourabh Jain
        0 siblings, 0 replies; 13+ messages in thread
      From: Sourabh Jain @ 2026-04-14 10:29 UTC (permalink / raw)
        To: litaliano00; +Cc: maddy, mpe, npiggin, chleroy, linuxppc-dev, linux-kernel
      
      
      
      On 14/04/26 03:07, litaliano00 wrote:
      > 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
      > 
      > 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 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.
      >
      
      
      phyp/RTAS is a black box to me, so it is hard to conclude
      what happens during fadump register/unregister/invalidate.
      However, based on QEMU's fadump implementation, all three
      operations do not seem very complex.
      
      That said, I am OK with a 60 sec timeout.
      
      I would also recommend adding pr_debug logs when the wait
      time is hit. It might help debug cases where the kernel
      reaches the 60 sec limit especially during boot.
      
      - Sourabh Jain
      
      >
      >     > 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 adri.vero.dev@gmail•com
      >
      >
      >     Adriano
      >
      >
      
      
      
      ^ permalink raw reply	[flat|nested] 13+ messages in thread

• * [PATCH v2] powerpc/fadump: Add timeout to RTAS busy-wait loops
    2026-04-06  6:15 [PATCH] powerpc/fadump: Add timeout to RTAS busy-wait loops Adriano Vero
    2026-04-07  4:06 ` Ritesh Harjani
    2026-04-13 13:50 ` Sourabh Jain
  @ 2026-04-19  6:50 ` Adriano Vero
    2026-05-06 19:21   ` Sourabh Jain
    2026-05-06 22:20   ` [PATCH v3] " Adriano Vero
    2 siblings, 2 replies; 13+ messages in thread
  From: Adriano Vero @ 2026-04-19  6:50 UTC (permalink / raw)
    To: maddy, mpe, sourabhjain
    Cc: npiggin, chleroy, linuxppc-dev, linux-kernel, 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 <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;
  -- 
  2.53.0
  
  
  
  ^ permalink raw reply related	[flat|nested] 13+ messages in thread

• • * Re: [PATCH v2] powerpc/fadump: Add timeout to RTAS busy-wait loops
      2026-04-19  6:50 ` [PATCH v2] " Adriano Vero
    @ 2026-05-06 19:21   ` Sourabh Jain
      2026-05-07  2:52     ` Adriano Vero
      2026-05-06 22:20   ` [PATCH v3] " Adriano Vero
      1 sibling, 1 reply; 13+ messages in thread
    From: Sourabh Jain @ 2026-05-06 19:21 UTC (permalink / raw)
      To: Adriano Vero, maddy, mpe; +Cc: npiggin, chleroy, linuxppc-dev, linux-kernel
    
    
    
    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
    
    
    ^ permalink raw reply	[flat|nested] 13+ messages in thread

  • • * Re: [PATCH v2] powerpc/fadump: Add timeout to RTAS busy-wait loops
        2026-05-06 19:21   ` Sourabh Jain
      @ 2026-05-07  2:52     ` Adriano Vero
        2026-05-07  4:07       ` Sourabh Jain
        0 siblings, 1 reply; 13+ messages in thread
      From: Adriano Vero @ 2026-05-07  2:52 UTC (permalink / raw)
        To: Sourabh Jain; +Cc: maddy, mpe, npiggin, chleroy, linuxppc-dev, linux-kernel
      
      Hi Sourabh,
      
      Thank you for the review. v3 with your Reviewed-by tag has been sent.
      
      Adriano
      
      
      On Wed, May 6, 2026 at 9:22 PM Sourabh Jain <sourabhjain@linux•ibm.com> 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 <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
      
      
      ^ permalink raw reply	[flat|nested] 13+ messages in thread

    • • * Re: [PATCH v2] powerpc/fadump: Add timeout to RTAS busy-wait loops
          2026-05-07  2:52     ` Adriano Vero
        @ 2026-05-07  4:07       ` Sourabh Jain
          0 siblings, 0 replies; 13+ messages in thread
        From: Sourabh Jain @ 2026-05-07  4:07 UTC (permalink / raw)
          To: Adriano Vero; +Cc: maddy, mpe, npiggin, chleroy, linuxppc-dev, linux-kernel
        
        
        
        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 <sourabhjain@linux•ibm.com> 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 <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
        
        
        
        ^ permalink raw reply	[flat|nested] 13+ messages in thread

  • * [PATCH v3] powerpc/fadump: Add timeout to RTAS busy-wait loops
      2026-04-19  6:50 ` [PATCH v2] " Adriano Vero
      2026-05-06 19:21   ` Sourabh Jain
    @ 2026-05-06 22:20   ` Adriano Vero
      2026-05-07  4:15     ` Sourabh Jain
      1 sibling, 1 reply; 13+ messages in thread
    From: Adriano Vero @ 2026-05-06 22:20 UTC (permalink / raw)
      To: maddy, mpe, sourabhjain
      Cc: npiggin, chleroy, linuxppc-dev, linux-kernel, 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 <adri.vero.dev@gmail•com>
    
    Reviewed-by: Sourabh Jain <sourabhjain@linux•ibm.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 eceb3289383e..3bb4ac2ab6cc 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 c109abf6befd..65fdab7b5b8d 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;
    -- 
    2.54.0
    
    
    
    ^ permalink raw reply related	[flat|nested] 13+ messages in thread

  • • * Re: [PATCH v3] powerpc/fadump: Add timeout to RTAS busy-wait loops
        2026-05-06 22:20   ` [PATCH v3] " Adriano Vero
      @ 2026-05-07  4:15     ` Sourabh Jain
        2026-05-07  8:08       ` Adriano Vero
        0 siblings, 1 reply; 13+ messages in thread
      From: Sourabh Jain @ 2026-05-07  4:15 UTC (permalink / raw)
        To: Adriano Vero, maddy, mpe; +Cc: npiggin, chleroy, linuxppc-dev, linux-kernel
      
      Hello Adriano,
      
      On 07/05/26 03:50, 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>
      >
      > Reviewed-by: Sourabh Jain <sourabhjain@linux•ibm.com>
      
      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/20260407124349.1698552-1-sourabhjain@linux.ibm.com/
      
      Thanks,
      Sourabh Jain
      
      > ---
      >   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 eceb3289383e..3bb4ac2ab6cc 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 c109abf6befd..65fdab7b5b8d 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;
      
      
      
      ^ permalink raw reply	[flat|nested] 13+ messages in thread

    • • * Re: [PATCH v3] powerpc/fadump: Add timeout to RTAS busy-wait loops
          2026-05-07  4:15     ` Sourabh Jain
        @ 2026-05-07  8:08       ` Adriano Vero
          0 siblings, 0 replies; 13+ messages in thread
        From: Adriano Vero @ 2026-05-07  8:08 UTC (permalink / raw)
          To: Sourabh Jain; +Cc: maddy, mpe, npiggin, chleroy, linuxppc-dev, linux-kernel
        
        Hi Sourabh,
        
        Understood, I'll keep that in mind for future patches. Thanks for
        the guidance.
        
        Adriano
        
        On Thu, May 7, 2026 at 6:16 AM Sourabh Jain <sourabhjain@linux•ibm.com> wrote:
        >
        > Hello Adriano,
        >
        > On 07/05/26 03:50, 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>
        > >
        > > Reviewed-by: Sourabh Jain <sourabhjain@linux•ibm.com>
        >
        > 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/20260407124349.1698552-1-sourabhjain@linux.ibm.com/
        >
        > Thanks,
        > Sourabh Jain
        >
        > > ---
        > >   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 eceb3289383e..3bb4ac2ab6cc 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 c109abf6befd..65fdab7b5b8d 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;
        >
        
        
        ^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2026-05-07  8:08 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-04-06  6:15 [PATCH] powerpc/fadump: Add timeout to RTAS busy-wait loops Adriano Vero
2026-04-07  4:06 ` Ritesh Harjani
2026-04-07  6:28   ` litaliano00
2026-04-13 13:50 ` Sourabh Jain
2026-04-13 21:37   ` litaliano00
2026-04-14 10:29     ` Sourabh Jain
2026-04-19  6:50 ` [PATCH v2] " Adriano Vero
2026-05-06 19:21   ` Sourabh Jain
2026-05-07  2:52     ` Adriano Vero
2026-05-07  4:07       ` Sourabh Jain
2026-05-06 22:20   ` [PATCH v3] " Adriano Vero
2026-05-07  4:15     ` Sourabh Jain
2026-05-07  8:08       ` Adriano Vero

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox