Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 16 Nov 2020 18:13:32 +0530
From:   Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
To:     Bhaumik Bhatt <bbhatt@codeaurora.org>
Cc:     linux-arm-msm@vger.kernel.org, hemantk@codeaurora.org,
        jhugo@codeaurora.org, loic.poulain@linaro.org,
        kvalo@codeaurora.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 3/6] bus: mhi: core: Add support to stop or start
 channel data transfers
Message-ID: <20201116124332.GK3926@Mani-XPS-13-9360>
References: <1605122473-12179-1-git-send-email-bbhatt@codeaurora.org>
 <1605122473-12179-4-git-send-email-bbhatt@codeaurora.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1605122473-12179-4-git-send-email-bbhatt@codeaurora.org>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk

On Wed, Nov 11, 2020 at 11:21:10AM -0800, Bhaumik Bhatt wrote:
> Some MHI client drivers may want to request a pause or halt of
> data transfer activity on their channels. Support for this does
> not exist and must be introduced, wherein the channel context is
> not reset or cleared but only the STOP channel command is issued.
> This would need to be paired with an API that allows resuming the
> data transfer activity on channels by use of the START channel
> command. This API assumes that the context information is already
> setup. Enable this using two new APIs, mhi_start_transfer() and
> mhi_stop_transfer().
> 
> Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
> ---
>  drivers/bus/mhi/core/main.c | 41 +++++++++++++++++++++++++++++++++++++++++
>  include/linux/mhi.h         | 19 +++++++++++++++++++
>  2 files changed, 60 insertions(+)
> 
> diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
> index 1226933..1a969f4 100644
> --- a/drivers/bus/mhi/core/main.c
> +++ b/drivers/bus/mhi/core/main.c
> @@ -1560,6 +1560,47 @@ void mhi_unprepare_from_transfer(struct mhi_device *mhi_dev)
>  }
>  EXPORT_SYMBOL_GPL(mhi_unprepare_from_transfer);
>  
> +static int mhi_update_transfer_state(struct mhi_device *mhi_dev,
> +				     enum mhi_ch_state_type to_state)
> +{
> +	struct mhi_controller *mhi_cntrl = mhi_dev->mhi_cntrl;
> +	struct mhi_chan *mhi_chan;
> +	int dir, ret;
> +
> +	for (dir = 0; dir < 2; dir++) {
> +		mhi_chan = dir ? mhi_dev->ul_chan : mhi_dev->dl_chan;
> +
> +		if (!mhi_chan)
> +			continue;
> +
> +		/*
> +		 * Bail out if one of the channels fail as client will reset
> +		 * both upon failure
> +		 */
> +		mutex_lock(&mhi_chan->mutex);

Hmm. The documentation about wait_for_completion*() used in
mhi_update_channel_state()says below,

"As all variants of wait_for_completion() can (obviously) block for a long
time depending on the nature of the activity they are waiting for, so in
most cases you probably don't want to call this with held mutexes."

> +		ret = mhi_update_channel_state(mhi_cntrl, mhi_chan, to_state);
> +		if (ret) {
> +			mutex_unlock(&mhi_chan->mutex);
> +			return ret;
> +		}
> +		mutex_unlock(&mhi_chan->mutex);
> +	}
> +
> +	return 0;
> +}
> +
> +int mhi_stop_transfer(struct mhi_device *mhi_dev)
> +{
> +	return mhi_update_transfer_state(mhi_dev, MHI_CH_STATE_TYPE_STOP);
> +}
> +EXPORT_SYMBOL_GPL(mhi_stop_transfer);
> +
> +int mhi_start_transfer(struct mhi_device *mhi_dev)
> +{
> +	return mhi_update_transfer_state(mhi_dev, MHI_CH_STATE_TYPE_START);
> +}
> +EXPORT_SYMBOL_GPL(mhi_start_transfer);
> +
>  int mhi_poll(struct mhi_device *mhi_dev, u32 budget)
>  {
>  	struct mhi_controller *mhi_cntrl = mhi_dev->mhi_cntrl;
> diff --git a/include/linux/mhi.h b/include/linux/mhi.h
> index 52b3c60..aee8494 100644
> --- a/include/linux/mhi.h
> +++ b/include/linux/mhi.h
> @@ -702,6 +702,25 @@ int mhi_prepare_for_transfer(struct mhi_device *mhi_dev);
>  void mhi_unprepare_from_transfer(struct mhi_device *mhi_dev);
>  
>  /**
> + * mhi_stop_transfer - Pauses ongoing channel activity by issuing the STOP
> + *                     channel command to both UL and DL channels. This command
> + *                     does not reset the channel context and the client drivers
> + *                     can issue mhi_start_transfer to resume activity.
> + * @mhi_dev: Device associated with the channels
> + */
> +int mhi_stop_transfer(struct mhi_device *mhi_dev);
> +
> +/**
> + * mhi_start_transfer - Resumes channel activity by issuing the START channel
> + *                      command to both UL and DL channels. This command assumes
> + *                      the channel context is already setup and the client
> + *                      drivers can issue mhi_stop_transfer to pause activity if
> + *                      required.
> + * @mhi_dev: Device associated with the channels
> + */
> +int mhi_start_transfer(struct mhi_device *mhi_dev);
> +
> +/**

Align the comment header properly.

Thanks,
Mani

>   * mhi_poll - Poll for any available data in DL direction
>   * @mhi_dev: Device associated with the channels
>   * @budget: # of events to process
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>