| Message ID | 20240104024819.848979-1-kai.heng.feng@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v3,1/2] ACPI: IPMI: Add helper to wait for when SMI is selected | expand |
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 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__*/
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(-)