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

[Sourabh Jain <sourabhjain@linux.ibm.com>]

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