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

[Adriano Vero]

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@xxxxxxxxxxxxx> 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@xxxxxxxxx>
> >
> > Reviewed-by: Sourabh Jain <sourabhjain@xxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxx/
>
> 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;
>