From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-ot1-f49.google.com (mail-ot1-f49.google.com [209.85.210.49]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id D580947D940 for ; Wed, 5 Aug 2026 21:07:30 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.210.49 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1785964052; cv=none; b=jTHRtM+rh1RywjRHJpxScq3vaBdyVI/up5GT4CBjj9KFV1ttepKcoBj0eiIG89u8iEvcS9MbU0eALu+kqejZ1WF5spVCs5GL4upkUybZbEtTgG5AYBPiJsG2905oj8WR6q82aAauCDT8sn037+kUTb8nF7ow/wphwIP62U+WxJY= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1785964052; c=relaxed/simple; bh=7InlVeKfquzxhdBmXAHx62+om+YG7FTDPy7VzYniCic=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=A/5oftx3MV/nugUfuip9X2hBzJVatn2e6iRiDetR1PxFSEu+WdpyzdU64qz+y0c+jIB/QVpM415wF3LSxhfrCWAi+gj+iqT58v6RtBe8g/pQI5Dk4fN+UKwat8hC++A2hTQvbrB/H34ATiJVK8QdcA+pYXsAWnLwKxr1a60bCA0= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=minyard.net; spf=pass smtp.mailfrom=minyard.net; dkim=pass (2048-bit key) header.d=minyard.net header.i=@minyard.net header.b=Ch+uB87P; arc=none smtp.client-ip=209.85.210.49 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=minyard.net Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=minyard.net Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=minyard.net header.i=@minyard.net header.b="Ch+uB87P" Received: by mail-ot1-f49.google.com with SMTP id 46e09a7af769-7e9d7464b71so740228a34.0 for ; Wed, 05 Aug 2026 14:07:30 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=minyard.net; s=google; t=1785964049; x=1786568849; darn=vger.kernel.org; h=in-reply-to:content-transfer-encoding:content-disposition :content-type:mime-version:references:reply-to:message-id:subject:cc :to:from:date:from:to:cc:subject:date:message-id:reply-to :content-type; bh=9lva6hKXKhGzdvKfvNsWp4a4M9nAvNtDPh8e57KYbHA=; b=Ch+uB87P08mIrxIYiYQ0cFdh57+fjDUsqy+gGQNIzA++B3F7LVQLhjN9FwoLQs9Eej uQd7swbUb33QKr7RkhL9I3WZ6VW0Ou/aoldQ3gOjQFw4DiRqhFTnZmkh4ENLpa14w2EF +b1Tpw06uMa0Rf1fbBo7gzkaqGzA0Pji+tHd/2Sn9kSinlfD7EvePENk2uDiJkMhGItr YFuA7vAk+MxddNAfzHBM4riVSF67U7wQnM2v4XQj0BJvkbgASsXYNH3I4yCHhZWEYSVX IC528ACEKJy2yd5twUEW1N6YjVM4isKoq172y66J5eju9i+PypmLOXrIyoApxS6Ob3qo e1yQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1785964049; x=1786568849; h=in-reply-to:content-transfer-encoding:content-disposition :content-type:mime-version:references:reply-to:message-id:subject:cc :to:from:date:x-gm-gg:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to:content-type; bh=9lva6hKXKhGzdvKfvNsWp4a4M9nAvNtDPh8e57KYbHA=; b=Or47UWvlR4ae9f4zXo60OqwPQLCME5v4CBb2FxsBJuZQeX5UrHgv052+TiWgB20dND yXnPieaWsDkz+H9kyeqxbNx+viKVKDUDQJ4d7wYC/4jFjqMsCi0cHsu8whS9phZ4RtUD lvcWemppnngbswlm43MRxowgeGl1Wfy3XzDBM8zk4G7+nLnVnc2kDRqpg1tUMxOJSNug Q3GzgYP21NhWKN/fqC5rOPDGSLMxEsSAcSL4R40uwaC3dZA5JkgUCYeh8MDzmvfN7RoS o5/pJBfpOnAknEmHbqGWC1tRVqppzwsRfuXSdiOyKFBmI857Zz3ibAMWHDMVxeP3E8na pb4Q== X-Forwarded-Encrypted: i=1; AHgh+RqFVVfJtN1yKKPLjUfIJtosXmyQCiYcsxJDmFJLYz1lZciI2lxpWqAyj0MtQfcYKMXBuHrxHqoRk023TUU=@vger.kernel.org X-Gm-Message-State: AOJu0YwYkGUAwAodqPs7unUJrO9c1JkihQ335o0bACFkhTYTZya88s0M I49wU9yqcOXyCaxRrcQMdT8w+vjOmzUTrsbLmvdtiRPeGZQLwRXd1U04rMUQKV1Kmz8cs5eiA+X gcWAWWgw= X-Gm-Gg: AR+sD13ARw7Ck+/kM8pAciooF6I3hgi7/R8oHYbwcbZg3171AzY9OcO/APjvMv7mxZN +rhzz9reA78PZTdugSelH97RiDO/gTZlPil7e4VzDWTsGiTE4Qd57bhwbVxExyRK8GvTGt+vA29 VOdBL29HFmILVYhqduz02bwQ0gG6q70jKvYZUPkceI7pk2gOBCuMQ5Si43H2BRlwMYAg4zLUQnY WiTEBEPKhnMOwu3d7zSqdhiRuad94J+UehJaQaJxYRvuW1LsTPodlcKc92PXn/YP3RHIwEQRxzS 4iFToH/V7/DX8yumwo0PRZyCA6MACs9Q0BFG/WevLEdicVyh+NwxwCWwV7WfqfBsTaqpEu63XGf G6CUyKtVjuXyhREjKZwXZggATxwqY2gUWL6fWX7BZ9LYai0K3ppVzGjaW88IoQ+C1RM8QjKYNo8 YKOCtXGNlbjfvUH+p0U1Y9zAzKT/BQmH0EqPk7CxMlIfHNUBaza1eWHgM7ENNZMV3qKTLNsIaCI zKpEMYrYFiazs0LD3fShznUx/od1SivWFxA7tMp90B5oak7YE0HaRwXeaYTiWUfHqRMRgSTqFAb 9n1YQxBz5w== X-Received: by 2002:a05:6830:3743:b0:7eb:89e4:8f5c with SMTP id 46e09a7af769-7f1e5ee179cmr5800298a34.14.1785964049592; Wed, 05 Aug 2026 14:07:29 -0700 (PDT) Received: from mail.minyard.net ([2001:470:b8f6:1b:d7f5:d7e4:846b:9400]) by smtp.gmail.com with ESMTPSA id 46e09a7af769-7f1df345ed1sm3383429a34.8.2026.08.05.14.07.28 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 05 Aug 2026 14:07:28 -0700 (PDT) Date: Wed, 5 Aug 2026 16:07:24 -0500 From: Corey Minyard To: =?utf-8?B?TWljaGHFgiBDxYJhcGnFhHNraQ==?= Cc: openipmi-developer@lists.sourceforge.net, linux-kernel@vger.kernel.org Subject: Re: [PATCH v2] ipmi:si: Add async init to ipmi_si Message-ID: Reply-To: corey@minyard.net References: <20260703150955.3943082-1-mclapinski@google.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: 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 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 > > --- > > 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 > > #include > > #include > > +#include > > #include "ipmi_si.h" > > #include "ipmi_si_sm.h" > > #include > > @@ -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