Re: [PATCH v2] ipmi:si: Add async init to ipmi_si

[Corey Minyard <corey@minyard.net>]

On Wed, Aug 05, 2026 at 04:55:42PM +0200, Michał Cłapiński wrote:
> On Fri, Jul 3, 2026 at 5:11 PM Michal Clapinski <mclapinski@google.com> wrote:
> >
> > Added a new config option to allow offloading individual calls to
> > try_smi_init(). Saves 100ms on my system.
> >
> > Signed-off-by: Michal Clapinski <mclapinski@google.com>
> > ---
> > v2:
> > - instead of offloading the whole init function, offload just the
> >   individual calls to try_smi_init()
> >
> > I didn't implement the periodic retry feature that was talked about
> > under v1 due to my lack of expertise. LMK if this is a deal-breaker.
> > ---
> >  drivers/char/ipmi/Kconfig        |  9 +++++++++
> >  drivers/char/ipmi/ipmi_si_intf.c | 32 ++++++++++++++++++++++++++++----
> >  2 files changed, 37 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/char/ipmi/Kconfig b/drivers/char/ipmi/Kconfig
> > index 669f76000197..c8fa445c1c17 100644
> > --- a/drivers/char/ipmi/Kconfig
> > +++ b/drivers/char/ipmi/Kconfig
> > @@ -67,6 +67,15 @@ config IPMI_SI
> >           Currently, only KCS and SMIC are supported.  If
> >           you are using IPMI, you should probably say "y" here.
> >
> > +config IPMI_SI_ASYNC_INIT
> > +       bool 'Asynchronous initialization of IPMI System Interface'
> > +       depends on IPMI_SI
> > +       default n
> > +       help
> > +         Offloads invidual SMI inits. It speeds up the boot time.
> > +         It also introduces a very small risk that something else might fail
> > +         if it depends on synchronous IPMI init.
> > +
> >  config IPMI_SSIF
> >         tristate 'IPMI SMBus handler (SSIF)'
> >         depends on I2C
> > diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
> > index 9a9d12be9bf7..504d5b8636ba 100644
> > --- a/drivers/char/ipmi/ipmi_si_intf.c
> > +++ b/drivers/char/ipmi/ipmi_si_intf.c
> > @@ -39,6 +39,7 @@
> >  #include <linux/rcupdate.h>
> >  #include <linux/ipmi.h>
> >  #include <linux/ipmi_smi.h>
> > +#include <linux/async.h>
> >  #include "ipmi_si.h"
> >  #include "ipmi_si_sm.h"
> >  #include <linux/string.h>
> > @@ -2174,6 +2175,17 @@ static bool __init ipmi_smi_info_same(struct smi_info *e1, struct smi_info *e2)
> >                 e1->io.addr_data == e2->io.addr_data);
> >  }
> >
> > +static ASYNC_DOMAIN_EXCLUSIVE(ipmi_si_async_domain);
> > +
> > +static void __init async_try_smi_init(void *data, async_cookie_t cookie)
> > +{
> > +       struct smi_info *smi = data;
> > +
> > +       mutex_lock(&smi_infos_lock);
> > +       try_smi_init(smi);
> > +       mutex_unlock(&smi_infos_lock);
> > +}
> > +
> >  static int __init init_ipmi_si(void)
> >  {
> >         struct smi_info *e, *e2;
> > @@ -2219,8 +2231,13 @@ static int __init init_ipmi_si(void)
> >                                 break;
> >                         }
> >                 }
> > -               if (!dup)
> > -                       try_smi_init(e);
> > +               if (!dup) {
> > +                       if (IS_ENABLED(CONFIG_IPMI_SI_ASYNC_INIT))
> > +                               async_schedule_domain(async_try_smi_init, e,
> > +                                                     &ipmi_si_async_domain);
> > +                       else
> > +                               try_smi_init(e);
> > +               }
> >         }
> >
> >         /*
> > @@ -2253,8 +2270,13 @@ static int __init init_ipmi_si(void)
> >                                 break;
> >                         }
> >                 }
> > -               if (!dup)
> > -                       try_smi_init(e);
> > +               if (!dup) {
> > +                       if (IS_ENABLED(CONFIG_IPMI_SI_ASYNC_INIT))
> > +                               async_schedule_domain(async_try_smi_init, e,
> > +                                                     &ipmi_si_async_domain);
> > +                       else
> > +                               try_smi_init(e);
> > +               }
> >         }
> >
> >         initialized = true;
> > @@ -2401,6 +2423,8 @@ static void cleanup_ipmi_si(void)
> >         if (!initialized)
> >                 return;
> >
> > +       async_synchronize_full_domain(&ipmi_si_async_domain);
> > +
> >         ipmi_si_pci_shutdown();
> >
> >         ipmi_si_ls2k_shutdown();
> 
> I've reviewed comments by sashiko:
> https://sashiko.dev/#/patchset/20260703150955.3943082-1-mclapinski%40google.com.

Hmm, I didn't get this.  That's strange.  I see that I was cc-ed, but I
don't have that email.

> It makes 3 points:
> 
> 1. async_try_smi_init is marked __init but it shouldn't be.
> That's valid. I'll fix that in v3.

Yeah, I missed that.

> 
> 2. async_schedule_domain can run code synchronously. That would result
> in a deadlock.
> 
> That is true however async_schedule_domain currently only runs
> synchronously if we're out of memory or there are 32k functions
> scheduled. It's not very probable but still worth fixing.

I didn't realize this.  That could be a problem.

You could use a nested mutex lock.  Not my favorite thing, but it's an
easy solution.

Other mechanisms exist to run things asynchronously, like workqueues.  I
don't think queue_work_on() will ever run synchronously.  It might also
help the next problem.

> 
> 3. I'm passing the struct "e" by pointer to an async function. In the
> meantime the lock guarding it will be released. Could that pointer
> become invalid before we acquire the lock again?
> 
> That again doesn't seem very probable.

As Sashiko pointed out, it could theoretically be hot-removed before
this runs.  Not likely, but I'm working on a modification on top of this
to retry if this fails.  Then it will become a lot more likely.

The right way to fix this, of course, is to add a refcount and a flag in
the structure to say it's deleted.

Or, if you had a workqueue, you could cancel and flush the work, I
think, to make sure it wasn't running, before doing the remove
operation.

> 
> I'm not sure how to proceed to fix points 2 and 3. v1 where I made the
> whole init_ipmi_si async was immune to those problems so maybe we
> could go back to it?

We could do that, but it's just going to end up being rewritten

> Alternatively I could keep the lock acquired and only release it when
> the last async_try_smi_init function finishes but that's obviously
> more complicated than just returning to v1.

Yeah, that's not a great solution.  You really don't want to be locking
and unlocking mutexes in different threads.

> 
> What do you think?

If you want to take a try at it, you can.  I can try to handle all this
if you like.

-corey