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

[Krishna Kurapati PSSNV <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.

The first one is not a patch. It is just a discussion thread I started 
to get community's opinion before on disconnect interrupt handling. The 
current series is based on top of [2] made by Bjorn (as you already 
found out) and as I mentioned in cover letter of my series.

Regards,
Krishna,