Received-SPF: pass (google.com: domain of linux-wireless+bounces-2863-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <d002afe9-3c01-413c-82d2-353db0356802@quicinc.com>
Date: Wed, 31 Jan 2024 15:39:26 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH RFC v2 2/8] bus: mhi: host: add new interfaces to handle
 MHI channels directly
Content-Language: en-US
To: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>,
        Kalle Valo
	<kvalo@kernel.org>
CC: <mhi@lists.linux.dev>, <ath11k@lists.infradead.org>,
        <linux-wireless@vger.kernel.org>
References: <20231127162022.518834-1-kvalo@kernel.org>
 <20231127162022.518834-3-kvalo@kernel.org> <20240130181938.GB4218@thinkpad>
From: Baochen Qiang <quic_bqiang@quicinc.com>
In-Reply-To: <20240130181938.GB4218@thinkpad>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit


On 1/31/2024 2:19 AM, Manivannan Sadhasivam wrote:
> On Mon, Nov 27, 2023 at 06:20:16PM +0200, Kalle Valo wrote:
>> From: Baochen Qiang <quic_bqiang@quicinc.com>
>>
>> When using mhi_power_down_no_destroy() MHI hosts need to unprepare MHI channels
>> by themselves.  Similarly, MHI stack will also not create new MHI device since
>> old devices were not destroyed, so MHI hosts need to prepare channels as well.
>> Hence add these two interfaces to make that possible.
>>
>> Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3.6510.30
>>
>> Signed-off-by: Baochen Qiang <quic_bqiang@quicinc.com>
>> Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>
>> ---
>>   drivers/bus/mhi/host/main.c | 107 ++++++++++++++++++++++++++++++++++++
>>   include/linux/mhi.h         |  20 ++++++-
>>   2 files changed, 126 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/bus/mhi/host/main.c b/drivers/bus/mhi/host/main.c
>> index d80975f4bba8..3f677fc628ad 100644
>> --- a/drivers/bus/mhi/host/main.c
>> +++ b/drivers/bus/mhi/host/main.c
>> @@ -1669,6 +1669,58 @@ int mhi_prepare_for_transfer_autoqueue(struct mhi_device *mhi_dev)
>>   }
>>   EXPORT_SYMBOL_GPL(mhi_prepare_for_transfer_autoqueue);
>>   
>> +static int ____mhi_prepare_for_transfer(struct device *dev, void *data)
> 
> "__mhi_prepare_all_for_transfer"

This is to prepare one single child device, I don't think a name like 
__mhi_prepare_all_for_transfer (with 'all' inside) make sense, right?
How about changing to "mhi_prepare_dev_for_transfer" or 
"mhi_prepare_single_for_transfer"?

> 
>> +{
>> +	struct mhi_device *mhi_dev;
>> +	struct mhi_chan *ul_chan, *dl_chan;
>> +	enum mhi_ee_type ee = MHI_EE_MAX;
> 
> Reverse Xmas order, please.
> 
>> +
>> +	if (dev->bus != &mhi_bus_type)
>> +		return 0;
>> +
>> +	mhi_dev = to_mhi_device(dev);
>> +
>> +	/* Only prepare virtual devices that are attached to bus */
> 
> "Only prepare virtual devices for the channels". Here and below.
> 
>> +	if (mhi_dev->dev_type == MHI_DEVICE_CONTROLLER)
>> +		return 0;
>> +
>> +	/* There are cases where there is no MHI client driver matches
>> +	 * this device, we are not allowed to do prepare for it.
>> +	 */
> 
> Use the preferred style for comment:
> 
> 	/*
> 	 * ...
> 	 */
> 
>> +	if (!mhi_dev->id)
>> +		return 0;
>> +
>> +	ul_chan = mhi_dev->ul_chan;
>> +	dl_chan = mhi_dev->dl_chan;
>> +
>> +	/*
>> +	 * If execution environment is specified, remove only those devices that
>> +	 * started in them based on ee_mask for the channels as we move on to a
>> +	 * different execution environment
>> +	 */
>> +	if (data)
>> +		ee = *(enum mhi_ee_type *)data;
>> +
>> +	if (ul_chan && ee != MHI_EE_MAX && !(ul_chan->ee_mask & BIT(ee)))
>> +		return 0;
>> +
>> +
> 
> Remove extra newline.
> 
>> +	if (dl_chan && ee != MHI_EE_MAX && !(dl_chan->ee_mask & BIT(ee)))
>> +		return 0;
>> +
>> +	if (dl_chan->pre_alloc)
>> +		return mhi_prepare_for_transfer_autoqueue(mhi_dev);
>> +	else
>> +		return mhi_prepare_for_transfer(mhi_dev);
>> +}
>> +
>> +int mhi_prepare_all_for_transfer(struct mhi_controller *mhi_cntrl)
>> +{
>> +	return device_for_each_child(&mhi_cntrl->mhi_dev->dev, NULL,
>> +				     ____mhi_prepare_for_transfer);
>> +}
>> +EXPORT_SYMBOL_GPL(mhi_prepare_all_for_transfer);
>> +
>>   void mhi_unprepare_from_transfer(struct mhi_device *mhi_dev)
>>   {
>>   	struct mhi_controller *mhi_cntrl = mhi_dev->mhi_cntrl;
>> @@ -1684,3 +1736,58 @@ void mhi_unprepare_from_transfer(struct mhi_device *mhi_dev)
>>   	}
>>   }
>>   EXPORT_SYMBOL_GPL(mhi_unprepare_from_transfer);
>> +
>> +static int ____mhi_unprepare_from_transfer(struct device *dev, void *data)
> 
> __mhi_unprepare_all_from_transfer

