* Re: [PATCH v2] powerpc/fadump: Add timeout to RTAS busy-wait loops [not found] ` <20260419065039.23495-1-adri.vero.dev@gmail.com> @ 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; 6+ 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] 6+ messages in thread
* Re: [PATCH v2] powerpc/fadump: Add timeout to RTAS busy-wait loops 2026-05-06 19:21 ` [PATCH v2] powerpc/fadump: Add timeout to RTAS busy-wait loops Sourabh Jain @ 2026-05-07 2:52 ` Adriano Vero 2026-05-07 4:07 ` Sourabh Jain 0 siblings, 1 reply; 6+ 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] 6+ 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; 6+ 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] 6+ messages in thread
* [PATCH v3] powerpc/fadump: Add timeout to RTAS busy-wait loops [not found] ` <20260419065039.23495-1-adri.vero.dev@gmail.com> 2026-05-06 19:21 ` [PATCH v2] powerpc/fadump: Add timeout to RTAS busy-wait loops Sourabh Jain @ 2026-05-06 22:20 ` Adriano Vero 2026-05-07 4:15 ` Sourabh Jain 1 sibling, 1 reply; 6+ 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] 6+ 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; 6+ 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] 6+ 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; 6+ 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] 6+ messages in thread
end of thread, other threads:[~2026-05-07 8:08 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20260406061542.22354-1-litaliano00.contact@gmail.com>
[not found] ` <20260419065039.23495-1-adri.vero.dev@gmail.com>
2026-05-06 19:21 ` [PATCH v2] powerpc/fadump: Add timeout to RTAS busy-wait loops 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