Re: [PATCH v7 5/8] bus: mhi: core: Handle firmware load using state worker

[Manivannan Sadhasivam]

On Fri, May 08, 2020 at 07:26:45PM -0700, Bhaumik Bhatt wrote:
> Upon power up, driver queues firmware worker thread if the execution
> environment is PBL. Firmware worker is blocked with a timeout until
> state worker gets a chance to run and unblock firmware worker. An
> endpoint power up failure can be seen if state worker gets a chance to
> run after firmware worker has timed out. Remove this dependency and
> handle firmware load directly using state worker thread.
> 
> Signed-off-by: Bhaumik Bhatt <bbhatt@xxxxxxxxxxxxxx>
> Reviewed-by: Jeffrey Hugo <jhugo@xxxxxxxxxxxxxx>

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

Thanks,
Mani

> ---
>  drivers/bus/mhi/core/boot.c     | 18 +++---------------
>  drivers/bus/mhi/core/init.c     |  1 -
>  drivers/bus/mhi/core/internal.h |  1 +
>  drivers/bus/mhi/core/pm.c       |  6 +-----
>  include/linux/mhi.h             |  2 --
>  5 files changed, 5 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/bus/mhi/core/boot.c b/drivers/bus/mhi/core/boot.c
> index ebad5eb..17c636b 100644
> --- a/drivers/bus/mhi/core/boot.c
> +++ b/drivers/bus/mhi/core/boot.c
> @@ -377,30 +377,18 @@ static void mhi_firmware_copy(struct mhi_controller *mhi_cntrl,
>  	}
>  }
>  
> -void mhi_fw_load_worker(struct work_struct *work)
> +void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl)
>  {
> -	struct mhi_controller *mhi_cntrl;
>  	const struct firmware *firmware = NULL;
>  	struct image_info *image_info;
> -	struct device *dev;
> +	struct device *dev = &mhi_cntrl->mhi_dev->dev;
>  	const char *fw_name;
>  	void *buf;
>  	dma_addr_t dma_addr;
>  	size_t size;
>  	int ret;
>  
> -	mhi_cntrl = container_of(work, struct mhi_controller, fw_worker);
> -	dev = &mhi_cntrl->mhi_dev->dev;
> -
> -	dev_dbg(dev, "Waiting for device to enter PBL from: %s\n",
> -		TO_MHI_EXEC_STR(mhi_cntrl->ee));
> -
> -	ret = wait_event_timeout(mhi_cntrl->state_event,
> -				 MHI_IN_PBL(mhi_cntrl->ee) ||
> -				 MHI_PM_IN_ERROR_STATE(mhi_cntrl->pm_state),
> -				 msecs_to_jiffies(mhi_cntrl->timeout_ms));
> -
> -	if (!ret || MHI_PM_IN_ERROR_STATE(mhi_cntrl->pm_state)) {
> +	if (MHI_PM_IN_ERROR_STATE(mhi_cntrl->pm_state)) {
>  		dev_err(dev, "Device MHI is not in valid state\n");
>  		return;
>  	}
> diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
> index 1a93d24..6882206 100644
> --- a/drivers/bus/mhi/core/init.c
> +++ b/drivers/bus/mhi/core/init.c
> @@ -835,7 +835,6 @@ int mhi_register_controller(struct mhi_controller *mhi_cntrl,
>  	spin_lock_init(&mhi_cntrl->wlock);
>  	INIT_WORK(&mhi_cntrl->st_worker, mhi_pm_st_worker);
>  	INIT_WORK(&mhi_cntrl->syserr_worker, mhi_pm_sys_err_worker);
> -	INIT_WORK(&mhi_cntrl->fw_worker, mhi_fw_load_worker);
>  	init_waitqueue_head(&mhi_cntrl->state_event);
>  
>  	mhi_cmd = mhi_cntrl->mhi_cmd;
> diff --git a/drivers/bus/mhi/core/internal.h b/drivers/bus/mhi/core/internal.h
> index 40c47f9..0965ca3 100644
> --- a/drivers/bus/mhi/core/internal.h
> +++ b/drivers/bus/mhi/core/internal.h
> @@ -627,6 +627,7 @@ void mhi_ring_chan_db(struct mhi_controller *mhi_cntrl,
>  void mhi_deinit_free_irq(struct mhi_controller *mhi_cntrl);
>  void mhi_rddm_prepare(struct mhi_controller *mhi_cntrl,
>  		      struct image_info *img_info);
> +void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl);
>  int mhi_prepare_channel(struct mhi_controller *mhi_cntrl,
>  			struct mhi_chan *mhi_chan);
>  int mhi_init_chan_ctxt(struct mhi_controller *mhi_cntrl,
> diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
> index e7c8318..3cc238a 100644
> --- a/drivers/bus/mhi/core/pm.c
> +++ b/drivers/bus/mhi/core/pm.c
> @@ -528,7 +528,6 @@ static void mhi_pm_disable_transition(struct mhi_controller *mhi_cntrl,
>  	dev_dbg(dev, "Waiting for all pending threads to complete\n");
>  	wake_up_all(&mhi_cntrl->state_event);
>  	flush_work(&mhi_cntrl->st_worker);
> -	flush_work(&mhi_cntrl->fw_worker);
>  
>  	dev_dbg(dev, "Reset all active channels and remove MHI devices\n");
>  	device_for_each_child(mhi_cntrl->cntrl_dev, NULL, mhi_destroy_device);
> @@ -643,7 +642,7 @@ void mhi_pm_st_worker(struct work_struct *work)
>  				mhi_cntrl->ee = mhi_get_exec_env(mhi_cntrl);
>  			write_unlock_irq(&mhi_cntrl->pm_lock);
>  			if (MHI_IN_PBL(mhi_cntrl->ee))
> -				wake_up_all(&mhi_cntrl->state_event);
> +				mhi_fw_load_handler(mhi_cntrl);
>  			break;
>  		case DEV_ST_TRANSITION_SBL:
>  			write_lock_irq(&mhi_cntrl->pm_lock);
> @@ -976,9 +975,6 @@ int mhi_async_power_up(struct mhi_controller *mhi_cntrl)
>  	next_state = MHI_IN_PBL(current_ee) ?
>  		DEV_ST_TRANSITION_PBL : DEV_ST_TRANSITION_READY;
>  
> -	if (next_state == DEV_ST_TRANSITION_PBL)
> -		schedule_work(&mhi_cntrl->fw_worker);
> -
>  	mhi_queue_state_transition(mhi_cntrl, next_state);
>  
>  	mutex_unlock(&mhi_cntrl->pm_mutex);
> diff --git a/include/linux/mhi.h b/include/linux/mhi.h
> index e909b8f..2b20b9c 100644
> --- a/include/linux/mhi.h
> +++ b/include/linux/mhi.h
> @@ -331,7 +331,6 @@ struct mhi_controller_config {
>   * @wlock: Lock for protecting device wakeup
>   * @mhi_link_info: Device bandwidth info
>   * @st_worker: State transition worker
> - * @fw_worker: Firmware download worker
>   * @syserr_worker: System error worker
>   * @state_event: State change event
>   * @status_cb: CB function to notify power states of the device (required)
> @@ -412,7 +411,6 @@ struct mhi_controller {
>  	spinlock_t wlock;
>  	struct mhi_link_info mhi_link_info;
>  	struct work_struct st_worker;
> -	struct work_struct fw_worker;
>  	struct work_struct syserr_worker;
>  	wait_queue_head_t state_event;
>  
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project