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

[Bryan O'Donoghue]

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@xxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxx/
[2]: 
https://lore.kernel.org/all/20231016-dwc3-refactor-v1-0-ab4a84165470@xxxxxxxxxxx/

Must be, nothing applies for me in this series.