From 03b4fa4b5dd9ecd11c27cdaeacf46453a948df5a Mon Sep 17 00:00:00 2001 From: Jeffrey Carlyle Date: Fri, 6 May 2016 11:29:37 -0700 Subject: [PATCH] greybus: Revert "bootrom: Implement timeouts to detect Module failures" This reverts commit 571348253032a86e4f0102d4dfadd390d0ea7e64. With this patch gb_bootrom_timedout was getting called in interrupt context and then proceeding to call functions that might block. In practical terms, this was leading to a kernel BUG at mm/vmalloc.c:1650. Signed-off-by: Jeffrey Carlyle Acked-by: Johan Hovold --- drivers/staging/greybus/bootrom.c | 124 +++++------------------------- 1 file changed, 19 insertions(+), 105 deletions(-) diff --git a/drivers/staging/greybus/bootrom.c b/drivers/staging/greybus/bootrom.c index baada45329a2..cf750beb3403 100644 --- a/drivers/staging/greybus/bootrom.c +++ b/drivers/staging/greybus/bootrom.c @@ -8,48 +8,24 @@ */ #include -#include -#include -#include #include "bootrom.h" #include "greybus.h" -/* Timeout, in jiffies, within which the next request must be received */ -#define NEXT_REQ_TIMEOUT_J msecs_to_jiffies(1000) struct gb_bootrom { struct gb_connection *connection; const struct firmware *fw; u8 protocol_major; u8 protocol_minor; - struct timer_list timer; - struct mutex mutex; /* Protects bootrom->fw */ }; static void free_firmware(struct gb_bootrom *bootrom) { - if (!bootrom->fw) - return; - release_firmware(bootrom->fw); bootrom->fw = NULL; } -static void gb_bootrom_timedout(unsigned long data) -{ - struct gb_bootrom *bootrom = (struct gb_bootrom *)data; - struct device *dev = &bootrom->connection->bundle->dev; - - dev_err(dev, "Timed out waiting for request from the Module\n"); - - mutex_lock(&bootrom->mutex); - free_firmware(bootrom); - mutex_unlock(&bootrom->mutex); - - /* TODO: Power-off Module ? */ -} - /* * The es2 chip doesn't have VID/PID programmed into the hardware and we need to * hack that up to distinguish different modules and their firmware blobs. @@ -101,7 +77,8 @@ static int download_firmware(struct gb_bootrom *bootrom, u8 stage) int rc; /* Already have a firmware, free it */ - free_firmware(bootrom); + if (bootrom->fw) + free_firmware(bootrom); /* * Create firmware name @@ -137,32 +114,25 @@ static int gb_bootrom_firmware_size_request(struct gb_operation *op) struct device *dev = &op->connection->bundle->dev; int ret; - /* Disable timeouts */ - del_timer_sync(&bootrom->timer); - if (op->request->payload_size != sizeof(*size_request)) { dev_err(dev, "%s: illegal size of firmware size request (%zu != %zu)\n", __func__, op->request->payload_size, sizeof(*size_request)); - ret = -EINVAL; - goto mod_timer; + return -EINVAL; } - mutex_lock(&bootrom->mutex); - ret = download_firmware(bootrom, size_request->stage); if (ret) { dev_err(dev, "%s: failed to download firmware (%d)\n", __func__, ret); - goto unlock; + return ret; } if (!gb_operation_response_alloc(op, sizeof(*size_response), GFP_KERNEL)) { dev_err(dev, "%s: error allocating response\n", __func__); free_firmware(bootrom); - ret = -ENOMEM; - goto unlock; + return -ENOMEM; } size_response = op->response->payload; @@ -170,44 +140,28 @@ static int gb_bootrom_firmware_size_request(struct gb_operation *op) dev_dbg(dev, "%s: firmware size %d bytes\n", __func__, size_response->size); -unlock: - mutex_unlock(&bootrom->mutex); - -mod_timer: - /* Refresh timeout */ - mod_timer(&bootrom->timer, jiffies + NEXT_REQ_TIMEOUT_J); - - return ret; + return 0; } static int gb_bootrom_get_firmware(struct gb_operation *op) { struct gb_bootrom *bootrom = gb_connection_get_data(op->connection); - const struct firmware *fw; + const struct firmware *fw = bootrom->fw; struct gb_bootrom_get_firmware_request *firmware_request; struct gb_bootrom_get_firmware_response *firmware_response; struct device *dev = &op->connection->bundle->dev; unsigned int offset, size; - int ret = 0; - - /* Disable timeouts */ - del_timer_sync(&bootrom->timer); if (op->request->payload_size != sizeof(*firmware_request)) { dev_err(dev, "%s: Illegal size of get firmware request (%zu %zu)\n", __func__, op->request->payload_size, sizeof(*firmware_request)); - ret = -EINVAL; - goto mod_timer; + return -EINVAL; } - mutex_lock(&bootrom->mutex); - - fw = bootrom->fw; if (!fw) { dev_err(dev, "%s: firmware not available\n", __func__); - ret = -EINVAL; - goto unlock; + return -EINVAL; } firmware_request = op->request->payload; @@ -217,15 +171,13 @@ static int gb_bootrom_get_firmware(struct gb_operation *op) if (offset >= fw->size || size > fw->size - offset) { dev_warn(dev, "bad firmware request (offs = %u, size = %u)\n", offset, size); - ret = -EINVAL; - goto unlock; + return -EINVAL; } if (!gb_operation_response_alloc(op, sizeof(*firmware_response) + size, GFP_KERNEL)) { dev_err(dev, "%s: error allocating response\n", __func__); - ret = -ENOMEM; - goto unlock; + return -ENOMEM; } firmware_response = op->response->payload; @@ -234,59 +186,36 @@ static int gb_bootrom_get_firmware(struct gb_operation *op) dev_dbg(dev, "responding with firmware (offs = %u, size = %u)\n", offset, size); -unlock: - mutex_unlock(&bootrom->mutex); - -mod_timer: - /* Refresh timeout */ - mod_timer(&bootrom->timer, jiffies + NEXT_REQ_TIMEOUT_J); - - return ret; + return 0; } static int gb_bootrom_ready_to_boot(struct gb_operation *op) { struct gb_connection *connection = op->connection; - struct gb_bootrom *bootrom = gb_connection_get_data(connection); struct gb_bootrom_ready_to_boot_request *rtb_request; struct device *dev = &connection->bundle->dev; u8 status; - int ret = 0; - - /* Disable timeouts */ - del_timer_sync(&bootrom->timer); if (op->request->payload_size != sizeof(*rtb_request)) { dev_err(dev, "%s: Illegal size of ready to boot request (%zu %zu)\n", __func__, op->request->payload_size, sizeof(*rtb_request)); - ret = -EINVAL; - goto mod_timer; + return -EINVAL; } rtb_request = op->request->payload; status = rtb_request->status; /* Return error if the blob was invalid */ - if (status == GB_BOOTROM_BOOT_STATUS_INVALID) { - ret = -EINVAL; - goto mod_timer; - } + if (status == GB_BOOTROM_BOOT_STATUS_INVALID) + return -EINVAL; /* * XXX Should we return error for insecure firmware? */ dev_dbg(dev, "ready to boot: 0x%x, 0\n", status); -mod_timer: - /* - * Refresh timeout, the Interface shall load the new personality and - * send a new hotplug request, which shall get rid of the bootrom - * connection. As that can take some time, increase the timeout a bit. - */ - mod_timer(&bootrom->timer, jiffies + 5 * NEXT_REQ_TIMEOUT_J); - - return ret; + return 0; } static int gb_bootrom_request_handler(struct gb_operation *op) @@ -375,11 +304,6 @@ static int gb_bootrom_probe(struct gb_bundle *bundle, bootrom->connection = connection; - mutex_init(&bootrom->mutex); - init_timer(&bootrom->timer); - bootrom->timer.function = gb_bootrom_timedout; - bootrom->timer.data = (unsigned long)bootrom; - greybus_set_drvdata(bundle, bootrom); ret = gb_connection_enable_tx(connection); @@ -405,9 +329,6 @@ static int gb_bootrom_probe(struct gb_bundle *bundle, goto err_connection_disable; } - /* Refresh timeout */ - mod_timer(&bootrom->timer, jiffies + NEXT_REQ_TIMEOUT_J); - dev_dbg(&bundle->dev, "AP_READY sent\n"); return 0; @@ -430,16 +351,9 @@ static void gb_bootrom_disconnect(struct gb_bundle *bundle) gb_connection_disable(bootrom->connection); - /* Disable timeouts */ - del_timer_sync(&bootrom->timer); - - /* - * Release firmware: - * - * As the connection and the timer are already disabled, we don't need - * to lock access to bootrom->fw here. - */ - free_firmware(bootrom); + /* Release firmware */ + if (bootrom->fw) + free_firmware(bootrom); gb_connection_destroy(bootrom->connection); kfree(bootrom); -- 2.39.5