* [PATCH v2] ipmi:si: Add async init to ipmi_si @ 2026-07-03 15:09 Michal Clapinski 2026-07-06 0:13 ` Corey Minyard 2026-08-05 14:55 ` Michał Cłapiński 0 siblings, 2 replies; 4+ messages in thread From: Michal Clapinski @ 2026-07-03 15:09 UTC (permalink / raw) To: Corey Minyard, openipmi-developer; +Cc: linux-kernel, Michal Clapinski 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(); -- 2.55.0.795.g602f6c329a-goog ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2] ipmi:si: Add async init to ipmi_si 2026-07-03 15:09 [PATCH v2] ipmi:si: Add async init to ipmi_si Michal Clapinski @ 2026-07-06 0:13 ` Corey Minyard 2026-08-05 14:55 ` Michał Cłapiński 1 sibling, 0 replies; 4+ messages in thread From: Corey Minyard @ 2026-07-06 0:13 UTC (permalink / raw) To: Michal Clapinski; +Cc: openipmi-developer, linux-kernel On Fri, Jul 03, 2026 at 05:09:55PM +0200, Michal Clapinski wrote: > Added a new config option to allow offloading individual calls to > try_smi_init(). Saves 100ms on my system. This looks good. In a few releases I may change the default to y, if I can remember it by then :-). It's in my next tree. -corey > > 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(); > -- > 2.55.0.795.g602f6c329a-goog > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] ipmi:si: Add async init to ipmi_si 2026-07-03 15:09 [PATCH v2] ipmi:si: Add async init to ipmi_si Michal Clapinski 2026-07-06 0:13 ` Corey Minyard @ 2026-08-05 14:55 ` Michał Cłapiński 2026-08-05 21:07 ` Corey Minyard 1 sibling, 1 reply; 4+ messages in thread From: Michał Cłapiński @ 2026-08-05 14:55 UTC (permalink / raw) To: Corey Minyard, openipmi-developer; +Cc: linux-kernel 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. 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? ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] ipmi:si: Add async init to ipmi_si 2026-08-05 14:55 ` Michał Cłapiński @ 2026-08-05 21:07 ` Corey Minyard 0 siblings, 0 replies; 4+ messages in thread From: Corey Minyard @ 2026-08-05 21:07 UTC (permalink / raw) To: Michał Cłapiński; +Cc: openipmi-developer, linux-kernel 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2026-08-05 21:07 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-07-03 15:09 [PATCH v2] ipmi:si: Add async init to ipmi_si Michal Clapinski 2026-07-06 0:13 ` Corey Minyard 2026-08-05 14:55 ` Michał Cłapiński 2026-08-05 21:07 ` Corey Minyard
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox