On Wednesday, July 14, 2010 06:54:54 am yakui.zhao@xxxxxxxxx wrote:
> +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:
This is missing the usual "the module will be called acpi_ipmi"
text.
But I don't think this is the right design. I think it's fine to
put all this code in a separate file and have a separate config
option for it.
However, I don't think acpi_ipmi and ipmi_si should be different
modules, because they both want to be connected to the same ACPI
device. Making acpi_ipmi a separate module means you have to
mess around with PNP core things that drivers shouldn't be using.
For example, you have match_device(), pnp_ipmi_match(), and
acpi_add_ipmi_devices() below. These all reimplement things
the PNP core already does.
The problem with replicating this in acpi_ipmi is that it breaks
the modularity of PNP. It means that hotplug of ACPI devices
won't work right, because acpi_ipmi won't find out about additions
or removals of IPI0001 devices.
This ACPI opregion stuff is basically an optional feature of
whatever driver claims IPI0001 devices, and I think we need to
set up and tear down the opregion handler at the same point we
claim and release the IPI0001 devices, i.e., in ipmi_si_intf.c
> +static void acpi_format_ipmi_response(struct acpi_ipmi_msg *msg,
> + acpi_integer *value, int timeout)
> +{
> + 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 (!timeout && !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);
> + return;
You never need a "return" at the end of a void function.
> +}
> +static void ipmi_destroy_tx_msg(struct acpi_ipmi_device *ipmi)
> +{
> + struct acpi_ipmi_msg *tx_msg = NULL, *temp;
No need to initialize tx_msg here; list_for_each_entry_safe()
always sets it before you can use it.
> + int count = 20;
> + 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 20 ticks to flush the tx message list */
> + while (count--) {
> + if (list_empty(&ipmi->tx_msg_list))
> + break;
> + schedule_timeout(1);
The length of time this waits depends on the arch's jiffy length.
The jiffy length is completely independent of how long it might
take to flush the message list. Instead of waiting for "1" jiffy,
you should use some expression involving HZ, or maybe use
msecs_to_jiffies().
> + }
> + 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 =
> + (struct acpi_ipmi_device *)user_msg_data;
You don't need a cast here, since user_msg_data is a "void *".
> + int msg_found = 0;
> + struct acpi_ipmi_msg *tx_msg = NULL;
No need to initialize tx_msg here.
> + struct pnp_dev *pnp_dev = ipmi_device->pnp_dev;
> +
> + if (msg->user != ipmi_device->user_interface) {
> + dev_warn(&pnp_dev->dev, "Incorrect IPMI user\n");
Include the user info here, e.g.,
dev_warn(..., "unexpected user %#x (expected %#x)\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) {
> + /* find the message id */
Superfluous comment; remove it.
> + msg_found = 1;
> + break;
> + }
> + }
> +
> + mutex_unlock(&ipmi_device->mutex_lock);
> + if (!msg_found) {
> + /* no matched msg is found . But we should free it */
Superfluous comment; remove it.
> + ipmi_free_recv_msg(msg);
> + dev_warn(&pnp_dev->dev, "Incorrect MSG is found\n");
Move the dev_warn() up a line and make it say something like this:
dev_warn(..., "unexpected response (msg ID %#x)\n", msg->msgid);
> + 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;
> + struct pnp_dev *device;
> + ipmi_user_t user;
> + int err;
> +
> + if (list_empty(&driver_data.ipmi_devices))
> + return;
You don't need to test for list_empty before using
list_for_each_entry().
> +
> + list_for_each_entry(ipmi_device, &driver_data.ipmi_devices, head) {
> + if (ipmi_device->user_interface) {
> + /*
> + * Only one user interface is allowed to be registered
> + * for one IPMI device.
> + * If we already create the user interface for
> + * one IPMI device, skip it
> + */
> + continue;
> + }
> + device = ipmi_device->pnp_dev;
> + if (dev == &device->dev) {
> + /*
> + * If the dev is identical to the ACPI device,
> + * create the user interface.
> + */
> + err = ipmi_create_user(iface, &driver_data.ipmi_hndlrs,
> + ipmi_device, &user);
> + if (err == 0) {
> + ipmi_device->user_interface = user;
> + ipmi_device->ipmi_ifnum = iface;
> + }
> +
> + break;
> + }
> + }
> +}
> +
> +static void ipmi_bmc_gone(int iface)
> +{
> + struct acpi_ipmi_device *ipmi_device, *temp;
> +
> + if (list_empty(&driver_data.ipmi_devices))
> + return;
You don't need to test for list_empty before using
list_for_each_entry_safe().
> +
> + list_for_each_entry_safe(ipmi_device, temp,
> + &driver_data.ipmi_devices, head) {
> + if (ipmi_device->user_interface &&
> + (ipmi_device->ipmi_ifnum == iface)) {
> + ipmi_destroy_user(ipmi_device->user_interface);
> + ipmi_device->user_interface = NULL;
> + ipmi_destroy_tx_msg(ipmi_device);
> + }
> + }
> +}
> + 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 for %s\n",
> + pnp_dev_name(pnp_dev));
I'm actually not sure what pnp_dev_name() is for. I'd rather not
add more uses of it. I think dev_warn() prints all the information
we need, even without using pnp_dev_name().
> +static const struct pnp_device_id ipmi_device_ids[] = {
> + {"IPI0001", 0},
> + {"", 0},
> +};
> +
> +static int match_device(struct pnp_dev *dev, const struct pnp_device_id *ids)
> +{
> + const struct pnp_device_id *id;
> + struct pnp_id *pnp_id;
> +
> + for (id = ids; id->id[0]; id++) {
> + pnp_id = dev->id;
> + if (!pnp_id)
> + return 0;
> +
> + while (pnp_id) {
> + if (!strncasecmp(pnp_id->id, (char *)id->id,
> + strlen((char *)id->id)))
> + return 1;
> + pnp_id = pnp_id->next;
> + };
> + }
> + return 0;
> +}
> +
> +static int pnp_ipmi_match(struct device *dev, void *data)
> +{
> + struct pnp_dev *pnp_dev;
> + struct acpi_ipmi_device *ipmi_device;
> + struct acpi_device *acpi_dev;
> +
> + pnp_dev = to_pnp_dev(dev);
> +
> + if (!match_device(pnp_dev, ipmi_device_ids))
> + return 0;
> +
> + ipmi_device = kzalloc(sizeof(*ipmi_device), GFP_KERNEL);
> + if (!ipmi_device) {
> + dev_warn(dev, "Can't allocate memory for acpi_ipmi_device\n");
> + return -ENOMEM;
> + }
> + acpi_dev = pnp_acpi_device(pnp_dev);
> + INIT_LIST_HEAD(&ipmi_device->head);
> + ipmi_device->pnp_dev = pnp_dev;
> + ipmi_device->handle = acpi_dev->handle;
> +
> + list_add_tail(&ipmi_device->head, &driver_data.ipmi_devices);
> + return 0;
> +}
> +
> +static void acpi_add_ipmi_devices(void)
> +{
> + struct acpi_ipmi_device *ipmi_device;
> +
> + bus_for_each_dev(&pnp_bus_type, NULL, NULL, pnp_ipmi_match);
> + if (list_empty(&driver_data.ipmi_devices))
> + return;
You don't need to test for an empty list here.
> +
> + list_for_each_entry(ipmi_device, &driver_data.ipmi_devices, head) {
> + mutex_init(&ipmi_device->mutex_lock);
> + INIT_LIST_HEAD(&ipmi_device->tx_msg_list);
> + ipmi_install_handler(ipmi_device);
> + }
> +}
> +
> +static void acpi_remove_ipmi_handlers(void)
> +{
> + struct acpi_ipmi_device *ipmi_device, *temp;
> +
> + list_for_each_entry_safe(ipmi_device, temp, &driver_data.ipmi_devices,
> + head) {
> + if (ipmi_device->user_interface) {
> + /*
> + * If the IPMI user interface is created, it
> + * should be destroyed.
> + */
> + ipmi_destroy_user(ipmi_device->user_interface);
> + ipmi_device->user_interface = NULL;
> + }
> + list_del(&ipmi_device->head);
> + if (!list_empty(&ipmi_device->tx_msg_list)) {
> + /* destroy the Tx_msg list */
> + ipmi_destroy_tx_msg(ipmi_device);
> + }
> + ipmi_remove_handler(ipmi_device);
> + kfree(ipmi_device);
> + }
> + return;
No "return" needed here.
> +}
> +
> +static int __init acpi_ipmi_init(void)
> +{
> + int result = 0;
> +
> + if (acpi_disabled)
> + return result;
> +
> + acpi_add_ipmi_devices();
> +
> + result = ipmi_smi_watcher_register(&driver_data.bmc_events);
> +
> + if (result)
> + acpi_remove_ipmi_handlers();
> +
> + return result;
> +}
> +
> +static void __exit acpi_ipmi_exit(void)
> +{
> + if (acpi_disabled)
> + return;
> +
> + acpi_remove_ipmi_handlers();
> + ipmi_smi_watcher_unregister(&driver_data.bmc_events);
> +
> + return;
No "return" needed here.
> +}
> +
> +module_init(acpi_ipmi_init);
> +module_exit(acpi_ipmi_exit);
>
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
