Message ID | 20240104024819.848979-1-kai.heng.feng@canonical.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel+bounces-16211-ouuuleilei=gmail.com@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:7301:6f82:b0:100:9c79:88ff with SMTP id tb2csp5384507dyb; Wed, 3 Jan 2024 18:49:03 -0800 (PST) X-Google-Smtp-Source: AGHT+IHQTOsRsxLPwIJ1L8vkS7JdPMLAmdRUanw7QRBGXjAW44cj8290+5QO6BCcQvpM5fUAo9/N X-Received: by 2002:a05:6358:9392:b0:174:f2c7:4448 with SMTP id h18-20020a056358939200b00174f2c74448mr18748861rwb.42.1704336543322; Wed, 03 Jan 2024 18:49:03 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1704336543; cv=none; d=google.com; s=arc-20160816; b=HpdwqfvH9FVa2frRg94/YIWsITn5Yszq1YpgjtNrx6eUhEw0Knp/XvcDxXf8n9SPMI /JqVrucszTDEibqCxpgH4F1v6swSJa44a3HLTrHf65aK8gJkIUbz/UhbvovRDa502nE5 WRbR6MfgIN0hvW0pVYaOZebZRtlPvzE7qcOjJvu/IK/rshuVkCtKOmNhitXeoTDYgZpK NhH0upXzg2sZg/PXs+uAcNsRDTjpNvd/Nir0Qs0us23zsnvhcpHVk19v3vIuOGza/QhT TdOco1JP3BsvVVwUbeRE8RvImN25BQ13ozUTOmjQoXv3SF0l5To6SCCQRUBwK1/ctNro v0mg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=content-transfer-encoding:mime-version:list-unsubscribe :list-subscribe:list-id:precedence:message-id:date:subject:cc:to :from:dkim-signature; bh=qJZEPxVyP2sE0E3LLTPNVpWVMnWfCSJTXOSyhRDwibE=; fh=EUsWKEM3fynHiFmpY91gAyxDPYrojFFdy7NIwo9HjIg=; b=Dj3BBJ2OND8RG3Z0WcGoYkquRUNm7aHWJXPwuFUvUBhDET5xwcxtbEg/dg0+UCDeWA 2zM+p1qNZnNj+8AYzg6Dv+H8scm39DS8kDdSvlOkyb4oyQ+WEa4j9bGKEGLQsEsYoj0X 5K+qwLXVqbu0YsrTnaO/VC0x15rLsnwoeWENcMN4ENEM6LJ/NGq5fSq9Mqiqf4KBxxWs 6sEuZg24dh+51LhfTEDPczqDMsm+fvSdd4qW0lee2WqWDSp9OC0XuOs8TzUUnXONxMLD BY0IxpjlcnVLE3BcfOmRbuc98WTMTrSimMC+/JKRSK9ZTWh4wGRV6eBghoaUz3TV84B/ Wh0A== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@canonical.com header.s=20210705 header.b=PHZgtxPd; spf=pass (google.com: domain of linux-kernel+bounces-16211-ouuuleilei=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) smtp.mailfrom="linux-kernel+bounces-16211-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=canonical.com Received: from sv.mirrors.kernel.org (sv.mirrors.kernel.org. [139.178.88.99]) by mx.google.com with ESMTPS id w22-20020a63f516000000b005cd813c23besi20569730pgh.415.2024.01.03.18.49.03 for <ouuuleilei@gmail.com> (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 03 Jan 2024 18:49:03 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel+bounces-16211-ouuuleilei=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99; Authentication-Results: mx.google.com; dkim=pass header.i=@canonical.com header.s=20210705 header.b=PHZgtxPd; spf=pass (google.com: domain of linux-kernel+bounces-16211-ouuuleilei=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) smtp.mailfrom="linux-kernel+bounces-16211-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=canonical.com Received: from smtp.subspace.kernel.org (wormhole.subspace.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by sv.mirrors.kernel.org (Postfix) with ESMTPS id 1776A286FCB for <ouuuleilei@gmail.com>; Thu, 4 Jan 2024 02:49:03 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 6723F10A27; Thu, 4 Jan 2024 02:48:39 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=canonical.com header.i=@canonical.com header.b="PHZgtxPd" X-Original-To: linux-kernel@vger.kernel.org Received: from smtp-relay-canonical-1.canonical.com (smtp-relay-canonical-1.canonical.com [185.125.188.121]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 38BCE1FBB; Thu, 4 Jan 2024 02:48:33 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=canonical.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=canonical.com Received: from HP-EliteBook-x360-830-G8-Notebook-PC.. (unknown [10.101.196.174]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by smtp-relay-canonical-1.canonical.com (Postfix) with ESMTPSA id A8B123F722; Thu, 4 Jan 2024 02:48:27 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=canonical.com; s=20210705; t=1704336511; bh=qJZEPxVyP2sE0E3LLTPNVpWVMnWfCSJTXOSyhRDwibE=; h=From:To:Cc:Subject:Date:Message-Id:MIME-Version; b=PHZgtxPdoqTnO98HMsw4+UrL6nE121jBQPBCtOT99H61V8UGfRussxW8hXnZvP+T+ x53XaYBR1hJAxqcSpTNlSwgqxFx1jxX1rOO5x/YvKm7vsKJIUU6Jh61E1zbMsggHOz rzNPVKwOWT+LLmZsxr2LYzBkbBlMEShFF8+uu9teXt87TvmBHS7uomjB0yyx02VVIq w2mT7ZtJGDAY9EZ9mexN9UZagH0wARM6coGHE5BkdQvaSGl59FdUsrj2XWW2JdTuy3 8bsPfci6L7smeta59ma/s1YVFaN7Wg0mmwonrxHDVT78YOcQ099SqBvFi1zDsXyeKA nCpYg7bNzQ7AQ== From: Kai-Heng Feng <kai.heng.feng@canonical.com> To: jdelvare@suse.com, linux@roeck-us.net Cc: Kai-Heng Feng <kai.heng.feng@canonical.com>, "Rafael J. Wysocki" <rafael@kernel.org>, Len Brown <lenb@kernel.org>, Robert Moore <robert.moore@intel.com>, linux-acpi@vger.kernel.org, linux-kernel@vger.kernel.org, acpica-devel@lists.linux.dev Subject: [PATCH v3 1/2] ACPI: IPMI: Add helper to wait for when SMI is selected Date: Thu, 4 Jan 2024 10:48:18 +0800 Message-Id: <20240104024819.848979-1-kai.heng.feng@canonical.com> X-Mailer: git-send-email 2.34.1 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: <linux-kernel.vger.kernel.org> List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org> List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1787126395435855865 X-GMAIL-MSGID: 1787126395435855865 |
Series |
[v3,1/2] ACPI: IPMI: Add helper to wait for when SMI is selected
|
|
Commit Message
Kai-Heng Feng
Jan. 4, 2024, 2:48 a.m. UTC
The function of acpi_power_meter module on Dell system requires IPMI
handler is installed and SMI is selected.
So add a helper to let acpi_power_meter know when IPMI handler and SMI
are ready.
Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
v3:
- New patch.
drivers/acpi/acpi_ipmi.c | 17 ++++++++++++++++-
include/acpi/acpi_bus.h | 5 +++++
2 files changed, 21 insertions(+), 1 deletion(-)
Comments
On Thu, Jan 4, 2024 at 3:48 AM Kai-Heng Feng <kai.heng.feng@canonical.com> wrote: > > The function of acpi_power_meter module on Dell system requires IPMI > handler is installed and SMI is selected. Does the firmware use _DEP to let the OS know about this dependency? > So add a helper to let acpi_power_meter know when IPMI handler and SMI > are ready. > > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > --- > v3: > - New patch. > > drivers/acpi/acpi_ipmi.c | 17 ++++++++++++++++- > include/acpi/acpi_bus.h | 5 +++++ > 2 files changed, 21 insertions(+), 1 deletion(-) > > diff --git a/drivers/acpi/acpi_ipmi.c b/drivers/acpi/acpi_ipmi.c > index 0555f68c2dfd..54862cab7171 100644 > --- a/drivers/acpi/acpi_ipmi.c > +++ b/drivers/acpi/acpi_ipmi.c > @@ -23,6 +23,8 @@ MODULE_LICENSE("GPL"); > #define IPMI_TIMEOUT (5000) > #define ACPI_IPMI_MAX_MSG_LENGTH 64 > > +static struct completion smi_selected; > + > struct acpi_ipmi_device { > /* the device list attached to driver_data.ipmi_devices */ > struct list_head head; > @@ -463,8 +465,10 @@ static void ipmi_register_bmc(int iface, struct device *dev) > if (temp->handle == handle) > goto err_lock; > } > - if (!driver_data.selected_smi) > + if (!driver_data.selected_smi) { > driver_data.selected_smi = ipmi_device; > + complete(&smi_selected); > + } > list_add_tail(&ipmi_device->head, &driver_data.ipmi_devices); > mutex_unlock(&driver_data.ipmi_lock); > > @@ -578,10 +582,21 @@ acpi_ipmi_space_handler(u32 function, acpi_physical_address address, > return status; > } > > +int acpi_wait_for_acpi_ipmi(void) > +{ > + long ret; > + > + ret = wait_for_completion_interruptible_timeout(&smi_selected, 2 * HZ); > + > + return ret > 0 ? 0 : -ETIMEDOUT; What will happen if the IPMI driver is unloaded after this has returned 0? > +} > +EXPORT_SYMBOL_GPL(acpi_wait_for_acpi_ipmi); > + > static int __init acpi_ipmi_init(void) > { > int result; > acpi_status status; > + init_completion(&smi_selected); > > if (acpi_disabled) > return 0; > diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h > index 1216d72c650f..afa6e4d4bf46 100644 > --- a/include/acpi/acpi_bus.h > +++ b/include/acpi/acpi_bus.h > @@ -821,11 +821,16 @@ static inline void acpi_put_acpi_dev(struct acpi_device *adev) > { > acpi_dev_put(adev); > } > + > +int acpi_wait_for_acpi_ipmi(void); > + > #else /* CONFIG_ACPI */ > > static inline int register_acpi_bus_type(void *bus) { return 0; } > static inline int unregister_acpi_bus_type(void *bus) { return 0; } > > +static inline int acpi_wait_for_acpi_ipmi(void) { return 0; } > + > #endif /* CONFIG_ACPI */ > > #endif /*__ACPI_BUS_H__*/ > --
On Thu, Jan 04, 2024 at 02:34:52PM +0100, Rafael J. Wysocki wrote: > On Thu, Jan 4, 2024 at 3:48 AM Kai-Heng Feng > <kai.heng.feng@canonical.com> wrote: > > > > The function of acpi_power_meter module on Dell system requires IPMI > > handler is installed and SMI is selected. > > Does the firmware use _DEP to let the OS know about this dependency? > > > So add a helper to let acpi_power_meter know when IPMI handler and SMI > > are ready. > > > > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > > --- > > v3: > > - New patch. > > > > drivers/acpi/acpi_ipmi.c | 17 ++++++++++++++++- > > include/acpi/acpi_bus.h | 5 +++++ > > 2 files changed, 21 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/acpi/acpi_ipmi.c b/drivers/acpi/acpi_ipmi.c > > index 0555f68c2dfd..54862cab7171 100644 > > --- a/drivers/acpi/acpi_ipmi.c > > +++ b/drivers/acpi/acpi_ipmi.c > > @@ -23,6 +23,8 @@ MODULE_LICENSE("GPL"); > > #define IPMI_TIMEOUT (5000) > > #define ACPI_IPMI_MAX_MSG_LENGTH 64 > > > > +static struct completion smi_selected; > > + > > struct acpi_ipmi_device { > > /* the device list attached to driver_data.ipmi_devices */ > > struct list_head head; > > @@ -463,8 +465,10 @@ static void ipmi_register_bmc(int iface, struct device *dev) > > if (temp->handle == handle) > > goto err_lock; > > } > > - if (!driver_data.selected_smi) > > + if (!driver_data.selected_smi) { > > driver_data.selected_smi = ipmi_device; > > + complete(&smi_selected); > > + } > > list_add_tail(&ipmi_device->head, &driver_data.ipmi_devices); > > mutex_unlock(&driver_data.ipmi_lock); > > > > @@ -578,10 +582,21 @@ acpi_ipmi_space_handler(u32 function, acpi_physical_address address, > > return status; > > } > > > > +int acpi_wait_for_acpi_ipmi(void) > > +{ > > + long ret; > > + > > + ret = wait_for_completion_interruptible_timeout(&smi_selected, 2 * HZ); > > + > > + return ret > 0 ? 0 : -ETIMEDOUT; > > What will happen if the IPMI driver is unloaded after this has returned 0? The IPMI driver can't be unloaded if it has a user. I've been following this, but I know little about ACPI. Beyond this solution, the only other solution I could come up with was to start the IPMI driver earlier. But then you are in a chicken-and-egg situation (https://dictionary.cambridge.org/dictionary/english/chicken-and-egg-situation). Which was the reason for the SPMI table, but that's really kind of useless for this, even if the SPMI table existed. -corey > > > +} > > +EXPORT_SYMBOL_GPL(acpi_wait_for_acpi_ipmi); > > + > > static int __init acpi_ipmi_init(void) > > { > > int result; > > acpi_status status; > > + init_completion(&smi_selected); > > > > if (acpi_disabled) > > return 0; > > diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h > > index 1216d72c650f..afa6e4d4bf46 100644 > > --- a/include/acpi/acpi_bus.h > > +++ b/include/acpi/acpi_bus.h > > @@ -821,11 +821,16 @@ static inline void acpi_put_acpi_dev(struct acpi_device *adev) > > { > > acpi_dev_put(adev); > > } > > + > > +int acpi_wait_for_acpi_ipmi(void); > > + > > #else /* CONFIG_ACPI */ > > > > static inline int register_acpi_bus_type(void *bus) { return 0; } > > static inline int unregister_acpi_bus_type(void *bus) { return 0; } > > > > +static inline int acpi_wait_for_acpi_ipmi(void) { return 0; } > > + > > #endif /* CONFIG_ACPI */ > > > > #endif /*__ACPI_BUS_H__*/ > > -- >
On Thu, Jan 4, 2024 at 4:42 PM Corey Minyard <minyard@acm.org> wrote: > > On Thu, Jan 04, 2024 at 02:34:52PM +0100, Rafael J. Wysocki wrote: > > On Thu, Jan 4, 2024 at 3:48 AM Kai-Heng Feng > > <kai.heng.feng@canonical.com> wrote: > > > > > > The function of acpi_power_meter module on Dell system requires IPMI > > > handler is installed and SMI is selected. > > > > Does the firmware use _DEP to let the OS know about this dependency? > > > > > So add a helper to let acpi_power_meter know when IPMI handler and SMI > > > are ready. > > > > > > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > > > --- > > > v3: > > > - New patch. > > > > > > drivers/acpi/acpi_ipmi.c | 17 ++++++++++++++++- > > > include/acpi/acpi_bus.h | 5 +++++ > > > 2 files changed, 21 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/acpi/acpi_ipmi.c b/drivers/acpi/acpi_ipmi.c > > > index 0555f68c2dfd..54862cab7171 100644 > > > --- a/drivers/acpi/acpi_ipmi.c > > > +++ b/drivers/acpi/acpi_ipmi.c > > > @@ -23,6 +23,8 @@ MODULE_LICENSE("GPL"); > > > #define IPMI_TIMEOUT (5000) > > > #define ACPI_IPMI_MAX_MSG_LENGTH 64 > > > > > > +static struct completion smi_selected; > > > + > > > struct acpi_ipmi_device { > > > /* the device list attached to driver_data.ipmi_devices */ > > > struct list_head head; > > > @@ -463,8 +465,10 @@ static void ipmi_register_bmc(int iface, struct device *dev) > > > if (temp->handle == handle) > > > goto err_lock; > > > } > > > - if (!driver_data.selected_smi) > > > + if (!driver_data.selected_smi) { > > > driver_data.selected_smi = ipmi_device; > > > + complete(&smi_selected); > > > + } > > > list_add_tail(&ipmi_device->head, &driver_data.ipmi_devices); > > > mutex_unlock(&driver_data.ipmi_lock); > > > > > > @@ -578,10 +582,21 @@ acpi_ipmi_space_handler(u32 function, acpi_physical_address address, > > > return status; > > > } > > > > > > +int acpi_wait_for_acpi_ipmi(void) > > > +{ > > > + long ret; > > > + > > > + ret = wait_for_completion_interruptible_timeout(&smi_selected, 2 * HZ); > > > + > > > + return ret > 0 ? 0 : -ETIMEDOUT; > > > > What will happen if the IPMI driver is unloaded after this has returned 0? > > The IPMI driver can't be unloaded if it has a user. Because of the use of the exported symbol, right? > I've been following this, but I know little about ACPI. Beyond this > solution, the only other solution I could come up with was to start the > IPMI driver earlier. But then you are in a chicken-and-egg situation > (https://dictionary.cambridge.org/dictionary/english/chicken-and-egg-situation). > Which was the reason for the SPMI table, but that's really kind of > useless for this, even if the SPMI table existed. Fine. Let me reply to the patch with some more comments then.
On Thu, Jan 4, 2024 at 3:48 AM Kai-Heng Feng <kai.heng.feng@canonical.com> wrote: > > The function of acpi_power_meter module on Dell system requires IPMI > handler is installed and SMI is selected. > > So add a helper to let acpi_power_meter know when IPMI handler and SMI > are ready. The changelog is a bit terse. It could describe the problem at hand in more detail, for example. Also it wouldn't hurt to provide a Link: tag pointing to a place where some extra details could be found. > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > --- > v3: > - New patch. > > drivers/acpi/acpi_ipmi.c | 17 ++++++++++++++++- > include/acpi/acpi_bus.h | 5 +++++ > 2 files changed, 21 insertions(+), 1 deletion(-) > > diff --git a/drivers/acpi/acpi_ipmi.c b/drivers/acpi/acpi_ipmi.c > index 0555f68c2dfd..54862cab7171 100644 > --- a/drivers/acpi/acpi_ipmi.c > +++ b/drivers/acpi/acpi_ipmi.c > @@ -23,6 +23,8 @@ MODULE_LICENSE("GPL"); > #define IPMI_TIMEOUT (5000) > #define ACPI_IPMI_MAX_MSG_LENGTH 64 > > +static struct completion smi_selected; > + > struct acpi_ipmi_device { > /* the device list attached to driver_data.ipmi_devices */ > struct list_head head; > @@ -463,8 +465,10 @@ static void ipmi_register_bmc(int iface, struct device *dev) > if (temp->handle == handle) > goto err_lock; > } > - if (!driver_data.selected_smi) > + if (!driver_data.selected_smi) { > driver_data.selected_smi = ipmi_device; > + complete(&smi_selected); It looks like the new completion is at least related to driver_data, so should it be a member of the latter? > + } > list_add_tail(&ipmi_device->head, &driver_data.ipmi_devices); > mutex_unlock(&driver_data.ipmi_lock); > > @@ -578,10 +582,21 @@ acpi_ipmi_space_handler(u32 function, acpi_physical_address address, > return status; > } > > +int acpi_wait_for_acpi_ipmi(void) > +{ > + long ret; > + > + ret = wait_for_completion_interruptible_timeout(&smi_selected, 2 * HZ); Why does it wait for 2 ticks and not 3 or 5? Also it would be nice to have a symbol defined for this timeout value. > + > + return ret > 0 ? 0 : -ETIMEDOUT; if (ret <= 0) return -ETIMEDOUT; return 0; pretty please. > +} > +EXPORT_SYMBOL_GPL(acpi_wait_for_acpi_ipmi); > + > static int __init acpi_ipmi_init(void) > { > int result; > acpi_status status; Empty line here, please. > + init_completion(&smi_selected); Does it really make sense to initialize it when ACPI is disabled? > > if (acpi_disabled) > return 0; > diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h > index 1216d72c650f..afa6e4d4bf46 100644 > --- a/include/acpi/acpi_bus.h > +++ b/include/acpi/acpi_bus.h > @@ -821,11 +821,16 @@ static inline void acpi_put_acpi_dev(struct acpi_device *adev) > { > acpi_dev_put(adev); > } > + > +int acpi_wait_for_acpi_ipmi(void); > + > #else /* CONFIG_ACPI */ > > static inline int register_acpi_bus_type(void *bus) { return 0; } > static inline int unregister_acpi_bus_type(void *bus) { return 0; } > > +static inline int acpi_wait_for_acpi_ipmi(void) { return 0; } > + > #endif /* CONFIG_ACPI */ > > #endif /*__ACPI_BUS_H__*/ > --
On Thu, Jan 04, 2024 at 06:11:29PM +0100, Rafael J. Wysocki wrote: > On Thu, Jan 4, 2024 at 4:42 PM Corey Minyard <minyard@acm.org> wrote: > > > > On Thu, Jan 04, 2024 at 02:34:52PM +0100, Rafael J. Wysocki wrote: > > > On Thu, Jan 4, 2024 at 3:48 AM Kai-Heng Feng > > > <kai.heng.feng@canonical.com> wrote: > > > > > > > > The function of acpi_power_meter module on Dell system requires IPMI > > > > handler is installed and SMI is selected. > > > > > > Does the firmware use _DEP to let the OS know about this dependency? > > > > > > > So add a helper to let acpi_power_meter know when IPMI handler and SMI > > > > are ready. > > > > > > > > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > > > > --- > > > > v3: > > > > - New patch. > > > > > > > > drivers/acpi/acpi_ipmi.c | 17 ++++++++++++++++- > > > > include/acpi/acpi_bus.h | 5 +++++ > > > > 2 files changed, 21 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/acpi/acpi_ipmi.c b/drivers/acpi/acpi_ipmi.c > > > > index 0555f68c2dfd..54862cab7171 100644 > > > > --- a/drivers/acpi/acpi_ipmi.c > > > > +++ b/drivers/acpi/acpi_ipmi.c > > > > @@ -23,6 +23,8 @@ MODULE_LICENSE("GPL"); > > > > #define IPMI_TIMEOUT (5000) > > > > #define ACPI_IPMI_MAX_MSG_LENGTH 64 > > > > > > > > +static struct completion smi_selected; > > > > + > > > > struct acpi_ipmi_device { > > > > /* the device list attached to driver_data.ipmi_devices */ > > > > struct list_head head; > > > > @@ -463,8 +465,10 @@ static void ipmi_register_bmc(int iface, struct device *dev) > > > > if (temp->handle == handle) > > > > goto err_lock; > > > > } > > > > - if (!driver_data.selected_smi) > > > > + if (!driver_data.selected_smi) { > > > > driver_data.selected_smi = ipmi_device; > > > > + complete(&smi_selected); > > > > + } > > > > list_add_tail(&ipmi_device->head, &driver_data.ipmi_devices); > > > > mutex_unlock(&driver_data.ipmi_lock); > > > > > > > > @@ -578,10 +582,21 @@ acpi_ipmi_space_handler(u32 function, acpi_physical_address address, > > > > return status; > > > > } > > > > > > > > +int acpi_wait_for_acpi_ipmi(void) > > > > +{ > > > > + long ret; > > > > + > > > > + ret = wait_for_completion_interruptible_timeout(&smi_selected, 2 * HZ); > > > > + > > > > + return ret > 0 ? 0 : -ETIMEDOUT; > > > > > > What will happen if the IPMI driver is unloaded after this has returned 0? > > > > The IPMI driver can't be unloaded if it has a user. > > Because of the use of the exported symbol, right? I wasn't thinking that, but yes, that would do it. It also keep track of users and doesn't allow the driver to be unloaded if anything has registered as a user, either. -corey > > > I've been following this, but I know little about ACPI. Beyond this > > solution, the only other solution I could come up with was to start the > > IPMI driver earlier. But then you are in a chicken-and-egg situation > > (https://dictionary.cambridge.org/dictionary/english/chicken-and-egg-situation). > > Which was the reason for the SPMI table, but that's really kind of > > useless for this, even if the SPMI table existed. > > Fine. > > Let me reply to the patch with some more comments then.
On Thu, Jan 4, 2024 at 9:35 PM Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Thu, Jan 4, 2024 at 3:48 AM Kai-Heng Feng > <kai.heng.feng@canonical.com> wrote: > > > > The function of acpi_power_meter module on Dell system requires IPMI > > handler is installed and SMI is selected. > > Does the firmware use _DEP to let the OS know about this dependency? No. _DEP is missing. Kai-Heng > > > So add a helper to let acpi_power_meter know when IPMI handler and SMI > > are ready. > > > > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > > --- > > v3: > > - New patch. > > > > drivers/acpi/acpi_ipmi.c | 17 ++++++++++++++++- > > include/acpi/acpi_bus.h | 5 +++++ > > 2 files changed, 21 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/acpi/acpi_ipmi.c b/drivers/acpi/acpi_ipmi.c > > index 0555f68c2dfd..54862cab7171 100644 > > --- a/drivers/acpi/acpi_ipmi.c > > +++ b/drivers/acpi/acpi_ipmi.c > > @@ -23,6 +23,8 @@ MODULE_LICENSE("GPL"); > > #define IPMI_TIMEOUT (5000) > > #define ACPI_IPMI_MAX_MSG_LENGTH 64 > > > > +static struct completion smi_selected; > > + > > struct acpi_ipmi_device { > > /* the device list attached to driver_data.ipmi_devices */ > > struct list_head head; > > @@ -463,8 +465,10 @@ static void ipmi_register_bmc(int iface, struct device *dev) > > if (temp->handle == handle) > > goto err_lock; > > } > > - if (!driver_data.selected_smi) > > + if (!driver_data.selected_smi) { > > driver_data.selected_smi = ipmi_device; > > + complete(&smi_selected); > > + } > > list_add_tail(&ipmi_device->head, &driver_data.ipmi_devices); > > mutex_unlock(&driver_data.ipmi_lock); > > > > @@ -578,10 +582,21 @@ acpi_ipmi_space_handler(u32 function, acpi_physical_address address, > > return status; > > } > > > > +int acpi_wait_for_acpi_ipmi(void) > > +{ > > + long ret; > > + > > + ret = wait_for_completion_interruptible_timeout(&smi_selected, 2 * HZ); > > + > > + return ret > 0 ? 0 : -ETIMEDOUT; > > What will happen if the IPMI driver is unloaded after this has returned 0? > > > +} > > +EXPORT_SYMBOL_GPL(acpi_wait_for_acpi_ipmi); > > + > > static int __init acpi_ipmi_init(void) > > { > > int result; > > acpi_status status; > > + init_completion(&smi_selected); > > > > if (acpi_disabled) > > return 0; > > diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h > > index 1216d72c650f..afa6e4d4bf46 100644 > > --- a/include/acpi/acpi_bus.h > > +++ b/include/acpi/acpi_bus.h > > @@ -821,11 +821,16 @@ static inline void acpi_put_acpi_dev(struct acpi_device *adev) > > { > > acpi_dev_put(adev); > > } > > + > > +int acpi_wait_for_acpi_ipmi(void); > > + > > #else /* CONFIG_ACPI */ > > > > static inline int register_acpi_bus_type(void *bus) { return 0; } > > static inline int unregister_acpi_bus_type(void *bus) { return 0; } > > > > +static inline int acpi_wait_for_acpi_ipmi(void) { return 0; } > > + > > #endif /* CONFIG_ACPI */ > > > > #endif /*__ACPI_BUS_H__*/ > > --
On Fri, Jan 5, 2024 at 1:25 AM Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Thu, Jan 4, 2024 at 3:48 AM Kai-Heng Feng > <kai.heng.feng@canonical.com> wrote: > > > > The function of acpi_power_meter module on Dell system requires IPMI > > handler is installed and SMI is selected. > > > > So add a helper to let acpi_power_meter know when IPMI handler and SMI > > are ready. > > The changelog is a bit terse. > > It could describe the problem at hand in more detail, for example. > > Also it wouldn't hurt to provide a Link: tag pointing to a place where > some extra details could be found. OK. Will add more info in next revision. > > > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > > --- > > v3: > > - New patch. > > > > drivers/acpi/acpi_ipmi.c | 17 ++++++++++++++++- > > include/acpi/acpi_bus.h | 5 +++++ > > 2 files changed, 21 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/acpi/acpi_ipmi.c b/drivers/acpi/acpi_ipmi.c > > index 0555f68c2dfd..54862cab7171 100644 > > --- a/drivers/acpi/acpi_ipmi.c > > +++ b/drivers/acpi/acpi_ipmi.c > > @@ -23,6 +23,8 @@ MODULE_LICENSE("GPL"); > > #define IPMI_TIMEOUT (5000) > > #define ACPI_IPMI_MAX_MSG_LENGTH 64 > > > > +static struct completion smi_selected; > > + > > struct acpi_ipmi_device { > > /* the device list attached to driver_data.ipmi_devices */ > > struct list_head head; > > @@ -463,8 +465,10 @@ static void ipmi_register_bmc(int iface, struct device *dev) > > if (temp->handle == handle) > > goto err_lock; > > } > > - if (!driver_data.selected_smi) > > + if (!driver_data.selected_smi) { > > driver_data.selected_smi = ipmi_device; > > + complete(&smi_selected); > > It looks like the new completion is at least related to driver_data, > so should it be a member of the latter? Sure thing. > > > + } > > list_add_tail(&ipmi_device->head, &driver_data.ipmi_devices); > > mutex_unlock(&driver_data.ipmi_lock); > > > > @@ -578,10 +582,21 @@ acpi_ipmi_space_handler(u32 function, acpi_physical_address address, > > return status; > > } > > > > +int acpi_wait_for_acpi_ipmi(void) > > +{ > > + long ret; > > + > > + ret = wait_for_completion_interruptible_timeout(&smi_selected, 2 * HZ); > > Why does it wait for 2 ticks and not 3 or 5? > > Also it would be nice to have a symbol defined for this timeout value. Sure. Will add a define and comment. > > > + > > + return ret > 0 ? 0 : -ETIMEDOUT; > > if (ret <= 0) > return -ETIMEDOUT; > > return 0; > > pretty please. OK. > > > +} > > +EXPORT_SYMBOL_GPL(acpi_wait_for_acpi_ipmi); > > + > > static int __init acpi_ipmi_init(void) > > { > > int result; > > acpi_status status; > > Empty line here, please. OK. > > > + init_completion(&smi_selected); > > Does it really make sense to initialize it when ACPI is disabled? OK, will move it further down. Kai-Heng > > > > > if (acpi_disabled) > > return 0; > > diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h > > index 1216d72c650f..afa6e4d4bf46 100644 > > --- a/include/acpi/acpi_bus.h > > +++ b/include/acpi/acpi_bus.h > > @@ -821,11 +821,16 @@ static inline void acpi_put_acpi_dev(struct acpi_device *adev) > > { > > acpi_dev_put(adev); > > } > > + > > +int acpi_wait_for_acpi_ipmi(void); > > + > > #else /* CONFIG_ACPI */ > > > > static inline int register_acpi_bus_type(void *bus) { return 0; } > > static inline int unregister_acpi_bus_type(void *bus) { return 0; } > > > > +static inline int acpi_wait_for_acpi_ipmi(void) { return 0; } > > + > > #endif /* CONFIG_ACPI */ > > > > #endif /*__ACPI_BUS_H__*/ > > --
diff --git a/drivers/acpi/acpi_ipmi.c b/drivers/acpi/acpi_ipmi.c index 0555f68c2dfd..54862cab7171 100644 --- a/drivers/acpi/acpi_ipmi.c +++ b/drivers/acpi/acpi_ipmi.c @@ -23,6 +23,8 @@ MODULE_LICENSE("GPL"); #define IPMI_TIMEOUT (5000) #define ACPI_IPMI_MAX_MSG_LENGTH 64 +static struct completion smi_selected; + struct acpi_ipmi_device { /* the device list attached to driver_data.ipmi_devices */ struct list_head head; @@ -463,8 +465,10 @@ static void ipmi_register_bmc(int iface, struct device *dev) if (temp->handle == handle) goto err_lock; } - if (!driver_data.selected_smi) + if (!driver_data.selected_smi) { driver_data.selected_smi = ipmi_device; + complete(&smi_selected); + } list_add_tail(&ipmi_device->head, &driver_data.ipmi_devices); mutex_unlock(&driver_data.ipmi_lock); @@ -578,10 +582,21 @@ acpi_ipmi_space_handler(u32 function, acpi_physical_address address, return status; } +int acpi_wait_for_acpi_ipmi(void) +{ + long ret; + + ret = wait_for_completion_interruptible_timeout(&smi_selected, 2 * HZ); + + return ret > 0 ? 0 : -ETIMEDOUT; +} +EXPORT_SYMBOL_GPL(acpi_wait_for_acpi_ipmi); + static int __init acpi_ipmi_init(void) { int result; acpi_status status; + init_completion(&smi_selected); if (acpi_disabled) return 0; diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h index 1216d72c650f..afa6e4d4bf46 100644 --- a/include/acpi/acpi_bus.h +++ b/include/acpi/acpi_bus.h @@ -821,11 +821,16 @@ static inline void acpi_put_acpi_dev(struct acpi_device *adev) { acpi_dev_put(adev); } + +int acpi_wait_for_acpi_ipmi(void); + #else /* CONFIG_ACPI */ static inline int register_acpi_bus_type(void *bus) { return 0; } static inline int unregister_acpi_bus_type(void *bus) { return 0; } +static inline int acpi_wait_for_acpi_ipmi(void) { return 0; } + #endif /* CONFIG_ACPI */ #endif /*__ACPI_BUS_H__*/