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

[Michał Cłapiński]

On Fri, Jul 3, 2026 at 5:11 PM Michal Clapinski <mclapinski@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
> 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.
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.

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.

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.

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?
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.

What do you think?