Re: [BISECTED - impi related]: acpi_power_meter: power*_average sysfs read hangs, mutex deadlock in hwmon_attr_show since v6.18.y

[Corey Minyard <corey@minyard.net>]

On Fri, Feb 06, 2026 at 01:08:56PM +0100, Rafael J. Wysocki wrote:
> On Thu, Feb 5, 2026 at 11:34 PM Guenter Roeck <linux@roeck-us.net> wrote:
> >
> > On Thu, Feb 05, 2026 at 08:04:12PM +0100, Rafael J. Wysocki wrote:
> > > Cc: Corey
> > >
> > > On Thu, Feb 5, 2026 at 6:51 PM Guenter Roeck <linux@roeck-us.net> wrote:
> > > >
> > > > On Thu, Feb 05, 2026 at 08:25:57AM +0100, Igor Raits wrote:
> > > > > On Wed, Feb 4, 2026 at 11:49 PM Guenter Roeck <linux@roeck-us.net> wrote:
> > > > > >
> > > > > > On 2/4/26 11:54, Igor Raits wrote:
> > > > > > > I have written a patch with the help of AI and it fixes the problem. Attached.
> > > > > > >
> > > > > >
> > > > > > "No MIME, no links, no compression, no attachments.  Just plain text"
> > > > >
> > > > > Sorry for that, I had assumed that attaching the file would make it in-line.
> > > > >
> > > > > > ... which means I can not provide inline feedback, which is the whole
> > > > > > point of the above.
> > > > > >
> > > > > > Your patch crosses subsystems, so it will need to be split in two
> > > > > > (assuming the ACPI side is even needed). Also, references to iDRAC
> > > > > > in common code seem inappropriate.
> > > > >
> > > > > Yes, this I believe was the essential part (it was the last piece in
> > > > > my testing which fixed the hanging):
> > > > >
> > > >
> > > > Then I'll need to ask differently: What happens if you drop the IPMI code,
> > > > and just keep the wait_for_completion -> wait_for_completion_timeout
> > > > change ? Would that be sufficient to solve the problem ?
> > >
> > > I'd rather say "Would that be sufficient to make the symptoms go
> > > away?" as it most likely papers over the real problem.
> > >
> >
> > Good point. Worse, it may result in UAF or memory leaks.
> >
> > > > Either case, the need for this change suggests that the ipmi change
> > > > may not be complete, since it should send a completion with an error.
> > >
> > > I think that reverting commit bc3a9d217755 ("ipmi:si: Gracefully
> > > handle if the BMC is non-functional") should also be considered as a
> > > possible way forward because it clearly did not improve things as
> > > expected, at least in this particular case.
> > >
> >
> > I tend to agree. I ran a number of AI code reviews over the patch, and
> > each time it finds new (and different) problems. The fact that the acpi
> > patch is still needed even after applying the ipmi changes suggests that
> > something is still missing in the ipmi code.
> >
> > > It evidently did something that confuses things quite a bit.  Either
> > > it is returning IPMI_BUS_ERR instead of IPMI_ERR_UNSPECIFIED, or it is
> > > the "hosed" state and refusing to accept messages.
> > >
> >
> > More than that. My latest AI results are below, just for reference
> > (using Gemini 3 with Chris Mason's debug prompts). The prompt I used
> > for this run is:
> 
> Well, I guess it's time to send a revert patch then.

Thanks for the CC.

Let's fix it right in the IPMI driver.

> 
> > "
> > The top commit in the linux/ directory results in hung tasks if the BMC
> > stops responding. Using @review-prompts/kernel/debugging.md analyze the
> > patch, identify the reason for the hung task problem, suggest and implement
> > a fix. Note that there may be more than one problem in the patch, so analyze
> > the complete patch and do not stop after fiding the first regression.
> > "
> >
> > I think that catches most of the problem, but not all of it.
> >
> > Guenter
> >
> > ---
> >
> > Summary of crash or warning:
> > Hung task detected in ipmi_si driver when BMC becomes non-functional.
> > Processes waiting for IPMI responses (e.g. ipmitool, monitoring agents) enter D state and never recover.
> >
> > Kernel version if available:
> > Top of tree (commit bc3a9d217755f65c137f145600f23bf1d6c31ea9)
> >
> > Machine type if available:
> > Generic Server with BMC
> >
> > Cleaned up copy of oops or stack trace:
> > [  120.123456] INFO: task ipmitool:1234 blocked for more than 120 seconds.
> > [  120.123457]       Not tainted 6.14.0-rc1 #1
> > [  120.123458] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
> > [  120.123459] task:ipmitool        state:D stack:    0 pid: 1234 ppid:  100 flags:0x00000000
> > [  120.123460] Call Trace:
> > [  120.123461]  <TASK>
> > [  120.123462]  __schedule+0x123/0x456
> > [  120.123463]  schedule+0x45/0x78
> > [  120.123464]  schedule_timeout+0x9a/0xbc
> > [  120.123465]  wait_for_completion+0xde/0xf0
> > [  120.123466]  ipmi_request_settime+0x123/0x145
> > [  120.123467]  ...
> > [  120.123468]  </TASK>
> >
> > Any other kernel messages you found relevant:
> > N/A
> >
> > Explanation of the problem:
> > 1. Hung Task:
> > The patch "ipmi:si: Gracefully handle if the BMC is non-functional" introduces a new state `SI_HOSED` to handle BMC failures. When the driver detects that the BMC is not responding, it transitions to `SI_HOSED` and fails the currently processing message (`curr_msg`). However, if a new message is queued via `sender()` (populating `waiting_msg`) during a recovery probe (state `SI_GETTING_FLAGS`), and that probe subsequently fails, the state machine transitions back to `SI_HOSED`. In this transition, the driver checks and fails `curr_msg`, but it neglects to check or fail `waiting_msg`. As a result, the `waiting_msg` remains in the queue indefinitely, causing the waiting process to hang.
> 
> That's quite convincing and it would explain the observed symptoms.

Yes, and it's a fairly easy fix, I think.  The waiting message just
needs to be returned in that case.  The following patch should do it:

diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
index 5459ffdde8dc..ff159b1162b9 100644
--- a/drivers/char/ipmi/ipmi_si_intf.c
+++ b/drivers/char/ipmi/ipmi_si_intf.c
@@ -809,6 +809,12 @@ static enum si_sm_result smi_event_handler(struct smi_info *smi_info,
                         */
                        return_hosed_msg(smi_info, IPMI_BUS_ERR);
                }
+               if (smi_info->waiting_msg != NULL) {
+                       /* Also handle if there was a message waiting. */
+                       smi_info->curr_msg = smi_info->waiting_msg;
+                       smi_info->waiting_msg = NULL;
+                       return_hosed_msg(smi_info, IPMI_BUS_ERR);
+               }
                smi_mod_timer(smi_info, jiffies + SI_TIMEOUT_HOSED);
                goto out;
        }


Thanks,

-corey

> 
> > 2. Excessive Polling (Timer Overwrite):
> > In `smi_timeout()`, the timer is unconditionally reset to a short timeout (e.g., 10ms) at the end of the function, unless the state machine is IDLE. When the state machine returns `SI_SM_HOSED`, `smi_event_handler()` correctly sets the timer to a long backoff (1 second). However, `smi_timeout()` subsequently overwrites this with the short timeout. This causes the driver to poll the hosed BMC every 10ms instead of backing off for 1 second, resulting in unnecessary CPU overhead.
> 
> Well, that's not nice, but it doesn't seem to be related to the
> observed symptoms.
> 
> > Functions, snippets and call traces of code related to the problem:
> >
> > drivers/char/ipmi/ipmi_si_intf.c: smi_event_handler()
> >
> >         } else if (si_sm_result == SI_SM_HOSED) {
> >                 /* ... */
> >                 smi_info->si_state = SI_HOSED;
> >                 if (smi_info->curr_msg != NULL) {
> >                         return_hosed_msg(smi_info, IPMI_BUS_ERR);
> >                 }
> >                 /* BUG 1: waiting_msg is NOT checked here! */
> >
> >                 smi_mod_timer(smi_info, jiffies + SI_TIMEOUT_HOSED); /* Sets 1s timeout */
> >                 goto out;
> >         }
> >
> > drivers/char/ipmi/ipmi_si_intf.c: smi_timeout()
> >
> >         smi_result = smi_event_handler(smi_info, time_diff);
> >         /* ... */
> >         if (smi_result != SI_SM_IDLE)
> >                 smi_mod_timer(smi_info, timeout); /* BUG 2: Overwrites 1s timeout with ~10ms */
> >
> > List of potential commits related to the problem:
> > bc3a9d217755f65c137f145600f23bf1d6c31ea9 ipmi:si: Gracefully handle if the BMC is non-functional
> >
> > Suggested fix:
> > 1. When entering `SI_HOSED` state in `smi_event_handler`, check for and fail any pending `waiting_msg` in addition to `curr_msg`.
> > 2. In `smi_timeout`, do not reset the timer if the state is `SI_HOSED`, preserving the backoff timeout set by `smi_event_handler`.
> >
> > ---
> > Suggested diffs from that run:
> >
> > diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
> > index 1c65275906b4..023b0e819f91 100644
> > --- a/drivers/char/ipmi/ipmi_si_intf.c
> > +++ b/drivers/char/ipmi/ipmi_si_intf.c
> > @@ -810,6 +810,11 @@ static enum si_sm_result smi_event_handler(struct smi_info *smi_info,
> >                          */
> >                         return_hosed_msg(smi_info, IPMI_BUS_ERR);
> >                 }
> > +               if (smi_info->waiting_msg != NULL) {
> > +                       smi_info->curr_msg = smi_info->waiting_msg;
> > +                       smi_info->waiting_msg = NULL;
> > +                       return_hosed_msg(smi_info, IPMI_BUS_ERR);
> > +               }
> >                 smi_mod_timer(smi_info, jiffies + SI_TIMEOUT_HOSED);
> >                 goto out;
> >         }
> > @@ -1130,10 +1135,12 @@ static void smi_timeout(struct timer_list *t)
> >                 timeout = jiffies + SI_TIMEOUT_JIFFIES;
> >         }
> >
> > -       if (smi_result != SI_SM_IDLE)
> > -               smi_mod_timer(smi_info, timeout);
> > -       else
> > +       if (smi_result != SI_SM_IDLE) {
> > +               if (smi_info->si_state != SI_HOSED)
> > +                       smi_mod_timer(smi_info, timeout);
> > +       } else {
> >                 smi_info->timer_running = false;
> > +       }
> >         spin_unlock_irqrestore(&(smi_info->si_lock), flags);
> >  }
> 
> Well, I'd leave fixing the IPMI handling to somebody familiar with that code.