* [RFC PATCH 0/4_v11] IPMI/ACPI: Install the ACPI IPMI opregion
@ 2010-10-26 9:14 yakui.zhao
2010-10-26 9:14 ` [RFC PATCH 1/4] IPMI: Add one interface to get more info of low-level IPMI device yakui.zhao
0 siblings, 1 reply; 24+ messages in thread
From: yakui.zhao @ 2010-10-26 9:14 UTC (permalink / raw)
To: openipmi-developer, linux-acpi; +Cc: minyard, lenb, Zhao Yakui
From: Zhao Yakui <yakui.zhao@intel.com>
Hi,
ACPI 4.0 spec adds the ACPI IPMI opregion, which means that the ACPI AML
code can also communicate with the BMC controller. This patch set is to
install the ACPI IPMI opregion space handler and enable the ACPI to access
the BMC controller through the IPMI message.
[RFC Patch 01/04]: Add one interface to get more info of low-level interface
It will return the corresponding info of low-level interface based
on the expected type(For example: SI_ACPI, SI_PCI and so on.)
[RFC Patch 02/04]: Remove the redudant definition of ipmi_addr_src type
[RFC Patch 03/04]: Add the document description about how to use the function
of ipmi_get_smi_info
[RFC Patch 04/04]: Install the IPMI space handler to enable ACPI to access the BMC
controller
V11->V12: Change back to copy mechanism in V10 and care the refcount of dev when
calling the function of ipmi_get_smi_info. At the same time the addr_source type is
not passed as the argument any more. Instead the caller does the comparison.
V10->V11: Delete the incorrect refcount in V10. At the same time the function of
ipmi_get_smi_info will return the reference pointer to avoid the memory copy.
V9-V10: Redefine the interface function to get more info of low-level IPMI
device. And add the document description about how to use it.
V8-V9: This patch set is based on the mechanism that add one interface that can
get more info of low-level IPMI device. For example: the ACPI device handle will
be returned for the pnp_acpi. Then the second patch will use the added interface
to check whether the IPMI device is what ACPI wanted to communicated with in the
new_smi callback function of smi_watcher. If it is, we will install the IPMI
opregion handler and enable ACPI to communicate with BMC.
V7->V8: Based on Bjorn's comment, use acpi ipmi notifier hook function to
avoid the discovery of ACPI pnp IPMI device again. Then in course of binding
/unbinding ipmi pnp driver with the corresponding device, the notifier chain
function will be called to install/uninstall the ACPI IPMI opregion space
handler.
V6->V7: Based on Corey Minyard's comments, the IPMI opregion handler is put
into the separate file again as it only uses the API interface provided
by IPMI system. Now it is put into the ACPI subsystem.
V5->V6: Adjust the patch order and refresh the patch set.
V4->V5: According to Bjorn's comment, remove the unnecessary comment.
The debug info will be printed by using dev_err/dev_warn instead of by
using printk directly.
V3->V4: According to Bjorn's comment, delete the meaningless variable
initialization and check. We also do some cleanup.
V2->V3: According to Bjorn's comment, this IPMI opregion code is put into
the IPMI subsystem(ipmi_si_intf.c). In such case the part of IPMI opregion
code will be installed automatically when IPMI detection module is loaded.
When the IPMI system interface is detected by loading PNP device driver, we
will record every IPMI system interface defined in ACPI namespace and then
install the corresponding IPMI opregion space handler, which is used to enable
ACPI AML code to access the BMC controller.
V1->V2: According to Bjorn's comment, we won't install the IPMI space handler
by loading an ACPI device driver. Instead we will enumerate the ACPI IPMI
device directly in ACPI device tree and then install the IPMI space handler.
Then ACPI AML code and access the BMC controller through the IPMI space
handler.
Best regards.
Yakui
^ permalink raw reply [flat|nested] 24+ messages in thread
* [RFC PATCH 1/4] IPMI: Add one interface to get more info of low-level IPMI device
2010-10-26 9:14 [RFC PATCH 0/4_v11] IPMI/ACPI: Install the ACPI IPMI opregion yakui.zhao
@ 2010-10-26 9:14 ` yakui.zhao
2010-10-26 9:14 ` [RFC PATCH 2/4] IPMI: Remove the redundant definition of ipmi_addr_src yakui.zhao
2010-10-27 16:13 ` [RFC PATCH 1/4] IPMI: Add one interface to get more info of low-level IPMI device Corey Minyard
0 siblings, 2 replies; 24+ messages in thread
From: yakui.zhao @ 2010-10-26 9:14 UTC (permalink / raw)
To: openipmi-developer, linux-acpi; +Cc: minyard, lenb, Zhao Yakui
From: Zhao Yakui <yakui.zhao@intel.com>
The IPMI smi_watcher will be used to catch the IPMI interface as they come or go.
In order to communicate with the correct IPMI device, it should be confirmed
whether it is what we wanted especially on the system with multiple IPMI
devices. But the new_smi callback function of smi_watcher provides very
limited info(only the interface number and dev pointer) and there is no
detailed info about the low level interface. For example: which mechansim
registers the IPMI interface(ACPI, PCI, DMI and so on).
This is to add one interface that can get more info of low-level IPMI
device. For example: the ACPI device handle will be returned for the pnp_acpi
IPMI device.
Signed-off-by: Zhao Yakui <yakui.zhao@intel.com>
---
drivers/char/ipmi/ipmi_msghandler.c | 46 ++++++++++++++++++++++++++++++++
drivers/char/ipmi/ipmi_si_intf.c | 49 +++++++++++++++++++++++++++++++---
include/linux/ipmi.h | 29 ++++++++++++++++++++
include/linux/ipmi_smi.h | 8 +++++
4 files changed, 127 insertions(+), 5 deletions(-)
diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
index 4f3f8c9..e323edb 100644
--- a/drivers/char/ipmi/ipmi_msghandler.c
+++ b/drivers/char/ipmi/ipmi_msghandler.c
@@ -970,6 +970,52 @@ out_kfree:
}
EXPORT_SYMBOL(ipmi_create_user);
+int ipmi_get_smi_info(int if_num, struct ipmi_smi_info *data)
+{
+ int rv = 0;
+ ipmi_smi_t intf;
+ struct ipmi_smi_handlers *handlers;
+
+ mutex_lock(&ipmi_interfaces_mutex);
+ list_for_each_entry_rcu(intf, &ipmi_interfaces, link) {
+ if (intf->intf_num == if_num)
+ goto found;
+ }
+ /* Not found, return an error */
+ rv = -EINVAL;
+ mutex_unlock(&ipmi_interfaces_mutex);
+ return rv;
+
+found:
+ handlers = intf->handlers;
+ rv = handlers->get_smi_info(intf->send_info, data);
+ mutex_unlock(&ipmi_interfaces_mutex);
+
+ return rv;
+}
+EXPORT_SYMBOL(ipmi_get_smi_info);
+
+void ipmi_put_smi_info(int if_num)
+{
+ ipmi_smi_t intf;
+ struct ipmi_smi_handlers *handlers;
+
+ mutex_lock(&ipmi_interfaces_mutex);
+ list_for_each_entry_rcu(intf, &ipmi_interfaces, link) {
+ if (intf->intf_num == if_num)
+ goto found;
+ }
+ /* Not found, return */
+ mutex_unlock(&ipmi_interfaces_mutex);
+ return;
+
+found:
+ handlers = intf->handlers;
+ handlers->put_smi_info(intf->send_info);
+ mutex_unlock(&ipmi_interfaces_mutex);
+}
+EXPORT_SYMBOL(ipmi_put_smi_info);
+
static void free_user(struct kref *ref)
{
ipmi_user_t user = container_of(ref, struct ipmi_user, refcount);
diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
index 7bd7c45..d313018 100644
--- a/drivers/char/ipmi/ipmi_si_intf.c
+++ b/drivers/char/ipmi/ipmi_si_intf.c
@@ -57,6 +57,7 @@
#include <asm/irq.h>
#include <linux/interrupt.h>
#include <linux/rcupdate.h>
+#include <linux/ipmi.h>
#include <linux/ipmi_smi.h>
#include <asm/io.h>
#include "ipmi_si_sm.h"
@@ -107,10 +108,6 @@ enum si_type {
};
static char *si_to_str[] = { "kcs", "smic", "bt" };
-enum ipmi_addr_src {
- SI_INVALID = 0, SI_HOTMOD, SI_HARDCODED, SI_SPMI, SI_ACPI, SI_SMBIOS,
- SI_PCI, SI_DEVICETREE, SI_DEFAULT
-};
static char *ipmi_addr_src_to_str[] = { NULL, "hotmod", "hardcoded", "SPMI",
"ACPI", "SMBIOS", "PCI",
"device-tree", "default" };
@@ -291,6 +288,12 @@ struct smi_info {
struct task_struct *thread;
struct list_head link;
+ /*
+ * Count the refcount of smi_data->dev related with the function
+ * of ipmi_get_smi_info/ipmi_put_smi_info.
+ */
+ atomic_t smi_info_ref;
+ struct ipmi_smi_info smi_data;
};
#define smi_inc_stat(smi, stat) \
@@ -1186,6 +1189,27 @@ static int smi_start_processing(void *send_info,
return 0;
}
+static int get_smi_info(void *send_info, struct ipmi_smi_info *data)
+{
+ struct smi_info *new_smi = send_info;
+ struct ipmi_smi_info *smi_data = &new_smi->smi_data;
+
+ get_device(new_smi->dev);
+ memcpy(data, smi_data, sizeof(*smi_data));
+ smi_data->addr_src = new_smi->addr_source;
+ atomic_inc(&new_smi->smi_info_ref);
+
+ return 0;
+}
+
+static void put_smi_info(void *send_info)
+{
+ struct smi_info *new_smi = send_info;
+
+ put_device(new_smi->dev);
+ atomic_dec(&new_smi->smi_info_ref);
+}
+
static void set_maintenance_mode(void *send_info, int enable)
{
struct smi_info *smi_info = send_info;
@@ -1197,6 +1221,8 @@ static void set_maintenance_mode(void *send_info, int enable)
static struct ipmi_smi_handlers handlers = {
.owner = THIS_MODULE,
.start_processing = smi_start_processing,
+ .get_smi_info = get_smi_info,
+ .put_smi_info = put_smi_info,
.sender = sender,
.request_events = request_events,
.set_maintenance_mode = set_maintenance_mode,
@@ -2143,6 +2169,8 @@ static int __devinit ipmi_pnp_probe(struct pnp_dev *dev,
return -ENOMEM;
info->addr_source = SI_ACPI;
+ info->smi_data.addr_info.acpi_info.acpi_handle =
+ acpi_dev->handle;
printk(KERN_INFO PFX "probing via ACPI\n");
handle = acpi_dev->handle;
@@ -3092,6 +3120,7 @@ static int try_smi_init(struct smi_info *new_smi)
{
int rv = 0;
int i;
+ struct ipmi_smi_info *smi_data;
printk(KERN_INFO PFX "Trying %s-specified %s state"
" machine at %s address 0x%lx, slave address 0x%x,"
@@ -3255,6 +3284,9 @@ static int try_smi_init(struct smi_info *new_smi)
dev_info(new_smi->dev, "IPMI %s interface initialized\n",
si_to_str[new_smi->si_type]);
+ smi_data = &new_smi->smi_data;
+ smi_data->dev = new_smi->dev;
+ atomic_set(&new_smi->smi_info_ref, 0);
return 0;
@@ -3501,7 +3533,14 @@ static void cleanup_one_si(struct smi_info *to_clean)
printk(KERN_ERR PFX "Unable to unregister device: errno=%d\n",
rv);
}
-
+ /* maybe the caller doesn't release the refcount of dev, which is
+ * increased by calling the function of ipmi_get_smi_info. So try
+ * to help it.
+ */
+ while (atomic_read(&to_clean->smi_info_ref)) {
+ put_device(to_clean->dev);
+ atomic_dec(&to_clean->smi_info_ref);
+ }
if (to_clean->handlers)
to_clean->handlers->cleanup(to_clean->si_sm);
diff --git a/include/linux/ipmi.h b/include/linux/ipmi.h
index 65aae34..9a0c72e 100644
--- a/include/linux/ipmi.h
+++ b/include/linux/ipmi.h
@@ -454,6 +454,35 @@ unsigned int ipmi_addr_length(int addr_type);
/* Validate that the given IPMI address is valid. */
int ipmi_validate_addr(struct ipmi_addr *addr, int len);
+enum ipmi_addr_src {
+ SI_INVALID = 0, SI_HOTMOD, SI_HARDCODED, SI_SPMI, SI_ACPI, SI_SMBIOS,
+ SI_PCI, SI_DEVICETREE, SI_DEFAULT
+};
+struct ipmi_smi_info {
+ enum ipmi_addr_src addr_src;
+ struct device *dev;
+ /*
+ * The addr_info can provide more detailed info of one IPMI device.
+ * Now only SI_ACPI info is provided. And it depends on the SI_ACPI
+ * address type. If the info is required for other address type, please
+ * add it.
+ */
+ union {
+ /* the acpi_info element is defined for the SI_ACPI
+ * address type
+ */
+ struct {
+ void *acpi_handle;
+ } acpi_info;
+ } addr_info;
+};
+
+/* This is to get the private info of ipmi_smi_t */
+extern int ipmi_get_smi_info(int if_num, struct ipmi_smi_info *data);
+/* This is to decrease refcount of dev added in the function of
+ * ipmi_get_smi_info
+ */
+extern void ipmi_put_smi_info(int if_num);
#endif /* __KERNEL__ */
diff --git a/include/linux/ipmi_smi.h b/include/linux/ipmi_smi.h
index 4b48318..b99942d 100644
--- a/include/linux/ipmi_smi.h
+++ b/include/linux/ipmi_smi.h
@@ -86,6 +86,14 @@ struct ipmi_smi_handlers {
int (*start_processing)(void *send_info,
ipmi_smi_t new_intf);
+ /*
+ * Get the detailed private info of the low level interface and store
+ * it into the structure of ipmi_smi_data. For example: the
+ * ACPI device handle will be returned for the pnp_acpi IPMI device.
+ */
+ int (*get_smi_info)(void *send_info, struct ipmi_smi_info *data);
+
+ void (*put_smi_info)(void *send_info);
/* Called to enqueue an SMI message to be sent. This
operation is not allowed to fail. If an error occurs, it
should report back the error in a received message. It may
--
1.5.4.5
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [RFC PATCH 2/4] IPMI: Remove the redundant definition of ipmi_addr_src
2010-10-26 9:14 ` [RFC PATCH 1/4] IPMI: Add one interface to get more info of low-level IPMI device yakui.zhao
@ 2010-10-26 9:14 ` yakui.zhao
2010-10-26 9:14 ` [RFC PATCH 3/4] IPMI: Add the document description of ipmi_get_smi_info yakui.zhao
2010-10-27 16:13 ` [RFC PATCH 1/4] IPMI: Add one interface to get more info of low-level IPMI device Corey Minyard
1 sibling, 1 reply; 24+ messages in thread
From: yakui.zhao @ 2010-10-26 9:14 UTC (permalink / raw)
To: openipmi-developer, linux-acpi; +Cc: minyard, lenb, Zhao Yakui
From: Zhao Yakui <yakui.zhao@intel.com>
The ipmi_addr_src is defined twice. One is in ipmi_smi_info and the other
is in generic smi_info. Remove the redunant definition in smi_info.
Signed-off-by: Zhao Yakui <yakui.zhao@intel.com>
---
drivers/char/ipmi/ipmi_si_intf.c | 34 ++++++++++++++++------------------
1 files changed, 16 insertions(+), 18 deletions(-)
diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
index d313018..14732a4 100644
--- a/drivers/char/ipmi/ipmi_si_intf.c
+++ b/drivers/char/ipmi/ipmi_si_intf.c
@@ -193,7 +193,6 @@ struct smi_info {
int (*irq_setup)(struct smi_info *info);
void (*irq_cleanup)(struct smi_info *info);
unsigned int io_size;
- enum ipmi_addr_src addr_source; /* ACPI, PCI, SMBIOS, hardcode, etc. */
void (*addr_source_cleanup)(struct smi_info *info);
void *addr_source_data;
@@ -1196,7 +1195,6 @@ static int get_smi_info(void *send_info, struct ipmi_smi_info *data)
get_device(new_smi->dev);
memcpy(data, smi_data, sizeof(*smi_data));
- smi_data->addr_src = new_smi->addr_source;
atomic_inc(&new_smi->smi_info_ref);
return 0;
@@ -1811,7 +1809,7 @@ static int hotmod_handler(const char *val, struct kernel_param *kp)
goto out;
}
- info->addr_source = SI_HOTMOD;
+ info->smi_data.addr_src = SI_HOTMOD;
info->si_type = si_type;
info->io.addr_data = addr;
info->io.addr_type = addr_space;
@@ -1874,7 +1872,7 @@ static __devinit void hardcode_find_bmc(void)
if (!info)
return;
- info->addr_source = SI_HARDCODED;
+ info->smi_data.addr_src = SI_HARDCODED;
printk(KERN_INFO PFX "probing via hardcoded address\n");
if (!si_type[i] || strcmp(si_type[i], "kcs") == 0) {
@@ -2060,7 +2058,7 @@ static __devinit int try_init_spmi(struct SPMITable *spmi)
return -ENOMEM;
}
- info->addr_source = SI_SPMI;
+ info->smi_data.addr_src = SI_SPMI;
printk(KERN_INFO PFX "probing via SPMI\n");
/* Figure out the interface type. */
@@ -2168,7 +2166,7 @@ static int __devinit ipmi_pnp_probe(struct pnp_dev *dev,
if (!info)
return -ENOMEM;
- info->addr_source = SI_ACPI;
+ info->smi_data.addr_src = SI_ACPI;
info->smi_data.addr_info.acpi_info.acpi_handle =
acpi_dev->handle;
printk(KERN_INFO PFX "probing via ACPI\n");
@@ -2353,7 +2351,7 @@ static __devinit void try_init_dmi(struct dmi_ipmi_data *ipmi_data)
return;
}
- info->addr_source = SI_SMBIOS;
+ info->smi_data.addr_src = SI_SMBIOS;
printk(KERN_INFO PFX "probing via SMBIOS\n");
switch (ipmi_data->type) {
@@ -2458,7 +2456,7 @@ static int __devinit ipmi_pci_probe(struct pci_dev *pdev,
if (!info)
return -ENOMEM;
- info->addr_source = SI_PCI;
+ info->smi_data.addr_src = SI_PCI;
dev_info(&pdev->dev, "probing via PCI");
switch (class_type) {
@@ -2604,7 +2602,7 @@ static int __devinit ipmi_of_probe(struct platform_device *dev,
}
info->si_type = (enum si_type) match->data;
- info->addr_source = SI_DEVICETREE;
+ info->smi_data.addr_src = SI_DEVICETREE;
info->irq_setup = std_irq_setup;
if (resource.flags & IORESOURCE_IO) {
@@ -3046,7 +3044,7 @@ static __devinit void default_find_bmc(void)
if (!info)
return;
- info->addr_source = SI_DEFAULT;
+ info->smi_data.addr_src = SI_DEFAULT;
info->si_type = ipmi_defaults[i].type;
info->io_setup = port_setup;
@@ -3093,7 +3091,7 @@ static int add_smi(struct smi_info *new_smi)
int rv = 0;
printk(KERN_INFO PFX "Adding %s-specified %s state machine",
- ipmi_addr_src_to_str[new_smi->addr_source],
+ ipmi_addr_src_to_str[new_smi->smi_data.addr_src],
si_to_str[new_smi->si_type]);
mutex_lock(&smi_infos_lock);
if (!is_new_interface(new_smi)) {
@@ -3125,7 +3123,7 @@ static int try_smi_init(struct smi_info *new_smi)
printk(KERN_INFO PFX "Trying %s-specified %s state"
" machine at %s address 0x%lx, slave address 0x%x,"
" irq %d\n",
- ipmi_addr_src_to_str[new_smi->addr_source],
+ ipmi_addr_src_to_str[new_smi->smi_data.addr_src],
si_to_str[new_smi->si_type],
addr_space_to_str[new_smi->io.addr_type],
new_smi->io.addr_data,
@@ -3173,7 +3171,7 @@ static int try_smi_init(struct smi_info *new_smi)
/* Do low-level detection first. */
if (new_smi->handlers->detect(new_smi->si_sm)) {
- if (new_smi->addr_source)
+ if (new_smi->smi_data.addr_src)
printk(KERN_INFO PFX "Interface detection failed\n");
rv = -ENODEV;
goto out_err;
@@ -3185,7 +3183,7 @@ static int try_smi_init(struct smi_info *new_smi)
*/
rv = try_get_dev_id(new_smi);
if (rv) {
- if (new_smi->addr_source)
+ if (new_smi->smi_data.addr_src)
printk(KERN_INFO PFX "There appears to be no BMC"
" at this location\n");
goto out_err;
@@ -3420,9 +3418,9 @@ static __devinit int init_ipmi_si(void)
/* Try to register a device if it has an IRQ and we either
haven't successfully registered a device yet or this
device has the same type as one we successfully registered */
- if (e->irq && (!type || e->addr_source == type)) {
+ if (e->irq && (!type || e->smi_data.addr_src == type)) {
if (!try_smi_init(e)) {
- type = e->addr_source;
+ type = e->smi_data.addr_src;
}
}
}
@@ -3436,9 +3434,9 @@ static __devinit int init_ipmi_si(void)
/* Fall back to the preferred device */
list_for_each_entry(e, &smi_infos, link) {
- if (!e->irq && (!type || e->addr_source == type)) {
+ if (!e->irq && (!type || e->smi_data.addr_src == type)) {
if (!try_smi_init(e)) {
- type = e->addr_source;
+ type = e->smi_data.addr_src;
}
}
}
--
1.5.4.5
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [RFC PATCH 3/4] IPMI: Add the document description of ipmi_get_smi_info
2010-10-26 9:14 ` [RFC PATCH 2/4] IPMI: Remove the redundant definition of ipmi_addr_src yakui.zhao
@ 2010-10-26 9:14 ` yakui.zhao
2010-10-26 9:14 ` [RFC PATCH 4/4] IPMI/ACPI: Add the IPMI opregion driver to enable ACPI to access BMC controller yakui.zhao
0 siblings, 1 reply; 24+ messages in thread
From: yakui.zhao @ 2010-10-26 9:14 UTC (permalink / raw)
To: openipmi-developer, linux-acpi; +Cc: minyard, lenb, Zhao Yakui
From: Zhao Yakui <yakui.zhao@intel.com>
Add the document description about how to use ipmi_get_smi_info/
ipmi_put_smi_info.
Signed-off-by: Zhao Yakui <yakui.zhao@intel.com>
---
Documentation/IPMI.txt | 41 +++++++++++++++++++++++++++++++++++++++++
1 files changed, 41 insertions(+), 0 deletions(-)
diff --git a/Documentation/IPMI.txt b/Documentation/IPMI.txt
index 69dd29e..4683424 100644
--- a/Documentation/IPMI.txt
+++ b/Documentation/IPMI.txt
@@ -533,6 +533,47 @@ completion during sending a panic event.
Other Pieces
------------
+Get the detailed info related with the IPMI device
+--------
+The IPMI smi_watcher will be used to catch the IPMI interface as they come or go.
+In order to communicate with the correct IPMI device, it should be confirmed
+ whether it is what we wanted especially on the system with multiple IPMI
+devices. But the new_smi callback function of smi_watcher provides very
+limited info(only the interface number and dev pointer) and there is no
+detailed info about the low level interface. For example: which mechansim
+registers the IPMI interface(ACPI, PCI, DMI and so on).
+ The function of ipmi_get_smi_info/ipmi_put_smi_info is added to get the
+detailed info of IPMI device. The following is the struct definition of
+ipmi_smi_info(Now only ACPI info is defined. If the info is required for
+other IPMI address type, please add it).
+struct ipmi_smi_info{
+ enum ipmi_addr_src addr_src;
+ struct device *dev;
+ /*
+ * The addr_info can provide more detailed info of one IPMI device.
+ * Now only SI_ACPI info is provided. And it depends on the SI_ACPI
+ * address type. If the info is required for other address type, please
+ * add it.
+ */
+ union {
+#ifdef CONFIG_ACPI
+ /* the acpi_info element is defined for the SI_ACPI
+ * address type
+ */
+ struct {
+ void *acpi_handle;
+ } acpi_info;
+#endif
+ u8 addr_data[8]; /* This is only to reserve 8 bytes space */
+ } addr_info;
+};
+ It is noted that the dev pointer is included in the above structure
+definition. In order to assure that the device is correctly accessed, the
+increment/decrement of the reference count is considered. The reference count
+is increased in the function of ipmi_get_smi_info while it is decreased in
+the function of ipmi_put_smi_info. In such case the ipmi_put_smi_info must be
+called after the ipmi_get_smi_info is called successfully.
+
Watchdog
--------
--
1.5.4.5
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [RFC PATCH 4/4] IPMI/ACPI: Add the IPMI opregion driver to enable ACPI to access BMC controller
2010-10-26 9:14 ` [RFC PATCH 3/4] IPMI: Add the document description of ipmi_get_smi_info yakui.zhao
@ 2010-10-26 9:14 ` yakui.zhao
0 siblings, 0 replies; 24+ messages in thread
From: yakui.zhao @ 2010-10-26 9:14 UTC (permalink / raw)
To: openipmi-developer, linux-acpi; +Cc: minyard, lenb, Zhao Yakui, Bjorn Helgaas
From: Zhao Yakui <yakui.zhao@intel.com>
ACPI 4.0 spec adds the ACPI IPMI opregion, which means that the ACPI AML
code can also communicate with the BMC controller. This is to install
the ACPI IPMI opregion and enable the ACPI to access the BMC controller
through the IPMI message.
It will create IPMI user interface for every IPMI device detected
in ACPI namespace and install the corresponding IPMI opregion space handler.
Then it can enable ACPI to access the BMC controller through the IPMI
message.
The following describes how to process the IPMI request in IPMI space handler:
1. format the IPMI message based on the request in AML code.
IPMI system address. Now the address type is SYSTEM_INTERFACE_ADDR_TYPE
IPMI net function & command
IPMI message payload
2. send the IPMI message by using the function of ipmi_request_settime
3. wait for the completion of IPMI message. It can be done in different
routes: One is in handled in IPMI user recv callback function. Another is
handled in timeout function.
4. format the IPMI response and return it to ACPI AML code.
At the same time it also addes the module dependency. The ACPI IPMI opregion
will depend on the IPMI subsystem.
Signed-off-by: Zhao Yakui <yakui.zhao@intel.com>
cc: Bjorn Helgaas <bjorn.helgaas@hp.com>
---
drivers/acpi/Kconfig | 11 +
drivers/acpi/Makefile | 1 +
drivers/acpi/acpi_ipmi.c | 518 ++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 530 insertions(+), 0 deletions(-)
create mode 100644 drivers/acpi/acpi_ipmi.c
diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index 3f3489c..a0c0365 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -209,6 +209,17 @@ config ACPI_PROCESSOR
To compile this driver as a module, choose M here:
the module will be called processor.
+config ACPI_IPMI
+ tristate "IPMI"
+ depends on EXPERIMENTAL && IPMI_SI && IPMI_HANDLER
+ default n
+ help
+ This driver enables the ACPI to access the BMC controller. And it
+ uses the IPMI request/response message to communicate with BMC
+ controller, which can be found on on the server.
+
+ To compile this driver as a module, choose M here:
+ the module will be called as acpi_ipmi.
config ACPI_HOTPLUG_CPU
bool
diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index 3d031d0..df4c4f0 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -69,5 +69,6 @@ processor-y += processor_idle.o processor_thermal.o
processor-$(CONFIG_CPU_FREQ) += processor_perflib.o
obj-$(CONFIG_ACPI_PROCESSOR_AGGREGATOR) += acpi_pad.o
+obj-$(CONFIG_ACPI_IPMI) += acpi_ipmi.o
obj-$(CONFIG_ACPI_APEI) += apei/
diff --git a/drivers/acpi/acpi_ipmi.c b/drivers/acpi/acpi_ipmi.c
new file mode 100644
index 0000000..6a99ab2
--- /dev/null
+++ b/drivers/acpi/acpi_ipmi.c
@@ -0,0 +1,518 @@
+/*
+ * acpi_ipmi.c - ACPI IPMI opregion
+ *
+ * Copyright (C) 2010 Intel Corporation
+ * Copyright (C) 2010 Zhao Yakui <yakui.zhao@intel.com>
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or (at
+ * your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/types.h>
+#include <linux/delay.h>
+#include <linux/proc_fs.h>
+#include <linux/seq_file.h>
+#include <linux/interrupt.h>
+#include <linux/list.h>
+#include <linux/spinlock.h>
+#include <linux/io.h>
+#include <acpi/acpi_bus.h>
+#include <acpi/acpi_drivers.h>
+#include <linux/ipmi.h>
+#include <linux/device.h>
+#include <linux/pnp.h>
+
+MODULE_AUTHOR("Zhao Yakui");
+MODULE_DESCRIPTION("ACPI IPMI Opregion driver");
+MODULE_LICENSE("GPL");
+
+#define IPMI_FLAGS_HANDLER_INSTALL 0
+
+#define ACPI_IPMI_OK 0
+#define ACPI_IPMI_TIMEOUT 0x10
+#define ACPI_IPMI_UNKNOWN 0x07
+/* the IPMI timeout is 5s */
+#define IPMI_TIMEOUT (5 * HZ)
+
+struct acpi_ipmi_device {
+ /* the device list attached to driver_data.ipmi_devices */
+ struct list_head head;
+ /* the IPMI request message list */
+ struct list_head tx_msg_list;
+ acpi_handle handle;
+ struct pnp_dev *pnp_dev;
+ ipmi_user_t user_interface;
+ struct mutex mutex_lock;
+ int ipmi_ifnum; /* IPMI interface number */
+ long curr_msgid;
+ unsigned long flags;
+};
+
+struct ipmi_driver_data {
+ struct list_head ipmi_devices;
+ struct ipmi_smi_watcher bmc_events;
+ struct ipmi_user_hndl ipmi_hndlrs;
+};
+
+struct acpi_ipmi_msg {
+ struct list_head head;
+ /*
+ * General speaking the addr type should be SI_ADDR_TYPE. And
+ * the addr channel should be BMC.
+ * In fact it can also be IPMB type. But we will have to
+ * parse it from the Netfn command buffer. It is so complex
+ * that it is skipped.
+ */
+ struct ipmi_addr addr;
+ long tx_msgid;
+ /* it is used to track whether the IPMI message is finished */
+ struct completion tx_complete;
+ struct kernel_ipmi_msg tx_message;
+ int msg_done;
+ /* tx data . And copy it from ACPI object buffer */
+ u8 tx_data[64];
+ int tx_len;
+ u8 rx_data[64];
+ int rx_len;
+ struct acpi_ipmi_device *device;
+};
+
+/* IPMI request/response buffer per ACPI 4.0, sec 5.5.2.4.3.2 */
+struct acpi_ipmi_buffer {
+ u8 status;
+ u8 length;
+ u8 data[64];
+};
+
+static void ipmi_register_bmc(int iface, struct device *dev);
+static void ipmi_bmc_gone(int iface);
+static void ipmi_msg_handler(struct ipmi_recv_msg *msg, void *user_msg_data);
+static void acpi_add_ipmi_device(struct acpi_ipmi_device *ipmi_device);
+static void acpi_remove_ipmi_device(struct acpi_ipmi_device *ipmi_device);
+
+static struct ipmi_driver_data driver_data = {
+ .ipmi_devices = LIST_HEAD_INIT(driver_data.ipmi_devices),
+ .bmc_events = {
+ .owner = THIS_MODULE,
+ .new_smi = ipmi_register_bmc,
+ .smi_gone = ipmi_bmc_gone,
+ },
+ .ipmi_hndlrs = {
+ .ipmi_recv_hndl = ipmi_msg_handler,
+ },
+};
+
+static struct acpi_ipmi_msg *acpi_alloc_ipmi_msg(struct acpi_ipmi_device *ipmi)
+{
+ struct acpi_ipmi_msg *ipmi_msg;
+ struct pnp_dev *pnp_dev = ipmi->pnp_dev;
+
+ ipmi_msg = kzalloc(sizeof(struct acpi_ipmi_msg), GFP_KERNEL);
+ if (!ipmi_msg) {
+ dev_warn(&pnp_dev->dev, "Can't allocate memory for ipmi_msg\n");
+ return NULL;
+ }
+ init_completion(&ipmi_msg->tx_complete);
+ INIT_LIST_HEAD(&ipmi_msg->head);
+ ipmi_msg->device = ipmi;
+ return ipmi_msg;
+}
+
+#define IPMI_OP_RGN_NETFN(offset) ((offset >> 8) & 0xff)
+#define IPMI_OP_RGN_CMD(offset) (offset & 0xff)
+static void acpi_format_ipmi_msg(struct acpi_ipmi_msg *tx_msg,
+ acpi_physical_address address,
+ acpi_integer *value)
+{
+ struct kernel_ipmi_msg *msg;
+ struct acpi_ipmi_buffer *buffer;
+ struct acpi_ipmi_device *device;
+
+ msg = &tx_msg->tx_message;
+ /*
+ * IPMI network function and command are encoded in the address
+ * within the IPMI OpRegion; see ACPI 4.0, sec 5.5.2.4.3.
+ */
+ msg->netfn = IPMI_OP_RGN_NETFN(address);
+ msg->cmd = IPMI_OP_RGN_CMD(address);
+ msg->data = tx_msg->tx_data;
+ /*
+ * value is the parameter passed by the IPMI opregion space handler.
+ * It points to the IPMI request message buffer
+ */
+ buffer = (struct acpi_ipmi_buffer *)value;
+ /* copy the tx message data */
+ msg->data_len = buffer->length;
+ memcpy(tx_msg->tx_data, buffer->data, msg->data_len);
+ /*
+ * now the default type is SYSTEM_INTERFACE and channel type is BMC.
+ * If the netfn is APP_REQUEST and the cmd is SEND_MESSAGE,
+ * the addr type should be changed to IPMB. Then we will have to parse
+ * the IPMI request message buffer to get the IPMB address.
+ * If so, please fix me.
+ */
+ tx_msg->addr.addr_type = IPMI_SYSTEM_INTERFACE_ADDR_TYPE;
+ tx_msg->addr.channel = IPMI_BMC_CHANNEL;
+ tx_msg->addr.data[0] = 0;
+
+ /* Get the msgid */
+ device = tx_msg->device;
+ mutex_lock(&device->mutex_lock);
+ device->curr_msgid++;
+ tx_msg->tx_msgid = device->curr_msgid;
+ mutex_unlock(&device->mutex_lock);
+}
+
+static void acpi_format_ipmi_response(struct acpi_ipmi_msg *msg,
+ acpi_integer *value, int rem_time)
+{
+ struct acpi_ipmi_buffer *buffer;
+
+ /*
+ * value is also used as output parameter. It represents the response
+ * IPMI message returned by IPMI command.
+ */
+ buffer = (struct acpi_ipmi_buffer *)value;
+ if (!rem_time && !msg->msg_done) {
+ buffer->status = ACPI_IPMI_TIMEOUT;
+ return;
+ }
+ /*
+ * If the flag of msg_done is not set or the recv length is zero, it
+ * means that the IPMI command is not executed correctly.
+ * The status code will be ACPI_IPMI_UNKNOWN.
+ */
+ if (!msg->msg_done || !msg->rx_len) {
+ buffer->status = ACPI_IPMI_UNKNOWN;
+ return;
+ }
+ /*
+ * If the IPMI response message is obtained correctly, the status code
+ * will be ACPI_IPMI_OK
+ */
+ buffer->status = ACPI_IPMI_OK;
+ buffer->length = msg->rx_len;
+ memcpy(buffer->data, msg->rx_data, msg->rx_len);
+}
+
+static void ipmi_flush_tx_msg(struct acpi_ipmi_device *ipmi)
+{
+ struct acpi_ipmi_msg *tx_msg, *temp;
+ int count = HZ / 10;
+ struct pnp_dev *pnp_dev = ipmi->pnp_dev;
+
+ list_for_each_entry_safe(tx_msg, temp, &ipmi->tx_msg_list, head) {
+ /* wake up the sleep thread on the Tx msg */
+ complete(&tx_msg->tx_complete);
+ }
+
+ /* wait for about 100ms to flush the tx message list */
+ while (count--) {
+ if (list_empty(&ipmi->tx_msg_list))
+ break;
+ schedule_timeout(1);
+ }
+ if (!list_empty(&ipmi->tx_msg_list))
+ dev_warn(&pnp_dev->dev, "tx msg list is not NULL\n");
+}
+
+static void ipmi_msg_handler(struct ipmi_recv_msg *msg, void *user_msg_data)
+{
+ struct acpi_ipmi_device *ipmi_device = user_msg_data;
+ int msg_found = 0;
+ struct acpi_ipmi_msg *tx_msg;
+ struct pnp_dev *pnp_dev = ipmi_device->pnp_dev;
+
+ if (msg->user != ipmi_device->user_interface) {
+ dev_warn(&pnp_dev->dev, "Unexpected response is returned. "
+ "returned user %p, expected user %p\n",
+ msg->user, ipmi_device->user_interface);
+ ipmi_free_recv_msg(msg);
+ return;
+ }
+ mutex_lock(&ipmi_device->mutex_lock);
+ list_for_each_entry(tx_msg, &ipmi_device->tx_msg_list, head) {
+ if (msg->msgid == tx_msg->tx_msgid) {
+ msg_found = 1;
+ break;
+ }
+ }
+
+ mutex_unlock(&ipmi_device->mutex_lock);
+ if (!msg_found) {
+ dev_warn(&pnp_dev->dev, "Unexpected response (msg id %ld) is "
+ "returned.\n", msg->msgid);
+ ipmi_free_recv_msg(msg);
+ return;
+ }
+
+ if (msg->msg.data_len) {
+ /* copy the response data to Rx_data buffer */
+ memcpy(tx_msg->rx_data, msg->msg_data, msg->msg.data_len);
+ tx_msg->rx_len = msg->msg.data_len;
+ tx_msg->msg_done = 1;
+ }
+ complete(&tx_msg->tx_complete);
+ ipmi_free_recv_msg(msg);
+};
+
+static void ipmi_register_bmc(int iface, struct device *dev)
+{
+ struct acpi_ipmi_device *ipmi_device, *temp;
+ struct pnp_dev *pnp_dev;
+ ipmi_user_t user;
+ int err;
+ struct ipmi_smi_info smi_data;
+ acpi_handle handle;
+
+ err = ipmi_get_smi_info(iface, &smi_data);
+
+ if (err)
+ return;
+
+ if (smi_data.addr_src != SI_ACPI) {
+ ipmi_put_smi_info(iface);
+ return;
+ }
+
+ handle = smi_data.addr_info.acpi_info.acpi_handle;
+
+ list_for_each_entry(temp, &driver_data.ipmi_devices, head) {
+ /*
+ * if the corresponding ACPI handle is already added
+ * to the device list, don't add it again.
+ */
+ if (temp->handle == handle)
+ goto out;
+ }
+
+ ipmi_device = kzalloc(sizeof(*ipmi_device), GFP_KERNEL);
+
+ if (!ipmi_device)
+ goto out;
+
+ pnp_dev = to_pnp_dev(smi_data.dev);
+ ipmi_device->handle = handle;
+ ipmi_device->pnp_dev = pnp_dev;
+
+ err = ipmi_create_user(iface, &driver_data.ipmi_hndlrs,
+ ipmi_device, &user);
+ if (err) {
+ dev_warn(&pnp_dev->dev, "Can't create IPMI user interface\n");
+ kfree(ipmi_device);
+ goto out;
+ }
+ acpi_add_ipmi_device(ipmi_device);
+ ipmi_device->user_interface = user;
+ ipmi_device->ipmi_ifnum = iface;
+ return;
+
+out:
+ ipmi_put_smi_info(iface);
+ return;
+}
+
+static void ipmi_bmc_gone(int iface)
+{
+ struct acpi_ipmi_device *ipmi_device, *temp;
+
+ list_for_each_entry_safe(ipmi_device, temp,
+ &driver_data.ipmi_devices, head) {
+ if (ipmi_device->ipmi_ifnum != iface)
+ continue;
+
+ if (ipmi_device->user_interface) {
+ ipmi_destroy_user(ipmi_device->user_interface);
+ ipmi_device->user_interface = NULL;
+ ipmi_flush_tx_msg(ipmi_device);
+ }
+ ipmi_put_smi_info(iface);
+ acpi_remove_ipmi_device(ipmi_device);
+ kfree(ipmi_device);
+
+ break;
+ }
+}
+/* --------------------------------------------------------------------------
+ * Address Space Management
+ * -------------------------------------------------------------------------- */
+/*
+ * This is the IPMI opregion space handler.
+ * @function: indicates the read/write. In fact as the IPMI message is driven
+ * by command, only write is meaningful.
+ * @address: This contains the netfn/command of IPMI request message.
+ * @bits : not used.
+ * @value : it is an in/out parameter. It points to the IPMI message buffer.
+ * Before the IPMI message is sent, it represents the actual request
+ * IPMI message. After the IPMI message is finished, it represents
+ * the response IPMI message returned by IPMI command.
+ * @handler_context: IPMI device context.
+ */
+
+static acpi_status
+acpi_ipmi_space_handler(u32 function, acpi_physical_address address,
+ u32 bits, acpi_integer *value,
+ void *handler_context, void *region_context)
+{
+ struct acpi_ipmi_msg *tx_msg;
+ struct acpi_ipmi_device *ipmi_device = handler_context;
+ int err, rem_time;
+ acpi_status status;
+ /*
+ * IPMI opregion message.
+ * IPMI message is firstly written to the BMC and system software
+ * can get the respsonse. So it is unmeaningful for the read access
+ * of IPMI opregion.
+ */
+ if ((function & ACPI_IO_MASK) == ACPI_READ)
+ return AE_TYPE;
+
+ if (!ipmi_device->user_interface)
+ return AE_NOT_EXIST;
+
+ tx_msg = acpi_alloc_ipmi_msg(ipmi_device);
+ if (!tx_msg)
+ return AE_NO_MEMORY;
+
+ acpi_format_ipmi_msg(tx_msg, address, value);
+ mutex_lock(&ipmi_device->mutex_lock);
+ list_add_tail(&tx_msg->head, &ipmi_device->tx_msg_list);
+ mutex_unlock(&ipmi_device->mutex_lock);
+ err = ipmi_request_settime(ipmi_device->user_interface,
+ &tx_msg->addr,
+ tx_msg->tx_msgid,
+ &tx_msg->tx_message,
+ NULL, 0, 0, 0);
+ if (err) {
+ status = AE_ERROR;
+ goto end_label;
+ }
+ rem_time = wait_for_completion_timeout(&tx_msg->tx_complete,
+ IPMI_TIMEOUT);
+ acpi_format_ipmi_response(tx_msg, value, rem_time);
+ status = AE_OK;
+
+end_label:
+ mutex_lock(&ipmi_device->mutex_lock);
+ list_del(&tx_msg->head);
+ mutex_unlock(&ipmi_device->mutex_lock);
+ kfree(tx_msg);
+ return status;
+}
+
+static void ipmi_remove_space_handler(struct acpi_ipmi_device *ipmi)
+{
+ if (!test_bit(IPMI_FLAGS_HANDLER_INSTALL, &ipmi->flags))
+ return;
+
+ acpi_remove_address_space_handler(ipmi->handle,
+ ACPI_ADR_SPACE_IPMI, &acpi_ipmi_space_handler);
+
+ clear_bit(IPMI_FLAGS_HANDLER_INSTALL, &ipmi->flags);
+}
+
+static int ipmi_install_space_handler(struct acpi_ipmi_device *ipmi)
+{
+ acpi_status status;
+
+ if (test_bit(IPMI_FLAGS_HANDLER_INSTALL, &ipmi->flags))
+ return 0;
+
+ status = acpi_install_address_space_handler(ipmi->handle,
+ ACPI_ADR_SPACE_IPMI,
+ &acpi_ipmi_space_handler,
+ NULL, ipmi);
+ if (ACPI_FAILURE(status)) {
+ struct pnp_dev *pnp_dev = ipmi->pnp_dev;
+ dev_warn(&pnp_dev->dev, "Can't register IPMI opregion space "
+ "handle\n");
+ return -EINVAL;
+ }
+ set_bit(IPMI_FLAGS_HANDLER_INSTALL, &ipmi->flags);
+ return 0;
+}
+
+static void acpi_add_ipmi_device(struct acpi_ipmi_device *ipmi_device)
+{
+
+ INIT_LIST_HEAD(&ipmi_device->head);
+ list_add_tail(&ipmi_device->head, &driver_data.ipmi_devices);
+
+ mutex_init(&ipmi_device->mutex_lock);
+ INIT_LIST_HEAD(&ipmi_device->tx_msg_list);
+ ipmi_install_space_handler(ipmi_device);
+}
+
+static void acpi_remove_ipmi_device(struct acpi_ipmi_device *ipmi_device)
+{
+ /*
+ * If the IPMI user interface is created, it should be
+ * destroyed.
+ */
+ if (ipmi_device->user_interface) {
+ ipmi_destroy_user(ipmi_device->user_interface);
+ ipmi_device->user_interface = NULL;
+ }
+ /* flush the Tx_msg list */
+ if (!list_empty(&ipmi_device->tx_msg_list))
+ ipmi_flush_tx_msg(ipmi_device);
+
+ list_del(&ipmi_device->head);
+ ipmi_remove_space_handler(ipmi_device);
+}
+
+static int __init acpi_ipmi_init(void)
+{
+ int result = 0;
+
+ if (acpi_disabled)
+ return result;
+
+ result = ipmi_smi_watcher_register(&driver_data.bmc_events);
+
+ return result;
+}
+
+static void __exit acpi_ipmi_exit(void)
+{
+ struct acpi_ipmi_device *ipmi_device, *temp;
+
+ if (acpi_disabled)
+ return;
+
+ ipmi_smi_watcher_unregister(&driver_data.bmc_events);
+
+ /*
+ * When one smi_watcher is unregistered, it is only deleted
+ * from the smi_watcher list. But the smi_gone callback function
+ * is not called. So explicitly uninstall the ACPI IPMI oregion
+ * handler and free it.
+ */
+ list_for_each_entry_safe(ipmi_device, temp,
+ &driver_data.ipmi_devices, head) {
+ acpi_remove_ipmi_device(ipmi_device);
+ ipmi_put_smi_info(ipmi_device->ipmi_ifnum);
+ kfree(ipmi_device);
+ }
+}
+
+module_init(acpi_ipmi_init);
+module_exit(acpi_ipmi_exit);
--
1.5.4.5
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [RFC PATCH 1/4] IPMI: Add one interface to get more info of low-level IPMI device
2010-10-26 9:14 ` [RFC PATCH 1/4] IPMI: Add one interface to get more info of low-level IPMI device yakui.zhao
2010-10-26 9:14 ` [RFC PATCH 2/4] IPMI: Remove the redundant definition of ipmi_addr_src yakui.zhao
@ 2010-10-27 16:13 ` Corey Minyard
2010-10-28 1:00 ` ykzhao
1 sibling, 1 reply; 24+ messages in thread
From: Corey Minyard @ 2010-10-27 16:13 UTC (permalink / raw)
To: yakui.zhao; +Cc: linux-acpi, openipmi-developer, lenb
I think you miss the point of a refcount. The refcount is to keep the
data structure around even if the device has gone away, so that any
users may not crash. Just having a counter in the IPMI structure and
then decrementing the device refcount is not going to accomplish anything.
And you don't really need to refcount the IPMI structure. (And if you
did, you would need to use a refcount structure and you would need to
keep from freeing the IPMI structure until the refcount went to zero.)
You just need to increment the refcount on the device structure, and
whoever gets the device structure needs to be responsible for
decrementing it's refcount.
I'd also prefer to not have a struct ipmi_smi_info in the IPMI data
structure. Just pull the data from existing sources.
-corey
On 10/26/2010 04:14 AM, yakui.zhao@intel.com wrote:
> From: Zhao Yakui<yakui.zhao@intel.com>
>
> The IPMI smi_watcher will be used to catch the IPMI interface as they come or go.
> In order to communicate with the correct IPMI device, it should be confirmed
> whether it is what we wanted especially on the system with multiple IPMI
> devices. But the new_smi callback function of smi_watcher provides very
> limited info(only the interface number and dev pointer) and there is no
> detailed info about the low level interface. For example: which mechansim
> registers the IPMI interface(ACPI, PCI, DMI and so on).
>
> This is to add one interface that can get more info of low-level IPMI
> device. For example: the ACPI device handle will be returned for the pnp_acpi
> IPMI device.
>
> Signed-off-by: Zhao Yakui<yakui.zhao@intel.com>
> ---
> drivers/char/ipmi/ipmi_msghandler.c | 46 ++++++++++++++++++++++++++++++++
> drivers/char/ipmi/ipmi_si_intf.c | 49 +++++++++++++++++++++++++++++++---
> include/linux/ipmi.h | 29 ++++++++++++++++++++
> include/linux/ipmi_smi.h | 8 +++++
> 4 files changed, 127 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
> index 4f3f8c9..e323edb 100644
> --- a/drivers/char/ipmi/ipmi_msghandler.c
> +++ b/drivers/char/ipmi/ipmi_msghandler.c
> @@ -970,6 +970,52 @@ out_kfree:
> }
> EXPORT_SYMBOL(ipmi_create_user);
>
> +int ipmi_get_smi_info(int if_num, struct ipmi_smi_info *data)
> +{
> + int rv = 0;
> + ipmi_smi_t intf;
> + struct ipmi_smi_handlers *handlers;
> +
> + mutex_lock(&ipmi_interfaces_mutex);
> + list_for_each_entry_rcu(intf,&ipmi_interfaces, link) {
> + if (intf->intf_num == if_num)
> + goto found;
> + }
> + /* Not found, return an error */
> + rv = -EINVAL;
> + mutex_unlock(&ipmi_interfaces_mutex);
> + return rv;
> +
> +found:
> + handlers = intf->handlers;
> + rv = handlers->get_smi_info(intf->send_info, data);
> + mutex_unlock(&ipmi_interfaces_mutex);
> +
> + return rv;
> +}
> +EXPORT_SYMBOL(ipmi_get_smi_info);
> +
> +void ipmi_put_smi_info(int if_num)
> +{
> + ipmi_smi_t intf;
> + struct ipmi_smi_handlers *handlers;
> +
> + mutex_lock(&ipmi_interfaces_mutex);
> + list_for_each_entry_rcu(intf,&ipmi_interfaces, link) {
> + if (intf->intf_num == if_num)
> + goto found;
> + }
> + /* Not found, return */
> + mutex_unlock(&ipmi_interfaces_mutex);
> + return;
> +
> +found:
> + handlers = intf->handlers;
> + handlers->put_smi_info(intf->send_info);
> + mutex_unlock(&ipmi_interfaces_mutex);
> +}
> +EXPORT_SYMBOL(ipmi_put_smi_info);
> +
> static void free_user(struct kref *ref)
> {
> ipmi_user_t user = container_of(ref, struct ipmi_user, refcount);
> diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
> index 7bd7c45..d313018 100644
> --- a/drivers/char/ipmi/ipmi_si_intf.c
> +++ b/drivers/char/ipmi/ipmi_si_intf.c
> @@ -57,6 +57,7 @@
> #include<asm/irq.h>
> #include<linux/interrupt.h>
> #include<linux/rcupdate.h>
> +#include<linux/ipmi.h>
> #include<linux/ipmi_smi.h>
> #include<asm/io.h>
> #include "ipmi_si_sm.h"
> @@ -107,10 +108,6 @@ enum si_type {
> };
> static char *si_to_str[] = { "kcs", "smic", "bt" };
>
> -enum ipmi_addr_src {
> - SI_INVALID = 0, SI_HOTMOD, SI_HARDCODED, SI_SPMI, SI_ACPI, SI_SMBIOS,
> - SI_PCI, SI_DEVICETREE, SI_DEFAULT
> -};
> static char *ipmi_addr_src_to_str[] = { NULL, "hotmod", "hardcoded", "SPMI",
> "ACPI", "SMBIOS", "PCI",
> "device-tree", "default" };
> @@ -291,6 +288,12 @@ struct smi_info {
> struct task_struct *thread;
>
> struct list_head link;
> + /*
> + * Count the refcount of smi_data->dev related with the function
> + * of ipmi_get_smi_info/ipmi_put_smi_info.
> + */
> + atomic_t smi_info_ref;
> + struct ipmi_smi_info smi_data;
> };
>
> #define smi_inc_stat(smi, stat) \
> @@ -1186,6 +1189,27 @@ static int smi_start_processing(void *send_info,
> return 0;
> }
>
> +static int get_smi_info(void *send_info, struct ipmi_smi_info *data)
> +{
> + struct smi_info *new_smi = send_info;
> + struct ipmi_smi_info *smi_data =&new_smi->smi_data;
> +
> + get_device(new_smi->dev);
> + memcpy(data, smi_data, sizeof(*smi_data));
> + smi_data->addr_src = new_smi->addr_source;
> + atomic_inc(&new_smi->smi_info_ref);
> +
> + return 0;
> +}
> +
> +static void put_smi_info(void *send_info)
> +{
> + struct smi_info *new_smi = send_info;
> +
> + put_device(new_smi->dev);
> + atomic_dec(&new_smi->smi_info_ref);
> +}
> +
> static void set_maintenance_mode(void *send_info, int enable)
> {
> struct smi_info *smi_info = send_info;
> @@ -1197,6 +1221,8 @@ static void set_maintenance_mode(void *send_info, int enable)
> static struct ipmi_smi_handlers handlers = {
> .owner = THIS_MODULE,
> .start_processing = smi_start_processing,
> + .get_smi_info = get_smi_info,
> + .put_smi_info = put_smi_info,
> .sender = sender,
> .request_events = request_events,
> .set_maintenance_mode = set_maintenance_mode,
> @@ -2143,6 +2169,8 @@ static int __devinit ipmi_pnp_probe(struct pnp_dev *dev,
> return -ENOMEM;
>
> info->addr_source = SI_ACPI;
> + info->smi_data.addr_info.acpi_info.acpi_handle =
> + acpi_dev->handle;
> printk(KERN_INFO PFX "probing via ACPI\n");
>
> handle = acpi_dev->handle;
> @@ -3092,6 +3120,7 @@ static int try_smi_init(struct smi_info *new_smi)
> {
> int rv = 0;
> int i;
> + struct ipmi_smi_info *smi_data;
>
> printk(KERN_INFO PFX "Trying %s-specified %s state"
> " machine at %s address 0x%lx, slave address 0x%x,"
> @@ -3255,6 +3284,9 @@ static int try_smi_init(struct smi_info *new_smi)
>
> dev_info(new_smi->dev, "IPMI %s interface initialized\n",
> si_to_str[new_smi->si_type]);
> + smi_data =&new_smi->smi_data;
> + smi_data->dev = new_smi->dev;
> + atomic_set(&new_smi->smi_info_ref, 0);
>
> return 0;
>
> @@ -3501,7 +3533,14 @@ static void cleanup_one_si(struct smi_info *to_clean)
> printk(KERN_ERR PFX "Unable to unregister device: errno=%d\n",
> rv);
> }
> -
> + /* maybe the caller doesn't release the refcount of dev, which is
> + * increased by calling the function of ipmi_get_smi_info. So try
> + * to help it.
> + */
> + while (atomic_read(&to_clean->smi_info_ref)) {
> + put_device(to_clean->dev);
> + atomic_dec(&to_clean->smi_info_ref);
> + }
> if (to_clean->handlers)
> to_clean->handlers->cleanup(to_clean->si_sm);
>
> diff --git a/include/linux/ipmi.h b/include/linux/ipmi.h
> index 65aae34..9a0c72e 100644
> --- a/include/linux/ipmi.h
> +++ b/include/linux/ipmi.h
> @@ -454,6 +454,35 @@ unsigned int ipmi_addr_length(int addr_type);
> /* Validate that the given IPMI address is valid. */
> int ipmi_validate_addr(struct ipmi_addr *addr, int len);
>
> +enum ipmi_addr_src {
> + SI_INVALID = 0, SI_HOTMOD, SI_HARDCODED, SI_SPMI, SI_ACPI, SI_SMBIOS,
> + SI_PCI, SI_DEVICETREE, SI_DEFAULT
> +};
> +struct ipmi_smi_info {
> + enum ipmi_addr_src addr_src;
> + struct device *dev;
> + /*
> + * The addr_info can provide more detailed info of one IPMI device.
> + * Now only SI_ACPI info is provided. And it depends on the SI_ACPI
> + * address type. If the info is required for other address type, please
> + * add it.
> + */
> + union {
> + /* the acpi_info element is defined for the SI_ACPI
> + * address type
> + */
> + struct {
> + void *acpi_handle;
> + } acpi_info;
> + } addr_info;
> +};
> +
> +/* This is to get the private info of ipmi_smi_t */
> +extern int ipmi_get_smi_info(int if_num, struct ipmi_smi_info *data);
> +/* This is to decrease refcount of dev added in the function of
> + * ipmi_get_smi_info
> + */
> +extern void ipmi_put_smi_info(int if_num);
> #endif /* __KERNEL__ */
>
>
> diff --git a/include/linux/ipmi_smi.h b/include/linux/ipmi_smi.h
> index 4b48318..b99942d 100644
> --- a/include/linux/ipmi_smi.h
> +++ b/include/linux/ipmi_smi.h
> @@ -86,6 +86,14 @@ struct ipmi_smi_handlers {
> int (*start_processing)(void *send_info,
> ipmi_smi_t new_intf);
>
> + /*
> + * Get the detailed private info of the low level interface and store
> + * it into the structure of ipmi_smi_data. For example: the
> + * ACPI device handle will be returned for the pnp_acpi IPMI device.
> + */
> + int (*get_smi_info)(void *send_info, struct ipmi_smi_info *data);
> +
> + void (*put_smi_info)(void *send_info);
> /* Called to enqueue an SMI message to be sent. This
> operation is not allowed to fail. If an error occurs, it
> should report back the error in a received message. It may
>
------------------------------------------------------------------------------
Nokia and AT&T present the 2010 Calling All Innovators-North America contest
Create new apps & games for the Nokia N8 for consumers in U.S. and Canada
$10 million total in prizes - $4M cash, 500 devices, nearly $6M in marketing
Develop with Nokia Qt SDK, Web Runtime, or Java and Publish to Ovi Store
http://p.sf.net/sfu/nokia-dev2dev
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC PATCH 1/4] IPMI: Add one interface to get more info of low-level IPMI device
2010-10-27 16:13 ` [RFC PATCH 1/4] IPMI: Add one interface to get more info of low-level IPMI device Corey Minyard
@ 2010-10-28 1:00 ` ykzhao
2010-11-02 5:33 ` ykzhao
0 siblings, 1 reply; 24+ messages in thread
From: ykzhao @ 2010-10-28 1:00 UTC (permalink / raw)
To: Corey Minyard; +Cc: openipmi-developer, linux-acpi, lenb
On Thu, 2010-10-28 at 00:13 +0800, Corey Minyard wrote:
> I think you miss the point of a refcount. The refcount is to keep the
> data structure around even if the device has gone away, so that any
> users may not crash. Just having a counter in the IPMI structure and
> then decrementing the device refcount is not going to accomplish anything.
>
> And you don't really need to refcount the IPMI structure. (And if you
> did, you would need to use a refcount structure and you would need to
> keep from freeing the IPMI structure until the refcount went to zero.)
> You just need to increment the refcount on the device structure, and
> whoever gets the device structure needs to be responsible for
> decrementing it's refcount.
Yes. After the copy mechanism is used, it is unnecessary to use the
refcount to protect the corresponding data structure.
In fact the purpose of adding another one refcount is to add/decrease
the refcount of dev in course of calling the function of
ipmi_get_smi_info/ipmi_put_smi_info.
But after I look at the smi_watcher mechanism, I find that the
refcount of dev is not released correctly in the following case.
Step 1: Other driver registers one smi_watcher and the
ipmi_get_smi_info is called in the new_smi callback function. (And the
driver will use the dev pointer before the ipmi_put_smi_info is
called).
Step 2: for any reason, the IPMI device will be unregistered by calling
cleanup_one_si. This function will remove it from the IPMI device list
firstly and then call the smi_gone for every smi_watcher.
Step 3: the ipmi_put_smi_info is called in smi_gone callback function.
In such case the ipmi_put_smi_info can't find the corresponding IPMI
device based on the given if_num and can't release the pointer of dev.
But after adding another refcount, we can handle the above scenario. At
the same time even when the ipmi_put_smi_info is not called by the
caller, it still can work.
Not sure whether the above thought is redundant?
> I'd also prefer to not have a struct ipmi_smi_info in the IPMI data
> structure. Just pull the data from existing sources.
At most cases the requirement is different for the different addr source
type. If we don't put the ipmi_smi_info in the IPMI data structure, I am
afraid that we will have to fill the corresponding info based on the
addr source type. Maybe it will be more complex.
Thanks.
Yakui
>
> -corey
>
> On 10/26/2010 04:14 AM, yakui.zhao@intel.com wrote:
> > From: Zhao Yakui<yakui.zhao@intel.com>
> >
> > The IPMI smi_watcher will be used to catch the IPMI interface as they come or go.
> > In order to communicate with the correct IPMI device, it should be confirmed
> > whether it is what we wanted especially on the system with multiple IPMI
> > devices. But the new_smi callback function of smi_watcher provides very
> > limited info(only the interface number and dev pointer) and there is no
> > detailed info about the low level interface. For example: which mechansim
> > registers the IPMI interface(ACPI, PCI, DMI and so on).
> >
> > This is to add one interface that can get more info of low-level IPMI
> > device. For example: the ACPI device handle will be returned for the pnp_acpi
> > IPMI device.
> >
> > Signed-off-by: Zhao Yakui<yakui.zhao@intel.com>
> > ---
> > drivers/char/ipmi/ipmi_msghandler.c | 46 ++++++++++++++++++++++++++++++++
> > drivers/char/ipmi/ipmi_si_intf.c | 49 +++++++++++++++++++++++++++++++---
> > include/linux/ipmi.h | 29 ++++++++++++++++++++
> > include/linux/ipmi_smi.h | 8 +++++
> > 4 files changed, 127 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
> > index 4f3f8c9..e323edb 100644
> > --- a/drivers/char/ipmi/ipmi_msghandler.c
> > +++ b/drivers/char/ipmi/ipmi_msghandler.c
> > @@ -970,6 +970,52 @@ out_kfree:
> > }
> > EXPORT_SYMBOL(ipmi_create_user);
> >
> > +int ipmi_get_smi_info(int if_num, struct ipmi_smi_info *data)
> > +{
> > + int rv = 0;
> > + ipmi_smi_t intf;
> > + struct ipmi_smi_handlers *handlers;
> > +
> > + mutex_lock(&ipmi_interfaces_mutex);
> > + list_for_each_entry_rcu(intf,&ipmi_interfaces, link) {
> > + if (intf->intf_num == if_num)
> > + goto found;
> > + }
> > + /* Not found, return an error */
> > + rv = -EINVAL;
> > + mutex_unlock(&ipmi_interfaces_mutex);
> > + return rv;
> > +
> > +found:
> > + handlers = intf->handlers;
> > + rv = handlers->get_smi_info(intf->send_info, data);
> > + mutex_unlock(&ipmi_interfaces_mutex);
> > +
> > + return rv;
> > +}
> > +EXPORT_SYMBOL(ipmi_get_smi_info);
> > +
> > +void ipmi_put_smi_info(int if_num)
> > +{
> > + ipmi_smi_t intf;
> > + struct ipmi_smi_handlers *handlers;
> > +
> > + mutex_lock(&ipmi_interfaces_mutex);
> > + list_for_each_entry_rcu(intf,&ipmi_interfaces, link) {
> > + if (intf->intf_num == if_num)
> > + goto found;
> > + }
> > + /* Not found, return */
> > + mutex_unlock(&ipmi_interfaces_mutex);
> > + return;
> > +
> > +found:
> > + handlers = intf->handlers;
> > + handlers->put_smi_info(intf->send_info);
> > + mutex_unlock(&ipmi_interfaces_mutex);
> > +}
> > +EXPORT_SYMBOL(ipmi_put_smi_info);
> > +
> > static void free_user(struct kref *ref)
> > {
> > ipmi_user_t user = container_of(ref, struct ipmi_user, refcount);
> > diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
> > index 7bd7c45..d313018 100644
> > --- a/drivers/char/ipmi/ipmi_si_intf.c
> > +++ b/drivers/char/ipmi/ipmi_si_intf.c
> > @@ -57,6 +57,7 @@
> > #include<asm/irq.h>
> > #include<linux/interrupt.h>
> > #include<linux/rcupdate.h>
> > +#include<linux/ipmi.h>
> > #include<linux/ipmi_smi.h>
> > #include<asm/io.h>
> > #include "ipmi_si_sm.h"
> > @@ -107,10 +108,6 @@ enum si_type {
> > };
> > static char *si_to_str[] = { "kcs", "smic", "bt" };
> >
> > -enum ipmi_addr_src {
> > - SI_INVALID = 0, SI_HOTMOD, SI_HARDCODED, SI_SPMI, SI_ACPI, SI_SMBIOS,
> > - SI_PCI, SI_DEVICETREE, SI_DEFAULT
> > -};
> > static char *ipmi_addr_src_to_str[] = { NULL, "hotmod", "hardcoded", "SPMI",
> > "ACPI", "SMBIOS", "PCI",
> > "device-tree", "default" };
> > @@ -291,6 +288,12 @@ struct smi_info {
> > struct task_struct *thread;
> >
> > struct list_head link;
> > + /*
> > + * Count the refcount of smi_data->dev related with the function
> > + * of ipmi_get_smi_info/ipmi_put_smi_info.
> > + */
> > + atomic_t smi_info_ref;
> > + struct ipmi_smi_info smi_data;
> > };
> >
> > #define smi_inc_stat(smi, stat) \
> > @@ -1186,6 +1189,27 @@ static int smi_start_processing(void *send_info,
> > return 0;
> > }
> >
> > +static int get_smi_info(void *send_info, struct ipmi_smi_info *data)
> > +{
> > + struct smi_info *new_smi = send_info;
> > + struct ipmi_smi_info *smi_data =&new_smi->smi_data;
> > +
> > + get_device(new_smi->dev);
> > + memcpy(data, smi_data, sizeof(*smi_data));
> > + smi_data->addr_src = new_smi->addr_source;
> > + atomic_inc(&new_smi->smi_info_ref);
> > +
> > + return 0;
> > +}
> > +
> > +static void put_smi_info(void *send_info)
> > +{
> > + struct smi_info *new_smi = send_info;
> > +
> > + put_device(new_smi->dev);
> > + atomic_dec(&new_smi->smi_info_ref);
> > +}
> > +
> > static void set_maintenance_mode(void *send_info, int enable)
> > {
> > struct smi_info *smi_info = send_info;
> > @@ -1197,6 +1221,8 @@ static void set_maintenance_mode(void *send_info, int enable)
> > static struct ipmi_smi_handlers handlers = {
> > .owner = THIS_MODULE,
> > .start_processing = smi_start_processing,
> > + .get_smi_info = get_smi_info,
> > + .put_smi_info = put_smi_info,
> > .sender = sender,
> > .request_events = request_events,
> > .set_maintenance_mode = set_maintenance_mode,
> > @@ -2143,6 +2169,8 @@ static int __devinit ipmi_pnp_probe(struct pnp_dev *dev,
> > return -ENOMEM;
> >
> > info->addr_source = SI_ACPI;
> > + info->smi_data.addr_info.acpi_info.acpi_handle =
> > + acpi_dev->handle;
> > printk(KERN_INFO PFX "probing via ACPI\n");
> >
> > handle = acpi_dev->handle;
> > @@ -3092,6 +3120,7 @@ static int try_smi_init(struct smi_info *new_smi)
> > {
> > int rv = 0;
> > int i;
> > + struct ipmi_smi_info *smi_data;
> >
> > printk(KERN_INFO PFX "Trying %s-specified %s state"
> > " machine at %s address 0x%lx, slave address 0x%x,"
> > @@ -3255,6 +3284,9 @@ static int try_smi_init(struct smi_info *new_smi)
> >
> > dev_info(new_smi->dev, "IPMI %s interface initialized\n",
> > si_to_str[new_smi->si_type]);
> > + smi_data =&new_smi->smi_data;
> > + smi_data->dev = new_smi->dev;
> > + atomic_set(&new_smi->smi_info_ref, 0);
> >
> > return 0;
> >
> > @@ -3501,7 +3533,14 @@ static void cleanup_one_si(struct smi_info *to_clean)
> > printk(KERN_ERR PFX "Unable to unregister device: errno=%d\n",
> > rv);
> > }
> > -
> > + /* maybe the caller doesn't release the refcount of dev, which is
> > + * increased by calling the function of ipmi_get_smi_info. So try
> > + * to help it.
> > + */
> > + while (atomic_read(&to_clean->smi_info_ref)) {
> > + put_device(to_clean->dev);
> > + atomic_dec(&to_clean->smi_info_ref);
> > + }
> > if (to_clean->handlers)
> > to_clean->handlers->cleanup(to_clean->si_sm);
> >
> > diff --git a/include/linux/ipmi.h b/include/linux/ipmi.h
> > index 65aae34..9a0c72e 100644
> > --- a/include/linux/ipmi.h
> > +++ b/include/linux/ipmi.h
> > @@ -454,6 +454,35 @@ unsigned int ipmi_addr_length(int addr_type);
> > /* Validate that the given IPMI address is valid. */
> > int ipmi_validate_addr(struct ipmi_addr *addr, int len);
> >
> > +enum ipmi_addr_src {
> > + SI_INVALID = 0, SI_HOTMOD, SI_HARDCODED, SI_SPMI, SI_ACPI, SI_SMBIOS,
> > + SI_PCI, SI_DEVICETREE, SI_DEFAULT
> > +};
> > +struct ipmi_smi_info {
> > + enum ipmi_addr_src addr_src;
> > + struct device *dev;
> > + /*
> > + * The addr_info can provide more detailed info of one IPMI device.
> > + * Now only SI_ACPI info is provided. And it depends on the SI_ACPI
> > + * address type. If the info is required for other address type, please
> > + * add it.
> > + */
> > + union {
> > + /* the acpi_info element is defined for the SI_ACPI
> > + * address type
> > + */
> > + struct {
> > + void *acpi_handle;
> > + } acpi_info;
> > + } addr_info;
> > +};
> > +
> > +/* This is to get the private info of ipmi_smi_t */
> > +extern int ipmi_get_smi_info(int if_num, struct ipmi_smi_info *data);
> > +/* This is to decrease refcount of dev added in the function of
> > + * ipmi_get_smi_info
> > + */
> > +extern void ipmi_put_smi_info(int if_num);
> > #endif /* __KERNEL__ */
> >
> >
> > diff --git a/include/linux/ipmi_smi.h b/include/linux/ipmi_smi.h
> > index 4b48318..b99942d 100644
> > --- a/include/linux/ipmi_smi.h
> > +++ b/include/linux/ipmi_smi.h
> > @@ -86,6 +86,14 @@ struct ipmi_smi_handlers {
> > int (*start_processing)(void *send_info,
> > ipmi_smi_t new_intf);
> >
> > + /*
> > + * Get the detailed private info of the low level interface and store
> > + * it into the structure of ipmi_smi_data. For example: the
> > + * ACPI device handle will be returned for the pnp_acpi IPMI device.
> > + */
> > + int (*get_smi_info)(void *send_info, struct ipmi_smi_info *data);
> > +
> > + void (*put_smi_info)(void *send_info);
> > /* Called to enqueue an SMI message to be sent. This
> > operation is not allowed to fail. If an error occurs, it
> > should report back the error in a received message. It may
> >
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC PATCH 1/4] IPMI: Add one interface to get more info of low-level IPMI device
2010-10-28 1:00 ` ykzhao
@ 2010-11-02 5:33 ` ykzhao
2010-11-04 0:41 ` Corey Minyard
0 siblings, 1 reply; 24+ messages in thread
From: ykzhao @ 2010-11-02 5:33 UTC (permalink / raw)
To: Corey Minyard; +Cc: openipmi-developer, linux-acpi, lenb
On Thu, 2010-10-28 at 09:00 +0800, ykzhao wrote:
> On Thu, 2010-10-28 at 00:13 +0800, Corey Minyard wrote:
> > I think you miss the point of a refcount. The refcount is to keep the
> > data structure around even if the device has gone away, so that any
> > users may not crash. Just having a counter in the IPMI structure and
> > then decrementing the device refcount is not going to accomplish anything.
> >
> > And you don't really need to refcount the IPMI structure. (And if you
> > did, you would need to use a refcount structure and you would need to
> > keep from freeing the IPMI structure until the refcount went to zero.)
> > You just need to increment the refcount on the device structure, and
> > whoever gets the device structure needs to be responsible for
> > decrementing it's refcount.
> Yes. After the copy mechanism is used, it is unnecessary to use the
> refcount to protect the corresponding data structure.
>
> In fact the purpose of adding another one refcount is to add/decrease
> the refcount of dev in course of calling the function of
> ipmi_get_smi_info/ipmi_put_smi_info.
> But after I look at the smi_watcher mechanism, I find that the
> refcount of dev is not released correctly in the following case.
> Step 1: Other driver registers one smi_watcher and the
> ipmi_get_smi_info is called in the new_smi callback function. (And the
> driver will use the dev pointer before the ipmi_put_smi_info is
> called).
> Step 2: for any reason, the IPMI device will be unregistered by calling
> cleanup_one_si. This function will remove it from the IPMI device list
> firstly and then call the smi_gone for every smi_watcher.
> Step 3: the ipmi_put_smi_info is called in smi_gone callback function.
> In such case the ipmi_put_smi_info can't find the corresponding IPMI
> device based on the given if_num and can't release the pointer of dev.
>
> But after adding another refcount, we can handle the above scenario. At
> the same time even when the ipmi_put_smi_info is not called by the
> caller, it still can work.
> Not sure whether the above thought is redundant?
Corey,
Is the above thought correct? If not, please correct me. In fact the
purpose of adding another refcount is to fix the incorrect inc/dec in
course of calling ipmi_get_smi_info/ipmi_put_smi_info.
>
>
> > I'd also prefer to not have a struct ipmi_smi_info in the IPMI data
> > structure. Just pull the data from existing sources.
>
> At most cases the requirement is different for the different addr source
> type. If we don't put the ipmi_smi_info in the IPMI data structure, I am
> afraid that we will have to fill the corresponding info based on the
> addr source type. Maybe it will be more complex.
If only pull data from the existing source, it can save some
memory-space. But it seems a bit complex as we have to fill the info
based on the corresponding addr source type. And this should be done
every time the function of ipmi_get_smi_info is called.
>
> Thanks.
> Yakui
>
> >
> > -corey
> >
> > On 10/26/2010 04:14 AM, yakui.zhao@intel.com wrote:
> > > From: Zhao Yakui<yakui.zhao@intel.com>
> > >
> > > The IPMI smi_watcher will be used to catch the IPMI interface as they come or go.
> > > In order to communicate with the correct IPMI device, it should be confirmed
> > > whether it is what we wanted especially on the system with multiple IPMI
> > > devices. But the new_smi callback function of smi_watcher provides very
> > > limited info(only the interface number and dev pointer) and there is no
> > > detailed info about the low level interface. For example: which mechansim
> > > registers the IPMI interface(ACPI, PCI, DMI and so on).
> > >
> > > This is to add one interface that can get more info of low-level IPMI
> > > device. For example: the ACPI device handle will be returned for the pnp_acpi
> > > IPMI device.
> > >
> > > Signed-off-by: Zhao Yakui<yakui.zhao@intel.com>
> > > ---
> > > drivers/char/ipmi/ipmi_msghandler.c | 46 ++++++++++++++++++++++++++++++++
> > > drivers/char/ipmi/ipmi_si_intf.c | 49 +++++++++++++++++++++++++++++++---
> > > include/linux/ipmi.h | 29 ++++++++++++++++++++
> > > include/linux/ipmi_smi.h | 8 +++++
> > > 4 files changed, 127 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
> > > index 4f3f8c9..e323edb 100644
> > > --- a/drivers/char/ipmi/ipmi_msghandler.c
> > > +++ b/drivers/char/ipmi/ipmi_msghandler.c
> > > @@ -970,6 +970,52 @@ out_kfree:
> > > }
> > > EXPORT_SYMBOL(ipmi_create_user);
> > >
> > > +int ipmi_get_smi_info(int if_num, struct ipmi_smi_info *data)
> > > +{
> > > + int rv = 0;
> > > + ipmi_smi_t intf;
> > > + struct ipmi_smi_handlers *handlers;
> > > +
> > > + mutex_lock(&ipmi_interfaces_mutex);
> > > + list_for_each_entry_rcu(intf,&ipmi_interfaces, link) {
> > > + if (intf->intf_num == if_num)
> > > + goto found;
> > > + }
> > > + /* Not found, return an error */
> > > + rv = -EINVAL;
> > > + mutex_unlock(&ipmi_interfaces_mutex);
> > > + return rv;
> > > +
> > > +found:
> > > + handlers = intf->handlers;
> > > + rv = handlers->get_smi_info(intf->send_info, data);
> > > + mutex_unlock(&ipmi_interfaces_mutex);
> > > +
> > > + return rv;
> > > +}
> > > +EXPORT_SYMBOL(ipmi_get_smi_info);
> > > +
> > > +void ipmi_put_smi_info(int if_num)
> > > +{
> > > + ipmi_smi_t intf;
> > > + struct ipmi_smi_handlers *handlers;
> > > +
> > > + mutex_lock(&ipmi_interfaces_mutex);
> > > + list_for_each_entry_rcu(intf,&ipmi_interfaces, link) {
> > > + if (intf->intf_num == if_num)
> > > + goto found;
> > > + }
> > > + /* Not found, return */
> > > + mutex_unlock(&ipmi_interfaces_mutex);
> > > + return;
> > > +
> > > +found:
> > > + handlers = intf->handlers;
> > > + handlers->put_smi_info(intf->send_info);
> > > + mutex_unlock(&ipmi_interfaces_mutex);
> > > +}
> > > +EXPORT_SYMBOL(ipmi_put_smi_info);
> > > +
> > > static void free_user(struct kref *ref)
> > > {
> > > ipmi_user_t user = container_of(ref, struct ipmi_user, refcount);
> > > diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
> > > index 7bd7c45..d313018 100644
> > > --- a/drivers/char/ipmi/ipmi_si_intf.c
> > > +++ b/drivers/char/ipmi/ipmi_si_intf.c
> > > @@ -57,6 +57,7 @@
> > > #include<asm/irq.h>
> > > #include<linux/interrupt.h>
> > > #include<linux/rcupdate.h>
> > > +#include<linux/ipmi.h>
> > > #include<linux/ipmi_smi.h>
> > > #include<asm/io.h>
> > > #include "ipmi_si_sm.h"
> > > @@ -107,10 +108,6 @@ enum si_type {
> > > };
> > > static char *si_to_str[] = { "kcs", "smic", "bt" };
> > >
> > > -enum ipmi_addr_src {
> > > - SI_INVALID = 0, SI_HOTMOD, SI_HARDCODED, SI_SPMI, SI_ACPI, SI_SMBIOS,
> > > - SI_PCI, SI_DEVICETREE, SI_DEFAULT
> > > -};
> > > static char *ipmi_addr_src_to_str[] = { NULL, "hotmod", "hardcoded", "SPMI",
> > > "ACPI", "SMBIOS", "PCI",
> > > "device-tree", "default" };
> > > @@ -291,6 +288,12 @@ struct smi_info {
> > > struct task_struct *thread;
> > >
> > > struct list_head link;
> > > + /*
> > > + * Count the refcount of smi_data->dev related with the function
> > > + * of ipmi_get_smi_info/ipmi_put_smi_info.
> > > + */
> > > + atomic_t smi_info_ref;
> > > + struct ipmi_smi_info smi_data;
> > > };
> > >
> > > #define smi_inc_stat(smi, stat) \
> > > @@ -1186,6 +1189,27 @@ static int smi_start_processing(void *send_info,
> > > return 0;
> > > }
> > >
> > > +static int get_smi_info(void *send_info, struct ipmi_smi_info *data)
> > > +{
> > > + struct smi_info *new_smi = send_info;
> > > + struct ipmi_smi_info *smi_data =&new_smi->smi_data;
> > > +
> > > + get_device(new_smi->dev);
> > > + memcpy(data, smi_data, sizeof(*smi_data));
> > > + smi_data->addr_src = new_smi->addr_source;
> > > + atomic_inc(&new_smi->smi_info_ref);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static void put_smi_info(void *send_info)
> > > +{
> > > + struct smi_info *new_smi = send_info;
> > > +
> > > + put_device(new_smi->dev);
> > > + atomic_dec(&new_smi->smi_info_ref);
> > > +}
> > > +
> > > static void set_maintenance_mode(void *send_info, int enable)
> > > {
> > > struct smi_info *smi_info = send_info;
> > > @@ -1197,6 +1221,8 @@ static void set_maintenance_mode(void *send_info, int enable)
> > > static struct ipmi_smi_handlers handlers = {
> > > .owner = THIS_MODULE,
> > > .start_processing = smi_start_processing,
> > > + .get_smi_info = get_smi_info,
> > > + .put_smi_info = put_smi_info,
> > > .sender = sender,
> > > .request_events = request_events,
> > > .set_maintenance_mode = set_maintenance_mode,
> > > @@ -2143,6 +2169,8 @@ static int __devinit ipmi_pnp_probe(struct pnp_dev *dev,
> > > return -ENOMEM;
> > >
> > > info->addr_source = SI_ACPI;
> > > + info->smi_data.addr_info.acpi_info.acpi_handle =
> > > + acpi_dev->handle;
> > > printk(KERN_INFO PFX "probing via ACPI\n");
> > >
> > > handle = acpi_dev->handle;
> > > @@ -3092,6 +3120,7 @@ static int try_smi_init(struct smi_info *new_smi)
> > > {
> > > int rv = 0;
> > > int i;
> > > + struct ipmi_smi_info *smi_data;
> > >
> > > printk(KERN_INFO PFX "Trying %s-specified %s state"
> > > " machine at %s address 0x%lx, slave address 0x%x,"
> > > @@ -3255,6 +3284,9 @@ static int try_smi_init(struct smi_info *new_smi)
> > >
> > > dev_info(new_smi->dev, "IPMI %s interface initialized\n",
> > > si_to_str[new_smi->si_type]);
> > > + smi_data =&new_smi->smi_data;
> > > + smi_data->dev = new_smi->dev;
> > > + atomic_set(&new_smi->smi_info_ref, 0);
> > >
> > > return 0;
> > >
> > > @@ -3501,7 +3533,14 @@ static void cleanup_one_si(struct smi_info *to_clean)
> > > printk(KERN_ERR PFX "Unable to unregister device: errno=%d\n",
> > > rv);
> > > }
> > > -
> > > + /* maybe the caller doesn't release the refcount of dev, which is
> > > + * increased by calling the function of ipmi_get_smi_info. So try
> > > + * to help it.
> > > + */
> > > + while (atomic_read(&to_clean->smi_info_ref)) {
> > > + put_device(to_clean->dev);
> > > + atomic_dec(&to_clean->smi_info_ref);
> > > + }
> > > if (to_clean->handlers)
> > > to_clean->handlers->cleanup(to_clean->si_sm);
> > >
> > > diff --git a/include/linux/ipmi.h b/include/linux/ipmi.h
> > > index 65aae34..9a0c72e 100644
> > > --- a/include/linux/ipmi.h
> > > +++ b/include/linux/ipmi.h
> > > @@ -454,6 +454,35 @@ unsigned int ipmi_addr_length(int addr_type);
> > > /* Validate that the given IPMI address is valid. */
> > > int ipmi_validate_addr(struct ipmi_addr *addr, int len);
> > >
> > > +enum ipmi_addr_src {
> > > + SI_INVALID = 0, SI_HOTMOD, SI_HARDCODED, SI_SPMI, SI_ACPI, SI_SMBIOS,
> > > + SI_PCI, SI_DEVICETREE, SI_DEFAULT
> > > +};
> > > +struct ipmi_smi_info {
> > > + enum ipmi_addr_src addr_src;
> > > + struct device *dev;
> > > + /*
> > > + * The addr_info can provide more detailed info of one IPMI device.
> > > + * Now only SI_ACPI info is provided. And it depends on the SI_ACPI
> > > + * address type. If the info is required for other address type, please
> > > + * add it.
> > > + */
> > > + union {
> > > + /* the acpi_info element is defined for the SI_ACPI
> > > + * address type
> > > + */
> > > + struct {
> > > + void *acpi_handle;
> > > + } acpi_info;
> > > + } addr_info;
> > > +};
> > > +
> > > +/* This is to get the private info of ipmi_smi_t */
> > > +extern int ipmi_get_smi_info(int if_num, struct ipmi_smi_info *data);
> > > +/* This is to decrease refcount of dev added in the function of
> > > + * ipmi_get_smi_info
> > > + */
> > > +extern void ipmi_put_smi_info(int if_num);
> > > #endif /* __KERNEL__ */
> > >
> > >
> > > diff --git a/include/linux/ipmi_smi.h b/include/linux/ipmi_smi.h
> > > index 4b48318..b99942d 100644
> > > --- a/include/linux/ipmi_smi.h
> > > +++ b/include/linux/ipmi_smi.h
> > > @@ -86,6 +86,14 @@ struct ipmi_smi_handlers {
> > > int (*start_processing)(void *send_info,
> > > ipmi_smi_t new_intf);
> > >
> > > + /*
> > > + * Get the detailed private info of the low level interface and store
> > > + * it into the structure of ipmi_smi_data. For example: the
> > > + * ACPI device handle will be returned for the pnp_acpi IPMI device.
> > > + */
> > > + int (*get_smi_info)(void *send_info, struct ipmi_smi_info *data);
> > > +
> > > + void (*put_smi_info)(void *send_info);
> > > /* Called to enqueue an SMI message to be sent. This
> > > operation is not allowed to fail. If an error occurs, it
> > > should report back the error in a received message. It may
> > >
> >
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC PATCH 1/4] IPMI: Add one interface to get more info of low-level IPMI device
2010-11-02 5:33 ` ykzhao
@ 2010-11-04 0:41 ` Corey Minyard
2010-11-15 7:52 ` ykzhao
0 siblings, 1 reply; 24+ messages in thread
From: Corey Minyard @ 2010-11-04 0:41 UTC (permalink / raw)
To: ykzhao; +Cc: linux-acpi, openipmi-developer, lenb
On 11/02/2010 12:33 AM, ykzhao wrote:
> On Thu, 2010-10-28 at 09:00 +0800, ykzhao wrote:
>
>> On Thu, 2010-10-28 at 00:13 +0800, Corey Minyard wrote:
>>
>>> I think you miss the point of a refcount. The refcount is to keep the
>>> data structure around even if the device has gone away, so that any
>>> users may not crash. Just having a counter in the IPMI structure and
>>> then decrementing the device refcount is not going to accomplish anything.
>>>
>>> And you don't really need to refcount the IPMI structure. (And if you
>>> did, you would need to use a refcount structure and you would need to
>>> keep from freeing the IPMI structure until the refcount went to zero.)
>>> You just need to increment the refcount on the device structure, and
>>> whoever gets the device structure needs to be responsible for
>>> decrementing it's refcount.
>>>
>> Yes. After the copy mechanism is used, it is unnecessary to use the
>> refcount to protect the corresponding data structure.
>>
>> In fact the purpose of adding another one refcount is to add/decrease
>> the refcount of dev in course of calling the function of
>> ipmi_get_smi_info/ipmi_put_smi_info.
>> But after I look at the smi_watcher mechanism, I find that the
>> refcount of dev is not released correctly in the following case.
>> Step 1: Other driver registers one smi_watcher and the
>> ipmi_get_smi_info is called in the new_smi callback function. (And the
>> driver will use the dev pointer before the ipmi_put_smi_info is
>> called).
>> Step 2: for any reason, the IPMI device will be unregistered by calling
>> cleanup_one_si. This function will remove it from the IPMI device list
>> firstly and then call the smi_gone for every smi_watcher.
>> Step 3: the ipmi_put_smi_info is called in smi_gone callback function.
>> In such case the ipmi_put_smi_info can't find the corresponding IPMI
>> device based on the given if_num and can't release the pointer of dev.
>>
>> But after adding another refcount, we can handle the above scenario. At
>> the same time even when the ipmi_put_smi_info is not called by the
>> caller, it still can work.
>> Not sure whether the above thought is redundant?
>>
> Corey,
> Is the above thought correct? If not, please correct me. In fact the
> purpose of adding another refcount is to fix the incorrect inc/dec in
> course of calling ipmi_get_smi_info/ipmi_put_smi_info.
>
No. The purpose of a refcount is to keep from freeing a data structure
while it is in use.
Let's say your code gets the device structure for the IPMI device, then
a task switch occurs. During the time it is not running, the IPMI
device is hotplugged away and no longer exists. The device is deleted.
When your code starts running again, it still has a pointer to that
device structure. Sure, the removal code may have run, but your code
still has a pointer to that device structure. Which has been freed. Bad.
This is what refcounts (well, krefs) do. In the above scenario, your
code will get the device with the kref incremented. All the above
happens, but since you have incremented the refcount on the device
structure, it will not be deleted. It will wait until you put
(decrement) the refcount before performing the free.
>
>>
>>
>>> I'd also prefer to not have a struct ipmi_smi_info in the IPMI data
>>> structure. Just pull the data from existing sources.
>>>
>> At most cases the requirement is different for the different addr source
>> type. If we don't put the ipmi_smi_info in the IPMI data structure, I am
>> afraid that we will have to fill the corresponding info based on the
>> addr source type. Maybe it will be more complex.
>>
> If only pull data from the existing source, it can save some
> memory-space. But it seems a bit complex as we have to fill the info
> based on the corresponding addr source type. And this should be done
> every time the function of ipmi_get_smi_info is called.
>
I don't think it's that complex, and I doubt that function is called
that often.
-corey
>
>> Thanks.
>> Yakui
>>
>>
>>> -corey
>>>
>>> On 10/26/2010 04:14 AM, yakui.zhao@intel.com wrote:
>>>
>>>> From: Zhao Yakui<yakui.zhao@intel.com>
>>>>
>>>> The IPMI smi_watcher will be used to catch the IPMI interface as they come or go.
>>>> In order to communicate with the correct IPMI device, it should be confirmed
>>>> whether it is what we wanted especially on the system with multiple IPMI
>>>> devices. But the new_smi callback function of smi_watcher provides very
>>>> limited info(only the interface number and dev pointer) and there is no
>>>> detailed info about the low level interface. For example: which mechansim
>>>> registers the IPMI interface(ACPI, PCI, DMI and so on).
>>>>
>>>> This is to add one interface that can get more info of low-level IPMI
>>>> device. For example: the ACPI device handle will be returned for the pnp_acpi
>>>> IPMI device.
>>>>
>>>> Signed-off-by: Zhao Yakui<yakui.zhao@intel.com>
>>>> ---
>>>> drivers/char/ipmi/ipmi_msghandler.c | 46 ++++++++++++++++++++++++++++++++
>>>> drivers/char/ipmi/ipmi_si_intf.c | 49 +++++++++++++++++++++++++++++++---
>>>> include/linux/ipmi.h | 29 ++++++++++++++++++++
>>>> include/linux/ipmi_smi.h | 8 +++++
>>>> 4 files changed, 127 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
>>>> index 4f3f8c9..e323edb 100644
>>>> --- a/drivers/char/ipmi/ipmi_msghandler.c
>>>> +++ b/drivers/char/ipmi/ipmi_msghandler.c
>>>> @@ -970,6 +970,52 @@ out_kfree:
>>>> }
>>>> EXPORT_SYMBOL(ipmi_create_user);
>>>>
>>>> +int ipmi_get_smi_info(int if_num, struct ipmi_smi_info *data)
>>>> +{
>>>> + int rv = 0;
>>>> + ipmi_smi_t intf;
>>>> + struct ipmi_smi_handlers *handlers;
>>>> +
>>>> + mutex_lock(&ipmi_interfaces_mutex);
>>>> + list_for_each_entry_rcu(intf,&ipmi_interfaces, link) {
>>>> + if (intf->intf_num == if_num)
>>>> + goto found;
>>>> + }
>>>> + /* Not found, return an error */
>>>> + rv = -EINVAL;
>>>> + mutex_unlock(&ipmi_interfaces_mutex);
>>>> + return rv;
>>>> +
>>>> +found:
>>>> + handlers = intf->handlers;
>>>> + rv = handlers->get_smi_info(intf->send_info, data);
>>>> + mutex_unlock(&ipmi_interfaces_mutex);
>>>> +
>>>> + return rv;
>>>> +}
>>>> +EXPORT_SYMBOL(ipmi_get_smi_info);
>>>> +
>>>> +void ipmi_put_smi_info(int if_num)
>>>> +{
>>>> + ipmi_smi_t intf;
>>>> + struct ipmi_smi_handlers *handlers;
>>>> +
>>>> + mutex_lock(&ipmi_interfaces_mutex);
>>>> + list_for_each_entry_rcu(intf,&ipmi_interfaces, link) {
>>>> + if (intf->intf_num == if_num)
>>>> + goto found;
>>>> + }
>>>> + /* Not found, return */
>>>> + mutex_unlock(&ipmi_interfaces_mutex);
>>>> + return;
>>>> +
>>>> +found:
>>>> + handlers = intf->handlers;
>>>> + handlers->put_smi_info(intf->send_info);
>>>> + mutex_unlock(&ipmi_interfaces_mutex);
>>>> +}
>>>> +EXPORT_SYMBOL(ipmi_put_smi_info);
>>>> +
>>>> static void free_user(struct kref *ref)
>>>> {
>>>> ipmi_user_t user = container_of(ref, struct ipmi_user, refcount);
>>>> diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
>>>> index 7bd7c45..d313018 100644
>>>> --- a/drivers/char/ipmi/ipmi_si_intf.c
>>>> +++ b/drivers/char/ipmi/ipmi_si_intf.c
>>>> @@ -57,6 +57,7 @@
>>>> #include<asm/irq.h>
>>>> #include<linux/interrupt.h>
>>>> #include<linux/rcupdate.h>
>>>> +#include<linux/ipmi.h>
>>>> #include<linux/ipmi_smi.h>
>>>> #include<asm/io.h>
>>>> #include "ipmi_si_sm.h"
>>>> @@ -107,10 +108,6 @@ enum si_type {
>>>> };
>>>> static char *si_to_str[] = { "kcs", "smic", "bt" };
>>>>
>>>> -enum ipmi_addr_src {
>>>> - SI_INVALID = 0, SI_HOTMOD, SI_HARDCODED, SI_SPMI, SI_ACPI, SI_SMBIOS,
>>>> - SI_PCI, SI_DEVICETREE, SI_DEFAULT
>>>> -};
>>>> static char *ipmi_addr_src_to_str[] = { NULL, "hotmod", "hardcoded", "SPMI",
>>>> "ACPI", "SMBIOS", "PCI",
>>>> "device-tree", "default" };
>>>> @@ -291,6 +288,12 @@ struct smi_info {
>>>> struct task_struct *thread;
>>>>
>>>> struct list_head link;
>>>> + /*
>>>> + * Count the refcount of smi_data->dev related with the function
>>>> + * of ipmi_get_smi_info/ipmi_put_smi_info.
>>>> + */
>>>> + atomic_t smi_info_ref;
>>>> + struct ipmi_smi_info smi_data;
>>>> };
>>>>
>>>> #define smi_inc_stat(smi, stat) \
>>>> @@ -1186,6 +1189,27 @@ static int smi_start_processing(void *send_info,
>>>> return 0;
>>>> }
>>>>
>>>> +static int get_smi_info(void *send_info, struct ipmi_smi_info *data)
>>>> +{
>>>> + struct smi_info *new_smi = send_info;
>>>> + struct ipmi_smi_info *smi_data =&new_smi->smi_data;
>>>> +
>>>> + get_device(new_smi->dev);
>>>> + memcpy(data, smi_data, sizeof(*smi_data));
>>>> + smi_data->addr_src = new_smi->addr_source;
>>>> + atomic_inc(&new_smi->smi_info_ref);
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static void put_smi_info(void *send_info)
>>>> +{
>>>> + struct smi_info *new_smi = send_info;
>>>> +
>>>> + put_device(new_smi->dev);
>>>> + atomic_dec(&new_smi->smi_info_ref);
>>>> +}
>>>> +
>>>> static void set_maintenance_mode(void *send_info, int enable)
>>>> {
>>>> struct smi_info *smi_info = send_info;
>>>> @@ -1197,6 +1221,8 @@ static void set_maintenance_mode(void *send_info, int enable)
>>>> static struct ipmi_smi_handlers handlers = {
>>>> .owner = THIS_MODULE,
>>>> .start_processing = smi_start_processing,
>>>> + .get_smi_info = get_smi_info,
>>>> + .put_smi_info = put_smi_info,
>>>> .sender = sender,
>>>> .request_events = request_events,
>>>> .set_maintenance_mode = set_maintenance_mode,
>>>> @@ -2143,6 +2169,8 @@ static int __devinit ipmi_pnp_probe(struct pnp_dev *dev,
>>>> return -ENOMEM;
>>>>
>>>> info->addr_source = SI_ACPI;
>>>> + info->smi_data.addr_info.acpi_info.acpi_handle =
>>>> + acpi_dev->handle;
>>>> printk(KERN_INFO PFX "probing via ACPI\n");
>>>>
>>>> handle = acpi_dev->handle;
>>>> @@ -3092,6 +3120,7 @@ static int try_smi_init(struct smi_info *new_smi)
>>>> {
>>>> int rv = 0;
>>>> int i;
>>>> + struct ipmi_smi_info *smi_data;
>>>>
>>>> printk(KERN_INFO PFX "Trying %s-specified %s state"
>>>> " machine at %s address 0x%lx, slave address 0x%x,"
>>>> @@ -3255,6 +3284,9 @@ static int try_smi_init(struct smi_info *new_smi)
>>>>
>>>> dev_info(new_smi->dev, "IPMI %s interface initialized\n",
>>>> si_to_str[new_smi->si_type]);
>>>> + smi_data =&new_smi->smi_data;
>>>> + smi_data->dev = new_smi->dev;
>>>> + atomic_set(&new_smi->smi_info_ref, 0);
>>>>
>>>> return 0;
>>>>
>>>> @@ -3501,7 +3533,14 @@ static void cleanup_one_si(struct smi_info *to_clean)
>>>> printk(KERN_ERR PFX "Unable to unregister device: errno=%d\n",
>>>> rv);
>>>> }
>>>> -
>>>> + /* maybe the caller doesn't release the refcount of dev, which is
>>>> + * increased by calling the function of ipmi_get_smi_info. So try
>>>> + * to help it.
>>>> + */
>>>> + while (atomic_read(&to_clean->smi_info_ref)) {
>>>> + put_device(to_clean->dev);
>>>> + atomic_dec(&to_clean->smi_info_ref);
>>>> + }
>>>> if (to_clean->handlers)
>>>> to_clean->handlers->cleanup(to_clean->si_sm);
>>>>
>>>> diff --git a/include/linux/ipmi.h b/include/linux/ipmi.h
>>>> index 65aae34..9a0c72e 100644
>>>> --- a/include/linux/ipmi.h
>>>> +++ b/include/linux/ipmi.h
>>>> @@ -454,6 +454,35 @@ unsigned int ipmi_addr_length(int addr_type);
>>>> /* Validate that the given IPMI address is valid. */
>>>> int ipmi_validate_addr(struct ipmi_addr *addr, int len);
>>>>
>>>> +enum ipmi_addr_src {
>>>> + SI_INVALID = 0, SI_HOTMOD, SI_HARDCODED, SI_SPMI, SI_ACPI, SI_SMBIOS,
>>>> + SI_PCI, SI_DEVICETREE, SI_DEFAULT
>>>> +};
>>>> +struct ipmi_smi_info {
>>>> + enum ipmi_addr_src addr_src;
>>>> + struct device *dev;
>>>> + /*
>>>> + * The addr_info can provide more detailed info of one IPMI device.
>>>> + * Now only SI_ACPI info is provided. And it depends on the SI_ACPI
>>>> + * address type. If the info is required for other address type, please
>>>> + * add it.
>>>> + */
>>>> + union {
>>>> + /* the acpi_info element is defined for the SI_ACPI
>>>> + * address type
>>>> + */
>>>> + struct {
>>>> + void *acpi_handle;
>>>> + } acpi_info;
>>>> + } addr_info;
>>>> +};
>>>> +
>>>> +/* This is to get the private info of ipmi_smi_t */
>>>> +extern int ipmi_get_smi_info(int if_num, struct ipmi_smi_info *data);
>>>> +/* This is to decrease refcount of dev added in the function of
>>>> + * ipmi_get_smi_info
>>>> + */
>>>> +extern void ipmi_put_smi_info(int if_num);
>>>> #endif /* __KERNEL__ */
>>>>
>>>>
>>>> diff --git a/include/linux/ipmi_smi.h b/include/linux/ipmi_smi.h
>>>> index 4b48318..b99942d 100644
>>>> --- a/include/linux/ipmi_smi.h
>>>> +++ b/include/linux/ipmi_smi.h
>>>> @@ -86,6 +86,14 @@ struct ipmi_smi_handlers {
>>>> int (*start_processing)(void *send_info,
>>>> ipmi_smi_t new_intf);
>>>>
>>>> + /*
>>>> + * Get the detailed private info of the low level interface and store
>>>> + * it into the structure of ipmi_smi_data. For example: the
>>>> + * ACPI device handle will be returned for the pnp_acpi IPMI device.
>>>> + */
>>>> + int (*get_smi_info)(void *send_info, struct ipmi_smi_info *data);
>>>> +
>>>> + void (*put_smi_info)(void *send_info);
>>>> /* Called to enqueue an SMI message to be sent. This
>>>> operation is not allowed to fail. If an error occurs, it
>>>> should report back the error in a received message. It may
>>>>
>>>>
>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>
------------------------------------------------------------------------------
The Next 800 Companies to Lead America's Growth: New Video Whitepaper
David G. Thomson, author of the best-selling book "Blueprint to a
Billion" shares his insights and actions to help propel your
business during the next growth cycle. Listen Now!
http://p.sf.net/sfu/SAP-dev2dev
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC PATCH 1/4] IPMI: Add one interface to get more info of low-level IPMI device
2010-11-04 0:41 ` Corey Minyard
@ 2010-11-15 7:52 ` ykzhao
2010-11-23 7:13 ` ykzhao
0 siblings, 1 reply; 24+ messages in thread
From: ykzhao @ 2010-11-15 7:52 UTC (permalink / raw)
To: Corey Minyard; +Cc: openipmi-developer, linux-acpi, lenb
On Thu, 2010-11-04 at 08:41 +0800, Corey Minyard wrote:
> On 11/02/2010 12:33 AM, ykzhao wrote:
> > On Thu, 2010-10-28 at 09:00 +0800, ykzhao wrote:
> >
> >> On Thu, 2010-10-28 at 00:13 +0800, Corey Minyard wrote:
> >>
> >>> I think you miss the point of a refcount. The refcount is to keep the
> >>> data structure around even if the device has gone away, so that any
> >>> users may not crash. Just having a counter in the IPMI structure and
> >>> then decrementing the device refcount is not going to accomplish anything.
> >>>
> >>> And you don't really need to refcount the IPMI structure. (And if you
> >>> did, you would need to use a refcount structure and you would need to
> >>> keep from freeing the IPMI structure until the refcount went to zero.)
> >>> You just need to increment the refcount on the device structure, and
> >>> whoever gets the device structure needs to be responsible for
> >>> decrementing it's refcount.
> >>>
> >> Yes. After the copy mechanism is used, it is unnecessary to use the
> >> refcount to protect the corresponding data structure.
> >>
> >> In fact the purpose of adding another one refcount is to add/decrease
> >> the refcount of dev in course of calling the function of
> >> ipmi_get_smi_info/ipmi_put_smi_info.
> >> But after I look at the smi_watcher mechanism, I find that the
> >> refcount of dev is not released correctly in the following case.
> >> Step 1: Other driver registers one smi_watcher and the
> >> ipmi_get_smi_info is called in the new_smi callback function. (And the
> >> driver will use the dev pointer before the ipmi_put_smi_info is
> >> called).
> >> Step 2: for any reason, the IPMI device will be unregistered by calling
> >> cleanup_one_si. This function will remove it from the IPMI device list
> >> firstly and then call the smi_gone for every smi_watcher.
> >> Step 3: the ipmi_put_smi_info is called in smi_gone callback function.
> >> In such case the ipmi_put_smi_info can't find the corresponding IPMI
> >> device based on the given if_num and can't release the pointer of dev.
> >>
> >> But after adding another refcount, we can handle the above scenario. At
> >> the same time even when the ipmi_put_smi_info is not called by the
> >> caller, it still can work.
> >> Not sure whether the above thought is redundant?
> >>
> > Corey,
> > Is the above thought correct? If not, please correct me. In fact the
> > purpose of adding another refcount is to fix the incorrect inc/dec in
> > course of calling ipmi_get_smi_info/ipmi_put_smi_info.
> >
>
Hi, Corey
Sorry for the late response.
> No. The purpose of a refcount is to keep from freeing a data structure
> while it is in use.
>
> Let's say your code gets the device structure for the IPMI device, then
> a task switch occurs. During the time it is not running, the IPMI
> device is hotplugged away and no longer exists. The device is deleted.
> When your code starts running again, it still has a pointer to that
> device structure. Sure, the removal code may have run, but your code
> still has a pointer to that device structure. Which has been freed. Bad.
Thanks for the detailed explanation about the refcount.
> This is what refcounts (well, krefs) do. In the above scenario, your
> code will get the device with the kref incremented. All the above
> happens, but since you have incremented the refcount on the device
> structure, it will not be deleted. It will wait until you put
> (decrement) the refcount before performing the free.
The above consideration is based on the assumption that the function of
ipmi_put_smi_info is called in the function of smi_watcher smi_gone
callback function. Not sure whether it is correct? Or is there any
problem about the definition of ipmi_put_smi_info?
void ipmi_put_smi_info(int if_num)
+{
+ ipmi_smi_t intf;
+ struct ipmi_smi_handlers *handlers;
+
+ mutex_lock(&ipmi_interfaces_mutex);
+ list_for_each_entry_rcu(intf, &ipmi_interfaces, link) {
+ if (intf->intf_num == if_num)
+ goto found;
+ }
+ /* Not found, return */
+ mutex_unlock(&ipmi_interfaces_mutex);
+ return;
+
+found:
+ handlers = intf->handlers;
+ handlers->put_smi_info(intf->send_info);
+ mutex_unlock(&ipmi_interfaces_mutex);
+}
The refcount of kref can't be decreased correctly in the following two
cases:
a. When one smi_watcher is unregistered by calling
ipmi_smi_watcher_unregister, we only delete it from the smi_watcher list
and won't call the smi_gone callback function.
b. The function of cleanup_one_si is to unregister one IPMI interface.
It will remove the ipmi interface from the corresponding ipmi list and
then call smi_gone callback function for every smi_watcher. As the
corresponding IPMI interface is already removed from the IPMI interface
list, it can't find the corresponding IPMI interface based on the
interface number. In such case the refcount of dev can't be released
correctly.
>
> >
> >>
> >>
> >>> I'd also prefer to not have a struct ipmi_smi_info in the IPMI data
> >>> structure. Just pull the data from existing sources.
> >>>
> >> At most cases the requirement is different for the different addr source
> >> type. If we don't put the ipmi_smi_info in the IPMI data structure, I am
> >> afraid that we will have to fill the corresponding info based on the
> >> addr source type. Maybe it will be more complex.
> >>
> > If only pull data from the existing source, it can save some
> > memory-space. But it seems a bit complex as we have to fill the info
> > based on the corresponding addr source type. And this should be done
> > every time the function of ipmi_get_smi_info is called.
> >
>
> I don't think it's that complex, and I doubt that function is called
> that often.
Ok. I will refresh the patch set and fill the corresponding data
structure from the existing source. But as I don't know what info is
required for the other ipmi type except the SI_ACPI type, I will only
add the info of SI_ACPI type.
>
> -corey
> >
> >> Thanks.
> >> Yakui
> >>
> >>
> >>> -corey
> >>>
> >>> On 10/26/2010 04:14 AM, yakui.zhao@intel.com wrote:
> >>>
> >>>> From: Zhao Yakui<yakui.zhao@intel.com>
> >>>>
> >>>> The IPMI smi_watcher will be used to catch the IPMI interface as they come or go.
> >>>> In order to communicate with the correct IPMI device, it should be confirmed
> >>>> whether it is what we wanted especially on the system with multiple IPMI
> >>>> devices. But the new_smi callback function of smi_watcher provides very
> >>>> limited info(only the interface number and dev pointer) and there is no
> >>>> detailed info about the low level interface. For example: which mechansim
> >>>> registers the IPMI interface(ACPI, PCI, DMI and so on).
> >>>>
> >>>> This is to add one interface that can get more info of low-level IPMI
> >>>> device. For example: the ACPI device handle will be returned for the pnp_acpi
> >>>> IPMI device.
> >>>>
> >>>> Signed-off-by: Zhao Yakui<yakui.zhao@intel.com>
> >>>> ---
> >>>> drivers/char/ipmi/ipmi_msghandler.c | 46 ++++++++++++++++++++++++++++++++
> >>>> drivers/char/ipmi/ipmi_si_intf.c | 49 +++++++++++++++++++++++++++++++---
> >>>> include/linux/ipmi.h | 29 ++++++++++++++++++++
> >>>> include/linux/ipmi_smi.h | 8 +++++
> >>>> 4 files changed, 127 insertions(+), 5 deletions(-)
> >>>>
> >>>> diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
> >>>> index 4f3f8c9..e323edb 100644
> >>>> --- a/drivers/char/ipmi/ipmi_msghandler.c
> >>>> +++ b/drivers/char/ipmi/ipmi_msghandler.c
> >>>> @@ -970,6 +970,52 @@ out_kfree:
> >>>> }
> >>>> EXPORT_SYMBOL(ipmi_create_user);
> >>>>
> >>>> +int ipmi_get_smi_info(int if_num, struct ipmi_smi_info *data)
> >>>> +{
> >>>> + int rv = 0;
> >>>> + ipmi_smi_t intf;
> >>>> + struct ipmi_smi_handlers *handlers;
> >>>> +
> >>>> + mutex_lock(&ipmi_interfaces_mutex);
> >>>> + list_for_each_entry_rcu(intf,&ipmi_interfaces, link) {
> >>>> + if (intf->intf_num == if_num)
> >>>> + goto found;
> >>>> + }
> >>>> + /* Not found, return an error */
> >>>> + rv = -EINVAL;
> >>>> + mutex_unlock(&ipmi_interfaces_mutex);
> >>>> + return rv;
> >>>> +
> >>>> +found:
> >>>> + handlers = intf->handlers;
> >>>> + rv = handlers->get_smi_info(intf->send_info, data);
> >>>> + mutex_unlock(&ipmi_interfaces_mutex);
> >>>> +
> >>>> + return rv;
> >>>> +}
> >>>> +EXPORT_SYMBOL(ipmi_get_smi_info);
> >>>> +
> >>>> +void ipmi_put_smi_info(int if_num)
> >>>> +{
> >>>> + ipmi_smi_t intf;
> >>>> + struct ipmi_smi_handlers *handlers;
> >>>> +
> >>>> + mutex_lock(&ipmi_interfaces_mutex);
> >>>> + list_for_each_entry_rcu(intf,&ipmi_interfaces, link) {
> >>>> + if (intf->intf_num == if_num)
> >>>> + goto found;
> >>>> + }
> >>>> + /* Not found, return */
> >>>> + mutex_unlock(&ipmi_interfaces_mutex);
> >>>> + return;
> >>>> +
> >>>> +found:
> >>>> + handlers = intf->handlers;
> >>>> + handlers->put_smi_info(intf->send_info);
> >>>> + mutex_unlock(&ipmi_interfaces_mutex);
> >>>> +}
> >>>> +EXPORT_SYMBOL(ipmi_put_smi_info);
> >>>> +
> >>>> static void free_user(struct kref *ref)
> >>>> {
> >>>> ipmi_user_t user = container_of(ref, struct ipmi_user, refcount);
> >>>> diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
> >>>> index 7bd7c45..d313018 100644
> >>>> --- a/drivers/char/ipmi/ipmi_si_intf.c
> >>>> +++ b/drivers/char/ipmi/ipmi_si_intf.c
> >>>> @@ -57,6 +57,7 @@
> >>>> #include<asm/irq.h>
> >>>> #include<linux/interrupt.h>
> >>>> #include<linux/rcupdate.h>
> >>>> +#include<linux/ipmi.h>
> >>>> #include<linux/ipmi_smi.h>
> >>>> #include<asm/io.h>
> >>>> #include "ipmi_si_sm.h"
> >>>> @@ -107,10 +108,6 @@ enum si_type {
> >>>> };
> >>>> static char *si_to_str[] = { "kcs", "smic", "bt" };
> >>>>
> >>>> -enum ipmi_addr_src {
> >>>> - SI_INVALID = 0, SI_HOTMOD, SI_HARDCODED, SI_SPMI, SI_ACPI, SI_SMBIOS,
> >>>> - SI_PCI, SI_DEVICETREE, SI_DEFAULT
> >>>> -};
> >>>> static char *ipmi_addr_src_to_str[] = { NULL, "hotmod", "hardcoded", "SPMI",
> >>>> "ACPI", "SMBIOS", "PCI",
> >>>> "device-tree", "default" };
> >>>> @@ -291,6 +288,12 @@ struct smi_info {
> >>>> struct task_struct *thread;
> >>>>
> >>>> struct list_head link;
> >>>> + /*
> >>>> + * Count the refcount of smi_data->dev related with the function
> >>>> + * of ipmi_get_smi_info/ipmi_put_smi_info.
> >>>> + */
> >>>> + atomic_t smi_info_ref;
> >>>> + struct ipmi_smi_info smi_data;
> >>>> };
> >>>>
> >>>> #define smi_inc_stat(smi, stat) \
> >>>> @@ -1186,6 +1189,27 @@ static int smi_start_processing(void *send_info,
> >>>> return 0;
> >>>> }
> >>>>
> >>>> +static int get_smi_info(void *send_info, struct ipmi_smi_info *data)
> >>>> +{
> >>>> + struct smi_info *new_smi = send_info;
> >>>> + struct ipmi_smi_info *smi_data =&new_smi->smi_data;
> >>>> +
> >>>> + get_device(new_smi->dev);
> >>>> + memcpy(data, smi_data, sizeof(*smi_data));
> >>>> + smi_data->addr_src = new_smi->addr_source;
> >>>> + atomic_inc(&new_smi->smi_info_ref);
> >>>> +
> >>>> + return 0;
> >>>> +}
> >>>> +
> >>>> +static void put_smi_info(void *send_info)
> >>>> +{
> >>>> + struct smi_info *new_smi = send_info;
> >>>> +
> >>>> + put_device(new_smi->dev);
> >>>> + atomic_dec(&new_smi->smi_info_ref);
> >>>> +}
> >>>> +
> >>>> static void set_maintenance_mode(void *send_info, int enable)
> >>>> {
> >>>> struct smi_info *smi_info = send_info;
> >>>> @@ -1197,6 +1221,8 @@ static void set_maintenance_mode(void *send_info, int enable)
> >>>> static struct ipmi_smi_handlers handlers = {
> >>>> .owner = THIS_MODULE,
> >>>> .start_processing = smi_start_processing,
> >>>> + .get_smi_info = get_smi_info,
> >>>> + .put_smi_info = put_smi_info,
> >>>> .sender = sender,
> >>>> .request_events = request_events,
> >>>> .set_maintenance_mode = set_maintenance_mode,
> >>>> @@ -2143,6 +2169,8 @@ static int __devinit ipmi_pnp_probe(struct pnp_dev *dev,
> >>>> return -ENOMEM;
> >>>>
> >>>> info->addr_source = SI_ACPI;
> >>>> + info->smi_data.addr_info.acpi_info.acpi_handle =
> >>>> + acpi_dev->handle;
> >>>> printk(KERN_INFO PFX "probing via ACPI\n");
> >>>>
> >>>> handle = acpi_dev->handle;
> >>>> @@ -3092,6 +3120,7 @@ static int try_smi_init(struct smi_info *new_smi)
> >>>> {
> >>>> int rv = 0;
> >>>> int i;
> >>>> + struct ipmi_smi_info *smi_data;
> >>>>
> >>>> printk(KERN_INFO PFX "Trying %s-specified %s state"
> >>>> " machine at %s address 0x%lx, slave address 0x%x,"
> >>>> @@ -3255,6 +3284,9 @@ static int try_smi_init(struct smi_info *new_smi)
> >>>>
> >>>> dev_info(new_smi->dev, "IPMI %s interface initialized\n",
> >>>> si_to_str[new_smi->si_type]);
> >>>> + smi_data =&new_smi->smi_data;
> >>>> + smi_data->dev = new_smi->dev;
> >>>> + atomic_set(&new_smi->smi_info_ref, 0);
> >>>>
> >>>> return 0;
> >>>>
> >>>> @@ -3501,7 +3533,14 @@ static void cleanup_one_si(struct smi_info *to_clean)
> >>>> printk(KERN_ERR PFX "Unable to unregister device: errno=%d\n",
> >>>> rv);
> >>>> }
> >>>> -
> >>>> + /* maybe the caller doesn't release the refcount of dev, which is
> >>>> + * increased by calling the function of ipmi_get_smi_info. So try
> >>>> + * to help it.
> >>>> + */
> >>>> + while (atomic_read(&to_clean->smi_info_ref)) {
> >>>> + put_device(to_clean->dev);
> >>>> + atomic_dec(&to_clean->smi_info_ref);
> >>>> + }
> >>>> if (to_clean->handlers)
> >>>> to_clean->handlers->cleanup(to_clean->si_sm);
> >>>>
> >>>> diff --git a/include/linux/ipmi.h b/include/linux/ipmi.h
> >>>> index 65aae34..9a0c72e 100644
> >>>> --- a/include/linux/ipmi.h
> >>>> +++ b/include/linux/ipmi.h
> >>>> @@ -454,6 +454,35 @@ unsigned int ipmi_addr_length(int addr_type);
> >>>> /* Validate that the given IPMI address is valid. */
> >>>> int ipmi_validate_addr(struct ipmi_addr *addr, int len);
> >>>>
> >>>> +enum ipmi_addr_src {
> >>>> + SI_INVALID = 0, SI_HOTMOD, SI_HARDCODED, SI_SPMI, SI_ACPI, SI_SMBIOS,
> >>>> + SI_PCI, SI_DEVICETREE, SI_DEFAULT
> >>>> +};
> >>>> +struct ipmi_smi_info {
> >>>> + enum ipmi_addr_src addr_src;
> >>>> + struct device *dev;
> >>>> + /*
> >>>> + * The addr_info can provide more detailed info of one IPMI device.
> >>>> + * Now only SI_ACPI info is provided. And it depends on the SI_ACPI
> >>>> + * address type. If the info is required for other address type, please
> >>>> + * add it.
> >>>> + */
> >>>> + union {
> >>>> + /* the acpi_info element is defined for the SI_ACPI
> >>>> + * address type
> >>>> + */
> >>>> + struct {
> >>>> + void *acpi_handle;
> >>>> + } acpi_info;
> >>>> + } addr_info;
> >>>> +};
> >>>> +
> >>>> +/* This is to get the private info of ipmi_smi_t */
> >>>> +extern int ipmi_get_smi_info(int if_num, struct ipmi_smi_info *data);
> >>>> +/* This is to decrease refcount of dev added in the function of
> >>>> + * ipmi_get_smi_info
> >>>> + */
> >>>> +extern void ipmi_put_smi_info(int if_num);
> >>>> #endif /* __KERNEL__ */
> >>>>
> >>>>
> >>>> diff --git a/include/linux/ipmi_smi.h b/include/linux/ipmi_smi.h
> >>>> index 4b48318..b99942d 100644
> >>>> --- a/include/linux/ipmi_smi.h
> >>>> +++ b/include/linux/ipmi_smi.h
> >>>> @@ -86,6 +86,14 @@ struct ipmi_smi_handlers {
> >>>> int (*start_processing)(void *send_info,
> >>>> ipmi_smi_t new_intf);
> >>>>
> >>>> + /*
> >>>> + * Get the detailed private info of the low level interface and store
> >>>> + * it into the structure of ipmi_smi_data. For example: the
> >>>> + * ACPI device handle will be returned for the pnp_acpi IPMI device.
> >>>> + */
> >>>> + int (*get_smi_info)(void *send_info, struct ipmi_smi_info *data);
> >>>> +
> >>>> + void (*put_smi_info)(void *send_info);
> >>>> /* Called to enqueue an SMI message to be sent. This
> >>>> operation is not allowed to fail. If an error occurs, it
> >>>> should report back the error in a received message. It may
> >>>>
> >>>>
> >>>
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> >> the body of a message to majordomo@vger.kernel.org
> >> More majordomo info at http://vger.kernel.org/majordomo-info.html
> >>
> >
>
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC PATCH 1/4] IPMI: Add one interface to get more info of low-level IPMI device
2010-11-15 7:52 ` ykzhao
@ 2010-11-23 7:13 ` ykzhao
0 siblings, 0 replies; 24+ messages in thread
From: ykzhao @ 2010-11-23 7:13 UTC (permalink / raw)
To: Corey Minyard; +Cc: linux-acpi, openipmi-developer, lenb
On Mon, 2010-11-15 at 15:52 +0800, ykzhao wrote:
> On Thu, 2010-11-04 at 08:41 +0800, Corey Minyard wrote:
> > On 11/02/2010 12:33 AM, ykzhao wrote:
> > > On Thu, 2010-10-28 at 09:00 +0800, ykzhao wrote:
> > >
> > >> On Thu, 2010-10-28 at 00:13 +0800, Corey Minyard wrote:
> > >>
> > >>> I think you miss the point of a refcount. The refcount is to keep the
> > >>> data structure around even if the device has gone away, so that any
> > >>> users may not crash. Just having a counter in the IPMI structure and
> > >>> then decrementing the device refcount is not going to accomplish anything.
> > >>>
> > >>> And you don't really need to refcount the IPMI structure. (And if you
> > >>> did, you would need to use a refcount structure and you would need to
> > >>> keep from freeing the IPMI structure until the refcount went to zero.)
> > >>> You just need to increment the refcount on the device structure, and
> > >>> whoever gets the device structure needs to be responsible for
> > >>> decrementing it's refcount.
> > >>>
> > >> Yes. After the copy mechanism is used, it is unnecessary to use the
> > >> refcount to protect the corresponding data structure.
> > >>
> > >> In fact the purpose of adding another one refcount is to add/decrease
> > >> the refcount of dev in course of calling the function of
> > >> ipmi_get_smi_info/ipmi_put_smi_info.
> > >> But after I look at the smi_watcher mechanism, I find that the
> > >> refcount of dev is not released correctly in the following case.
> > >> Step 1: Other driver registers one smi_watcher and the
> > >> ipmi_get_smi_info is called in the new_smi callback function. (And the
> > >> driver will use the dev pointer before the ipmi_put_smi_info is
> > >> called).
> > >> Step 2: for any reason, the IPMI device will be unregistered by calling
> > >> cleanup_one_si. This function will remove it from the IPMI device list
> > >> firstly and then call the smi_gone for every smi_watcher.
> > >> Step 3: the ipmi_put_smi_info is called in smi_gone callback function.
> > >> In such case the ipmi_put_smi_info can't find the corresponding IPMI
> > >> device based on the given if_num and can't release the pointer of dev.
> > >>
> > >> But after adding another refcount, we can handle the above scenario. At
> > >> the same time even when the ipmi_put_smi_info is not called by the
> > >> caller, it still can work.
> > >> Not sure whether the above thought is redundant?
> > >>
> > > Corey,
> > > Is the above thought correct? If not, please correct me. In fact the
> > > purpose of adding another refcount is to fix the incorrect inc/dec in
> > > course of calling ipmi_get_smi_info/ipmi_put_smi_info.
> > >
> >
>
> Hi, Corey
> Sorry for the late response.
>
> > No. The purpose of a refcount is to keep from freeing a data structure
> > while it is in use.
> >
> > Let's say your code gets the device structure for the IPMI device, then
> > a task switch occurs. During the time it is not running, the IPMI
> > device is hotplugged away and no longer exists. The device is deleted.
> > When your code starts running again, it still has a pointer to that
> > device structure. Sure, the removal code may have run, but your code
> > still has a pointer to that device structure. Which has been freed. Bad.
>
> Thanks for the detailed explanation about the refcount.
>
> > This is what refcounts (well, krefs) do. In the above scenario, your
> > code will get the device with the kref incremented. All the above
> > happens, but since you have incremented the refcount on the device
> > structure, it will not be deleted. It will wait until you put
> > (decrement) the refcount before performing the free.
>
> The above consideration is based on the assumption that the function of
> ipmi_put_smi_info is called in the function of smi_watcher smi_gone
> callback function. Not sure whether it is correct? Or is there any
> problem about the definition of ipmi_put_smi_info?
>
> void ipmi_put_smi_info(int if_num)
> +{
> + ipmi_smi_t intf;
> + struct ipmi_smi_handlers *handlers;
> +
> + mutex_lock(&ipmi_interfaces_mutex);
> + list_for_each_entry_rcu(intf, &ipmi_interfaces, link) {
> + if (intf->intf_num == if_num)
> + goto found;
> + }
> + /* Not found, return */
> + mutex_unlock(&ipmi_interfaces_mutex);
> + return;
> +
> +found:
> + handlers = intf->handlers;
> + handlers->put_smi_info(intf->send_info);
> + mutex_unlock(&ipmi_interfaces_mutex);
> +}
>
>
>
> The refcount of kref can't be decreased correctly in the following two
> cases:
> a. When one smi_watcher is unregistered by calling
> ipmi_smi_watcher_unregister, we only delete it from the smi_watcher list
> and won't call the smi_gone callback function.
> b. The function of cleanup_one_si is to unregister one IPMI interface.
> It will remove the ipmi interface from the corresponding ipmi list and
> then call smi_gone callback function for every smi_watcher. As the
> corresponding IPMI interface is already removed from the IPMI interface
> list, it can't find the corresponding IPMI interface based on the
> interface number. In such case the refcount of dev can't be released
> correctly.
Hi, Corey
What do you think about the above description for the refcount of dev?
Does it make sense that the following prototype is used for the
ipmi_put_smi_info?
void ipmi_put_smi_info(struct ipmi_smi_info *data)
{
put_device(data->dev);
}
> >
> > >
> > >>
> > >>
> > >>> I'd also prefer to not have a struct ipmi_smi_info in the IPMI data
> > >>> structure. Just pull the data from existing sources.
> > >>>
> > >> At most cases the requirement is different for the different addr source
> > >> type. If we don't put the ipmi_smi_info in the IPMI data structure, I am
> > >> afraid that we will have to fill the corresponding info based on the
> > >> addr source type. Maybe it will be more complex.
> > >>
> > > If only pull data from the existing source, it can save some
> > > memory-space. But it seems a bit complex as we have to fill the info
> > > based on the corresponding addr source type. And this should be done
> > > every time the function of ipmi_get_smi_info is called.
> > >
> >
> > I don't think it's that complex, and I doubt that function is called
> > that often.
>
> Ok. I will refresh the patch set and fill the corresponding data
> structure from the existing source. But as I don't know what info is
> required for the other ipmi type except the SI_ACPI type, I will only
> add the info of SI_ACPI type.
For the device type and dev pointer, I can get it from the current
smi_info directly. But I meet with the following issues when pulling
data from the existing source to fill the specific data.
a. no specific info is contained in smi_info. For example: ACPI handle
is important to the SI_ACPI type. But the corresponding handle is stored
in smi_info.
b. Need to add a lot of If/else macro definition to handle the
corresponding IPMI device type.
Any idea about the above issues?
Thanks.
>
> >
> > -corey
> > >
> > >> Thanks.
> > >> Yakui
> > >>
> > >>
> > >>> -corey
> > >>>
> > >>> On 10/26/2010 04:14 AM, yakui.zhao@intel.com wrote:
> > >>>
> > >>>> From: Zhao Yakui<yakui.zhao@intel.com>
> > >>>>
> > >>>> The IPMI smi_watcher will be used to catch the IPMI interface as they come or go.
> > >>>> In order to communicate with the correct IPMI device, it should be confirmed
> > >>>> whether it is what we wanted especially on the system with multiple IPMI
> > >>>> devices. But the new_smi callback function of smi_watcher provides very
> > >>>> limited info(only the interface number and dev pointer) and there is no
> > >>>> detailed info about the low level interface. For example: which mechansim
> > >>>> registers the IPMI interface(ACPI, PCI, DMI and so on).
> > >>>>
> > >>>> This is to add one interface that can get more info of low-level IPMI
> > >>>> device. For example: the ACPI device handle will be returned for the pnp_acpi
> > >>>> IPMI device.
> > >>>>
> > >>>> Signed-off-by: Zhao Yakui<yakui.zhao@intel.com>
> > >>>> ---
> > >>>> drivers/char/ipmi/ipmi_msghandler.c | 46 ++++++++++++++++++++++++++++++++
> > >>>> drivers/char/ipmi/ipmi_si_intf.c | 49 +++++++++++++++++++++++++++++++---
> > >>>> include/linux/ipmi.h | 29 ++++++++++++++++++++
> > >>>> include/linux/ipmi_smi.h | 8 +++++
> > >>>> 4 files changed, 127 insertions(+), 5 deletions(-)
> > >>>>
> > >>>> diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
> > >>>> index 4f3f8c9..e323edb 100644
> > >>>> --- a/drivers/char/ipmi/ipmi_msghandler.c
> > >>>> +++ b/drivers/char/ipmi/ipmi_msghandler.c
> > >>>> @@ -970,6 +970,52 @@ out_kfree:
> > >>>> }
> > >>>> EXPORT_SYMBOL(ipmi_create_user);
> > >>>>
> > >>>> +int ipmi_get_smi_info(int if_num, struct ipmi_smi_info *data)
> > >>>> +{
> > >>>> + int rv = 0;
> > >>>> + ipmi_smi_t intf;
> > >>>> + struct ipmi_smi_handlers *handlers;
> > >>>> +
> > >>>> + mutex_lock(&ipmi_interfaces_mutex);
> > >>>> + list_for_each_entry_rcu(intf,&ipmi_interfaces, link) {
> > >>>> + if (intf->intf_num == if_num)
> > >>>> + goto found;
> > >>>> + }
> > >>>> + /* Not found, return an error */
> > >>>> + rv = -EINVAL;
> > >>>> + mutex_unlock(&ipmi_interfaces_mutex);
> > >>>> + return rv;
> > >>>> +
> > >>>> +found:
> > >>>> + handlers = intf->handlers;
> > >>>> + rv = handlers->get_smi_info(intf->send_info, data);
> > >>>> + mutex_unlock(&ipmi_interfaces_mutex);
> > >>>> +
> > >>>> + return rv;
> > >>>> +}
> > >>>> +EXPORT_SYMBOL(ipmi_get_smi_info);
> > >>>> +
> > >>>> +void ipmi_put_smi_info(int if_num)
> > >>>> +{
> > >>>> + ipmi_smi_t intf;
> > >>>> + struct ipmi_smi_handlers *handlers;
> > >>>> +
> > >>>> + mutex_lock(&ipmi_interfaces_mutex);
> > >>>> + list_for_each_entry_rcu(intf,&ipmi_interfaces, link) {
> > >>>> + if (intf->intf_num == if_num)
> > >>>> + goto found;
> > >>>> + }
> > >>>> + /* Not found, return */
> > >>>> + mutex_unlock(&ipmi_interfaces_mutex);
> > >>>> + return;
> > >>>> +
> > >>>> +found:
> > >>>> + handlers = intf->handlers;
> > >>>> + handlers->put_smi_info(intf->send_info);
> > >>>> + mutex_unlock(&ipmi_interfaces_mutex);
> > >>>> +}
> > >>>> +EXPORT_SYMBOL(ipmi_put_smi_info);
> > >>>> +
> > >>>> static void free_user(struct kref *ref)
> > >>>> {
> > >>>> ipmi_user_t user = container_of(ref, struct ipmi_user, refcount);
> > >>>> diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
> > >>>> index 7bd7c45..d313018 100644
> > >>>> --- a/drivers/char/ipmi/ipmi_si_intf.c
> > >>>> +++ b/drivers/char/ipmi/ipmi_si_intf.c
> > >>>> @@ -57,6 +57,7 @@
> > >>>> #include<asm/irq.h>
> > >>>> #include<linux/interrupt.h>
> > >>>> #include<linux/rcupdate.h>
> > >>>> +#include<linux/ipmi.h>
> > >>>> #include<linux/ipmi_smi.h>
> > >>>> #include<asm/io.h>
> > >>>> #include "ipmi_si_sm.h"
> > >>>> @@ -107,10 +108,6 @@ enum si_type {
> > >>>> };
> > >>>> static char *si_to_str[] = { "kcs", "smic", "bt" };
> > >>>>
> > >>>> -enum ipmi_addr_src {
> > >>>> - SI_INVALID = 0, SI_HOTMOD, SI_HARDCODED, SI_SPMI, SI_ACPI, SI_SMBIOS,
> > >>>> - SI_PCI, SI_DEVICETREE, SI_DEFAULT
> > >>>> -};
> > >>>> static char *ipmi_addr_src_to_str[] = { NULL, "hotmod", "hardcoded", "SPMI",
> > >>>> "ACPI", "SMBIOS", "PCI",
> > >>>> "device-tree", "default" };
> > >>>> @@ -291,6 +288,12 @@ struct smi_info {
> > >>>> struct task_struct *thread;
> > >>>>
> > >>>> struct list_head link;
> > >>>> + /*
> > >>>> + * Count the refcount of smi_data->dev related with the function
> > >>>> + * of ipmi_get_smi_info/ipmi_put_smi_info.
> > >>>> + */
> > >>>> + atomic_t smi_info_ref;
> > >>>> + struct ipmi_smi_info smi_data;
> > >>>> };
> > >>>>
> > >>>> #define smi_inc_stat(smi, stat) \
> > >>>> @@ -1186,6 +1189,27 @@ static int smi_start_processing(void *send_info,
> > >>>> return 0;
> > >>>> }
> > >>>>
> > >>>> +static int get_smi_info(void *send_info, struct ipmi_smi_info *data)
> > >>>> +{
> > >>>> + struct smi_info *new_smi = send_info;
> > >>>> + struct ipmi_smi_info *smi_data =&new_smi->smi_data;
> > >>>> +
> > >>>> + get_device(new_smi->dev);
> > >>>> + memcpy(data, smi_data, sizeof(*smi_data));
> > >>>> + smi_data->addr_src = new_smi->addr_source;
> > >>>> + atomic_inc(&new_smi->smi_info_ref);
> > >>>> +
> > >>>> + return 0;
> > >>>> +}
> > >>>> +
> > >>>> +static void put_smi_info(void *send_info)
> > >>>> +{
> > >>>> + struct smi_info *new_smi = send_info;
> > >>>> +
> > >>>> + put_device(new_smi->dev);
> > >>>> + atomic_dec(&new_smi->smi_info_ref);
> > >>>> +}
> > >>>> +
> > >>>> static void set_maintenance_mode(void *send_info, int enable)
> > >>>> {
> > >>>> struct smi_info *smi_info = send_info;
> > >>>> @@ -1197,6 +1221,8 @@ static void set_maintenance_mode(void *send_info, int enable)
> > >>>> static struct ipmi_smi_handlers handlers = {
> > >>>> .owner = THIS_MODULE,
> > >>>> .start_processing = smi_start_processing,
> > >>>> + .get_smi_info = get_smi_info,
> > >>>> + .put_smi_info = put_smi_info,
> > >>>> .sender = sender,
> > >>>> .request_events = request_events,
> > >>>> .set_maintenance_mode = set_maintenance_mode,
> > >>>> @@ -2143,6 +2169,8 @@ static int __devinit ipmi_pnp_probe(struct pnp_dev *dev,
> > >>>> return -ENOMEM;
> > >>>>
> > >>>> info->addr_source = SI_ACPI;
> > >>>> + info->smi_data.addr_info.acpi_info.acpi_handle =
> > >>>> + acpi_dev->handle;
> > >>>> printk(KERN_INFO PFX "probing via ACPI\n");
> > >>>>
> > >>>> handle = acpi_dev->handle;
> > >>>> @@ -3092,6 +3120,7 @@ static int try_smi_init(struct smi_info *new_smi)
> > >>>> {
> > >>>> int rv = 0;
> > >>>> int i;
> > >>>> + struct ipmi_smi_info *smi_data;
> > >>>>
> > >>>> printk(KERN_INFO PFX "Trying %s-specified %s state"
> > >>>> " machine at %s address 0x%lx, slave address 0x%x,"
> > >>>> @@ -3255,6 +3284,9 @@ static int try_smi_init(struct smi_info *new_smi)
> > >>>>
> > >>>> dev_info(new_smi->dev, "IPMI %s interface initialized\n",
> > >>>> si_to_str[new_smi->si_type]);
> > >>>> + smi_data =&new_smi->smi_data;
> > >>>> + smi_data->dev = new_smi->dev;
> > >>>> + atomic_set(&new_smi->smi_info_ref, 0);
> > >>>>
> > >>>> return 0;
> > >>>>
> > >>>> @@ -3501,7 +3533,14 @@ static void cleanup_one_si(struct smi_info *to_clean)
> > >>>> printk(KERN_ERR PFX "Unable to unregister device: errno=%d\n",
> > >>>> rv);
> > >>>> }
> > >>>> -
> > >>>> + /* maybe the caller doesn't release the refcount of dev, which is
> > >>>> + * increased by calling the function of ipmi_get_smi_info. So try
> > >>>> + * to help it.
> > >>>> + */
> > >>>> + while (atomic_read(&to_clean->smi_info_ref)) {
> > >>>> + put_device(to_clean->dev);
> > >>>> + atomic_dec(&to_clean->smi_info_ref);
> > >>>> + }
> > >>>> if (to_clean->handlers)
> > >>>> to_clean->handlers->cleanup(to_clean->si_sm);
> > >>>>
> > >>>> diff --git a/include/linux/ipmi.h b/include/linux/ipmi.h
> > >>>> index 65aae34..9a0c72e 100644
> > >>>> --- a/include/linux/ipmi.h
> > >>>> +++ b/include/linux/ipmi.h
> > >>>> @@ -454,6 +454,35 @@ unsigned int ipmi_addr_length(int addr_type);
> > >>>> /* Validate that the given IPMI address is valid. */
> > >>>> int ipmi_validate_addr(struct ipmi_addr *addr, int len);
> > >>>>
> > >>>> +enum ipmi_addr_src {
> > >>>> + SI_INVALID = 0, SI_HOTMOD, SI_HARDCODED, SI_SPMI, SI_ACPI, SI_SMBIOS,
> > >>>> + SI_PCI, SI_DEVICETREE, SI_DEFAULT
> > >>>> +};
> > >>>> +struct ipmi_smi_info {
> > >>>> + enum ipmi_addr_src addr_src;
> > >>>> + struct device *dev;
> > >>>> + /*
> > >>>> + * The addr_info can provide more detailed info of one IPMI device.
> > >>>> + * Now only SI_ACPI info is provided. And it depends on the SI_ACPI
> > >>>> + * address type. If the info is required for other address type, please
> > >>>> + * add it.
> > >>>> + */
> > >>>> + union {
> > >>>> + /* the acpi_info element is defined for the SI_ACPI
> > >>>> + * address type
> > >>>> + */
> > >>>> + struct {
> > >>>> + void *acpi_handle;
> > >>>> + } acpi_info;
> > >>>> + } addr_info;
> > >>>> +};
> > >>>> +
> > >>>> +/* This is to get the private info of ipmi_smi_t */
> > >>>> +extern int ipmi_get_smi_info(int if_num, struct ipmi_smi_info *data);
> > >>>> +/* This is to decrease refcount of dev added in the function of
> > >>>> + * ipmi_get_smi_info
> > >>>> + */
> > >>>> +extern void ipmi_put_smi_info(int if_num);
> > >>>> #endif /* __KERNEL__ */
> > >>>>
> > >>>>
> > >>>> diff --git a/include/linux/ipmi_smi.h b/include/linux/ipmi_smi.h
> > >>>> index 4b48318..b99942d 100644
> > >>>> --- a/include/linux/ipmi_smi.h
> > >>>> +++ b/include/linux/ipmi_smi.h
> > >>>> @@ -86,6 +86,14 @@ struct ipmi_smi_handlers {
> > >>>> int (*start_processing)(void *send_info,
> > >>>> ipmi_smi_t new_intf);
> > >>>>
> > >>>> + /*
> > >>>> + * Get the detailed private info of the low level interface and store
> > >>>> + * it into the structure of ipmi_smi_data. For example: the
> > >>>> + * ACPI device handle will be returned for the pnp_acpi IPMI device.
> > >>>> + */
> > >>>> + int (*get_smi_info)(void *send_info, struct ipmi_smi_info *data);
> > >>>> +
> > >>>> + void (*put_smi_info)(void *send_info);
> > >>>> /* Called to enqueue an SMI message to be sent. This
> > >>>> operation is not allowed to fail. If an error occurs, it
> > >>>> should report back the error in a received message. It may
> > >>>>
> > >>>>
> > >>>
> > >> --
> > >> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> > >> the body of a message to majordomo@vger.kernel.org
> > >> More majordomo info at http://vger.kernel.org/majordomo-info.html
> > >>
> > >
> >
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
------------------------------------------------------------------------------
Increase Visibility of Your 3D Game App & Earn a Chance To Win $500!
Tap into the largest installed PC base & get more eyes on your game by
optimizing for Intel(R) Graphics Technology. Get started today with the
Intel(R) Software Partner Program. Five $500 cash prizes are up for grabs.
http://p.sf.net/sfu/intelisp-dev2dev
_______________________________________________
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC PATCH 1/4] IPMI: Add one interface to get more info of low-level IPMI device
2010-11-30 0:36 ` ykzhao
@ 2010-12-06 1:06 ` ykzhao
0 siblings, 0 replies; 24+ messages in thread
From: ykzhao @ 2010-12-06 1:06 UTC (permalink / raw)
To: minyard; +Cc: openipmi-developer, lenb, linux-acpi
On Tue, 2010-11-30 at 08:36 +0800, ykzhao wrote:
> On Tue, 2010-11-30 at 08:26 +0800, yakui.zhao@intel.com wrote:
> > From: Zhao Yakui <yakui.zhao@intel.com>
> >
> > The IPMI smi_watcher will be used to catch the IPMI interface as they come or go.
> > In order to communicate with the correct IPMI device, it should be confirmed
> > whether it is what we wanted especially on the system with multiple IPMI
> > devices. But the new_smi callback function of smi_watcher provides very
> > limited info(only the interface number and dev pointer) and there is no
> > detailed info about the low level interface. For example: which mechansim
> > registers the IPMI interface(ACPI, PCI, DMI and so on).
> >
> > This is to add one interface that can get more info of low-level IPMI
> > device. For example: the ACPI device handle will be returned for the pnp_acpi
> > IPMI device.
>
> Hi, Corey
>
> The following is updated in the patch set.
> a. Change the function prototype of ipmi_put_smi_info.
> The corresponding parameter is changed from "
> Ipmi_put_smi_info(int iface) to ipmi_put_smi_info(struct ipmi_smi_info
> *data)
>
> b. Remove the incorrect refcount in previous patch set.
>
> But for the issues about pulling data from the current source, very
> sorry that I still statically construct them in the corresponding
> smi_info as I meet with the following two issues when trying to pull
> data from the current source.
> 1. no specific info is contained in smi_info. For example: ACPI handle
> is important to the SI_ACPI type. But the corresponding handle is stored
> in smi_info.
> b. Need to add a lot of If/else macro definitions to handle the
> corresponding IPMI device type.
Hi, Corey
Any comments about the updated patch set?
Thanks.
>
> Thanks.
>
> >
> > Signed-off-by: Zhao Yakui <yakui.zhao@intel.com>
> > ---
> > drivers/char/ipmi/ipmi_msghandler.c | 31 +++++++++++++++++++++++++++++++
> > drivers/char/ipmi/ipmi_si_intf.c | 24 ++++++++++++++++++++----
> > include/linux/ipmi.h | 29 +++++++++++++++++++++++++++++
> > include/linux/ipmi_smi.h | 7 +++++++
> > 4 files changed, 87 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
> > index 2fe72f8..a90b3ec 100644
> > --- a/drivers/char/ipmi/ipmi_msghandler.c
> > +++ b/drivers/char/ipmi/ipmi_msghandler.c
> > @@ -970,6 +970,37 @@ out_kfree:
> > }
> > EXPORT_SYMBOL(ipmi_create_user);
> >
> > +int ipmi_get_smi_info(int if_num, struct ipmi_smi_info *data)
> > +{
> > + int rv = 0;
> > + ipmi_smi_t intf;
> > + struct ipmi_smi_handlers *handlers;
> > +
> > + mutex_lock(&ipmi_interfaces_mutex);
> > + list_for_each_entry_rcu(intf, &ipmi_interfaces, link) {
> > + if (intf->intf_num == if_num)
> > + goto found;
> > + }
> > + /* Not found, return an error */
> > + rv = -EINVAL;
> > + mutex_unlock(&ipmi_interfaces_mutex);
> > + return rv;
> > +
> > +found:
> > + handlers = intf->handlers;
> > + rv = handlers->get_smi_info(intf->send_info, data);
> > + mutex_unlock(&ipmi_interfaces_mutex);
> > +
> > + return rv;
> > +}
> > +EXPORT_SYMBOL(ipmi_get_smi_info);
> > +
> > +void ipmi_put_smi_info(struct ipmi_smi_info *data)
> > +{
> > + put_device(data->dev);
> > +}
> > +EXPORT_SYMBOL(ipmi_put_smi_info);
> > +
> > static void free_user(struct kref *ref)
> > {
> > ipmi_user_t user = container_of(ref, struct ipmi_user, refcount);
> > diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
> > index 035da9e..a41afca 100644
> > --- a/drivers/char/ipmi/ipmi_si_intf.c
> > +++ b/drivers/char/ipmi/ipmi_si_intf.c
> > @@ -57,6 +57,7 @@
> > #include <asm/irq.h>
> > #include <linux/interrupt.h>
> > #include <linux/rcupdate.h>
> > +#include <linux/ipmi.h>
> > #include <linux/ipmi_smi.h>
> > #include <asm/io.h>
> > #include "ipmi_si_sm.h"
> > @@ -107,10 +108,6 @@ enum si_type {
> > };
> > static char *si_to_str[] = { "kcs", "smic", "bt" };
> >
> > -enum ipmi_addr_src {
> > - SI_INVALID = 0, SI_HOTMOD, SI_HARDCODED, SI_SPMI, SI_ACPI, SI_SMBIOS,
> > - SI_PCI, SI_DEVICETREE, SI_DEFAULT
> > -};
> > static char *ipmi_addr_src_to_str[] = { NULL, "hotmod", "hardcoded", "SPMI",
> > "ACPI", "SMBIOS", "PCI",
> > "device-tree", "default" };
> > @@ -291,6 +288,7 @@ struct smi_info {
> > struct task_struct *thread;
> >
> > struct list_head link;
> > + struct ipmi_smi_info smi_data;
> > };
> >
> > #define smi_inc_stat(smi, stat) \
> > @@ -1186,6 +1184,18 @@ static int smi_start_processing(void *send_info,
> > return 0;
> > }
> >
> > +static int get_smi_info(void *send_info, struct ipmi_smi_info *data)
> > +{
> > + struct smi_info *new_smi = send_info;
> > + struct ipmi_smi_info *smi_data = &new_smi->smi_data;
> > +
> > + memcpy(data, smi_data, sizeof(*smi_data));
> > + data->addr_src = new_smi->addr_source;
> > + get_device(new_smi->dev);
> > +
> > + return 0;
> > +}
> > +
> > static void set_maintenance_mode(void *send_info, int enable)
> > {
> > struct smi_info *smi_info = send_info;
> > @@ -1197,6 +1207,7 @@ static void set_maintenance_mode(void *send_info, int enable)
> > static struct ipmi_smi_handlers handlers = {
> > .owner = THIS_MODULE,
> > .start_processing = smi_start_processing,
> > + .get_smi_info = get_smi_info,
> > .sender = sender,
> > .request_events = request_events,
> > .set_maintenance_mode = set_maintenance_mode,
> > @@ -2153,6 +2164,8 @@ static int __devinit ipmi_pnp_probe(struct pnp_dev *dev,
> > return -ENOMEM;
> >
> > info->addr_source = SI_ACPI;
> > + info->smi_data.addr_info.acpi_info.acpi_handle =
> > + acpi_dev->handle;
> > printk(KERN_INFO PFX "probing via ACPI\n");
> >
> > handle = acpi_dev->handle;
> > @@ -3102,6 +3115,7 @@ static int try_smi_init(struct smi_info *new_smi)
> > {
> > int rv = 0;
> > int i;
> > + struct ipmi_smi_info *smi_data;
> >
> > printk(KERN_INFO PFX "Trying %s-specified %s state"
> > " machine at %s address 0x%lx, slave address 0x%x,"
> > @@ -3263,6 +3277,8 @@ static int try_smi_init(struct smi_info *new_smi)
> > dev_info(new_smi->dev, "IPMI %s interface initialized\n",
> > si_to_str[new_smi->si_type]);
> >
> > + smi_data = &new_smi->smi_data;
> > + smi_data->dev = new_smi->dev;
> > return 0;
> >
> > out_err_stop_timer:
> > diff --git a/include/linux/ipmi.h b/include/linux/ipmi.h
> > index 65aae34..956bd41 100644
> > --- a/include/linux/ipmi.h
> > +++ b/include/linux/ipmi.h
> > @@ -454,6 +454,35 @@ unsigned int ipmi_addr_length(int addr_type);
> > /* Validate that the given IPMI address is valid. */
> > int ipmi_validate_addr(struct ipmi_addr *addr, int len);
> >
> > +enum ipmi_addr_src {
> > + SI_INVALID = 0, SI_HOTMOD, SI_HARDCODED, SI_SPMI, SI_ACPI, SI_SMBIOS,
> > + SI_PCI, SI_DEVICETREE, SI_DEFAULT
> > +};
> > +struct ipmi_smi_info {
> > + enum ipmi_addr_src addr_src;
> > + struct device *dev;
> > + /*
> > + * The addr_info can provide more detailed info of one IPMI device.
> > + * Now only SI_ACPI info is provided. And it depends on the SI_ACPI
> > + * address type. If the info is required for other address type, please
> > + * add it.
> > + */
> > + union {
> > + /* the acpi_info element is defined for the SI_ACPI
> > + * address type
> > + */
> > + struct {
> > + void *acpi_handle;
> > + } acpi_info;
> > + } addr_info;
> > +};
> > +
> > +/* This is to get the private info of ipmi_smi_t */
> > +extern int ipmi_get_smi_info(int if_num, struct ipmi_smi_info *data);
> > +/* This is to decrease refcount of dev added in the function of
> > + * ipmi_get_smi_info
> > + */
> > +extern void ipmi_put_smi_info(struct ipmi_smi_info *data);
> > #endif /* __KERNEL__ */
> >
> >
> > diff --git a/include/linux/ipmi_smi.h b/include/linux/ipmi_smi.h
> > index 4b48318..deabdcc 100644
> > --- a/include/linux/ipmi_smi.h
> > +++ b/include/linux/ipmi_smi.h
> > @@ -86,6 +86,13 @@ struct ipmi_smi_handlers {
> > int (*start_processing)(void *send_info,
> > ipmi_smi_t new_intf);
> >
> > + /*
> > + * Get the detailed private info of the low level interface and store
> > + * it into the structure of ipmi_smi_data. For example: the
> > + * ACPI device handle will be returned for the pnp_acpi IPMI device.
> > + */
> > + int (*get_smi_info)(void *send_info, struct ipmi_smi_info *data);
> > +
> > /* Called to enqueue an SMI message to be sent. This
> > operation is not allowed to fail. If an error occurs, it
> > should report back the error in a received message. It may
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC PATCH 1/4] IPMI: Add one interface to get more info of low-level IPMI device
2010-11-30 0:26 ` [RFC PATCH 1/4] IPMI: Add one interface to get more info of low-level IPMI device yakui.zhao
@ 2010-11-30 0:36 ` ykzhao
2010-12-06 1:06 ` ykzhao
0 siblings, 1 reply; 24+ messages in thread
From: ykzhao @ 2010-11-30 0:36 UTC (permalink / raw)
To: minyard; +Cc: openipmi-developer, lenb, linux-acpi
On Tue, 2010-11-30 at 08:26 +0800, yakui.zhao@intel.com wrote:
> From: Zhao Yakui <yakui.zhao@intel.com>
>
> The IPMI smi_watcher will be used to catch the IPMI interface as they come or go.
> In order to communicate with the correct IPMI device, it should be confirmed
> whether it is what we wanted especially on the system with multiple IPMI
> devices. But the new_smi callback function of smi_watcher provides very
> limited info(only the interface number and dev pointer) and there is no
> detailed info about the low level interface. For example: which mechansim
> registers the IPMI interface(ACPI, PCI, DMI and so on).
>
> This is to add one interface that can get more info of low-level IPMI
> device. For example: the ACPI device handle will be returned for the pnp_acpi
> IPMI device.
Hi, Corey
The following is updated in the patch set.
a. Change the function prototype of ipmi_put_smi_info.
The corresponding parameter is changed from "
Ipmi_put_smi_info(int iface) to ipmi_put_smi_info(struct ipmi_smi_info
*data)
b. Remove the incorrect refcount in previous patch set.
But for the issues about pulling data from the current source, very
sorry that I still statically construct them in the corresponding
smi_info as I meet with the following two issues when trying to pull
data from the current source.
1. no specific info is contained in smi_info. For example: ACPI handle
is important to the SI_ACPI type. But the corresponding handle is stored
in smi_info.
b. Need to add a lot of If/else macro definitions to handle the
corresponding IPMI device type.
Thanks.
>
> Signed-off-by: Zhao Yakui <yakui.zhao@intel.com>
> ---
> drivers/char/ipmi/ipmi_msghandler.c | 31 +++++++++++++++++++++++++++++++
> drivers/char/ipmi/ipmi_si_intf.c | 24 ++++++++++++++++++++----
> include/linux/ipmi.h | 29 +++++++++++++++++++++++++++++
> include/linux/ipmi_smi.h | 7 +++++++
> 4 files changed, 87 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
> index 2fe72f8..a90b3ec 100644
> --- a/drivers/char/ipmi/ipmi_msghandler.c
> +++ b/drivers/char/ipmi/ipmi_msghandler.c
> @@ -970,6 +970,37 @@ out_kfree:
> }
> EXPORT_SYMBOL(ipmi_create_user);
>
> +int ipmi_get_smi_info(int if_num, struct ipmi_smi_info *data)
> +{
> + int rv = 0;
> + ipmi_smi_t intf;
> + struct ipmi_smi_handlers *handlers;
> +
> + mutex_lock(&ipmi_interfaces_mutex);
> + list_for_each_entry_rcu(intf, &ipmi_interfaces, link) {
> + if (intf->intf_num == if_num)
> + goto found;
> + }
> + /* Not found, return an error */
> + rv = -EINVAL;
> + mutex_unlock(&ipmi_interfaces_mutex);
> + return rv;
> +
> +found:
> + handlers = intf->handlers;
> + rv = handlers->get_smi_info(intf->send_info, data);
> + mutex_unlock(&ipmi_interfaces_mutex);
> +
> + return rv;
> +}
> +EXPORT_SYMBOL(ipmi_get_smi_info);
> +
> +void ipmi_put_smi_info(struct ipmi_smi_info *data)
> +{
> + put_device(data->dev);
> +}
> +EXPORT_SYMBOL(ipmi_put_smi_info);
> +
> static void free_user(struct kref *ref)
> {
> ipmi_user_t user = container_of(ref, struct ipmi_user, refcount);
> diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
> index 035da9e..a41afca 100644
> --- a/drivers/char/ipmi/ipmi_si_intf.c
> +++ b/drivers/char/ipmi/ipmi_si_intf.c
> @@ -57,6 +57,7 @@
> #include <asm/irq.h>
> #include <linux/interrupt.h>
> #include <linux/rcupdate.h>
> +#include <linux/ipmi.h>
> #include <linux/ipmi_smi.h>
> #include <asm/io.h>
> #include "ipmi_si_sm.h"
> @@ -107,10 +108,6 @@ enum si_type {
> };
> static char *si_to_str[] = { "kcs", "smic", "bt" };
>
> -enum ipmi_addr_src {
> - SI_INVALID = 0, SI_HOTMOD, SI_HARDCODED, SI_SPMI, SI_ACPI, SI_SMBIOS,
> - SI_PCI, SI_DEVICETREE, SI_DEFAULT
> -};
> static char *ipmi_addr_src_to_str[] = { NULL, "hotmod", "hardcoded", "SPMI",
> "ACPI", "SMBIOS", "PCI",
> "device-tree", "default" };
> @@ -291,6 +288,7 @@ struct smi_info {
> struct task_struct *thread;
>
> struct list_head link;
> + struct ipmi_smi_info smi_data;
> };
>
> #define smi_inc_stat(smi, stat) \
> @@ -1186,6 +1184,18 @@ static int smi_start_processing(void *send_info,
> return 0;
> }
>
> +static int get_smi_info(void *send_info, struct ipmi_smi_info *data)
> +{
> + struct smi_info *new_smi = send_info;
> + struct ipmi_smi_info *smi_data = &new_smi->smi_data;
> +
> + memcpy(data, smi_data, sizeof(*smi_data));
> + data->addr_src = new_smi->addr_source;
> + get_device(new_smi->dev);
> +
> + return 0;
> +}
> +
> static void set_maintenance_mode(void *send_info, int enable)
> {
> struct smi_info *smi_info = send_info;
> @@ -1197,6 +1207,7 @@ static void set_maintenance_mode(void *send_info, int enable)
> static struct ipmi_smi_handlers handlers = {
> .owner = THIS_MODULE,
> .start_processing = smi_start_processing,
> + .get_smi_info = get_smi_info,
> .sender = sender,
> .request_events = request_events,
> .set_maintenance_mode = set_maintenance_mode,
> @@ -2153,6 +2164,8 @@ static int __devinit ipmi_pnp_probe(struct pnp_dev *dev,
> return -ENOMEM;
>
> info->addr_source = SI_ACPI;
> + info->smi_data.addr_info.acpi_info.acpi_handle =
> + acpi_dev->handle;
> printk(KERN_INFO PFX "probing via ACPI\n");
>
> handle = acpi_dev->handle;
> @@ -3102,6 +3115,7 @@ static int try_smi_init(struct smi_info *new_smi)
> {
> int rv = 0;
> int i;
> + struct ipmi_smi_info *smi_data;
>
> printk(KERN_INFO PFX "Trying %s-specified %s state"
> " machine at %s address 0x%lx, slave address 0x%x,"
> @@ -3263,6 +3277,8 @@ static int try_smi_init(struct smi_info *new_smi)
> dev_info(new_smi->dev, "IPMI %s interface initialized\n",
> si_to_str[new_smi->si_type]);
>
> + smi_data = &new_smi->smi_data;
> + smi_data->dev = new_smi->dev;
> return 0;
>
> out_err_stop_timer:
> diff --git a/include/linux/ipmi.h b/include/linux/ipmi.h
> index 65aae34..956bd41 100644
> --- a/include/linux/ipmi.h
> +++ b/include/linux/ipmi.h
> @@ -454,6 +454,35 @@ unsigned int ipmi_addr_length(int addr_type);
> /* Validate that the given IPMI address is valid. */
> int ipmi_validate_addr(struct ipmi_addr *addr, int len);
>
> +enum ipmi_addr_src {
> + SI_INVALID = 0, SI_HOTMOD, SI_HARDCODED, SI_SPMI, SI_ACPI, SI_SMBIOS,
> + SI_PCI, SI_DEVICETREE, SI_DEFAULT
> +};
> +struct ipmi_smi_info {
> + enum ipmi_addr_src addr_src;
> + struct device *dev;
> + /*
> + * The addr_info can provide more detailed info of one IPMI device.
> + * Now only SI_ACPI info is provided. And it depends on the SI_ACPI
> + * address type. If the info is required for other address type, please
> + * add it.
> + */
> + union {
> + /* the acpi_info element is defined for the SI_ACPI
> + * address type
> + */
> + struct {
> + void *acpi_handle;
> + } acpi_info;
> + } addr_info;
> +};
> +
> +/* This is to get the private info of ipmi_smi_t */
> +extern int ipmi_get_smi_info(int if_num, struct ipmi_smi_info *data);
> +/* This is to decrease refcount of dev added in the function of
> + * ipmi_get_smi_info
> + */
> +extern void ipmi_put_smi_info(struct ipmi_smi_info *data);
> #endif /* __KERNEL__ */
>
>
> diff --git a/include/linux/ipmi_smi.h b/include/linux/ipmi_smi.h
> index 4b48318..deabdcc 100644
> --- a/include/linux/ipmi_smi.h
> +++ b/include/linux/ipmi_smi.h
> @@ -86,6 +86,13 @@ struct ipmi_smi_handlers {
> int (*start_processing)(void *send_info,
> ipmi_smi_t new_intf);
>
> + /*
> + * Get the detailed private info of the low level interface and store
> + * it into the structure of ipmi_smi_data. For example: the
> + * ACPI device handle will be returned for the pnp_acpi IPMI device.
> + */
> + int (*get_smi_info)(void *send_info, struct ipmi_smi_info *data);
> +
> /* Called to enqueue an SMI message to be sent. This
> operation is not allowed to fail. If an error occurs, it
> should report back the error in a received message. It may
^ permalink raw reply [flat|nested] 24+ messages in thread
* [RFC PATCH 1/4] IPMI: Add one interface to get more info of low-level IPMI device
2010-11-30 0:26 [RFC PATCH 0/4_v13] IPMI/ACPI: Install the ACPI IPMI opregion yakui.zhao
@ 2010-11-30 0:26 ` yakui.zhao
2010-11-30 0:36 ` ykzhao
0 siblings, 1 reply; 24+ messages in thread
From: yakui.zhao @ 2010-11-30 0:26 UTC (permalink / raw)
To: minyard; +Cc: openipmi-developer, lenb, linux-acpi, Zhao Yakui
From: Zhao Yakui <yakui.zhao@intel.com>
The IPMI smi_watcher will be used to catch the IPMI interface as they come or go.
In order to communicate with the correct IPMI device, it should be confirmed
whether it is what we wanted especially on the system with multiple IPMI
devices. But the new_smi callback function of smi_watcher provides very
limited info(only the interface number and dev pointer) and there is no
detailed info about the low level interface. For example: which mechansim
registers the IPMI interface(ACPI, PCI, DMI and so on).
This is to add one interface that can get more info of low-level IPMI
device. For example: the ACPI device handle will be returned for the pnp_acpi
IPMI device.
Signed-off-by: Zhao Yakui <yakui.zhao@intel.com>
---
drivers/char/ipmi/ipmi_msghandler.c | 31 +++++++++++++++++++++++++++++++
drivers/char/ipmi/ipmi_si_intf.c | 24 ++++++++++++++++++++----
include/linux/ipmi.h | 29 +++++++++++++++++++++++++++++
include/linux/ipmi_smi.h | 7 +++++++
4 files changed, 87 insertions(+), 4 deletions(-)
diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
index 2fe72f8..a90b3ec 100644
--- a/drivers/char/ipmi/ipmi_msghandler.c
+++ b/drivers/char/ipmi/ipmi_msghandler.c
@@ -970,6 +970,37 @@ out_kfree:
}
EXPORT_SYMBOL(ipmi_create_user);
+int ipmi_get_smi_info(int if_num, struct ipmi_smi_info *data)
+{
+ int rv = 0;
+ ipmi_smi_t intf;
+ struct ipmi_smi_handlers *handlers;
+
+ mutex_lock(&ipmi_interfaces_mutex);
+ list_for_each_entry_rcu(intf, &ipmi_interfaces, link) {
+ if (intf->intf_num == if_num)
+ goto found;
+ }
+ /* Not found, return an error */
+ rv = -EINVAL;
+ mutex_unlock(&ipmi_interfaces_mutex);
+ return rv;
+
+found:
+ handlers = intf->handlers;
+ rv = handlers->get_smi_info(intf->send_info, data);
+ mutex_unlock(&ipmi_interfaces_mutex);
+
+ return rv;
+}
+EXPORT_SYMBOL(ipmi_get_smi_info);
+
+void ipmi_put_smi_info(struct ipmi_smi_info *data)
+{
+ put_device(data->dev);
+}
+EXPORT_SYMBOL(ipmi_put_smi_info);
+
static void free_user(struct kref *ref)
{
ipmi_user_t user = container_of(ref, struct ipmi_user, refcount);
diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
index 035da9e..a41afca 100644
--- a/drivers/char/ipmi/ipmi_si_intf.c
+++ b/drivers/char/ipmi/ipmi_si_intf.c
@@ -57,6 +57,7 @@
#include <asm/irq.h>
#include <linux/interrupt.h>
#include <linux/rcupdate.h>
+#include <linux/ipmi.h>
#include <linux/ipmi_smi.h>
#include <asm/io.h>
#include "ipmi_si_sm.h"
@@ -107,10 +108,6 @@ enum si_type {
};
static char *si_to_str[] = { "kcs", "smic", "bt" };
-enum ipmi_addr_src {
- SI_INVALID = 0, SI_HOTMOD, SI_HARDCODED, SI_SPMI, SI_ACPI, SI_SMBIOS,
- SI_PCI, SI_DEVICETREE, SI_DEFAULT
-};
static char *ipmi_addr_src_to_str[] = { NULL, "hotmod", "hardcoded", "SPMI",
"ACPI", "SMBIOS", "PCI",
"device-tree", "default" };
@@ -291,6 +288,7 @@ struct smi_info {
struct task_struct *thread;
struct list_head link;
+ struct ipmi_smi_info smi_data;
};
#define smi_inc_stat(smi, stat) \
@@ -1186,6 +1184,18 @@ static int smi_start_processing(void *send_info,
return 0;
}
+static int get_smi_info(void *send_info, struct ipmi_smi_info *data)
+{
+ struct smi_info *new_smi = send_info;
+ struct ipmi_smi_info *smi_data = &new_smi->smi_data;
+
+ memcpy(data, smi_data, sizeof(*smi_data));
+ data->addr_src = new_smi->addr_source;
+ get_device(new_smi->dev);
+
+ return 0;
+}
+
static void set_maintenance_mode(void *send_info, int enable)
{
struct smi_info *smi_info = send_info;
@@ -1197,6 +1207,7 @@ static void set_maintenance_mode(void *send_info, int enable)
static struct ipmi_smi_handlers handlers = {
.owner = THIS_MODULE,
.start_processing = smi_start_processing,
+ .get_smi_info = get_smi_info,
.sender = sender,
.request_events = request_events,
.set_maintenance_mode = set_maintenance_mode,
@@ -2153,6 +2164,8 @@ static int __devinit ipmi_pnp_probe(struct pnp_dev *dev,
return -ENOMEM;
info->addr_source = SI_ACPI;
+ info->smi_data.addr_info.acpi_info.acpi_handle =
+ acpi_dev->handle;
printk(KERN_INFO PFX "probing via ACPI\n");
handle = acpi_dev->handle;
@@ -3102,6 +3115,7 @@ static int try_smi_init(struct smi_info *new_smi)
{
int rv = 0;
int i;
+ struct ipmi_smi_info *smi_data;
printk(KERN_INFO PFX "Trying %s-specified %s state"
" machine at %s address 0x%lx, slave address 0x%x,"
@@ -3263,6 +3277,8 @@ static int try_smi_init(struct smi_info *new_smi)
dev_info(new_smi->dev, "IPMI %s interface initialized\n",
si_to_str[new_smi->si_type]);
+ smi_data = &new_smi->smi_data;
+ smi_data->dev = new_smi->dev;
return 0;
out_err_stop_timer:
diff --git a/include/linux/ipmi.h b/include/linux/ipmi.h
index 65aae34..956bd41 100644
--- a/include/linux/ipmi.h
+++ b/include/linux/ipmi.h
@@ -454,6 +454,35 @@ unsigned int ipmi_addr_length(int addr_type);
/* Validate that the given IPMI address is valid. */
int ipmi_validate_addr(struct ipmi_addr *addr, int len);
+enum ipmi_addr_src {
+ SI_INVALID = 0, SI_HOTMOD, SI_HARDCODED, SI_SPMI, SI_ACPI, SI_SMBIOS,
+ SI_PCI, SI_DEVICETREE, SI_DEFAULT
+};
+struct ipmi_smi_info {
+ enum ipmi_addr_src addr_src;
+ struct device *dev;
+ /*
+ * The addr_info can provide more detailed info of one IPMI device.
+ * Now only SI_ACPI info is provided. And it depends on the SI_ACPI
+ * address type. If the info is required for other address type, please
+ * add it.
+ */
+ union {
+ /* the acpi_info element is defined for the SI_ACPI
+ * address type
+ */
+ struct {
+ void *acpi_handle;
+ } acpi_info;
+ } addr_info;
+};
+
+/* This is to get the private info of ipmi_smi_t */
+extern int ipmi_get_smi_info(int if_num, struct ipmi_smi_info *data);
+/* This is to decrease refcount of dev added in the function of
+ * ipmi_get_smi_info
+ */
+extern void ipmi_put_smi_info(struct ipmi_smi_info *data);
#endif /* __KERNEL__ */
diff --git a/include/linux/ipmi_smi.h b/include/linux/ipmi_smi.h
index 4b48318..deabdcc 100644
--- a/include/linux/ipmi_smi.h
+++ b/include/linux/ipmi_smi.h
@@ -86,6 +86,13 @@ struct ipmi_smi_handlers {
int (*start_processing)(void *send_info,
ipmi_smi_t new_intf);
+ /*
+ * Get the detailed private info of the low level interface and store
+ * it into the structure of ipmi_smi_data. For example: the
+ * ACPI device handle will be returned for the pnp_acpi IPMI device.
+ */
+ int (*get_smi_info)(void *send_info, struct ipmi_smi_info *data);
+
/* Called to enqueue an SMI message to be sent. This
operation is not allowed to fail. If an error occurs, it
should report back the error in a received message. It may
--
1.5.4.5
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [RFC PATCH 1/4] IPMI: Add one interface to get more info of low-level IPMI device
2010-10-25 3:25 ` Corey Minyard
@ 2010-10-25 7:00 ` ykzhao
0 siblings, 0 replies; 24+ messages in thread
From: ykzhao @ 2010-10-25 7:00 UTC (permalink / raw)
To: Corey Minyard; +Cc: openipmi-developer, linux-acpi, lenb
On Mon, 2010-10-25 at 11:25 +0800, Corey Minyard wrote:
> Well, no, you are returning a pointer to something that is in the smi
> data structure. For that you would need a refcount, because that
> structure can cease to exist asynchronously to your code. Instead, just
> pass in the structure you want to fill in. Like:
>
> int ipmi_get_smi_info(int if_num, struct ipmi_smi_info *data)
> {
> /* checks here */
>
> handlers = intf->handlers;
> rv = handlers->get_smi_info(intf->send_info, data);
> }
>
> static int get_smi_info(void *send_info,
> struct ipmi_smi_info *data)
> {
> struct smi_info *new_smi = send_info;
>
> data->smi_info.dev = new_smi->dev;
> data->addr_src = new_smi->addr_source;
> data->smi_info.addr_info = new_smi->addr_info;
>
> return 0;
> }
>
You are right. In theory the refcount should be added if it returns the
pointer of ipmi_smi_info. But as it resides in the smi_info, it will
always exist before the smi_info is released.
OK. To make the thing simpler, I will use the mechanism you recommended.
>
>
> Why are you passing an address type in to the function? That means you
> would have to know the address type to get anything out of if. It would
> be better to just return the info and let the user do the comparison.
> That way, if something wanted to fetch the info to get the device, or
> some other info, you don't have to try every address type.
OK. I will remove the argument of address type.
>
> Speaking of the device, that is a refcounted structure. You can't just
> pass it back without doing refcounts on it.
Yes. In the previous-version patch set, the refcount of dev is increased
when the ipmi_get_smi_info is called. And it is decreased when the
ipmi_put_smi_info. But after the discussion, it seems that I
misunderstand the refcount and then I remove it in this version.
Ok, I will call it back.
>
> Can you rename the union addr_info, and comment that which union
> structure is used depends on the address type?
>
> Also, I don't know anything about this ACPI handle, but is that a
> permanent structure? Can you just grab it like you do and pass it
> around? I would guess not.
Sorry that I don't know the requirement for other address type when defining the
structure of ipmi_smi_info. But for the ACPI address type, the acpi handle is very
important. So it is defined in the union structure.
>
> -corey
>
> On 10/22/2010 04:10 AM, yakui.zhao@intel.com wrote:
> > From: Zhao Yakui<yakui.zhao@intel.com>
> >
> > The IPMI smi_watcher will be used to catch the IPMI interface as they come or go.
> > In order to communicate with the correct IPMI device, it should be confirmed
> > whether it is what we wanted especially on the system with multiple IPMI
> > devices. But the new_smi callback function of smi_watcher provides very
> > limited info(only the interface number and dev pointer) and there is no
> > detailed info about the low level interface. For example: which mechansim
> > registers the IPMI interface(ACPI, PCI, DMI and so on).
> >
> > This is to add one interface that can get more info of low-level IPMI
> > device. For example: the ACPI device handle will be returned for the pnp_acpi
> > IPMI device.
> >
> > Signed-off-by: Zhao Yakui<yakui.zhao@intel.com>
> > ---
> > In this patch only the info of pnp_acpi IPMI is provided. If the detailed info
> > is also required for other mechanism, please add them in the structure of
> > ipmi_smi_info.
> >
> > drivers/char/ipmi/ipmi_msghandler.c | 26 ++++++++++++++++++++++++++
> > drivers/char/ipmi/ipmi_si_intf.c | 28 ++++++++++++++++++++++++----
> > include/linux/ipmi.h | 23 +++++++++++++++++++++++
> > include/linux/ipmi_smi.h | 11 +++++++++++
> > 4 files changed, 84 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
> > index 4f3f8c9..6f4da59 100644
> > --- a/drivers/char/ipmi/ipmi_msghandler.c
> > +++ b/drivers/char/ipmi/ipmi_msghandler.c
> > @@ -970,6 +970,32 @@ out_kfree:
> > }
> > EXPORT_SYMBOL(ipmi_create_user);
> >
> > +int ipmi_get_smi_info(int if_num, enum ipmi_addr_src type,
> > + struct ipmi_smi_info **data)
> > +{
> > + int rv = 0;
> > + ipmi_smi_t intf;
> > + struct ipmi_smi_handlers *handlers;
> > +
> > + mutex_lock(&ipmi_interfaces_mutex);
> > + list_for_each_entry_rcu(intf,&ipmi_interfaces, link) {
> > + if (intf->intf_num == if_num)
> > + goto found;
> > + }
> > + /* Not found, return an error */
> > + rv = -EINVAL;
> > + mutex_unlock(&ipmi_interfaces_mutex);
> > + return rv;
> > +
> > +found:
> > + handlers = intf->handlers;
> > + rv = handlers->get_smi_info(intf->send_info, type, data);
> > + mutex_unlock(&ipmi_interfaces_mutex);
> > +
> > + return rv;
> > +}
> > +EXPORT_SYMBOL(ipmi_get_smi_info);
> > +
> > static void free_user(struct kref *ref)
> > {
> > ipmi_user_t user = container_of(ref, struct ipmi_user, refcount);
> > diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
> > index 7bd7c45..73b1c82 100644
> > --- a/drivers/char/ipmi/ipmi_si_intf.c
> > +++ b/drivers/char/ipmi/ipmi_si_intf.c
> > @@ -57,6 +57,7 @@
> > #include<asm/irq.h>
> > #include<linux/interrupt.h>
> > #include<linux/rcupdate.h>
> > +#include<linux/ipmi.h>
> > #include<linux/ipmi_smi.h>
> > #include<asm/io.h>
> > #include "ipmi_si_sm.h"
> > @@ -107,10 +108,6 @@ enum si_type {
> > };
> > static char *si_to_str[] = { "kcs", "smic", "bt" };
> >
> > -enum ipmi_addr_src {
> > - SI_INVALID = 0, SI_HOTMOD, SI_HARDCODED, SI_SPMI, SI_ACPI, SI_SMBIOS,
> > - SI_PCI, SI_DEVICETREE, SI_DEFAULT
> > -};
> > static char *ipmi_addr_src_to_str[] = { NULL, "hotmod", "hardcoded", "SPMI",
> > "ACPI", "SMBIOS", "PCI",
> > "device-tree", "default" };
> > @@ -291,6 +288,7 @@ struct smi_info {
> > struct task_struct *thread;
> >
> > struct list_head link;
> > + struct ipmi_smi_info smi_data;
> > };
> >
> > #define smi_inc_stat(smi, stat) \
> > @@ -1186,6 +1184,22 @@ static int smi_start_processing(void *send_info,
> > return 0;
> > }
> >
> > +static int get_smi_info(void *send_info,
> > + enum ipmi_addr_src type,
> > + struct ipmi_smi_info **data)
> > +{
> > + struct smi_info *new_smi = send_info;
> > + struct ipmi_smi_info *smi_data =&new_smi->smi_data;
> > +
> > + if (new_smi->addr_source != type)
> > + return -EINVAL;
> > +
> > + smi_data->addr_src = type;
> > +
> > + *data = smi_data;
> > + return 0;
> > +}
> > +
> > static void set_maintenance_mode(void *send_info, int enable)
> > {
> > struct smi_info *smi_info = send_info;
> > @@ -1197,6 +1211,7 @@ static void set_maintenance_mode(void *send_info, int enable)
> > static struct ipmi_smi_handlers handlers = {
> > .owner = THIS_MODULE,
> > .start_processing = smi_start_processing,
> > + .get_smi_info = get_smi_info,
> > .sender = sender,
> > .request_events = request_events,
> > .set_maintenance_mode = set_maintenance_mode,
> > @@ -2143,6 +2158,8 @@ static int __devinit ipmi_pnp_probe(struct pnp_dev *dev,
> > return -ENOMEM;
> >
> > info->addr_source = SI_ACPI;
> > + info->smi_data.smi_info.acpi_info.acpi_handle =
> > + acpi_dev->handle;
> > printk(KERN_INFO PFX "probing via ACPI\n");
> >
> > handle = acpi_dev->handle;
> > @@ -3092,6 +3109,7 @@ static int try_smi_init(struct smi_info *new_smi)
> > {
> > int rv = 0;
> > int i;
> > + struct ipmi_smi_info *smi_data =&new_smi->smi_data;
> >
> > printk(KERN_INFO PFX "Trying %s-specified %s state"
> > " machine at %s address 0x%lx, slave address 0x%x,"
> > @@ -3217,6 +3235,8 @@ static int try_smi_init(struct smi_info *new_smi)
> > new_smi->dev_registered = 1;
> > }
> >
> > + smi_data->dev = new_smi->dev;
> > +
> > rv = ipmi_register_smi(&handlers,
> > new_smi,
> > &new_smi->device_id,
> > diff --git a/include/linux/ipmi.h b/include/linux/ipmi.h
> > index 65aae34..1ce428f 100644
> > --- a/include/linux/ipmi.h
> > +++ b/include/linux/ipmi.h
> > @@ -454,6 +454,29 @@ unsigned int ipmi_addr_length(int addr_type);
> > /* Validate that the given IPMI address is valid. */
> > int ipmi_validate_addr(struct ipmi_addr *addr, int len);
> >
> > +enum ipmi_addr_src {
> > + SI_INVALID = 0, SI_HOTMOD, SI_HARDCODED, SI_SPMI, SI_ACPI, SI_SMBIOS,
> > + SI_PCI, SI_DEVICETREE, SI_DEFAULT
> > +};
> > +struct ipmi_smi_info{
> > + enum ipmi_addr_src addr_src;
> > + struct device *dev;
> > + union {
> > + /*
> > + * Now only SI_ACPI info is provided. If the info is required
> > + * for other type, please add it
> > + */
> > +#ifdef CONFIG_ACPI
> > + struct {
> > + void *acpi_handle;
> > + } acpi_info;
> > +#endif
> > + } smi_info;
> > +};
> > +
> > +/* This is to get the private info of ipmi_smi_t. The pointer is returned */
> > +extern int ipmi_get_smi_info(int if_num, enum ipmi_addr_src type,
> > + struct ipmi_smi_info **data);
> > #endif /* __KERNEL__ */
> >
> >
> > diff --git a/include/linux/ipmi_smi.h b/include/linux/ipmi_smi.h
> > index 4b48318..bfa8f40 100644
> > --- a/include/linux/ipmi_smi.h
> > +++ b/include/linux/ipmi_smi.h
> > @@ -86,6 +86,17 @@ struct ipmi_smi_handlers {
> > int (*start_processing)(void *send_info,
> > ipmi_smi_t new_intf);
> >
> > + /*
> > + * Get the detailed private info of the low level interface and return
> > + * the corresponding pointer.
> > + * For example: the ACPI device handle will be returned for the
> > + * pnp_acpi IPMI device. Of course it will firstly compare the
> > + * interface type of low-level interface(Hardcoded type, DMI,
> > + * ACPI and so on). If the type doesn't match, it will return -EINVAL.
> > + */
> > + int (*get_smi_info)(void *send_info, enum ipmi_addr_src type,
> > + struct ipmi_smi_info **data);
> > +
> > /* Called to enqueue an SMI message to be sent. This
> > operation is not allowed to fail. If an error occurs, it
> > should report back the error in a received message. It may
> >
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC PATCH 1/4] IPMI: Add one interface to get more info of low-level IPMI device
2010-10-22 9:10 ` [RFC PATCH 1/4] IPMI: Add one interface to get more info of low-level IPMI device yakui.zhao
2010-10-25 1:19 ` ykzhao
@ 2010-10-25 3:25 ` Corey Minyard
2010-10-25 7:00 ` ykzhao
1 sibling, 1 reply; 24+ messages in thread
From: Corey Minyard @ 2010-10-25 3:25 UTC (permalink / raw)
To: yakui.zhao; +Cc: openipmi-developer, linux-acpi, lenb
Well, no, you are returning a pointer to something that is in the smi
data structure. For that you would need a refcount, because that
structure can cease to exist asynchronously to your code. Instead, just
pass in the structure you want to fill in. Like:
int ipmi_get_smi_info(int if_num, struct ipmi_smi_info *data)
{
/* checks here */
handlers = intf->handlers;
rv = handlers->get_smi_info(intf->send_info, data);
}
static int get_smi_info(void *send_info,
struct ipmi_smi_info *data)
{
struct smi_info *new_smi = send_info;
data->smi_info.dev = new_smi->dev;
data->addr_src = new_smi->addr_source;
data->smi_info.addr_info = new_smi->addr_info;
return 0;
}
Why are you passing an address type in to the function? That means you
would have to know the address type to get anything out of if. It would
be better to just return the info and let the user do the comparison.
That way, if something wanted to fetch the info to get the device, or
some other info, you don't have to try every address type.
Speaking of the device, that is a refcounted structure. You can't just
pass it back without doing refcounts on it.
Can you rename the union addr_info, and comment that which union
structure is used depends on the address type?
Also, I don't know anything about this ACPI handle, but is that a
permanent structure? Can you just grab it like you do and pass it
around? I would guess not.
-corey
On 10/22/2010 04:10 AM, yakui.zhao@intel.com wrote:
> From: Zhao Yakui<yakui.zhao@intel.com>
>
> The IPMI smi_watcher will be used to catch the IPMI interface as they come or go.
> In order to communicate with the correct IPMI device, it should be confirmed
> whether it is what we wanted especially on the system with multiple IPMI
> devices. But the new_smi callback function of smi_watcher provides very
> limited info(only the interface number and dev pointer) and there is no
> detailed info about the low level interface. For example: which mechansim
> registers the IPMI interface(ACPI, PCI, DMI and so on).
>
> This is to add one interface that can get more info of low-level IPMI
> device. For example: the ACPI device handle will be returned for the pnp_acpi
> IPMI device.
>
> Signed-off-by: Zhao Yakui<yakui.zhao@intel.com>
> ---
> In this patch only the info of pnp_acpi IPMI is provided. If the detailed info
> is also required for other mechanism, please add them in the structure of
> ipmi_smi_info.
>
> drivers/char/ipmi/ipmi_msghandler.c | 26 ++++++++++++++++++++++++++
> drivers/char/ipmi/ipmi_si_intf.c | 28 ++++++++++++++++++++++++----
> include/linux/ipmi.h | 23 +++++++++++++++++++++++
> include/linux/ipmi_smi.h | 11 +++++++++++
> 4 files changed, 84 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
> index 4f3f8c9..6f4da59 100644
> --- a/drivers/char/ipmi/ipmi_msghandler.c
> +++ b/drivers/char/ipmi/ipmi_msghandler.c
> @@ -970,6 +970,32 @@ out_kfree:
> }
> EXPORT_SYMBOL(ipmi_create_user);
>
> +int ipmi_get_smi_info(int if_num, enum ipmi_addr_src type,
> + struct ipmi_smi_info **data)
> +{
> + int rv = 0;
> + ipmi_smi_t intf;
> + struct ipmi_smi_handlers *handlers;
> +
> + mutex_lock(&ipmi_interfaces_mutex);
> + list_for_each_entry_rcu(intf,&ipmi_interfaces, link) {
> + if (intf->intf_num == if_num)
> + goto found;
> + }
> + /* Not found, return an error */
> + rv = -EINVAL;
> + mutex_unlock(&ipmi_interfaces_mutex);
> + return rv;
> +
> +found:
> + handlers = intf->handlers;
> + rv = handlers->get_smi_info(intf->send_info, type, data);
> + mutex_unlock(&ipmi_interfaces_mutex);
> +
> + return rv;
> +}
> +EXPORT_SYMBOL(ipmi_get_smi_info);
> +
> static void free_user(struct kref *ref)
> {
> ipmi_user_t user = container_of(ref, struct ipmi_user, refcount);
> diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
> index 7bd7c45..73b1c82 100644
> --- a/drivers/char/ipmi/ipmi_si_intf.c
> +++ b/drivers/char/ipmi/ipmi_si_intf.c
> @@ -57,6 +57,7 @@
> #include<asm/irq.h>
> #include<linux/interrupt.h>
> #include<linux/rcupdate.h>
> +#include<linux/ipmi.h>
> #include<linux/ipmi_smi.h>
> #include<asm/io.h>
> #include "ipmi_si_sm.h"
> @@ -107,10 +108,6 @@ enum si_type {
> };
> static char *si_to_str[] = { "kcs", "smic", "bt" };
>
> -enum ipmi_addr_src {
> - SI_INVALID = 0, SI_HOTMOD, SI_HARDCODED, SI_SPMI, SI_ACPI, SI_SMBIOS,
> - SI_PCI, SI_DEVICETREE, SI_DEFAULT
> -};
> static char *ipmi_addr_src_to_str[] = { NULL, "hotmod", "hardcoded", "SPMI",
> "ACPI", "SMBIOS", "PCI",
> "device-tree", "default" };
> @@ -291,6 +288,7 @@ struct smi_info {
> struct task_struct *thread;
>
> struct list_head link;
> + struct ipmi_smi_info smi_data;
> };
>
> #define smi_inc_stat(smi, stat) \
> @@ -1186,6 +1184,22 @@ static int smi_start_processing(void *send_info,
> return 0;
> }
>
> +static int get_smi_info(void *send_info,
> + enum ipmi_addr_src type,
> + struct ipmi_smi_info **data)
> +{
> + struct smi_info *new_smi = send_info;
> + struct ipmi_smi_info *smi_data =&new_smi->smi_data;
> +
> + if (new_smi->addr_source != type)
> + return -EINVAL;
> +
> + smi_data->addr_src = type;
> +
> + *data = smi_data;
> + return 0;
> +}
> +
> static void set_maintenance_mode(void *send_info, int enable)
> {
> struct smi_info *smi_info = send_info;
> @@ -1197,6 +1211,7 @@ static void set_maintenance_mode(void *send_info, int enable)
> static struct ipmi_smi_handlers handlers = {
> .owner = THIS_MODULE,
> .start_processing = smi_start_processing,
> + .get_smi_info = get_smi_info,
> .sender = sender,
> .request_events = request_events,
> .set_maintenance_mode = set_maintenance_mode,
> @@ -2143,6 +2158,8 @@ static int __devinit ipmi_pnp_probe(struct pnp_dev *dev,
> return -ENOMEM;
>
> info->addr_source = SI_ACPI;
> + info->smi_data.smi_info.acpi_info.acpi_handle =
> + acpi_dev->handle;
> printk(KERN_INFO PFX "probing via ACPI\n");
>
> handle = acpi_dev->handle;
> @@ -3092,6 +3109,7 @@ static int try_smi_init(struct smi_info *new_smi)
> {
> int rv = 0;
> int i;
> + struct ipmi_smi_info *smi_data =&new_smi->smi_data;
>
> printk(KERN_INFO PFX "Trying %s-specified %s state"
> " machine at %s address 0x%lx, slave address 0x%x,"
> @@ -3217,6 +3235,8 @@ static int try_smi_init(struct smi_info *new_smi)
> new_smi->dev_registered = 1;
> }
>
> + smi_data->dev = new_smi->dev;
> +
> rv = ipmi_register_smi(&handlers,
> new_smi,
> &new_smi->device_id,
> diff --git a/include/linux/ipmi.h b/include/linux/ipmi.h
> index 65aae34..1ce428f 100644
> --- a/include/linux/ipmi.h
> +++ b/include/linux/ipmi.h
> @@ -454,6 +454,29 @@ unsigned int ipmi_addr_length(int addr_type);
> /* Validate that the given IPMI address is valid. */
> int ipmi_validate_addr(struct ipmi_addr *addr, int len);
>
> +enum ipmi_addr_src {
> + SI_INVALID = 0, SI_HOTMOD, SI_HARDCODED, SI_SPMI, SI_ACPI, SI_SMBIOS,
> + SI_PCI, SI_DEVICETREE, SI_DEFAULT
> +};
> +struct ipmi_smi_info{
> + enum ipmi_addr_src addr_src;
> + struct device *dev;
> + union {
> + /*
> + * Now only SI_ACPI info is provided. If the info is required
> + * for other type, please add it
> + */
> +#ifdef CONFIG_ACPI
> + struct {
> + void *acpi_handle;
> + } acpi_info;
> +#endif
> + } smi_info;
> +};
> +
> +/* This is to get the private info of ipmi_smi_t. The pointer is returned */
> +extern int ipmi_get_smi_info(int if_num, enum ipmi_addr_src type,
> + struct ipmi_smi_info **data);
> #endif /* __KERNEL__ */
>
>
> diff --git a/include/linux/ipmi_smi.h b/include/linux/ipmi_smi.h
> index 4b48318..bfa8f40 100644
> --- a/include/linux/ipmi_smi.h
> +++ b/include/linux/ipmi_smi.h
> @@ -86,6 +86,17 @@ struct ipmi_smi_handlers {
> int (*start_processing)(void *send_info,
> ipmi_smi_t new_intf);
>
> + /*
> + * Get the detailed private info of the low level interface and return
> + * the corresponding pointer.
> + * For example: the ACPI device handle will be returned for the
> + * pnp_acpi IPMI device. Of course it will firstly compare the
> + * interface type of low-level interface(Hardcoded type, DMI,
> + * ACPI and so on). If the type doesn't match, it will return -EINVAL.
> + */
> + int (*get_smi_info)(void *send_info, enum ipmi_addr_src type,
> + struct ipmi_smi_info **data);
> +
> /* Called to enqueue an SMI message to be sent. This
> operation is not allowed to fail. If an error occurs, it
> should report back the error in a received message. It may
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC PATCH 1/4] IPMI: Add one interface to get more info of low-level IPMI device
2010-10-22 9:10 ` [RFC PATCH 1/4] IPMI: Add one interface to get more info of low-level IPMI device yakui.zhao
@ 2010-10-25 1:19 ` ykzhao
2010-10-25 3:25 ` Corey Minyard
1 sibling, 0 replies; 24+ messages in thread
From: ykzhao @ 2010-10-25 1:19 UTC (permalink / raw)
To: openipmi-developer; +Cc: linux-acpi, minyard, lenb
On Fri, 2010-10-22 at 17:10 +0800, Zhao, Yakui wrote:
> From: Zhao Yakui <yakui.zhao@intel.com>
>
> The IPMI smi_watcher will be used to catch the IPMI interface as they come or go.
> In order to communicate with the correct IPMI device, it should be confirmed
> whether it is what we wanted especially on the system with multiple IPMI
> devices. But the new_smi callback function of smi_watcher provides very
> limited info(only the interface number and dev pointer) and there is no
> detailed info about the low level interface. For example: which mechansim
> registers the IPMI interface(ACPI, PCI, DMI and so on).
>
> This is to add one interface that can get more info of low-level IPMI
> device. For example: the ACPI device handle will be returned for the pnp_acpi
> IPMI device.
Hi, Corey
Does this patch set make sense?
In this patch set the reference pointer is returned instead of
memory copy.
Thanks.
Yakui
>
> Signed-off-by: Zhao Yakui <yakui.zhao@intel.com>
> ---
> In this patch only the info of pnp_acpi IPMI is provided. If the detailed info
> is also required for other mechanism, please add them in the structure of
> ipmi_smi_info.
>
> drivers/char/ipmi/ipmi_msghandler.c | 26 ++++++++++++++++++++++++++
> drivers/char/ipmi/ipmi_si_intf.c | 28 ++++++++++++++++++++++++----
> include/linux/ipmi.h | 23 +++++++++++++++++++++++
> include/linux/ipmi_smi.h | 11 +++++++++++
> 4 files changed, 84 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
> index 4f3f8c9..6f4da59 100644
> --- a/drivers/char/ipmi/ipmi_msghandler.c
> +++ b/drivers/char/ipmi/ipmi_msghandler.c
> @@ -970,6 +970,32 @@ out_kfree:
> }
> EXPORT_SYMBOL(ipmi_create_user);
>
> +int ipmi_get_smi_info(int if_num, enum ipmi_addr_src type,
> + struct ipmi_smi_info **data)
> +{
> + int rv = 0;
> + ipmi_smi_t intf;
> + struct ipmi_smi_handlers *handlers;
> +
> + mutex_lock(&ipmi_interfaces_mutex);
> + list_for_each_entry_rcu(intf, &ipmi_interfaces, link) {
> + if (intf->intf_num == if_num)
> + goto found;
> + }
> + /* Not found, return an error */
> + rv = -EINVAL;
> + mutex_unlock(&ipmi_interfaces_mutex);
> + return rv;
> +
> +found:
> + handlers = intf->handlers;
> + rv = handlers->get_smi_info(intf->send_info, type, data);
> + mutex_unlock(&ipmi_interfaces_mutex);
> +
> + return rv;
> +}
> +EXPORT_SYMBOL(ipmi_get_smi_info);
> +
> static void free_user(struct kref *ref)
> {
> ipmi_user_t user = container_of(ref, struct ipmi_user, refcount);
> diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
> index 7bd7c45..73b1c82 100644
> --- a/drivers/char/ipmi/ipmi_si_intf.c
> +++ b/drivers/char/ipmi/ipmi_si_intf.c
> @@ -57,6 +57,7 @@
> #include <asm/irq.h>
> #include <linux/interrupt.h>
> #include <linux/rcupdate.h>
> +#include <linux/ipmi.h>
> #include <linux/ipmi_smi.h>
> #include <asm/io.h>
> #include "ipmi_si_sm.h"
> @@ -107,10 +108,6 @@ enum si_type {
> };
> static char *si_to_str[] = { "kcs", "smic", "bt" };
>
> -enum ipmi_addr_src {
> - SI_INVALID = 0, SI_HOTMOD, SI_HARDCODED, SI_SPMI, SI_ACPI, SI_SMBIOS,
> - SI_PCI, SI_DEVICETREE, SI_DEFAULT
> -};
> static char *ipmi_addr_src_to_str[] = { NULL, "hotmod", "hardcoded", "SPMI",
> "ACPI", "SMBIOS", "PCI",
> "device-tree", "default" };
> @@ -291,6 +288,7 @@ struct smi_info {
> struct task_struct *thread;
>
> struct list_head link;
> + struct ipmi_smi_info smi_data;
> };
>
> #define smi_inc_stat(smi, stat) \
> @@ -1186,6 +1184,22 @@ static int smi_start_processing(void *send_info,
> return 0;
> }
>
> +static int get_smi_info(void *send_info,
> + enum ipmi_addr_src type,
> + struct ipmi_smi_info **data)
> +{
> + struct smi_info *new_smi = send_info;
> + struct ipmi_smi_info *smi_data = &new_smi->smi_data;
> +
> + if (new_smi->addr_source != type)
> + return -EINVAL;
> +
> + smi_data->addr_src = type;
> +
> + *data = smi_data;
> + return 0;
> +}
> +
> static void set_maintenance_mode(void *send_info, int enable)
> {
> struct smi_info *smi_info = send_info;
> @@ -1197,6 +1211,7 @@ static void set_maintenance_mode(void *send_info, int enable)
> static struct ipmi_smi_handlers handlers = {
> .owner = THIS_MODULE,
> .start_processing = smi_start_processing,
> + .get_smi_info = get_smi_info,
> .sender = sender,
> .request_events = request_events,
> .set_maintenance_mode = set_maintenance_mode,
> @@ -2143,6 +2158,8 @@ static int __devinit ipmi_pnp_probe(struct pnp_dev *dev,
> return -ENOMEM;
>
> info->addr_source = SI_ACPI;
> + info->smi_data.smi_info.acpi_info.acpi_handle =
> + acpi_dev->handle;
> printk(KERN_INFO PFX "probing via ACPI\n");
>
> handle = acpi_dev->handle;
> @@ -3092,6 +3109,7 @@ static int try_smi_init(struct smi_info *new_smi)
> {
> int rv = 0;
> int i;
> + struct ipmi_smi_info *smi_data = &new_smi->smi_data;
>
> printk(KERN_INFO PFX "Trying %s-specified %s state"
> " machine at %s address 0x%lx, slave address 0x%x,"
> @@ -3217,6 +3235,8 @@ static int try_smi_init(struct smi_info *new_smi)
> new_smi->dev_registered = 1;
> }
>
> + smi_data->dev = new_smi->dev;
> +
> rv = ipmi_register_smi(&handlers,
> new_smi,
> &new_smi->device_id,
> diff --git a/include/linux/ipmi.h b/include/linux/ipmi.h
> index 65aae34..1ce428f 100644
> --- a/include/linux/ipmi.h
> +++ b/include/linux/ipmi.h
> @@ -454,6 +454,29 @@ unsigned int ipmi_addr_length(int addr_type);
> /* Validate that the given IPMI address is valid. */
> int ipmi_validate_addr(struct ipmi_addr *addr, int len);
>
> +enum ipmi_addr_src {
> + SI_INVALID = 0, SI_HOTMOD, SI_HARDCODED, SI_SPMI, SI_ACPI, SI_SMBIOS,
> + SI_PCI, SI_DEVICETREE, SI_DEFAULT
> +};
> +struct ipmi_smi_info{
> + enum ipmi_addr_src addr_src;
> + struct device *dev;
> + union {
> + /*
> + * Now only SI_ACPI info is provided. If the info is required
> + * for other type, please add it
> + */
> +#ifdef CONFIG_ACPI
> + struct {
> + void *acpi_handle;
> + } acpi_info;
> +#endif
> + } smi_info;
> +};
> +
> +/* This is to get the private info of ipmi_smi_t. The pointer is returned */
> +extern int ipmi_get_smi_info(int if_num, enum ipmi_addr_src type,
> + struct ipmi_smi_info **data);
> #endif /* __KERNEL__ */
>
>
> diff --git a/include/linux/ipmi_smi.h b/include/linux/ipmi_smi.h
> index 4b48318..bfa8f40 100644
> --- a/include/linux/ipmi_smi.h
> +++ b/include/linux/ipmi_smi.h
> @@ -86,6 +86,17 @@ struct ipmi_smi_handlers {
> int (*start_processing)(void *send_info,
> ipmi_smi_t new_intf);
>
> + /*
> + * Get the detailed private info of the low level interface and return
> + * the corresponding pointer.
> + * For example: the ACPI device handle will be returned for the
> + * pnp_acpi IPMI device. Of course it will firstly compare the
> + * interface type of low-level interface(Hardcoded type, DMI,
> + * ACPI and so on). If the type doesn't match, it will return -EINVAL.
> + */
> + int (*get_smi_info)(void *send_info, enum ipmi_addr_src type,
> + struct ipmi_smi_info **data);
> +
> /* Called to enqueue an SMI message to be sent. This
> operation is not allowed to fail. If an error occurs, it
> should report back the error in a received message. It may
^ permalink raw reply [flat|nested] 24+ messages in thread
* [RFC PATCH 1/4] IPMI: Add one interface to get more info of low-level IPMI device
2010-10-22 9:10 [RFC PATCH 0/4_v11] IPMI/ACPI: Install the ACPI IPMI opregion yakui.zhao
@ 2010-10-22 9:10 ` yakui.zhao
2010-10-25 1:19 ` ykzhao
2010-10-25 3:25 ` Corey Minyard
0 siblings, 2 replies; 24+ messages in thread
From: yakui.zhao @ 2010-10-22 9:10 UTC (permalink / raw)
To: openipmi-developer, linux-acpi; +Cc: minyard, lenb, Zhao Yakui
From: Zhao Yakui <yakui.zhao@intel.com>
The IPMI smi_watcher will be used to catch the IPMI interface as they come or go.
In order to communicate with the correct IPMI device, it should be confirmed
whether it is what we wanted especially on the system with multiple IPMI
devices. But the new_smi callback function of smi_watcher provides very
limited info(only the interface number and dev pointer) and there is no
detailed info about the low level interface. For example: which mechansim
registers the IPMI interface(ACPI, PCI, DMI and so on).
This is to add one interface that can get more info of low-level IPMI
device. For example: the ACPI device handle will be returned for the pnp_acpi
IPMI device.
Signed-off-by: Zhao Yakui <yakui.zhao@intel.com>
---
In this patch only the info of pnp_acpi IPMI is provided. If the detailed info
is also required for other mechanism, please add them in the structure of
ipmi_smi_info.
drivers/char/ipmi/ipmi_msghandler.c | 26 ++++++++++++++++++++++++++
drivers/char/ipmi/ipmi_si_intf.c | 28 ++++++++++++++++++++++++----
include/linux/ipmi.h | 23 +++++++++++++++++++++++
include/linux/ipmi_smi.h | 11 +++++++++++
4 files changed, 84 insertions(+), 4 deletions(-)
diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
index 4f3f8c9..6f4da59 100644
--- a/drivers/char/ipmi/ipmi_msghandler.c
+++ b/drivers/char/ipmi/ipmi_msghandler.c
@@ -970,6 +970,32 @@ out_kfree:
}
EXPORT_SYMBOL(ipmi_create_user);
+int ipmi_get_smi_info(int if_num, enum ipmi_addr_src type,
+ struct ipmi_smi_info **data)
+{
+ int rv = 0;
+ ipmi_smi_t intf;
+ struct ipmi_smi_handlers *handlers;
+
+ mutex_lock(&ipmi_interfaces_mutex);
+ list_for_each_entry_rcu(intf, &ipmi_interfaces, link) {
+ if (intf->intf_num == if_num)
+ goto found;
+ }
+ /* Not found, return an error */
+ rv = -EINVAL;
+ mutex_unlock(&ipmi_interfaces_mutex);
+ return rv;
+
+found:
+ handlers = intf->handlers;
+ rv = handlers->get_smi_info(intf->send_info, type, data);
+ mutex_unlock(&ipmi_interfaces_mutex);
+
+ return rv;
+}
+EXPORT_SYMBOL(ipmi_get_smi_info);
+
static void free_user(struct kref *ref)
{
ipmi_user_t user = container_of(ref, struct ipmi_user, refcount);
diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
index 7bd7c45..73b1c82 100644
--- a/drivers/char/ipmi/ipmi_si_intf.c
+++ b/drivers/char/ipmi/ipmi_si_intf.c
@@ -57,6 +57,7 @@
#include <asm/irq.h>
#include <linux/interrupt.h>
#include <linux/rcupdate.h>
+#include <linux/ipmi.h>
#include <linux/ipmi_smi.h>
#include <asm/io.h>
#include "ipmi_si_sm.h"
@@ -107,10 +108,6 @@ enum si_type {
};
static char *si_to_str[] = { "kcs", "smic", "bt" };
-enum ipmi_addr_src {
- SI_INVALID = 0, SI_HOTMOD, SI_HARDCODED, SI_SPMI, SI_ACPI, SI_SMBIOS,
- SI_PCI, SI_DEVICETREE, SI_DEFAULT
-};
static char *ipmi_addr_src_to_str[] = { NULL, "hotmod", "hardcoded", "SPMI",
"ACPI", "SMBIOS", "PCI",
"device-tree", "default" };
@@ -291,6 +288,7 @@ struct smi_info {
struct task_struct *thread;
struct list_head link;
+ struct ipmi_smi_info smi_data;
};
#define smi_inc_stat(smi, stat) \
@@ -1186,6 +1184,22 @@ static int smi_start_processing(void *send_info,
return 0;
}
+static int get_smi_info(void *send_info,
+ enum ipmi_addr_src type,
+ struct ipmi_smi_info **data)
+{
+ struct smi_info *new_smi = send_info;
+ struct ipmi_smi_info *smi_data = &new_smi->smi_data;
+
+ if (new_smi->addr_source != type)
+ return -EINVAL;
+
+ smi_data->addr_src = type;
+
+ *data = smi_data;
+ return 0;
+}
+
static void set_maintenance_mode(void *send_info, int enable)
{
struct smi_info *smi_info = send_info;
@@ -1197,6 +1211,7 @@ static void set_maintenance_mode(void *send_info, int enable)
static struct ipmi_smi_handlers handlers = {
.owner = THIS_MODULE,
.start_processing = smi_start_processing,
+ .get_smi_info = get_smi_info,
.sender = sender,
.request_events = request_events,
.set_maintenance_mode = set_maintenance_mode,
@@ -2143,6 +2158,8 @@ static int __devinit ipmi_pnp_probe(struct pnp_dev *dev,
return -ENOMEM;
info->addr_source = SI_ACPI;
+ info->smi_data.smi_info.acpi_info.acpi_handle =
+ acpi_dev->handle;
printk(KERN_INFO PFX "probing via ACPI\n");
handle = acpi_dev->handle;
@@ -3092,6 +3109,7 @@ static int try_smi_init(struct smi_info *new_smi)
{
int rv = 0;
int i;
+ struct ipmi_smi_info *smi_data = &new_smi->smi_data;
printk(KERN_INFO PFX "Trying %s-specified %s state"
" machine at %s address 0x%lx, slave address 0x%x,"
@@ -3217,6 +3235,8 @@ static int try_smi_init(struct smi_info *new_smi)
new_smi->dev_registered = 1;
}
+ smi_data->dev = new_smi->dev;
+
rv = ipmi_register_smi(&handlers,
new_smi,
&new_smi->device_id,
diff --git a/include/linux/ipmi.h b/include/linux/ipmi.h
index 65aae34..1ce428f 100644
--- a/include/linux/ipmi.h
+++ b/include/linux/ipmi.h
@@ -454,6 +454,29 @@ unsigned int ipmi_addr_length(int addr_type);
/* Validate that the given IPMI address is valid. */
int ipmi_validate_addr(struct ipmi_addr *addr, int len);
+enum ipmi_addr_src {
+ SI_INVALID = 0, SI_HOTMOD, SI_HARDCODED, SI_SPMI, SI_ACPI, SI_SMBIOS,
+ SI_PCI, SI_DEVICETREE, SI_DEFAULT
+};
+struct ipmi_smi_info{
+ enum ipmi_addr_src addr_src;
+ struct device *dev;
+ union {
+ /*
+ * Now only SI_ACPI info is provided. If the info is required
+ * for other type, please add it
+ */
+#ifdef CONFIG_ACPI
+ struct {
+ void *acpi_handle;
+ } acpi_info;
+#endif
+ } smi_info;
+};
+
+/* This is to get the private info of ipmi_smi_t. The pointer is returned */
+extern int ipmi_get_smi_info(int if_num, enum ipmi_addr_src type,
+ struct ipmi_smi_info **data);
#endif /* __KERNEL__ */
diff --git a/include/linux/ipmi_smi.h b/include/linux/ipmi_smi.h
index 4b48318..bfa8f40 100644
--- a/include/linux/ipmi_smi.h
+++ b/include/linux/ipmi_smi.h
@@ -86,6 +86,17 @@ struct ipmi_smi_handlers {
int (*start_processing)(void *send_info,
ipmi_smi_t new_intf);
+ /*
+ * Get the detailed private info of the low level interface and return
+ * the corresponding pointer.
+ * For example: the ACPI device handle will be returned for the
+ * pnp_acpi IPMI device. Of course it will firstly compare the
+ * interface type of low-level interface(Hardcoded type, DMI,
+ * ACPI and so on). If the type doesn't match, it will return -EINVAL.
+ */
+ int (*get_smi_info)(void *send_info, enum ipmi_addr_src type,
+ struct ipmi_smi_info **data);
+
/* Called to enqueue an SMI message to be sent. This
operation is not allowed to fail. If an error occurs, it
should report back the error in a received message. It may
--
1.5.4.5
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [RFC PATCH 1/4] IPMI: Add one interface to get more info of low-level IPMI device
2010-10-15 3:03 ` Corey Minyard
@ 2010-10-15 5:07 ` ykzhao
0 siblings, 0 replies; 24+ messages in thread
From: ykzhao @ 2010-10-15 5:07 UTC (permalink / raw)
To: Corey Minyard; +Cc: openipmi-developer, linux-acpi, lenb
On Fri, 2010-10-15 at 11:03 +0800, Corey Minyard wrote:
> On 10/14/2010 08:07 PM, ykzhao wrote:
> >
> >> The way you are doing it, there is no need for a refcount, since you are
> >> making a copy of the data.
> >>
> >> Is a copy or a pointer better? A pointer is generally preferred, it
> >> keeps from having to either store data on the stack or dynamically
> >> allocate it for the copy. But it's not a huge deal in this case. A
> >> pointer will require you to dynamically allocate the smi_info structure
> >> so you can free it separately. But then only the top-level put routine
> >> is required, it can simply free the structure if the refcount is zero.
> >>
> > When the pointer mechanism is used, we will have to allocate the
> > smi_info structure dynamically. Every time the function of
> > ipmi_get_smi_info, it will be allocated dynamically. And if it fails in
> > the allocation, we can't return the expected value.
> >
> Well, you misunderstand. You allocate one copy when the SMI info is
> created. And you return a pointer to that with the refcount
> incremented. No need to allocate a new one on each call. Use the
> refcounts to know when to free it.
The ipmi_smi_info is defined statically in the structure of smi_info.
struct smi_info {
......
struct ipmi_smi_info smi_data;
};
If the pointer mechanism is selected, we can return the pointer of
smi_data. And in such case it seems unnecessary to know when to free it.
>
> > But when the copy is used, it will be much simpler. It is the caller's
> > responsibility to prepare the corresponding data structure. They can
> > define it on stack. Of course they can also dynamically allocate it.
> >
> > Can we choose the copy mechanism to make it much simpler?
> >
> Sure, I think I already said this :). Just get rid of the refcount stuff.
OK. I will remove the refcount stuff.
> -corey
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC PATCH 1/4] IPMI: Add one interface to get more info of low-level IPMI device
2010-10-15 1:07 ` ykzhao
@ 2010-10-15 3:03 ` Corey Minyard
2010-10-15 5:07 ` ykzhao
0 siblings, 1 reply; 24+ messages in thread
From: Corey Minyard @ 2010-10-15 3:03 UTC (permalink / raw)
To: ykzhao; +Cc: openipmi-developer, linux-acpi, lenb
On 10/14/2010 08:07 PM, ykzhao wrote:
>
>> The way you are doing it, there is no need for a refcount, since you are
>> making a copy of the data.
>>
>> Is a copy or a pointer better? A pointer is generally preferred, it
>> keeps from having to either store data on the stack or dynamically
>> allocate it for the copy. But it's not a huge deal in this case. A
>> pointer will require you to dynamically allocate the smi_info structure
>> so you can free it separately. But then only the top-level put routine
>> is required, it can simply free the structure if the refcount is zero.
>>
> When the pointer mechanism is used, we will have to allocate the
> smi_info structure dynamically. Every time the function of
> ipmi_get_smi_info, it will be allocated dynamically. And if it fails in
> the allocation, we can't return the expected value.
>
Well, you misunderstand. You allocate one copy when the SMI info is
created. And you return a pointer to that with the refcount
incremented. No need to allocate a new one on each call. Use the
refcounts to know when to free it.
> But when the copy is used, it will be much simpler. It is the caller's
> responsibility to prepare the corresponding data structure. They can
> define it on stack. Of course they can also dynamically allocate it.
>
> Can we choose the copy mechanism to make it much simpler?
>
Sure, I think I already said this :). Just get rid of the refcount stuff.
-corey
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC PATCH 1/4] IPMI: Add one interface to get more info of low-level IPMI device
2010-10-14 16:53 ` Corey Minyard
@ 2010-10-15 1:07 ` ykzhao
2010-10-15 3:03 ` Corey Minyard
0 siblings, 1 reply; 24+ messages in thread
From: ykzhao @ 2010-10-15 1:07 UTC (permalink / raw)
To: Corey Minyard; +Cc: openipmi-developer, linux-acpi, lenb
On Fri, 2010-10-15 at 00:53 +0800, Corey Minyard wrote:
> These are better, I'm ok with the other patches (well, I didn't review
> the ACPI changes, but I know little about that).
>
> You do have the refcounting stuff wrong, though. You only need a
> refcount if you receive a pointer to the information. For instance, you
> would have:
>
> int ipmi_get_smi_info(int if_num, enum ipmi_addr_src type,
> struct ipmi_smi_info **data);
>
> and
>
> void ipmi_put_smi_info(struct ipmi_smi_info *data);
Thanks for the comments.
The updated structure of ipmi_smi_info will contain the dev pointer. In
order to assure that the dev pointer is safely accessed, the refcount of
dev will be increased/decreased. At the same time to avoid that the
caller increases/decrease the refcount explicitly, the wrapper is
added.
If the refcount of dev pointer is not considered in the caller, IMO the
refcount can be ignored.
>
> The way you are doing it, there is no need for a refcount, since you are
> making a copy of the data.
>
> Is a copy or a pointer better? A pointer is generally preferred, it
> keeps from having to either store data on the stack or dynamically
> allocate it for the copy. But it's not a huge deal in this case. A
> pointer will require you to dynamically allocate the smi_info structure
> so you can free it separately. But then only the top-level put routine
> is required, it can simply free the structure if the refcount is zero.
When the pointer mechanism is used, we will have to allocate the
smi_info structure dynamically. Every time the function of
ipmi_get_smi_info, it will be allocated dynamically. And if it fails in
the allocation, we can't return the expected value.
But when the copy is used, it will be much simpler. It is the caller's
responsibility to prepare the corresponding data structure. They can
define it on stack. Of course they can also dynamically allocate it.
Can we choose the copy mechanism to make it much simpler?
Thanks.
>
> -corey
>
> On 10/12/2010 02:47 AM, yakui.zhao@intel.com wrote:
> > From: Zhao Yakui<yakui.zhao@intel.com>
> >
> > The IPMI smi_watcher will be used to catch the IPMI interface as they come or go.
> > In order to communicate with the correct IPMI device, it should be confirmed
> > whether it is what we wanted especially on the system with multiple IPMI
> > devices. But the new_smi callback function of smi_watcher provides very
> > limited info(only the interface number and dev pointer) and there is no
> > detailed info about the low level interface. For example: which mechansim
> > registers the IPMI interface(ACPI, PCI, DMI and so on).
> >
> > This is to add one interface that can get more info of low-level IPMI
> > device. For example: the ACPI device handle will be returned for the pnp_acpi
> > IPMI device.
> >
> > Signed-off-by: Zhao Yakui<yakui.zhao@intel.com>
> > ---
> > In this patch only the info of pnp_acpi IPMI is provided. If the detailed info
> > is also required for other mechanism, please add them in the structure of
> > ipmi_smi_info.
> >
> > drivers/char/ipmi/ipmi_msghandler.c | 47 +++++++++++++++++++++++++++++++++++
> > drivers/char/ipmi/ipmi_si_intf.c | 35 +++++++++++++++++++++++---
> > include/linux/ipmi.h | 25 ++++++++++++++++++
> > include/linux/ipmi_smi.h | 12 +++++++++
> > 4 files changed, 115 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
> > index 4f3f8c9..29e373d 100644
> > --- a/drivers/char/ipmi/ipmi_msghandler.c
> > +++ b/drivers/char/ipmi/ipmi_msghandler.c
> > @@ -970,6 +970,53 @@ out_kfree:
> > }
> > EXPORT_SYMBOL(ipmi_create_user);
> >
> > +int ipmi_get_smi_info(int if_num, enum ipmi_addr_src type,
> > + struct ipmi_smi_info *data)
> > +{
> > + int rv = 0;
> > + ipmi_smi_t intf;
> > + struct ipmi_smi_handlers *handlers;
> > +
> > + mutex_lock(&ipmi_interfaces_mutex);
> > + list_for_each_entry_rcu(intf,&ipmi_interfaces, link) {
> > + if (intf->intf_num == if_num)
> > + goto found;
> > + }
> > + /* Not found, return an error */
> > + rv = -EINVAL;
> > + mutex_unlock(&ipmi_interfaces_mutex);
> > + return rv;
> > +
> > +found:
> > + handlers = intf->handlers;
> > + rv = handlers->get_smi_info(intf->send_info, type, data);
> > + mutex_unlock(&ipmi_interfaces_mutex);
> > +
> > + return rv;
> > +}
> > +EXPORT_SYMBOL(ipmi_get_smi_info);
> > +
> > +void ipmi_put_smi_info(int if_num)
> > +{
> > + ipmi_smi_t intf;
> > + struct ipmi_smi_handlers *handlers;
> > +
> > + mutex_lock(&ipmi_interfaces_mutex);
> > + list_for_each_entry_rcu(intf,&ipmi_interfaces, link) {
> > + if (intf->intf_num == if_num)
> > + goto found;
> > + }
> > + /* Not found, return */
> > + mutex_unlock(&ipmi_interfaces_mutex);
> > + return;
> > +
> > +found:
> > + handlers = intf->handlers;
> > + handlers->put_smi_info(intf->send_info);
> > + mutex_unlock(&ipmi_interfaces_mutex);
> > +}
> > +EXPORT_SYMBOL(ipmi_put_smi_info);
> > +
> > static void free_user(struct kref *ref)
> > {
> > ipmi_user_t user = container_of(ref, struct ipmi_user, refcount);
> > diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
> > index 7bd7c45..4885709 100644
> > --- a/drivers/char/ipmi/ipmi_si_intf.c
> > +++ b/drivers/char/ipmi/ipmi_si_intf.c
> > @@ -57,6 +57,7 @@
> > #include<asm/irq.h>
> > #include<linux/interrupt.h>
> > #include<linux/rcupdate.h>
> > +#include<linux/ipmi.h>
> > #include<linux/ipmi_smi.h>
> > #include<asm/io.h>
> > #include "ipmi_si_sm.h"
> > @@ -107,10 +108,6 @@ enum si_type {
> > };
> > static char *si_to_str[] = { "kcs", "smic", "bt" };
> >
> > -enum ipmi_addr_src {
> > - SI_INVALID = 0, SI_HOTMOD, SI_HARDCODED, SI_SPMI, SI_ACPI, SI_SMBIOS,
> > - SI_PCI, SI_DEVICETREE, SI_DEFAULT
> > -};
> > static char *ipmi_addr_src_to_str[] = { NULL, "hotmod", "hardcoded", "SPMI",
> > "ACPI", "SMBIOS", "PCI",
> > "device-tree", "default" };
> > @@ -291,6 +288,7 @@ struct smi_info {
> > struct task_struct *thread;
> >
> > struct list_head link;
> > + struct ipmi_smi_info smi_data;
> > };
> >
> > #define smi_inc_stat(smi, stat) \
> > @@ -1186,6 +1184,31 @@ static int smi_start_processing(void *send_info,
> > return 0;
> > }
> >
> > +static int get_smi_info(void *send_info,
> > + enum ipmi_addr_src type,
> > + struct ipmi_smi_info *data)
> > +{
> > + struct smi_info *new_smi = send_info;
> > + struct ipmi_smi_info *smi_data =&new_smi->smi_data;
> > +
> > + if (new_smi->addr_source != type)
> > + return -EINVAL;
> > +
> > + get_device(new_smi->dev);
> > + memcpy(data, smi_data, sizeof(*smi_data));
> > + smi_data->addr_src = type;
> > + smi_data->dev = new_smi->dev;
> > +
> > + return 0;
> > +}
> > +
> > +static void put_smi_info(void *send_info)
> > +{
> > + struct smi_info *new_smi = send_info;
> > +
> > + put_device(new_smi->dev);
> > +}
> > +
> > static void set_maintenance_mode(void *send_info, int enable)
> > {
> > struct smi_info *smi_info = send_info;
> > @@ -1197,6 +1220,8 @@ static void set_maintenance_mode(void *send_info, int enable)
> > static struct ipmi_smi_handlers handlers = {
> > .owner = THIS_MODULE,
> > .start_processing = smi_start_processing,
> > + .get_smi_info = get_smi_info,
> > + .put_smi_info = put_smi_info,
> > .sender = sender,
> > .request_events = request_events,
> > .set_maintenance_mode = set_maintenance_mode,
> > @@ -2143,6 +2168,8 @@ static int __devinit ipmi_pnp_probe(struct pnp_dev *dev,
> > return -ENOMEM;
> >
> > info->addr_source = SI_ACPI;
> > + info->smi_data.smi_info.acpi_info.acpi_handle =
> > + acpi_dev->handle;
> > printk(KERN_INFO PFX "probing via ACPI\n");
> >
> > handle = acpi_dev->handle;
> > diff --git a/include/linux/ipmi.h b/include/linux/ipmi.h
> > index 65aae34..2d30551 100644
> > --- a/include/linux/ipmi.h
> > +++ b/include/linux/ipmi.h
> > @@ -454,6 +454,31 @@ unsigned int ipmi_addr_length(int addr_type);
> > /* Validate that the given IPMI address is valid. */
> > int ipmi_validate_addr(struct ipmi_addr *addr, int len);
> >
> > +enum ipmi_addr_src {
> > + SI_INVALID = 0, SI_HOTMOD, SI_HARDCODED, SI_SPMI, SI_ACPI, SI_SMBIOS,
> > + SI_PCI, SI_DEVICETREE, SI_DEFAULT
> > +};
> > +struct ipmi_smi_info{
> > + enum ipmi_addr_src addr_src;
> > + struct device *dev;
> > + union {
> > + /*
> > + * Now only SI_ACPI info is provided. If the info is required
> > + * for other type, please add it
> > + */
> > +#ifdef CONFIG_ACPI
> > + struct {
> > + void *acpi_handle;
> > + } acpi_info;
> > +#endif
> > + } smi_info;
> > +};
> > +
> > +/* This is to get the private info of ipmi_smi_t */
> > +extern int ipmi_get_smi_info(int if_num, enum ipmi_addr_src type,
> > + struct ipmi_smi_info *data);
> > +/* This is to decrease refcount added in the function of ipmi_get_smi_info */
> > +extern void ipmi_put_smi_info(int if_num);
> > #endif /* __KERNEL__ */
> >
> >
> > diff --git a/include/linux/ipmi_smi.h b/include/linux/ipmi_smi.h
> > index 4b48318..b9a53c0 100644
> > --- a/include/linux/ipmi_smi.h
> > +++ b/include/linux/ipmi_smi.h
> > @@ -86,6 +86,18 @@ struct ipmi_smi_handlers {
> > int (*start_processing)(void *send_info,
> > ipmi_smi_t new_intf);
> >
> > + /*
> > + * Get the detailed private info of the low level interface and store
> > + * it into the structure of ipmi_smi_data. For example: the
> > + * ACPI device handle will be returned for the pnp_acpi IPMI device.
> > + * Of course it will firstly compare the interface type of low-level
> > + * interface(Hardcoded type, DMI, ACPI and so on). If the type doesn't
> > + * match, it will return -EINVAL.
> > + */
> > + int (*get_smi_info)(void *send_info, enum ipmi_addr_src type,
> > + struct ipmi_smi_info *data);
> > +
> > + void (*put_smi_info)(void *send_info);
> > /* Called to enqueue an SMI message to be sent. This
> > operation is not allowed to fail. If an error occurs, it
> > should report back the error in a received message. It may
> >
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC PATCH 1/4] IPMI: Add one interface to get more info of low-level IPMI device
2010-10-12 7:47 ` [RFC PATCH 1/4] IPMI: Add one interface to get more info of low-level IPMI device yakui.zhao
2010-10-12 7:51 ` ykzhao
@ 2010-10-14 16:53 ` Corey Minyard
2010-10-15 1:07 ` ykzhao
1 sibling, 1 reply; 24+ messages in thread
From: Corey Minyard @ 2010-10-14 16:53 UTC (permalink / raw)
To: yakui.zhao; +Cc: linux-acpi, openipmi-developer, lenb
These are better, I'm ok with the other patches (well, I didn't review
the ACPI changes, but I know little about that).
You do have the refcounting stuff wrong, though. You only need a
refcount if you receive a pointer to the information. For instance, you
would have:
int ipmi_get_smi_info(int if_num, enum ipmi_addr_src type,
struct ipmi_smi_info **data);
and
void ipmi_put_smi_info(struct ipmi_smi_info *data);
The way you are doing it, there is no need for a refcount, since you are
making a copy of the data.
Is a copy or a pointer better? A pointer is generally preferred, it
keeps from having to either store data on the stack or dynamically
allocate it for the copy. But it's not a huge deal in this case. A
pointer will require you to dynamically allocate the smi_info structure
so you can free it separately. But then only the top-level put routine
is required, it can simply free the structure if the refcount is zero.
-corey
On 10/12/2010 02:47 AM, yakui.zhao@intel.com wrote:
> From: Zhao Yakui<yakui.zhao@intel.com>
>
> The IPMI smi_watcher will be used to catch the IPMI interface as they come or go.
> In order to communicate with the correct IPMI device, it should be confirmed
> whether it is what we wanted especially on the system with multiple IPMI
> devices. But the new_smi callback function of smi_watcher provides very
> limited info(only the interface number and dev pointer) and there is no
> detailed info about the low level interface. For example: which mechansim
> registers the IPMI interface(ACPI, PCI, DMI and so on).
>
> This is to add one interface that can get more info of low-level IPMI
> device. For example: the ACPI device handle will be returned for the pnp_acpi
> IPMI device.
>
> Signed-off-by: Zhao Yakui<yakui.zhao@intel.com>
> ---
> In this patch only the info of pnp_acpi IPMI is provided. If the detailed info
> is also required for other mechanism, please add them in the structure of
> ipmi_smi_info.
>
> drivers/char/ipmi/ipmi_msghandler.c | 47 +++++++++++++++++++++++++++++++++++
> drivers/char/ipmi/ipmi_si_intf.c | 35 +++++++++++++++++++++++---
> include/linux/ipmi.h | 25 ++++++++++++++++++
> include/linux/ipmi_smi.h | 12 +++++++++
> 4 files changed, 115 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
> index 4f3f8c9..29e373d 100644
> --- a/drivers/char/ipmi/ipmi_msghandler.c
> +++ b/drivers/char/ipmi/ipmi_msghandler.c
> @@ -970,6 +970,53 @@ out_kfree:
> }
> EXPORT_SYMBOL(ipmi_create_user);
>
> +int ipmi_get_smi_info(int if_num, enum ipmi_addr_src type,
> + struct ipmi_smi_info *data)
> +{
> + int rv = 0;
> + ipmi_smi_t intf;
> + struct ipmi_smi_handlers *handlers;
> +
> + mutex_lock(&ipmi_interfaces_mutex);
> + list_for_each_entry_rcu(intf,&ipmi_interfaces, link) {
> + if (intf->intf_num == if_num)
> + goto found;
> + }
> + /* Not found, return an error */
> + rv = -EINVAL;
> + mutex_unlock(&ipmi_interfaces_mutex);
> + return rv;
> +
> +found:
> + handlers = intf->handlers;
> + rv = handlers->get_smi_info(intf->send_info, type, data);
> + mutex_unlock(&ipmi_interfaces_mutex);
> +
> + return rv;
> +}
> +EXPORT_SYMBOL(ipmi_get_smi_info);
> +
> +void ipmi_put_smi_info(int if_num)
> +{
> + ipmi_smi_t intf;
> + struct ipmi_smi_handlers *handlers;
> +
> + mutex_lock(&ipmi_interfaces_mutex);
> + list_for_each_entry_rcu(intf,&ipmi_interfaces, link) {
> + if (intf->intf_num == if_num)
> + goto found;
> + }
> + /* Not found, return */
> + mutex_unlock(&ipmi_interfaces_mutex);
> + return;
> +
> +found:
> + handlers = intf->handlers;
> + handlers->put_smi_info(intf->send_info);
> + mutex_unlock(&ipmi_interfaces_mutex);
> +}
> +EXPORT_SYMBOL(ipmi_put_smi_info);
> +
> static void free_user(struct kref *ref)
> {
> ipmi_user_t user = container_of(ref, struct ipmi_user, refcount);
> diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
> index 7bd7c45..4885709 100644
> --- a/drivers/char/ipmi/ipmi_si_intf.c
> +++ b/drivers/char/ipmi/ipmi_si_intf.c
> @@ -57,6 +57,7 @@
> #include<asm/irq.h>
> #include<linux/interrupt.h>
> #include<linux/rcupdate.h>
> +#include<linux/ipmi.h>
> #include<linux/ipmi_smi.h>
> #include<asm/io.h>
> #include "ipmi_si_sm.h"
> @@ -107,10 +108,6 @@ enum si_type {
> };
> static char *si_to_str[] = { "kcs", "smic", "bt" };
>
> -enum ipmi_addr_src {
> - SI_INVALID = 0, SI_HOTMOD, SI_HARDCODED, SI_SPMI, SI_ACPI, SI_SMBIOS,
> - SI_PCI, SI_DEVICETREE, SI_DEFAULT
> -};
> static char *ipmi_addr_src_to_str[] = { NULL, "hotmod", "hardcoded", "SPMI",
> "ACPI", "SMBIOS", "PCI",
> "device-tree", "default" };
> @@ -291,6 +288,7 @@ struct smi_info {
> struct task_struct *thread;
>
> struct list_head link;
> + struct ipmi_smi_info smi_data;
> };
>
> #define smi_inc_stat(smi, stat) \
> @@ -1186,6 +1184,31 @@ static int smi_start_processing(void *send_info,
> return 0;
> }
>
> +static int get_smi_info(void *send_info,
> + enum ipmi_addr_src type,
> + struct ipmi_smi_info *data)
> +{
> + struct smi_info *new_smi = send_info;
> + struct ipmi_smi_info *smi_data =&new_smi->smi_data;
> +
> + if (new_smi->addr_source != type)
> + return -EINVAL;
> +
> + get_device(new_smi->dev);
> + memcpy(data, smi_data, sizeof(*smi_data));
> + smi_data->addr_src = type;
> + smi_data->dev = new_smi->dev;
> +
> + return 0;
> +}
> +
> +static void put_smi_info(void *send_info)
> +{
> + struct smi_info *new_smi = send_info;
> +
> + put_device(new_smi->dev);
> +}
> +
> static void set_maintenance_mode(void *send_info, int enable)
> {
> struct smi_info *smi_info = send_info;
> @@ -1197,6 +1220,8 @@ static void set_maintenance_mode(void *send_info, int enable)
> static struct ipmi_smi_handlers handlers = {
> .owner = THIS_MODULE,
> .start_processing = smi_start_processing,
> + .get_smi_info = get_smi_info,
> + .put_smi_info = put_smi_info,
> .sender = sender,
> .request_events = request_events,
> .set_maintenance_mode = set_maintenance_mode,
> @@ -2143,6 +2168,8 @@ static int __devinit ipmi_pnp_probe(struct pnp_dev *dev,
> return -ENOMEM;
>
> info->addr_source = SI_ACPI;
> + info->smi_data.smi_info.acpi_info.acpi_handle =
> + acpi_dev->handle;
> printk(KERN_INFO PFX "probing via ACPI\n");
>
> handle = acpi_dev->handle;
> diff --git a/include/linux/ipmi.h b/include/linux/ipmi.h
> index 65aae34..2d30551 100644
> --- a/include/linux/ipmi.h
> +++ b/include/linux/ipmi.h
> @@ -454,6 +454,31 @@ unsigned int ipmi_addr_length(int addr_type);
> /* Validate that the given IPMI address is valid. */
> int ipmi_validate_addr(struct ipmi_addr *addr, int len);
>
> +enum ipmi_addr_src {
> + SI_INVALID = 0, SI_HOTMOD, SI_HARDCODED, SI_SPMI, SI_ACPI, SI_SMBIOS,
> + SI_PCI, SI_DEVICETREE, SI_DEFAULT
> +};
> +struct ipmi_smi_info{
> + enum ipmi_addr_src addr_src;
> + struct device *dev;
> + union {
> + /*
> + * Now only SI_ACPI info is provided. If the info is required
> + * for other type, please add it
> + */
> +#ifdef CONFIG_ACPI
> + struct {
> + void *acpi_handle;
> + } acpi_info;
> +#endif
> + } smi_info;
> +};
> +
> +/* This is to get the private info of ipmi_smi_t */
> +extern int ipmi_get_smi_info(int if_num, enum ipmi_addr_src type,
> + struct ipmi_smi_info *data);
> +/* This is to decrease refcount added in the function of ipmi_get_smi_info */
> +extern void ipmi_put_smi_info(int if_num);
> #endif /* __KERNEL__ */
>
>
> diff --git a/include/linux/ipmi_smi.h b/include/linux/ipmi_smi.h
> index 4b48318..b9a53c0 100644
> --- a/include/linux/ipmi_smi.h
> +++ b/include/linux/ipmi_smi.h
> @@ -86,6 +86,18 @@ struct ipmi_smi_handlers {
> int (*start_processing)(void *send_info,
> ipmi_smi_t new_intf);
>
> + /*
> + * Get the detailed private info of the low level interface and store
> + * it into the structure of ipmi_smi_data. For example: the
> + * ACPI device handle will be returned for the pnp_acpi IPMI device.
> + * Of course it will firstly compare the interface type of low-level
> + * interface(Hardcoded type, DMI, ACPI and so on). If the type doesn't
> + * match, it will return -EINVAL.
> + */
> + int (*get_smi_info)(void *send_info, enum ipmi_addr_src type,
> + struct ipmi_smi_info *data);
> +
> + void (*put_smi_info)(void *send_info);
> /* Called to enqueue an SMI message to be sent. This
> operation is not allowed to fail. If an error occurs, it
> should report back the error in a received message. It may
>
------------------------------------------------------------------------------
Beautiful is writing same markup. Internet Explorer 9 supports
standards for HTML5, CSS3, SVG 1.1, ECMAScript5, and DOM L2 & L3.
Spend less time writing and rewriting code and more time creating great
experiences on the web. Be a part of the beta today.
http://p.sf.net/sfu/beautyoftheweb
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC PATCH 1/4] IPMI: Add one interface to get more info of low-level IPMI device
2010-10-12 7:47 ` [RFC PATCH 1/4] IPMI: Add one interface to get more info of low-level IPMI device yakui.zhao
@ 2010-10-12 7:51 ` ykzhao
2010-10-14 16:53 ` Corey Minyard
1 sibling, 0 replies; 24+ messages in thread
From: ykzhao @ 2010-10-12 7:51 UTC (permalink / raw)
To: openipmi-developer; +Cc: linux-acpi, minyard, lenb
On Tue, 2010-10-12 at 15:47 +0800, Zhao, Yakui wrote:
> From: Zhao Yakui <yakui.zhao@intel.com>
>
> The IPMI smi_watcher will be used to catch the IPMI interface as they come or go.
> In order to communicate with the correct IPMI device, it should be confirmed
> whether it is what we wanted especially on the system with multiple IPMI
> devices. But the new_smi callback function of smi_watcher provides very
> limited info(only the interface number and dev pointer) and there is no
> detailed info about the low level interface. For example: which mechansim
> registers the IPMI interface(ACPI, PCI, DMI and so on).
>
> This is to add one interface that can get more info of low-level IPMI
> device. For example: the ACPI device handle will be returned for the pnp_acpi
> IPMI device.
Hi, Corey
How about the updated patch? Does it make sense to you?
thanks.
Yakui
>
> Signed-off-by: Zhao Yakui <yakui.zhao@intel.com>
> ---
> In this patch only the info of pnp_acpi IPMI is provided. If the detailed info
> is also required for other mechanism, please add them in the structure of
> ipmi_smi_info.
>
> drivers/char/ipmi/ipmi_msghandler.c | 47 +++++++++++++++++++++++++++++++++++
> drivers/char/ipmi/ipmi_si_intf.c | 35 +++++++++++++++++++++++---
> include/linux/ipmi.h | 25 ++++++++++++++++++
> include/linux/ipmi_smi.h | 12 +++++++++
> 4 files changed, 115 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
> index 4f3f8c9..29e373d 100644
> --- a/drivers/char/ipmi/ipmi_msghandler.c
> +++ b/drivers/char/ipmi/ipmi_msghandler.c
> @@ -970,6 +970,53 @@ out_kfree:
> }
> EXPORT_SYMBOL(ipmi_create_user);
>
> +int ipmi_get_smi_info(int if_num, enum ipmi_addr_src type,
> + struct ipmi_smi_info *data)
> +{
> + int rv = 0;
> + ipmi_smi_t intf;
> + struct ipmi_smi_handlers *handlers;
> +
> + mutex_lock(&ipmi_interfaces_mutex);
> + list_for_each_entry_rcu(intf, &ipmi_interfaces, link) {
> + if (intf->intf_num == if_num)
> + goto found;
> + }
> + /* Not found, return an error */
> + rv = -EINVAL;
> + mutex_unlock(&ipmi_interfaces_mutex);
> + return rv;
> +
> +found:
> + handlers = intf->handlers;
> + rv = handlers->get_smi_info(intf->send_info, type, data);
> + mutex_unlock(&ipmi_interfaces_mutex);
> +
> + return rv;
> +}
> +EXPORT_SYMBOL(ipmi_get_smi_info);
> +
> +void ipmi_put_smi_info(int if_num)
> +{
> + ipmi_smi_t intf;
> + struct ipmi_smi_handlers *handlers;
> +
> + mutex_lock(&ipmi_interfaces_mutex);
> + list_for_each_entry_rcu(intf, &ipmi_interfaces, link) {
> + if (intf->intf_num == if_num)
> + goto found;
> + }
> + /* Not found, return */
> + mutex_unlock(&ipmi_interfaces_mutex);
> + return;
> +
> +found:
> + handlers = intf->handlers;
> + handlers->put_smi_info(intf->send_info);
> + mutex_unlock(&ipmi_interfaces_mutex);
> +}
> +EXPORT_SYMBOL(ipmi_put_smi_info);
> +
> static void free_user(struct kref *ref)
> {
> ipmi_user_t user = container_of(ref, struct ipmi_user, refcount);
> diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
> index 7bd7c45..4885709 100644
> --- a/drivers/char/ipmi/ipmi_si_intf.c
> +++ b/drivers/char/ipmi/ipmi_si_intf.c
> @@ -57,6 +57,7 @@
> #include <asm/irq.h>
> #include <linux/interrupt.h>
> #include <linux/rcupdate.h>
> +#include <linux/ipmi.h>
> #include <linux/ipmi_smi.h>
> #include <asm/io.h>
> #include "ipmi_si_sm.h"
> @@ -107,10 +108,6 @@ enum si_type {
> };
> static char *si_to_str[] = { "kcs", "smic", "bt" };
>
> -enum ipmi_addr_src {
> - SI_INVALID = 0, SI_HOTMOD, SI_HARDCODED, SI_SPMI, SI_ACPI, SI_SMBIOS,
> - SI_PCI, SI_DEVICETREE, SI_DEFAULT
> -};
> static char *ipmi_addr_src_to_str[] = { NULL, "hotmod", "hardcoded", "SPMI",
> "ACPI", "SMBIOS", "PCI",
> "device-tree", "default" };
> @@ -291,6 +288,7 @@ struct smi_info {
> struct task_struct *thread;
>
> struct list_head link;
> + struct ipmi_smi_info smi_data;
> };
>
> #define smi_inc_stat(smi, stat) \
> @@ -1186,6 +1184,31 @@ static int smi_start_processing(void *send_info,
> return 0;
> }
>
> +static int get_smi_info(void *send_info,
> + enum ipmi_addr_src type,
> + struct ipmi_smi_info *data)
> +{
> + struct smi_info *new_smi = send_info;
> + struct ipmi_smi_info *smi_data = &new_smi->smi_data;
> +
> + if (new_smi->addr_source != type)
> + return -EINVAL;
> +
> + get_device(new_smi->dev);
> + memcpy(data, smi_data, sizeof(*smi_data));
> + smi_data->addr_src = type;
> + smi_data->dev = new_smi->dev;
> +
> + return 0;
> +}
> +
> +static void put_smi_info(void *send_info)
> +{
> + struct smi_info *new_smi = send_info;
> +
> + put_device(new_smi->dev);
> +}
> +
> static void set_maintenance_mode(void *send_info, int enable)
> {
> struct smi_info *smi_info = send_info;
> @@ -1197,6 +1220,8 @@ static void set_maintenance_mode(void *send_info, int enable)
> static struct ipmi_smi_handlers handlers = {
> .owner = THIS_MODULE,
> .start_processing = smi_start_processing,
> + .get_smi_info = get_smi_info,
> + .put_smi_info = put_smi_info,
> .sender = sender,
> .request_events = request_events,
> .set_maintenance_mode = set_maintenance_mode,
> @@ -2143,6 +2168,8 @@ static int __devinit ipmi_pnp_probe(struct pnp_dev *dev,
> return -ENOMEM;
>
> info->addr_source = SI_ACPI;
> + info->smi_data.smi_info.acpi_info.acpi_handle =
> + acpi_dev->handle;
> printk(KERN_INFO PFX "probing via ACPI\n");
>
> handle = acpi_dev->handle;
> diff --git a/include/linux/ipmi.h b/include/linux/ipmi.h
> index 65aae34..2d30551 100644
> --- a/include/linux/ipmi.h
> +++ b/include/linux/ipmi.h
> @@ -454,6 +454,31 @@ unsigned int ipmi_addr_length(int addr_type);
> /* Validate that the given IPMI address is valid. */
> int ipmi_validate_addr(struct ipmi_addr *addr, int len);
>
> +enum ipmi_addr_src {
> + SI_INVALID = 0, SI_HOTMOD, SI_HARDCODED, SI_SPMI, SI_ACPI, SI_SMBIOS,
> + SI_PCI, SI_DEVICETREE, SI_DEFAULT
> +};
> +struct ipmi_smi_info{
> + enum ipmi_addr_src addr_src;
> + struct device *dev;
> + union {
> + /*
> + * Now only SI_ACPI info is provided. If the info is required
> + * for other type, please add it
> + */
> +#ifdef CONFIG_ACPI
> + struct {
> + void *acpi_handle;
> + } acpi_info;
> +#endif
> + } smi_info;
> +};
> +
> +/* This is to get the private info of ipmi_smi_t */
> +extern int ipmi_get_smi_info(int if_num, enum ipmi_addr_src type,
> + struct ipmi_smi_info *data);
> +/* This is to decrease refcount added in the function of ipmi_get_smi_info */
> +extern void ipmi_put_smi_info(int if_num);
> #endif /* __KERNEL__ */
>
>
> diff --git a/include/linux/ipmi_smi.h b/include/linux/ipmi_smi.h
> index 4b48318..b9a53c0 100644
> --- a/include/linux/ipmi_smi.h
> +++ b/include/linux/ipmi_smi.h
> @@ -86,6 +86,18 @@ struct ipmi_smi_handlers {
> int (*start_processing)(void *send_info,
> ipmi_smi_t new_intf);
>
> + /*
> + * Get the detailed private info of the low level interface and store
> + * it into the structure of ipmi_smi_data. For example: the
> + * ACPI device handle will be returned for the pnp_acpi IPMI device.
> + * Of course it will firstly compare the interface type of low-level
> + * interface(Hardcoded type, DMI, ACPI and so on). If the type doesn't
> + * match, it will return -EINVAL.
> + */
> + int (*get_smi_info)(void *send_info, enum ipmi_addr_src type,
> + struct ipmi_smi_info *data);
> +
> + void (*put_smi_info)(void *send_info);
> /* Called to enqueue an SMI message to be sent. This
> operation is not allowed to fail. If an error occurs, it
> should report back the error in a received message. It may
^ permalink raw reply [flat|nested] 24+ messages in thread
* [RFC PATCH 1/4] IPMI: Add one interface to get more info of low-level IPMI device
2010-10-12 7:47 [RFC PATCH 0/4_v10] IPMI/ACPI: Install the ACPI IPMI opregion yakui.zhao
@ 2010-10-12 7:47 ` yakui.zhao
2010-10-12 7:51 ` ykzhao
2010-10-14 16:53 ` Corey Minyard
0 siblings, 2 replies; 24+ messages in thread
From: yakui.zhao @ 2010-10-12 7:47 UTC (permalink / raw)
To: openipmi-developer, linux-acpi; +Cc: minyard, lenb, Zhao Yakui
From: Zhao Yakui <yakui.zhao@intel.com>
The IPMI smi_watcher will be used to catch the IPMI interface as they come or go.
In order to communicate with the correct IPMI device, it should be confirmed
whether it is what we wanted especially on the system with multiple IPMI
devices. But the new_smi callback function of smi_watcher provides very
limited info(only the interface number and dev pointer) and there is no
detailed info about the low level interface. For example: which mechansim
registers the IPMI interface(ACPI, PCI, DMI and so on).
This is to add one interface that can get more info of low-level IPMI
device. For example: the ACPI device handle will be returned for the pnp_acpi
IPMI device.
Signed-off-by: Zhao Yakui <yakui.zhao@intel.com>
---
In this patch only the info of pnp_acpi IPMI is provided. If the detailed info
is also required for other mechanism, please add them in the structure of
ipmi_smi_info.
drivers/char/ipmi/ipmi_msghandler.c | 47 +++++++++++++++++++++++++++++++++++
drivers/char/ipmi/ipmi_si_intf.c | 35 +++++++++++++++++++++++---
include/linux/ipmi.h | 25 ++++++++++++++++++
include/linux/ipmi_smi.h | 12 +++++++++
4 files changed, 115 insertions(+), 4 deletions(-)
diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
index 4f3f8c9..29e373d 100644
--- a/drivers/char/ipmi/ipmi_msghandler.c
+++ b/drivers/char/ipmi/ipmi_msghandler.c
@@ -970,6 +970,53 @@ out_kfree:
}
EXPORT_SYMBOL(ipmi_create_user);
+int ipmi_get_smi_info(int if_num, enum ipmi_addr_src type,
+ struct ipmi_smi_info *data)
+{
+ int rv = 0;
+ ipmi_smi_t intf;
+ struct ipmi_smi_handlers *handlers;
+
+ mutex_lock(&ipmi_interfaces_mutex);
+ list_for_each_entry_rcu(intf, &ipmi_interfaces, link) {
+ if (intf->intf_num == if_num)
+ goto found;
+ }
+ /* Not found, return an error */
+ rv = -EINVAL;
+ mutex_unlock(&ipmi_interfaces_mutex);
+ return rv;
+
+found:
+ handlers = intf->handlers;
+ rv = handlers->get_smi_info(intf->send_info, type, data);
+ mutex_unlock(&ipmi_interfaces_mutex);
+
+ return rv;
+}
+EXPORT_SYMBOL(ipmi_get_smi_info);
+
+void ipmi_put_smi_info(int if_num)
+{
+ ipmi_smi_t intf;
+ struct ipmi_smi_handlers *handlers;
+
+ mutex_lock(&ipmi_interfaces_mutex);
+ list_for_each_entry_rcu(intf, &ipmi_interfaces, link) {
+ if (intf->intf_num == if_num)
+ goto found;
+ }
+ /* Not found, return */
+ mutex_unlock(&ipmi_interfaces_mutex);
+ return;
+
+found:
+ handlers = intf->handlers;
+ handlers->put_smi_info(intf->send_info);
+ mutex_unlock(&ipmi_interfaces_mutex);
+}
+EXPORT_SYMBOL(ipmi_put_smi_info);
+
static void free_user(struct kref *ref)
{
ipmi_user_t user = container_of(ref, struct ipmi_user, refcount);
diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
index 7bd7c45..4885709 100644
--- a/drivers/char/ipmi/ipmi_si_intf.c
+++ b/drivers/char/ipmi/ipmi_si_intf.c
@@ -57,6 +57,7 @@
#include <asm/irq.h>
#include <linux/interrupt.h>
#include <linux/rcupdate.h>
+#include <linux/ipmi.h>
#include <linux/ipmi_smi.h>
#include <asm/io.h>
#include "ipmi_si_sm.h"
@@ -107,10 +108,6 @@ enum si_type {
};
static char *si_to_str[] = { "kcs", "smic", "bt" };
-enum ipmi_addr_src {
- SI_INVALID = 0, SI_HOTMOD, SI_HARDCODED, SI_SPMI, SI_ACPI, SI_SMBIOS,
- SI_PCI, SI_DEVICETREE, SI_DEFAULT
-};
static char *ipmi_addr_src_to_str[] = { NULL, "hotmod", "hardcoded", "SPMI",
"ACPI", "SMBIOS", "PCI",
"device-tree", "default" };
@@ -291,6 +288,7 @@ struct smi_info {
struct task_struct *thread;
struct list_head link;
+ struct ipmi_smi_info smi_data;
};
#define smi_inc_stat(smi, stat) \
@@ -1186,6 +1184,31 @@ static int smi_start_processing(void *send_info,
return 0;
}
+static int get_smi_info(void *send_info,
+ enum ipmi_addr_src type,
+ struct ipmi_smi_info *data)
+{
+ struct smi_info *new_smi = send_info;
+ struct ipmi_smi_info *smi_data = &new_smi->smi_data;
+
+ if (new_smi->addr_source != type)
+ return -EINVAL;
+
+ get_device(new_smi->dev);
+ memcpy(data, smi_data, sizeof(*smi_data));
+ smi_data->addr_src = type;
+ smi_data->dev = new_smi->dev;
+
+ return 0;
+}
+
+static void put_smi_info(void *send_info)
+{
+ struct smi_info *new_smi = send_info;
+
+ put_device(new_smi->dev);
+}
+
static void set_maintenance_mode(void *send_info, int enable)
{
struct smi_info *smi_info = send_info;
@@ -1197,6 +1220,8 @@ static void set_maintenance_mode(void *send_info, int enable)
static struct ipmi_smi_handlers handlers = {
.owner = THIS_MODULE,
.start_processing = smi_start_processing,
+ .get_smi_info = get_smi_info,
+ .put_smi_info = put_smi_info,
.sender = sender,
.request_events = request_events,
.set_maintenance_mode = set_maintenance_mode,
@@ -2143,6 +2168,8 @@ static int __devinit ipmi_pnp_probe(struct pnp_dev *dev,
return -ENOMEM;
info->addr_source = SI_ACPI;
+ info->smi_data.smi_info.acpi_info.acpi_handle =
+ acpi_dev->handle;
printk(KERN_INFO PFX "probing via ACPI\n");
handle = acpi_dev->handle;
diff --git a/include/linux/ipmi.h b/include/linux/ipmi.h
index 65aae34..2d30551 100644
--- a/include/linux/ipmi.h
+++ b/include/linux/ipmi.h
@@ -454,6 +454,31 @@ unsigned int ipmi_addr_length(int addr_type);
/* Validate that the given IPMI address is valid. */
int ipmi_validate_addr(struct ipmi_addr *addr, int len);
+enum ipmi_addr_src {
+ SI_INVALID = 0, SI_HOTMOD, SI_HARDCODED, SI_SPMI, SI_ACPI, SI_SMBIOS,
+ SI_PCI, SI_DEVICETREE, SI_DEFAULT
+};
+struct ipmi_smi_info{
+ enum ipmi_addr_src addr_src;
+ struct device *dev;
+ union {
+ /*
+ * Now only SI_ACPI info is provided. If the info is required
+ * for other type, please add it
+ */
+#ifdef CONFIG_ACPI
+ struct {
+ void *acpi_handle;
+ } acpi_info;
+#endif
+ } smi_info;
+};
+
+/* This is to get the private info of ipmi_smi_t */
+extern int ipmi_get_smi_info(int if_num, enum ipmi_addr_src type,
+ struct ipmi_smi_info *data);
+/* This is to decrease refcount added in the function of ipmi_get_smi_info */
+extern void ipmi_put_smi_info(int if_num);
#endif /* __KERNEL__ */
diff --git a/include/linux/ipmi_smi.h b/include/linux/ipmi_smi.h
index 4b48318..b9a53c0 100644
--- a/include/linux/ipmi_smi.h
+++ b/include/linux/ipmi_smi.h
@@ -86,6 +86,18 @@ struct ipmi_smi_handlers {
int (*start_processing)(void *send_info,
ipmi_smi_t new_intf);
+ /*
+ * Get the detailed private info of the low level interface and store
+ * it into the structure of ipmi_smi_data. For example: the
+ * ACPI device handle will be returned for the pnp_acpi IPMI device.
+ * Of course it will firstly compare the interface type of low-level
+ * interface(Hardcoded type, DMI, ACPI and so on). If the type doesn't
+ * match, it will return -EINVAL.
+ */
+ int (*get_smi_info)(void *send_info, enum ipmi_addr_src type,
+ struct ipmi_smi_info *data);
+
+ void (*put_smi_info)(void *send_info);
/* Called to enqueue an SMI message to be sent. This
operation is not allowed to fail. If an error occurs, it
should report back the error in a received message. It may
--
1.5.4.5
^ permalink raw reply related [flat|nested] 24+ messages in thread
end of thread, other threads:[~2010-12-06 1:08 UTC | newest]
Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-10-26 9:14 [RFC PATCH 0/4_v11] IPMI/ACPI: Install the ACPI IPMI opregion yakui.zhao
2010-10-26 9:14 ` [RFC PATCH 1/4] IPMI: Add one interface to get more info of low-level IPMI device yakui.zhao
2010-10-26 9:14 ` [RFC PATCH 2/4] IPMI: Remove the redundant definition of ipmi_addr_src yakui.zhao
2010-10-26 9:14 ` [RFC PATCH 3/4] IPMI: Add the document description of ipmi_get_smi_info yakui.zhao
2010-10-26 9:14 ` [RFC PATCH 4/4] IPMI/ACPI: Add the IPMI opregion driver to enable ACPI to access BMC controller yakui.zhao
2010-10-27 16:13 ` [RFC PATCH 1/4] IPMI: Add one interface to get more info of low-level IPMI device Corey Minyard
2010-10-28 1:00 ` ykzhao
2010-11-02 5:33 ` ykzhao
2010-11-04 0:41 ` Corey Minyard
2010-11-15 7:52 ` ykzhao
2010-11-23 7:13 ` ykzhao
-- strict thread matches above, loose matches on Subject: below --
2010-11-30 0:26 [RFC PATCH 0/4_v13] IPMI/ACPI: Install the ACPI IPMI opregion yakui.zhao
2010-11-30 0:26 ` [RFC PATCH 1/4] IPMI: Add one interface to get more info of low-level IPMI device yakui.zhao
2010-11-30 0:36 ` ykzhao
2010-12-06 1:06 ` ykzhao
2010-10-22 9:10 [RFC PATCH 0/4_v11] IPMI/ACPI: Install the ACPI IPMI opregion yakui.zhao
2010-10-22 9:10 ` [RFC PATCH 1/4] IPMI: Add one interface to get more info of low-level IPMI device yakui.zhao
2010-10-25 1:19 ` ykzhao
2010-10-25 3:25 ` Corey Minyard
2010-10-25 7:00 ` ykzhao
2010-10-12 7:47 [RFC PATCH 0/4_v10] IPMI/ACPI: Install the ACPI IPMI opregion yakui.zhao
2010-10-12 7:47 ` [RFC PATCH 1/4] IPMI: Add one interface to get more info of low-level IPMI device yakui.zhao
2010-10-12 7:51 ` ykzhao
2010-10-14 16:53 ` Corey Minyard
2010-10-15 1:07 ` ykzhao
2010-10-15 3:03 ` Corey Minyard
2010-10-15 5:07 ` ykzhao
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.