From 09e14ad7e8dc2b9393da5ac1d404036a6ef6f8ea Mon Sep 17 00:00:00 2001 From: Pavan Kumar Chilamkurthi Date: Thu, 23 Jan 2020 12:10:52 -0800 Subject: [PATCH] msm: camera: fd: Fix race condition in resetting ready_to_process Do not unlock frame_req_mutex until the request is added into processing list after submitting the frame to hw. This can cause a race condition where Flush comes before adding the frame into processing list and thinks there is no frame with HW. And there is a possibility that FrameDone IRQ thread can get the mutex and thinks no frame in processing list - causing never to reset the flag ready_to_process. This will cause all subsequent camera usage to fail as requests will never be submitted to hw. CRs-Fixed: 2600443 Change-Id: Icd12e155192fccd9adbc7b774b073ca623f1d7ba Signed-off-by: Pavan Kumar Chilamkurthi --- drivers/cam_fd/fd_hw_mgr/cam_fd_hw_mgr.c | 82 +++++++++++++------ drivers/cam_fd/fd_hw_mgr/cam_fd_hw_mgr.h | 3 + .../cam_fd/fd_hw_mgr/fd_hw/cam_fd_hw_core.c | 1 + .../cam_fd/fd_hw_mgr/fd_hw/cam_fd_hw_intf.h | 2 + 4 files changed, 64 insertions(+), 24 deletions(-) diff --git a/drivers/cam_fd/fd_hw_mgr/cam_fd_hw_mgr.c b/drivers/cam_fd/fd_hw_mgr/cam_fd_hw_mgr.c index f90ffc4eb7c4..644767d9e513 100644 --- a/drivers/cam_fd/fd_hw_mgr/cam_fd_hw_mgr.c +++ b/drivers/cam_fd/fd_hw_mgr/cam_fd_hw_mgr.c @@ -240,6 +240,15 @@ static int cam_fd_mgr_util_release_device(struct cam_fd_hw_mgr *hw_mgr, mutex_lock(&hw_mgr->hw_mgr_mutex); hw_device->num_ctxts--; + + if (!hw_device->num_ctxts) { + mutex_lock(&hw_device->lock); + hw_device->ready_to_process = true; + hw_device->req_id = -1; + hw_device->cur_hw_ctx = NULL; + mutex_unlock(&hw_device->lock); + } + mutex_unlock(&hw_mgr->hw_mgr_mutex); hw_ctx->device_index = -1; @@ -277,6 +286,11 @@ static int cam_fd_mgr_util_select_device(struct cam_fd_hw_mgr *hw_mgr, (!fd_acquire_args->get_raw_results || hw_device->hw_caps.raw_results_available)) { CAM_DBG(CAM_FD, "Found dedicated HW Index=%d", i); + mutex_lock(&hw_device->lock); + hw_device->ready_to_process = true; + hw_device->req_id = -1; + hw_device->cur_hw_ctx = NULL; + mutex_unlock(&hw_device->lock); hw_device->num_ctxts++; break; } @@ -294,7 +308,9 @@ static int cam_fd_mgr_util_select_device(struct cam_fd_hw_mgr *hw_mgr, (!fd_acquire_args->get_raw_results || hw_device->hw_caps.raw_results_available)) { hw_device->num_ctxts++; - CAM_DBG(CAM_FD, "Found sharing HW Index=%d", i); + CAM_DBG(CAM_FD, + "Found sharing HW Index=%d, num_ctxts=%d", + i, hw_device->num_ctxts); break; } } @@ -846,6 +862,11 @@ static int cam_fd_mgr_util_submit_frame(void *priv, void *data) mutex_lock(&hw_device->lock); if (hw_device->ready_to_process == false) { + if (hw_mgr->num_pending_frames > 6) { + CAM_WARN(CAM_FD, + "Device busy for longer time with cur_hw_ctx=%pK, ReqId=%lld", + hw_device->cur_hw_ctx, hw_device->req_id); + } mutex_unlock(&hw_device->lock); mutex_unlock(&hw_mgr->frame_req_mutex); return -EBUSY; @@ -854,7 +875,8 @@ static int cam_fd_mgr_util_submit_frame(void *priv, void *data) trace_cam_submit_to_hw("FD", frame_req->request_id); list_del_init(&frame_req->list); - mutex_unlock(&hw_mgr->frame_req_mutex); + hw_mgr->num_pending_frames--; + list_add_tail(&frame_req->list, &hw_mgr->frame_processing_list); if (hw_device->hw_intf->hw_ops.start) { start_args.hw_ctx = hw_ctx; @@ -869,12 +891,16 @@ static int cam_fd_mgr_util_submit_frame(void *priv, void *data) sizeof(start_args)); if (rc) { CAM_ERR(CAM_FD, "Failed in HW Start %d", rc); + list_del_init(&frame_req->list); mutex_unlock(&hw_device->lock); + mutex_unlock(&hw_mgr->frame_req_mutex); goto put_req_into_free_list; } } else { CAM_ERR(CAM_FD, "Invalid hw_ops.start"); + list_del_init(&frame_req->list); mutex_unlock(&hw_device->lock); + mutex_unlock(&hw_mgr->frame_req_mutex); rc = -EPERM; goto put_req_into_free_list; } @@ -883,30 +909,9 @@ static int cam_fd_mgr_util_submit_frame(void *priv, void *data) hw_device->cur_hw_ctx = hw_ctx; hw_device->req_id = frame_req->request_id; mutex_unlock(&hw_device->lock); - frame_req->submit_timestamp = ktime_get(); - - rc = cam_fd_mgr_util_put_frame_req( - &hw_mgr->frame_processing_list, &frame_req); - if (rc) { - CAM_ERR(CAM_FD, - "Failed in putting frame req in processing list"); - goto stop_unlock; - } + mutex_unlock(&hw_mgr->frame_req_mutex); return rc; - -stop_unlock: - if (hw_device->hw_intf->hw_ops.stop) { - struct cam_fd_hw_stop_args stop_args; - - stop_args.hw_ctx = hw_ctx; - stop_args.ctx_hw_private = hw_ctx->ctx_hw_private; - stop_args.hw_req_private = &frame_req->hw_req_private; - if (hw_device->hw_intf->hw_ops.stop( - hw_device->hw_intf->hw_priv, &stop_args, - sizeof(stop_args))) - CAM_ERR(CAM_FD, "Failed in HW Stop %d", rc); - } put_req_into_free_list: cam_fd_mgr_util_put_frame_req(&hw_mgr->frame_free_list, &frame_req); @@ -1274,6 +1279,7 @@ static int cam_fd_mgr_hw_start(void *hw_mgr_priv, void *mgr_start_args) if (hw_device->hw_intf->hw_ops.init) { hw_init_args.hw_ctx = hw_ctx; hw_init_args.ctx_hw_private = hw_ctx->ctx_hw_private; + hw_init_args.is_hw_reset = false; if (fd_core->hw_static_info->enable_errata_wa.skip_reset) hw_init_args.reset_required = false; else @@ -1286,6 +1292,14 @@ static int cam_fd_mgr_hw_start(void *hw_mgr_priv, void *mgr_start_args) CAM_ERR(CAM_FD, "Failed in HW Init %d", rc); return rc; } + + if (hw_init_args.is_hw_reset) { + mutex_lock(&hw_device->lock); + hw_device->ready_to_process = true; + hw_device->req_id = -1; + hw_device->cur_hw_ctx = NULL; + mutex_unlock(&hw_device->lock); + } } else { CAM_ERR(CAM_FD, "Invalid init function"); return -EINVAL; @@ -1333,6 +1347,7 @@ static int cam_fd_mgr_hw_flush_req(void *hw_mgr_priv, if (frame_req->request_id != flush_req->request_id) continue; + hw_mgr->num_pending_frames--; list_del_init(&frame_req->list); break; } @@ -1345,6 +1360,7 @@ static int cam_fd_mgr_hw_flush_req(void *hw_mgr_priv, if (frame_req->request_id != flush_req->request_id) continue; + hw_mgr->num_pending_frames--; list_del_init(&frame_req->list); break; } @@ -1427,6 +1443,7 @@ static int cam_fd_mgr_hw_flush_ctx(void *hw_mgr_priv, if (frame_req->hw_ctx != hw_ctx) continue; + hw_mgr->num_pending_frames--; list_del_init(&frame_req->list); } @@ -1435,6 +1452,7 @@ static int cam_fd_mgr_hw_flush_ctx(void *hw_mgr_priv, if (frame_req->hw_ctx != hw_ctx) continue; + hw_mgr->num_pending_frames--; list_del_init(&frame_req->list); } @@ -1816,6 +1834,7 @@ static int cam_fd_mgr_hw_config(void *hw_mgr_priv, void *hw_config_args) struct cam_fd_mgr_frame_request *frame_req; int rc; int i; + uint64_t req_id; if (!hw_mgr || !config) { CAM_ERR(CAM_FD, "Invalid arguments %pK %pK", hw_mgr, config); @@ -1834,6 +1853,7 @@ static int cam_fd_mgr_hw_config(void *hw_mgr_priv, void *hw_config_args) } frame_req = config->priv; + req_id = frame_req->request_id; trace_cam_apply_req("FD", frame_req->request_id); CAM_DBG(CAM_FD, "FrameHWConfig : Frame[%lld]", frame_req->request_id); @@ -1863,6 +1883,13 @@ static int cam_fd_mgr_hw_config(void *hw_mgr_priv, void *hw_config_args) goto put_free_list; } + mutex_lock(&g_fd_hw_mgr.frame_req_mutex); + hw_mgr->num_pending_frames++; + CAM_DBG(CAM_FD, + "Adding ctx[%pK] Req[%llu] : Total number of pending frames %d", + hw_ctx, req_id, hw_mgr->num_pending_frames); + mutex_unlock(&g_fd_hw_mgr.frame_req_mutex); + rc = cam_fd_mgr_util_schedule_frame_worker_task(hw_mgr); if (rc) { CAM_ERR(CAM_FD, "Worker task scheduling failed %d", rc); @@ -1882,6 +1909,10 @@ remove_and_put_free_list: cam_fd_mgr_util_get_frame_req( &hw_mgr->frame_pending_list_normal, &frame_req); } + + mutex_lock(&g_fd_hw_mgr.frame_req_mutex); + hw_mgr->num_pending_frames--; + mutex_unlock(&g_fd_hw_mgr.frame_req_mutex); put_free_list: cam_fd_mgr_util_put_frame_req(&hw_mgr->frame_free_list, &frame_req); @@ -1952,6 +1983,8 @@ int cam_fd_hw_mgr_init(struct device_node *of_node, hw_device->valid = true; hw_device->hw_intf = hw_intf; hw_device->ready_to_process = true; + hw_device->req_id = -1; + hw_device->cur_hw_ctx = NULL; if (hw_device->hw_intf->hw_ops.process_cmd) { struct cam_fd_hw_cmd_set_irq_cb irq_cb_args; @@ -2004,6 +2037,7 @@ int cam_fd_hw_mgr_init(struct device_node *of_node, g_fd_hw_mgr.device_iommu.secure = -1; g_fd_hw_mgr.cdm_iommu.non_secure = -1; g_fd_hw_mgr.cdm_iommu.secure = -1; + g_fd_hw_mgr.num_pending_frames = 0; rc = cam_smmu_get_handle("fd", &g_fd_hw_mgr.device_iommu.non_secure); diff --git a/drivers/cam_fd/fd_hw_mgr/cam_fd_hw_mgr.h b/drivers/cam_fd/fd_hw_mgr/cam_fd_hw_mgr.h index 61b3ae3b40a5..d453923b09de 100644 --- a/drivers/cam_fd/fd_hw_mgr/cam_fd_hw_mgr.h +++ b/drivers/cam_fd/fd_hw_mgr/cam_fd_hw_mgr.h @@ -159,6 +159,8 @@ struct cam_fd_mgr_work_data { * @work : Worker handle * @work_data : Worker data * @fd_caps : FD driver capabilities + * @num_pending_frames : Number of total frames pending for processing + * across contexts */ struct cam_fd_hw_mgr { struct list_head free_ctx_list; @@ -182,6 +184,7 @@ struct cam_fd_hw_mgr { struct cam_req_mgr_core_workq *work; struct cam_fd_mgr_work_data *work_data; struct cam_fd_query_cap_cmd fd_caps; + uint32_t num_pending_frames; }; #endif /* _CAM_FD_HW_MGR_H_ */ diff --git a/drivers/cam_fd/fd_hw_mgr/fd_hw/cam_fd_hw_core.c b/drivers/cam_fd/fd_hw_mgr/fd_hw/cam_fd_hw_core.c index 80e30ad6c64e..49cee7922f9c 100644 --- a/drivers/cam_fd/fd_hw_mgr/fd_hw/cam_fd_hw_core.c +++ b/drivers/cam_fd/fd_hw_mgr/fd_hw/cam_fd_hw_core.c @@ -753,6 +753,7 @@ int cam_fd_hw_init(void *hw_priv, void *init_hw_args, uint32_t arg_size) CAM_ERR(CAM_FD, "Reset Failed, rc=%d", rc); goto disable_soc; } + init_args->is_hw_reset = true; } cam_fd_hw_util_enable_power_on_settings(fd_hw); diff --git a/drivers/cam_fd/fd_hw_mgr/fd_hw/cam_fd_hw_intf.h b/drivers/cam_fd/fd_hw_mgr/fd_hw/cam_fd_hw_intf.h index fd8720856583..9a600b91ca15 100644 --- a/drivers/cam_fd/fd_hw_mgr/fd_hw/cam_fd_hw_intf.h +++ b/drivers/cam_fd/fd_hw_mgr/fd_hw/cam_fd_hw_intf.h @@ -163,11 +163,13 @@ struct cam_fd_hw_release_args { * @hw_ctx : HW context for which init is requested * @ctx_hw_private : HW layer's private information specific to this hw context * @reset_required : Indicates if the reset is required during init or not + * @is_hw_reset : Output from hw layer, whether hw is reset on this init */ struct cam_fd_hw_init_args { void *hw_ctx; void *ctx_hw_private; bool reset_required; + bool is_hw_reset; }; /**