same as above.

> 
>> +{
>> +	struct mhi_device *mhi_dev;
>> +	struct mhi_chan *ul_chan, *dl_chan;
>> +	enum mhi_ee_type ee = MHI_EE_MAX;
>> +
>> +	if (dev->bus != &mhi_bus_type)
>> +		return 0;
>> +
>> +	mhi_dev = to_mhi_device(dev);
>> +
>> +	/* Only unprepare virtual devices that are attached to bus */
>> +	if (mhi_dev->dev_type == MHI_DEVICE_CONTROLLER)
>> +		return 0;
>> +
>> +	/* There are cases where there is no MHI client driver matches
>> +	 * this device, so it is not probed or prepared, no need to
>> +	 * do unprepare for it.
>> +	 */
>> +	if (!mhi_dev->id)
>> +		return 0;
>> +
>> +	ul_chan = mhi_dev->ul_chan;
>> +	dl_chan = mhi_dev->dl_chan;
>> +
>> +	/*
>> +	 * If execution environment is specified, remove only those devices that
>> +	 * started in them based on ee_mask for the channels as we move on to a
>> +	 * different execution environment
>> +	 */
>> +	if (data)
>> +		ee = *(enum mhi_ee_type *)data;
>> +
>> +	if (ul_chan) {
>> +		if (ee != MHI_EE_MAX && !(ul_chan->ee_mask & BIT(ee)))
>> +			return 0;
>> +	}
>> +
>> +	if (dl_chan) {
>> +		if (ee != MHI_EE_MAX && !(dl_chan->ee_mask & BIT(ee)))
>> +			return 0;
>> +	}
>> +
>> +	mhi_unprepare_from_transfer(mhi_dev);
>> +
>> +	return 0;
>> +}
>> +
>> +int mhi_unprepare_all_from_transfer(struct mhi_controller *mhi_cntrl)
>> +{
>> +	return device_for_each_child(&mhi_cntrl->mhi_dev->dev, NULL,
>> +				     ____mhi_unprepare_from_transfer);
>> +}
>> +EXPORT_SYMBOL_GPL(mhi_unprepare_all_from_transfer);
>> diff --git a/include/linux/mhi.h b/include/linux/mhi.h
>> index ae092bc8b97e..dcf62a57056a 100644
>> --- a/include/linux/mhi.h
>> +++ b/include/linux/mhi.h
>> @@ -668,7 +668,7 @@ static inline void mhi_power_down(struct mhi_controller *mhi_cntrl, bool gracefu
>>    * destroy struct devices. This is a variant for mhi_power_down() and is a
>>    * workaround to make it possible to use mhi_power_up() in a resume
>>    * handler. When using this variant the caller must also call
>> - * mhi_prepare_all_for_transfer_autoqueue() and
>> + * mhi_prepare_all_for_transfer() and
> 
> This change belongs to previous patch.
> 
>>    * mhi_unprepare_all_from_transfer().
>>    *
>>    * @mhi_cntrl: MHI controller
>> @@ -842,4 +842,22 @@ int mhi_queue_skb(struct mhi_device *mhi_dev, enum dma_data_direction dir,
>>    */
>>   bool mhi_queue_is_full(struct mhi_device *mhi_dev, enum dma_data_direction dir);
>>   
>> +/**
>> + * mhi_prepare_all_for_transfer - if you are using
>> + * mhi_power_down_no_destroy() variant this needs to be called after
>> + * calling mhi_power_up().
> 
> Add info about what this API does also.
> 
>> + *
>> + * @mhi_cntrl: MHI controller
>> + */
>> +int mhi_prepare_all_for_transfer(struct mhi_controller *mhi_cntrl);
>> +
>> +/**
>> + * mhi_unprepare_all_from_transfer - if you are using
>> + * mhi_power_down_no_destroy() variant this function needs to be called
>> + * before calling mhi_power_down_no_destroy().
> 
> Same as above.
> 
> - Mani
> 
>> + *
>> + * @mhi_cntrl: MHI controller
>> + */
>> +int mhi_unprepare_all_from_transfer(struct mhi_controller *mhi_cntrl);
>> +
>>   #endif /* _MHI_H_ */
>> -- 
>> 2.39.2
>>
>>
>