video: driver: fix core lock acquire and release sequence

[1] Added return type to strict_check() api and bail out if
    strict_check fails.
[2] Fix all the failures with #1.
[3] Added WARN_ON() for strict_check failure.
[4] Ensured &core->lock is acquired before calling below api's.
    - __write_register
    - __write_register_masked
    - __iface_cmdq_write_relaxed
    - __suspend
    - __resume
    - venus_hfi_core_init
    - venus_hfi_core_deinit.

Change-Id: I7f0a3ca6c2aec2758220c90bff9260367f10820b
Signed-off-by: Govindaraj Rajagopal <[email protected]>

driver/platform/waipio/src/msm_vidc_waipio.c

@@ -73,7 +73,6 @@ static struct msm_platform_core_capability core_data_waipio[] = {
 	{HW_RESPONSE_TIMEOUT, HW_RESPONSE_TIMEOUT_VALUE}, /* 1000 ms */
 	{SW_PC_DELAY,         SW_PC_DELAY_VALUE        }, /* 1500 ms (>HW_RESPONSE_TIMEOUT)*/
 	{FW_UNLOAD_DELAY,     FW_UNLOAD_DELAY_VALUE    }, /* 3000 ms (>SW_PC_DELAY)*/
-	{DEBUG_TIMEOUT, 0},
 	// TODO: review below entries, and if required rename as PREFETCH
 	{PREFIX_BUF_COUNT_PIX, 18},
 	{PREFIX_BUF_SIZE_PIX, 13434880}, /* Calculated by VIDEO_RAW_BUFFER_SIZE for 4096x2160 UBWC */

driver/variant/iris2/src/msm_vidc_iris2.c

@@ -153,8 +153,9 @@
 
 static int __interrupt_init_iris2(struct msm_vidc_core *vidc_core)
 {
-	u32 mask_val = 0;
 	struct msm_vidc_core *core = vidc_core;
+	u32 mask_val = 0;
+	int rc = 0;
 
 	if (!core) {
 		d_vpr_e("%s: invalid params\n", __func__);
@@ -167,7 +168,9 @@ static int __interrupt_init_iris2(struct msm_vidc_core *vidc_core)
 	/* Write 0 to unmask CPU and WD interrupts */
 	mask_val &= ~(WRAPPER_INTR_MASK_A2HWD_BMSK_IRIS2|
 			WRAPPER_INTR_MASK_A2HCPU_BMSK_IRIS2);
-	__write_register(core, WRAPPER_INTR_MASK_IRIS2, mask_val);
+	rc = __write_register(core, WRAPPER_INTR_MASK_IRIS2, mask_val);
+	if (rc)
+		return rc;
 
 	return 0;
 }
@@ -175,27 +178,50 @@ static int __interrupt_init_iris2(struct msm_vidc_core *vidc_core)
 static int __setup_ucregion_memory_map_iris2(struct msm_vidc_core *vidc_core)
 {
 	struct msm_vidc_core *core = vidc_core;
+	u32 value;
+	int rc = 0;
 
 	if (!core) {
 		d_vpr_e("%s: invalid params\n", __func__);
 		return -EINVAL;
 	}
 
-	__write_register(core, UC_REGION_ADDR_IRIS2,
-			(u32)core->iface_q_table.align_device_addr);
-	__write_register(core, UC_REGION_SIZE_IRIS2, SHARED_QSIZE);
-	__write_register(core, QTBL_ADDR_IRIS2,
-			(u32)core->iface_q_table.align_device_addr);
-	__write_register(core, QTBL_INFO_IRIS2, 0x01);
-	/* update queues vaddr for debug purpose */
-	__write_register(core, CPU_CS_VCICMDARG0_IRIS2,
-			(u32)((u64)core->iface_q_table.align_virtual_addr));
-	__write_register(core, CPU_CS_VCICMDARG1_IRIS2,
-		(u32)((u64)core->iface_q_table.align_virtual_addr >> 32));
+	value = (u32)core->iface_q_table.align_device_addr;
+	rc = __write_register(core, UC_REGION_ADDR_IRIS2, value);
+	if (rc)
+		return rc;
+
+	value = SHARED_QSIZE;
+	rc = __write_register(core, UC_REGION_SIZE_IRIS2, value);
+	if (rc)
+		return rc;
 
-	if(core->sfr.align_device_addr)
-		__write_register(core, SFR_ADDR_IRIS2,
-			(u32)core->sfr.align_device_addr + VIDEO_ARCH_LX);
+	value = (u32)core->iface_q_table.align_device_addr;
+	rc = __write_register(core, QTBL_ADDR_IRIS2, value);
+	if (rc)
+		return rc;
+
+	rc = __write_register(core, QTBL_INFO_IRIS2, 0x01);
+	if (rc)
+		return rc;
+
+	/* update queues vaddr for debug purpose */
+	value = (u32)((u64)core->iface_q_table.align_virtual_addr);
+	rc = __write_register(core, CPU_CS_VCICMDARG0_IRIS2, value);
+	if (rc)
+		return rc;
+
+	value = (u32)((u64)core->iface_q_table.align_virtual_addr >> 32);
+	rc = __write_register(core, CPU_CS_VCICMDARG1_IRIS2, value);
+	if (rc)
+		return rc;
+
+	if (core->sfr.align_device_addr) {
+		value = (u32)core->sfr.align_device_addr + VIDEO_ARCH_LX;
+		rc = __write_register(core, SFR_ADDR_IRIS2, value);
+		if (rc)
+			return rc;
+	}
 
 	return 0;
 }
@@ -204,6 +230,7 @@ static int __power_off_iris2(struct msm_vidc_core *vidc_core)
 {
 	u32 lpi_status, reg_status = 0, count = 0, max_count = 10;
 	struct msm_vidc_core *core = vidc_core;
+	int rc = 0;
 
 	if (!core) {
 		d_vpr_e("%s: invalid params\n", __func__);
@@ -218,13 +245,18 @@ static int __power_off_iris2(struct msm_vidc_core *vidc_core)
 	core->intr_status = 0;
 
 	/* HPG 6.1.2 Step 1  */
-	__write_register(core, CPU_CS_X2RPMh_IRIS2, 0x3);
+	rc = __write_register(core, CPU_CS_X2RPMh_IRIS2, 0x3);
+	if (rc)
+		return rc;
 
 	/* HPG 6.1.2 Step 2, noc to low power */
 	//if (core->res->vpu_ver == VPU_VERSION_IRIS2_1)
 	//	goto skip_aon_mvp_noc;
 
-	__write_register(core, AON_WRAPPER_MVP_NOC_LPI_CONTROL, 0x1);
+	rc = __write_register(core, AON_WRAPPER_MVP_NOC_LPI_CONTROL, 0x1);
+	if (rc)
+		return rc;
+
 	while (!reg_status && count < max_count) {
 		lpi_status =
 			 __read_register(core,
@@ -240,8 +272,10 @@ static int __power_off_iris2(struct msm_vidc_core *vidc_core)
 
 //skip_aon_mvp_noc:
 	/* HPG 6.1.2 Step 3, debug bridge to low power */
-	__write_register(core,
-		WRAPPER_DEBUG_BRIDGE_LPI_CONTROL_IRIS2, 0x7);
+	rc = __write_register(core, WRAPPER_DEBUG_BRIDGE_LPI_CONTROL_IRIS2, 0x7);
+	if (rc)
+		return rc;
+
 	reg_status = 0;
 	count = 0;
 	while ((reg_status != 0x7) && count < max_count) {
@@ -257,8 +291,10 @@ static int __power_off_iris2(struct msm_vidc_core *vidc_core)
 		d_vpr_e("DBLP Set: status %d\n", reg_status);
 
 	/* HPG 6.1.2 Step 4, debug bridge to lpi release */
-	__write_register(core,
-		WRAPPER_DEBUG_BRIDGE_LPI_CONTROL_IRIS2, 0x0);
+	rc = __write_register(core, WRAPPER_DEBUG_BRIDGE_LPI_CONTROL_IRIS2, 0x0);
+	if (rc)
+		return rc;
+
 	lpi_status = 0x1;
 	count = 0;
 	while (lpi_status && count < max_count) {
@@ -348,14 +384,17 @@ skip_power_off:
 static int __raise_interrupt_iris2(struct msm_vidc_core *vidc_core)
 {
 	struct msm_vidc_core *core = vidc_core;
+	int rc = 0;
 
 	if (!core) {
 		d_vpr_e("%s: invalid params\n", __func__);
 		return -EINVAL;
 	}
 
-	__write_register(core, CPU_IC_SOFTINT_IRIS2,
-				1 << CPU_IC_SOFTINT_H2A_SHFT_IRIS2);
+	rc = __write_register(core, CPU_IC_SOFTINT_IRIS2, 1 << CPU_IC_SOFTINT_H2A_SHFT_IRIS2);
+	if (rc)
+		return rc;
+
 	return 0;
 }
 
@@ -419,8 +458,9 @@ static int __noc_error_info_iris2(struct msm_vidc_core *vidc_core)
 
 static int __clear_interrupt_iris2(struct msm_vidc_core *vidc_core)
 {
-	u32 intr_status = 0, mask = 0;
 	struct msm_vidc_core *core = vidc_core;
+	u32 intr_status = 0, mask = 0;
+	int rc = 0;
 
 	if (!core) {
 		d_vpr_e("%s: NULL core\n", __func__);
@@ -441,7 +481,9 @@ static int __clear_interrupt_iris2(struct msm_vidc_core *vidc_core)
 		core->spur_count++;
 	}
 
-	__write_register(core, CPU_CS_A2HSOFTINTCLR_IRIS2, 1);
+	rc = __write_register(core, CPU_CS_A2HSOFTINTCLR_IRIS2, 1);
+	if (rc)
+		return rc;
 
 	return 0;
 }
@@ -459,7 +501,10 @@ static int __boot_firmware_iris2(struct msm_vidc_core *vidc_core)
 
 	ctrl_init_val = BIT(0);
 
-	__write_register(core, CTRL_INIT_IRIS2, ctrl_init_val);
+	rc = __write_register(core, CTRL_INIT_IRIS2, ctrl_init_val);
+	if (rc)
+		return rc;
+
 	while (!ctrl_status && count < max_tries) {
 		ctrl_status = __read_register(core, CTRL_STATUS_IRIS2);
 		if ((ctrl_status & CTRL_ERROR_STATUS__M_IRIS2) == 0x4) {
@@ -477,8 +522,13 @@ static int __boot_firmware_iris2(struct msm_vidc_core *vidc_core)
 	}
 
 	/* Enable interrupt before sending commands to venus */
-	__write_register(core, CPU_CS_H2XSOFTINTEN_IRIS2, 0x1);
-	__write_register(core, CPU_CS_X2RPMh_IRIS2, 0x0);
+	rc = __write_register(core, CPU_CS_H2XSOFTINTEN_IRIS2, 0x1);
+	if (rc)
+		return rc;
+
+	rc = __write_register(core, CPU_CS_X2RPMh_IRIS2, 0x0);
+	if (rc)
+		return rc;
 
 	return rc;
 }

driver/vidc/inc/msm_vidc_debug.h

@@ -115,11 +115,6 @@ enum vidc_msg_prio {
 		} \
 	} while (0)
 
-#define MSM_VIDC_ERROR(value)					\
-	do {	if (value)					\
-			d_vpr_e("BugOn");		\
-	} while (0)
-
 enum msm_vidc_debugfs_event {
 	MSM_VIDC_DEBUGFS_EVENT_ETB,
 	MSM_VIDC_DEBUGFS_EVENT_EBD,

driver/vidc/inc/msm_vidc_internal.h

@@ -317,7 +317,6 @@ enum msm_vidc_core_capability_type {
 	FW_UNLOAD,
 	FW_UNLOAD_DELAY,
 	HW_RESPONSE_TIMEOUT,
-	DEBUG_TIMEOUT,
 	PREFIX_BUF_COUNT_PIX,
 	PREFIX_BUF_SIZE_PIX,
 	PREFIX_BUF_COUNT_NON_PIX,

driver/vidc/inc/venus_hfi.h

@@ -57,7 +57,7 @@ void venus_hfi_work_handler(struct work_struct *work);
 void venus_hfi_pm_work_handler(struct work_struct *work);
 irqreturn_t venus_hfi_isr(int irq, void *data);
 
-void __write_register(struct msm_vidc_core *core,
+int __write_register(struct msm_vidc_core *core,
 		u32 reg, u32 value);
 int __read_register(struct msm_vidc_core *core, u32 reg);
 int __iface_cmdq_write(struct msm_vidc_core *core,

driver/vidc/src/msm_vidc_driver.c

@@ -340,6 +340,23 @@ void print_vb2_buffer(const char *str, struct msm_vidc_inst *inst,
 	}
 }
 
+static void __fatal_error(bool fatal)
+{
+	WARN_ON(fatal);
+}
+
+static int __strict_check(struct msm_vidc_core *core, const char *function)
+{
+	bool fatal = !mutex_is_locked(&core->lock);
+
+	__fatal_error(fatal);
+
+	if (fatal)
+		d_vpr_e("%s: strict check failed\n", function);
+
+	return fatal ? -EINVAL : 0;
+}
+
 enum msm_vidc_buffer_type v4l2_type_to_driver(u32 type, const char *func)
 {
 	enum msm_vidc_buffer_type buffer_type = 0;
@@ -3996,18 +4013,6 @@ unlock:
 	return rc;
 }
 
-static void __fatal_error(struct msm_vidc_core *core, bool fatal)
-{
-	return;
-	fatal &= core->capabilities[HW_RESPONSE_TIMEOUT].value;
-	MSM_VIDC_ERROR(fatal);
-}
-
-static void __strict_check(struct msm_vidc_core *core)
-{
-	__fatal_error(core, !mutex_is_locked(&core->lock));
-}
-
 static int msm_vidc_core_init_wait(struct msm_vidc_core *core)
 {
 	const int interval = 40;
@@ -4018,7 +4023,9 @@ static int msm_vidc_core_init_wait(struct msm_vidc_core *core)
 		return -EINVAL;
 	}
 
-	__strict_check(core);
+	rc = __strict_check(core, __func__);
+	if (rc)
+		return rc;
 
 	if (core->state != MSM_VIDC_CORE_INIT_WAIT)
 		return 0;

driver/vidc/src/msm_vidc_probe.c

@@ -478,7 +478,7 @@ static int msm_vidc_probe(struct platform_device *pdev)
 	}
 
 	/* How did we end up here? */
-	MSM_VIDC_ERROR(1);
+	WARN_ON(1);
 	return -EINVAL;
 }
 

driver/vidc/src/venus_hfi.c

@@ -175,16 +175,21 @@ static void __dump_packet(u8 *packet, const char *function, void *qinfo)
 	}
 }
 
-static void __fatal_error(struct msm_vidc_core *core, bool fatal)
+static void __fatal_error(bool fatal)
 {
-	return;
-	fatal &= core->capabilities[HW_RESPONSE_TIMEOUT].value;
-	MSM_VIDC_ERROR(fatal);
+	WARN_ON(fatal);
 }
 
-static void __strict_check(struct msm_vidc_core *core)
+static int __strict_check(struct msm_vidc_core *core, const char *function)
 {
-	__fatal_error(core, !mutex_is_locked(&core->lock));
+	bool fatal = !mutex_is_locked(&core->lock);
+
+	__fatal_error(fatal);
+
+	if (fatal)
+		d_vpr_e("%s: strict check failed\n", function);
+
+	return fatal ? -EINVAL : 0;
 }
 
 bool __core_in_valid_state(struct msm_vidc_core *core)
@@ -202,11 +207,14 @@ static bool __valdiate_session(struct msm_vidc_core *core,
 {
 	bool valid = false;
 	struct msm_vidc_inst *temp;
+	int rc = 0;
 
 	if (!core || !inst)
 		return false;
 
-	__strict_check(core);
+	rc = __strict_check(core, __func__);
+	if (rc)
+		return false;
 
 	list_for_each_entry(temp, &core->instances, list) {
 		if (temp == inst) {
@@ -220,23 +228,25 @@ static bool __valdiate_session(struct msm_vidc_core *core,
 	return valid;
 }
 
-void __write_register(struct msm_vidc_core *core,
+int __write_register(struct msm_vidc_core *core,
 		u32 reg, u32 value)
 {
 	u32 hwiosymaddr = reg;
 	u8 *base_addr;
+	int rc = 0;
 
 	if (!core) {
 		d_vpr_e("%s: invalid params\n", __func__);
-		return;
+		return -EINVAL;
 	}
 
-	__strict_check(core);
+	rc = __strict_check(core, __func__);
+	if (rc)
+		return rc;
 
 	if (!core->power_enabled) {
 		d_vpr_e("HFI Write register failed : Power is OFF\n");
-		__fatal_error(core, true);
-		return;
+		return -EINVAL;
 	}
 
 	base_addr = core->register_base_addr;
@@ -249,6 +259,8 @@ void __write_register(struct msm_vidc_core *core,
 	 * Memory barrier to make sure value is written into the register.
 	 */
 	wmb();
+
+	return rc;
 }
 
 /*
@@ -256,24 +268,26 @@ void __write_register(struct msm_vidc_core *core,
  * only bits 0 & 4 will be updated with corresponding bits from value. To update
  * entire register with value, set mask = 0xFFFFFFFF.
  */
-void __write_register_masked(struct msm_vidc_core *core,
+static int __write_register_masked(struct msm_vidc_core *core,
 		u32 reg, u32 value, u32 mask)
 {
 	u32 prev_val, new_val;
 	u8 *base_addr;
+	int rc = 0;
 
 	if (!core) {
 		d_vpr_e("%s: invalid params\n", __func__);
-		return;
+		return -EINVAL;
 	}
 
-	__strict_check(core);
+	rc = __strict_check(core, __func__);
+	if (rc)
+		return rc;
 
 	if (!core->power_enabled) {
 		d_vpr_e("%s: register write failed, power is off\n",
 			__func__);
-		__fatal_error(core, true);
-		return;
+		return -EINVAL;
 	}
 
 	base_addr = core->register_base_addr;
@@ -294,6 +308,8 @@ void __write_register_masked(struct msm_vidc_core *core,
 	 * Memory barrier to make sure value is written into the register.
 	 */
 	wmb();
+
+	return rc;
 }
 
 int __read_register(struct msm_vidc_core *core, u32 reg)
@@ -306,11 +322,8 @@ int __read_register(struct msm_vidc_core *core, u32 reg)
 		return -EINVAL;
 	}
 
-	__strict_check(core);
-
 	if (!core->power_enabled) {
 		d_vpr_e("HFI Read register failed : Power is OFF\n");
-		__fatal_error(core, true);
 		return -EINVAL;
 	}
 
@@ -401,7 +414,7 @@ static int __acquire_regulator(struct msm_vidc_core *core,
 		if (!regulator_is_enabled(rinfo->regulator)) {
 			d_vpr_e("%s: Regulator is not enabled %s\n",
 				__func__, rinfo->name);
-			__fatal_error(core, true);
+			__fatal_error(true);
 		}
 	}
 
@@ -436,7 +449,7 @@ static int __hand_off_regulator(struct msm_vidc_core *core,
 		if (!regulator_is_enabled(rinfo->regulator)) {
 			d_vpr_e("%s: Regulator is not enabled %s\n",
 				__func__, rinfo->name);
-			__fatal_error(core, true);
+			__fatal_error(true);
 		}
 	}
 
@@ -467,22 +480,26 @@ err_reg_handoff_failed:
 	return rc;
 }
 
-static void __set_registers(struct msm_vidc_core *core)
+static int __set_registers(struct msm_vidc_core *core)
 {
 	struct reg_set *reg_set;
-	int i;
+	int i, rc = 0;
 
 	if (!core || !core->dt) {
 		d_vpr_e("core resources null, cannot set registers\n");
-		return;
+		return -EINVAL;
 	}
 
 	reg_set = &core->dt->reg_set;
 	for (i = 0; i < reg_set->count; i++) {
-		__write_register_masked(core, reg_set->reg_tbl[i].reg,
+		rc = __write_register_masked(core, reg_set->reg_tbl[i].reg,
 				reg_set->reg_tbl[i].value,
 				reg_set->reg_tbl[i].mask);
+		if (rc)
+			return rc;
 	}
+
+	return rc;
 }
 
 static int __vote_bandwidth(struct bus_info *bus,
@@ -864,7 +881,7 @@ static int __iface_cmdq_write_relaxed(struct msm_vidc_core *core,
 {
 	struct msm_vidc_iface_q_info *q_info;
 	//struct vidc_hal_cmd_pkt_hdr *cmd_packet;
-	int result = -E2BIG;
+	int rc = -E2BIG;
 
 	if (!core || !pkt) {
 		d_vpr_e("%s: invalid params %pK %pK\n",
@@ -872,11 +889,13 @@ static int __iface_cmdq_write_relaxed(struct msm_vidc_core *core,
 		return -EINVAL;
 	}
 
-	__strict_check(core);
+	rc = __strict_check(core, __func__);
+	if (rc)
+		return rc;
 
 	if (!__core_in_valid_state(core)) {
 		d_vpr_e("%s: fw not in init state\n", __func__);
-		result = -EINVAL;
+		rc = -EINVAL;
 		goto err_q_null;
 	}
 
@@ -891,7 +910,7 @@ static int __iface_cmdq_write_relaxed(struct msm_vidc_core *core,
 
 	if (!q_info->q_array.align_virtual_addr) {
 		d_vpr_e("cannot write to shared CMD Q's\n");
-		result = -ENODATA;
+		rc = -ENODATA;
 		goto err_q_null;
 	}
 
@@ -902,14 +921,14 @@ static int __iface_cmdq_write_relaxed(struct msm_vidc_core *core,
 
 	if (!__write_queue(q_info, (u8 *)pkt, requires_interrupt)) {
 		__schedule_power_collapse_work(core);
-		result = 0;
+		rc = 0;
 	} else {
 		d_vpr_e("__iface_cmdq_write: queue full\n");
 	}
 
 err_q_write:
 err_q_null:
-	return result;
+	return rc;
 }
 
 int __iface_cmdq_write(struct msm_vidc_core *core,
@@ -947,8 +966,6 @@ int __iface_msgq_read(struct msm_vidc_core *core, void *pkt)
 		return -EINVAL;
 	}
 
-	__strict_check(core);
-
 	if (!__core_in_valid_state(core)) {
 		d_vpr_e("%s: fw not in init state\n", __func__);
 		rc = -EINVAL;
@@ -989,8 +1006,6 @@ int __iface_dbgq_read(struct msm_vidc_core *core, void *pkt)
 		return -EINVAL;
 	}
 
-	__strict_check(core);
-
 	q_info = &core->iface_queues[VIDC_IFACEQ_DBGQ_IDX];
 	if (!q_info->q_array.align_virtual_addr) {
 		d_vpr_e("cannot read from shared DBG Q's\n");
@@ -1840,7 +1855,7 @@ static int __disable_regulator(struct regulator_info *rinfo,
 disable_regulator_failed:
 
 	/* Bring attention to this issue */
-	__fatal_error(core, true);
+	__fatal_error(true);
 	return rc;
 }
 
@@ -2010,7 +2025,7 @@ static int __enable_subcaches(struct msm_vidc_core *core)
 		if (rc) {
 			d_vpr_e("Failed to activate %s: %d\n",
 				sinfo->name, rc);
-			__fatal_error(core, true);
+			__fatal_error(true);
 			goto err_activate_fail;
 		}
 		sinfo->isactive = true;
@@ -2209,6 +2224,10 @@ static int __suspend(struct msm_vidc_core *core)
 		return 0;
 	}
 
+	rc = __strict_check(core, __func__);
+	if (rc)
+		return rc;
+
 	d_vpr_h("Entering suspend\n");
 
 	rc = __tzbsp_set_video_state(TZBSP_VIDEO_STATE_SUSPEND);
@@ -2241,6 +2260,10 @@ static int __resume(struct msm_vidc_core *core)
 		return -EINVAL;
 	}
 
+	rc = __strict_check(core, __func__);
+	if (rc)
+		return rc;
+
 	d_vpr_h("Resuming from power collapse\n");
 	rc = __venus_power_on(core);
 	if (rc) {
@@ -2698,6 +2721,7 @@ void venus_hfi_work_handler(struct work_struct *work)
 	num_responses = __response_handler(core);
 
 err_no_work:
+
 	if (!call_venus_op(core, watchdog, core, core->intr_status))
 		enable_irq(core->dt->irq);
 }
@@ -2792,7 +2816,9 @@ int venus_hfi_core_init(struct msm_vidc_core *core)
 	}
 	d_vpr_h("%s(): core %pK\n", __func__, core);
 
-	__strict_check(core);
+	rc = __strict_check(core, __func__);
+	if (rc)
+		return rc;
 
 	core->handoff_done = 0;
 
@@ -2840,12 +2866,17 @@ error:
 
 int venus_hfi_core_deinit(struct msm_vidc_core *core)
 {
+	int rc = 0;
+
 	if (!core) {
 		d_vpr_h("%s(): invalid params\n", __func__);
 		return -EINVAL;
 	}
 	d_vpr_h("%s(): core %pK\n", __func__, core);
-	__strict_check(core);
+	rc = __strict_check(core, __func__);
+	if (rc)
+		return rc;
+
 	if (core->state == MSM_VIDC_CORE_DEINIT)
 		return 0;
 	__resume(core);
@@ -2917,6 +2948,7 @@ int venus_hfi_trigger_ssr(struct msm_vidc_core *core, u32 type,
 		return -EINVAL;
 	}
 
+	core_lock(core, __func__);
 	payload[0] = client_id << 4 | type;
 	payload[1] = addr;
 
@@ -2924,7 +2956,7 @@ int venus_hfi_trigger_ssr(struct msm_vidc_core *core, u32 type,
 			   0 /*session_id*/,
 			   core->header_id++);
 	if (rc)
-		goto err_ssr_pkt;
+		goto unlock;
 
 	/* HFI_CMD_SSR */
 	rc = hfi_create_packet(core->packet, core->packet_size,
@@ -2936,16 +2968,17 @@ int venus_hfi_trigger_ssr(struct msm_vidc_core *core, u32 type,
 				   core->packet_id++,
 				   &payload, sizeof(u64));
 	if (rc)
-		goto err_ssr_pkt;
+		goto unlock;
 
 	rc = __iface_cmdq_write(core, core->packet);
 	if (rc)
-		return rc;
+		goto unlock;
 
-	return 0;
+unlock:
+	core_unlock(core, __func__);
+	if (rc)
+		d_vpr_e("%s(): failed\n", __func__);
 
-err_ssr_pkt:
-	d_vpr_e("%s: create packet failed\n", __func__);
 	return rc;
 }
 
@@ -2987,7 +3020,7 @@ int venus_hfi_session_open(struct msm_vidc_inst *inst)
 
 unlock:
 	core_unlock(core, __func__);
-	return 0;
+	return rc;
 }
 
 int venus_hfi_session_set_codec(struct msm_vidc_inst *inst)
@@ -3087,11 +3120,12 @@ int venus_hfi_session_close(struct msm_vidc_inst *inst)
 		return -EINVAL;
 	}
 	core = inst->core;
+	core_lock(core, __func__);
 
-	__strict_check(core);
-
-	if (!__valdiate_session(core, inst, __func__))
-		return -EINVAL;
+	if (!__valdiate_session(core, inst, __func__)) {
+		rc = -EINVAL;
+		goto unlock;
+	}
 
 	rc = hfi_packet_session_command(inst,
 				HFI_CMD_CLOSE,
@@ -3103,9 +3137,15 @@ int venus_hfi_session_close(struct msm_vidc_inst *inst)
 				HFI_PAYLOAD_NONE,
 				NULL,
 				0);
-	if (!rc)
-		rc = __iface_cmdq_write(inst->core, inst->packet);
+	if (rc)
+		goto unlock;
+
+	rc = __iface_cmdq_write(inst->core, inst->packet);
+	if (rc)
+		goto unlock;
 
+unlock:
+	core_unlock(core, __func__);
 	return rc;
 }