Re: [PATCH v1 2/5] bus: mhi: core: Handle disable transitions in state worker

[Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>]

On Mon, May 11, 2020 at 07:03:06PM -0700, Hemant Kumar wrote:
> Mission mode transition is handled by state worker thread but
> power off is not. There is a possibility while mission mode
> transition is in progress which calls MHI client driver probe,
> power off is issued by MHI controller. This results into client
> driver probe and remove running in parallel and causes use after
> free situation. By queuing disable transition work when mission
> mode is in progress prevents the race condition.
> 
> Signed-off-by: Hemant Kumar <hemantk@codeaurora.org>
> Reviewed-by: Jeffrey Hugo <jhugo@codeaurora.org>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Thanks,
Mani

> ---
>  drivers/bus/mhi/core/init.c     |  1 +
>  drivers/bus/mhi/core/internal.h |  1 +
>  drivers/bus/mhi/core/pm.c       | 11 ++++++++++-
>  3 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
> index 3a853c5..12207cc 100644
> --- a/drivers/bus/mhi/core/init.c
> +++ b/drivers/bus/mhi/core/init.c
> @@ -35,6 +35,7 @@
>  	[DEV_ST_TRANSITION_SBL] = "SBL",
>  	[DEV_ST_TRANSITION_MISSION_MODE] = "MISSION_MODE",
>  	[DEV_ST_TRANSITION_SYS_ERR] = "SYS_ERR",
> +	[DEV_ST_TRANSITION_DISABLE] = "DISABLE",
>  };
>  
>  const char * const mhi_state_str[MHI_STATE_MAX] = {
> diff --git a/drivers/bus/mhi/core/internal.h b/drivers/bus/mhi/core/internal.h
> index f01283b..b1f640b 100644
> --- a/drivers/bus/mhi/core/internal.h
> +++ b/drivers/bus/mhi/core/internal.h
> @@ -387,6 +387,7 @@ enum dev_st_transition {
>  	DEV_ST_TRANSITION_SBL,
>  	DEV_ST_TRANSITION_MISSION_MODE,
>  	DEV_ST_TRANSITION_SYS_ERR,
> +	DEV_ST_TRANSITION_DISABLE,
>  	DEV_ST_TRANSITION_MAX,
>  };
>  
> diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
> index d9964d4..345f197 100644
> --- a/drivers/bus/mhi/core/pm.c
> +++ b/drivers/bus/mhi/core/pm.c
> @@ -657,6 +657,10 @@ void mhi_pm_st_worker(struct work_struct *work)
>  			mhi_pm_disable_transition
>  				(mhi_cntrl, MHI_PM_SYS_ERR_PROCESS);
>  			break;
> +		case DEV_ST_TRANSITION_DISABLE:
> +			mhi_pm_disable_transition
> +				(mhi_cntrl, MHI_PM_SHUTDOWN_PROCESS);
> +			break;
>  		default:
>  			break;
>  		}
> @@ -1011,7 +1015,12 @@ void mhi_power_down(struct mhi_controller *mhi_cntrl, bool graceful)
>  				to_mhi_pm_state_str(MHI_PM_LD_ERR_FATAL_DETECT),
>  				to_mhi_pm_state_str(mhi_cntrl->pm_state));
>  	}
> -	mhi_pm_disable_transition(mhi_cntrl, MHI_PM_SHUTDOWN_PROCESS);
> +
> +	mhi_queue_state_transition(mhi_cntrl, DEV_ST_TRANSITION_DISABLE);
> +
> +	/* Wait for shutdown to complete */
> +	flush_work(&mhi_cntrl->st_worker);
> +
>  	mhi_deinit_free_irq(mhi_cntrl);
>  
>  	if (!mhi_cntrl->pre_init) {
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project