Re: [PATCH V3] firmware loader: Fix the race FW_STATUS_DONE is followed by class_timeout
On Thu, Nov 8, 2012 at 7:14 PM, Chuansheng Liu wrote: > > There is a race as below when calling request_firmware(): > CPU1 CPU2 > write 0 > loading > mutex_lock(_lock) > ... > set_bit FW_STATUS_DONE class_timeout is coming >set_bit FW_STATUS_ABORT > complete_all > ... > mutex_unlock(_lock) > > In this time, the bit FW_STATUS_DONE and FW_STATUS_ABORT are set, > and request_firmware() will return failure due to condition in > _request_firmware_load(): > if (!buf->size || test_bit(FW_STATUS_ABORT, >status)) > retval = -ENOENT; > > But from the above scenerio, it should be a successful requesting. > So we need judge if the bit FW_STATUS_DONE is already set before > calling fw_load_abort() in timeout function. > > As Ming's proposal, we need change the timer into sched_work to > benefit from using _lock mutex also. > > Signed-off-by: liu chuansheng Acked-by: Ming Lei Thanks, -- Ming Lei -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V3] firmware loader: Fix the race FW_STATUS_DONE is followed by class_timeout
On Thu, Nov 8, 2012 at 7:14 PM, Chuansheng Liu chuansheng@intel.com wrote: There is a race as below when calling request_firmware(): CPU1 CPU2 write 0 loading mutex_lock(fw_lock) ... set_bit FW_STATUS_DONE class_timeout is coming set_bit FW_STATUS_ABORT complete_all completion ... mutex_unlock(fw_lock) In this time, the bit FW_STATUS_DONE and FW_STATUS_ABORT are set, and request_firmware() will return failure due to condition in _request_firmware_load(): if (!buf-size || test_bit(FW_STATUS_ABORT, buf-status)) retval = -ENOENT; But from the above scenerio, it should be a successful requesting. So we need judge if the bit FW_STATUS_DONE is already set before calling fw_load_abort() in timeout function. As Ming's proposal, we need change the timer into sched_work to benefit from using fw_lock mutex also. Signed-off-by: liu chuansheng chuansheng@intel.com Acked-by: Ming Lei ming@canonical.com Thanks, -- Ming Lei -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH V3] firmware loader: Fix the race FW_STATUS_DONE is followed by class_timeout
There is a race as below when calling request_firmware(): CPU1 CPU2 write 0 > loading mutex_lock(_lock) ... set_bit FW_STATUS_DONE class_timeout is coming set_bit FW_STATUS_ABORT complete_all ... mutex_unlock(_lock) In this time, the bit FW_STATUS_DONE and FW_STATUS_ABORT are set, and request_firmware() will return failure due to condition in _request_firmware_load(): if (!buf->size || test_bit(FW_STATUS_ABORT, >status)) retval = -ENOENT; But from the above scenerio, it should be a successful requesting. So we need judge if the bit FW_STATUS_DONE is already set before calling fw_load_abort() in timeout function. As Ming's proposal, we need change the timer into sched_work to benefit from using _lock mutex also. Signed-off-by: liu chuansheng --- drivers/base/firmware_class.c | 24 1 files changed, 16 insertions(+), 8 deletions(-) diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c index 8945f4e..b44ed35 100644 --- a/drivers/base/firmware_class.c +++ b/drivers/base/firmware_class.c @@ -143,7 +143,7 @@ struct fw_cache_entry { }; struct firmware_priv { - struct timer_list timeout; + struct delayed_work timeout_work; bool nowait; struct device dev; struct firmware_buf *buf; @@ -667,11 +667,18 @@ static struct bin_attribute firmware_attr_data = { .write = firmware_data_write, }; -static void firmware_class_timeout(u_long data) +static void firmware_class_timeout_work(struct work_struct *work) { - struct firmware_priv *fw_priv = (struct firmware_priv *) data; + struct firmware_priv *fw_priv = container_of(work, + struct firmware_priv, timeout_work.work); + mutex_lock(_lock); + if (test_bit(FW_STATUS_DONE, &(fw_priv->buf->status))) { + mutex_unlock(_lock); + return; + } fw_load_abort(fw_priv); + mutex_unlock(_lock); } static struct firmware_priv * @@ -690,8 +697,8 @@ fw_create_instance(struct firmware *firmware, const char *fw_name, fw_priv->nowait = nowait; fw_priv->fw = firmware; - setup_timer(_priv->timeout, - firmware_class_timeout, (u_long) fw_priv); + INIT_DELAYED_WORK(_priv->timeout_work, + firmware_class_timeout_work); f_dev = _priv->dev; @@ -858,7 +865,9 @@ static int _request_firmware_load(struct firmware_priv *fw_priv, bool uevent, dev_dbg(f_dev->parent, "firmware: direct-loading" " firmware %s\n", buf->fw_id); + mutex_lock(_lock); set_bit(FW_STATUS_DONE, >status); + mutex_unlock(_lock); complete_all(>completion); direct_load = 1; goto handle_fw; @@ -894,15 +903,14 @@ static int _request_firmware_load(struct firmware_priv *fw_priv, bool uevent, dev_set_uevent_suppress(f_dev, false); dev_dbg(f_dev, "firmware: requesting %s\n", buf->fw_id); if (timeout != MAX_SCHEDULE_TIMEOUT) - mod_timer(_priv->timeout, - round_jiffies_up(jiffies + timeout)); + schedule_delayed_work(_priv->timeout_work, timeout); kobject_uevent(_priv->dev.kobj, KOBJ_ADD); } wait_for_completion(>completion); - del_timer_sync(_priv->timeout); + cancel_delayed_work_sync(_priv->timeout_work); handle_fw: mutex_lock(_lock); -- 1.7.0.4 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH V3] firmware loader: Fix the race FW_STATUS_DONE is followed by class_timeout
There is a race as below when calling request_firmware(): CPU1 CPU2 write 0 loading mutex_lock(fw_lock) ... set_bit FW_STATUS_DONE class_timeout is coming set_bit FW_STATUS_ABORT complete_all completion ... mutex_unlock(fw_lock) In this time, the bit FW_STATUS_DONE and FW_STATUS_ABORT are set, and request_firmware() will return failure due to condition in _request_firmware_load(): if (!buf-size || test_bit(FW_STATUS_ABORT, buf-status)) retval = -ENOENT; But from the above scenerio, it should be a successful requesting. So we need judge if the bit FW_STATUS_DONE is already set before calling fw_load_abort() in timeout function. As Ming's proposal, we need change the timer into sched_work to benefit from using fw_lock mutex also. Signed-off-by: liu chuansheng chuansheng@intel.com --- drivers/base/firmware_class.c | 24 1 files changed, 16 insertions(+), 8 deletions(-) diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c index 8945f4e..b44ed35 100644 --- a/drivers/base/firmware_class.c +++ b/drivers/base/firmware_class.c @@ -143,7 +143,7 @@ struct fw_cache_entry { }; struct firmware_priv { - struct timer_list timeout; + struct delayed_work timeout_work; bool nowait; struct device dev; struct firmware_buf *buf; @@ -667,11 +667,18 @@ static struct bin_attribute firmware_attr_data = { .write = firmware_data_write, }; -static void firmware_class_timeout(u_long data) +static void firmware_class_timeout_work(struct work_struct *work) { - struct firmware_priv *fw_priv = (struct firmware_priv *) data; + struct firmware_priv *fw_priv = container_of(work, + struct firmware_priv, timeout_work.work); + mutex_lock(fw_lock); + if (test_bit(FW_STATUS_DONE, (fw_priv-buf-status))) { + mutex_unlock(fw_lock); + return; + } fw_load_abort(fw_priv); + mutex_unlock(fw_lock); } static struct firmware_priv * @@ -690,8 +697,8 @@ fw_create_instance(struct firmware *firmware, const char *fw_name, fw_priv-nowait = nowait; fw_priv-fw = firmware; - setup_timer(fw_priv-timeout, - firmware_class_timeout, (u_long) fw_priv); + INIT_DELAYED_WORK(fw_priv-timeout_work, + firmware_class_timeout_work); f_dev = fw_priv-dev; @@ -858,7 +865,9 @@ static int _request_firmware_load(struct firmware_priv *fw_priv, bool uevent, dev_dbg(f_dev-parent, firmware: direct-loading firmware %s\n, buf-fw_id); + mutex_lock(fw_lock); set_bit(FW_STATUS_DONE, buf-status); + mutex_unlock(fw_lock); complete_all(buf-completion); direct_load = 1; goto handle_fw; @@ -894,15 +903,14 @@ static int _request_firmware_load(struct firmware_priv *fw_priv, bool uevent, dev_set_uevent_suppress(f_dev, false); dev_dbg(f_dev, firmware: requesting %s\n, buf-fw_id); if (timeout != MAX_SCHEDULE_TIMEOUT) - mod_timer(fw_priv-timeout, - round_jiffies_up(jiffies + timeout)); + schedule_delayed_work(fw_priv-timeout_work, timeout); kobject_uevent(fw_priv-dev.kobj, KOBJ_ADD); } wait_for_completion(buf-completion); - del_timer_sync(fw_priv-timeout); + cancel_delayed_work_sync(fw_priv-timeout_work); handle_fw: mutex_lock(fw_lock); -- 1.7.0.4 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/