Re: [RFC 2/8] usb: dwc3: core: Register vendor hooks for dwc3-qcom

[Bryan O'Donoghue <bryan.odonoghue@linaro.org>]

On 03/11/2023 18:49, Krishna Kurapati PSSNV wrote:
> 
> 
> On 11/4/2023 12:15 AM, Krishna Kurapati PSSNV wrote:
>>
>>
>> On 11/3/2023 8:44 PM, Bryan O'Donoghue wrote:
>>> On 17/10/2023 14:18, Krishna Kurapati wrote:
>>>>
>>>> The following are the requirements aimed in this implementation:
>>>>
>>>> 1. When enum in device mode, Glue/core must stay active.
>>>>
>>>> 2. When cable is connected but UDC is not written yet, then glue/core
>>>> must be suspended.
>>>>
>>>> 3. Upon removing cable in device mode, the disconnect event must be
>>>> generated and unblock runtime suspend for dwc3 core.
>>>>
>>>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
>>>
>>
>> Hi Bryan,
>>
>>> What happens to this code if you
>>>
>>> static int count;
>>>
>>> 1. sleep in dwc3_probe for 10 milliseconds
>>> 2. return -EPROBE_DEFER
>>> 3. if count++ < 5 goto 1
>>>
>>> i.e. if we simulate say waiting on a PHY driver to probe in dwc3_probe()
>>>
>> The vendor hooks are used in __dwc3_set_mode and role_switch_set calls 
>> in core and drd files respectively. These are invoked only if we are 
>> OTG capable. The drd_work is initialized in core_init_mode which is 
>> called at the end of dwc3_probe. If dwc3_probe fails and gets deferred 
>> before that, none of the vendor hooks will be fired and 
>> dwc3_qcom_probe is also deferred.
>>
>> However I see that if core_init_mode fails (the cleanup is already 
>> done in drd to prevent set_role from getting invoked already),  I need 
>> to cleanup vendor hooks in error path of dwc3_probe().
>>
>>> and what happens if we introduce a 100 millsecond sleep into 
>>> dwc3_qcom_probe() - and run a fake disconnect event from 
>>> dwc3_qcom_probe_core() directly ?
>>>
>>> In other words if make it that dwc3_probe() completes and struct 
>>> dwc3_glue_ops->notify_cable_disconnect() fires prior to 
>>> dwc3_qcom_probe_core() completing ?
>>>
>>> i.e. I don't immediately see how you've solved the probe() completion 
>>> race condition here.
>>>
>> Just wanted to understand the situation clearly. Is this the sequence 
>> you are referring to ?
>>
>> 1. dwc3_probe is successful and role switch is registered properly.
>> 2. added delay after dwc3_qcom_probe_core and before interconnect_init
>> 3. Between this delay, we got a disconnect notificiation from glink
>> 4. We are clearing the qscratch reg in case of device mode and 
>> un-registering notifier in case of host mode.
>>
>> If so, firstly I don't see any issue if we process disconnect event 
>> before qcom probe is complete. If we reached this stage, the 
>> clocks/gdsc is definitely ON and register accesses are good to go.
>>
>> If we are in host mode at this point, we would just unregister to 
>> usb-core notifier and mark last busy. If we are in device mode, we 
>> would just clear the hs_phy_ctrl reg of qscratch. After the 100ms 
>> delay you mentioned we would call dwc3_remove anyways and cleanup the 
>> vendor hooks. But is the concern here that, what if we enter 
>> runtime_suspend at this point ?
>>
> 
> Just to clarify one more thing. The probe completion requirement came in 
> because, before the device tree was flattened, dwc3-qcom and core are 
> two different platform devices. And if the dwc3 core device probe got 
> deferred, dwc3-qcom probe still gets successfully completed. The glue 
> would never know when to register vendor hook callbacks to dwc3-core as 
> it would never know when the core probe was completed.
> 
> That is the reason we wanted to find out accurate point where core probe 
> is done to ensure we can properly register these callbacks.

Are you saying to you require/rely on both of these series being applied 
first ?

[1]: 
https://lore.kernel.org/all/af60c05b-4a0f-51b8-486a-1fc601602515@quicinc.com/
[2]: 
https://lore.kernel.org/all/20231016-dwc3-refactor-v1-0-ab4a84165470@quicinc.com/

Must be, nothing applies for me in this series.