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

[Baochen Qiang <quic_bqiang@quicinc.com>]

On 11/13/2023 12:18 AM, Jeffrey Hugo wrote:
> 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@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 | 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.

Sorry for misunderstood your comment. Will refine it in next version.


>
>>
>>
>>>> +    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);
>>
>