[PATCH RFC 2/8] bus: mhi: host: add new interfaces to handle MHI channels directly

[Jeffrey Hugo]

On 11/11/2023 8:59 PM, Baochen Qiang wrote:
> 
> On 11/11/2023 1:14 AM, Jeffrey Hugo wrote:
>> On 11/10/2023 3:21 AM, Kalle Valo wrote:
>>> From: Baochen Qiang <quic_bqiang at 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 at quicinc.com>
>>> Signed-off-by: Kalle Valo <quic_kvalo at quicinc.com>
>>> ---
>>>   drivers/bus/mhi/host/main.c | 91 +++++++++++++++++++++++++++++++++++++
>>>   include/linux/mhi.h         | 18 ++++++++
>>>   2 files changed, 109 insertions(+)
>>>
>>> diff --git a/drivers/bus/mhi/host/main.c b/drivers/bus/mhi/host/main.c
>>> index dcf627b36e82..9bcf8a49c000 100644
>>> --- a/drivers/bus/mhi/host/main.c
>>> +++ b/drivers/bus/mhi/host/main.c
>>> @@ -1667,6 +1667,49 @@ 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_autoqueue(struct device 
>>> *dev, void *data)
>>> +{
>>> +    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 prepare virtual devices thats attached to bus */
>>
>> "that are"?
>>
> It means MHI devices with a type of MHI_DEVICE_XFER. See also 
> mhi_destroy_device();

I think you are confused about my comment.  "thats" is not correct 
English.  I was suggesting you replace it with "that are", but there are 
many ways to reword the comment.

> 
> 
>>> +    if (mhi_dev->dev_type == MHI_DEVICE_CONTROLLER)
>>> +        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;
>>> +
>>> +
>>> +    if (dl_chan && ee != MHI_EE_MAX && !(dl_chan->ee_mask & BIT(ee)))
>>> +        return 0;
>>> +
>>> +    return mhi_prepare_for_transfer_autoqueue(mhi_dev);
>>> +}
>>> +
>>> +int mhi_prepare_all_for_transfer_autoqueue(struct mhi_controller 
>>> *mhi_cntrl)
>>> +{
>>> +    return device_for_each_child(&mhi_cntrl->mhi_dev->dev, NULL,
>>> +                     __mhi_prepare_for_transfer_autoqueue);
>>> +}
>>> +EXPORT_SYMBOL_GPL(mhi_prepare_all_for_transfer_autoqueue);
>>
>> This seems broken.  It appears to configure all channels as autoqueue, 
>> regardless of how the controller initially configured them.  This 
>> would only be safe to use if all channels were configured for 
>> autoqueue, but would silently cause issues otherwise.
> 
> Thanks for pointing that. Yes, it is not correct to treat all channels 
> as autoqueue regardless of its initial configuration. So how about 
> change as below:

Seems ok.

> 
> /* The difference between mhi_prepare_for_transfer_autoqueue() and 
> mhi_prepare_for_transfer() comes from how to treat downlink channel */
> 
> mhi_prepare_for_transfer_dev(struct device *dev, ...)
> 
> {
> 
> ...
> 
> dl_chan = mhi_dev->dl_chan;
> 
> ...
> 
> if (dl_chan->pre_alloc)
> 
>         mhi_prepare_for_transfer_autoqueue(dev);
> 
> else
> 
>         mhi_prepare_for_transfer(dev);
> 
> }
> 
> /* And then iterate all devices and call mhi_prepare_for_transfer_dev() 
> for each. */
> 
> 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_dev);
> }
> EXPORT_SYMBOL_GPL(mhi_prepare_all_for_transfer);
>