Re: [PATCH 0/3] Implement role-switch notifications from dwc3-drd to dwc3-qcom

[Peter Chen <peter.chen@kernel.org>]

On 21-07-07 14:03:19, Bjorn Andersson wrote:
> On Tue 06 Jul 20:57 CDT 2021, Peter Chen wrote:
> 
> Allow me to reorder your two questions:
> 
> > And why using a notifier need to concern core's deferral probe?
> 
> The problem at hand calls for the core for somehow invoking
> dwc3_qcom_vbus_overrride_enable() with a pointer to dwc3_qcom passed.
> 
> This means that dwc3-qcom somehow needs to inform the dwc3-core about
> this (and stash the pointer). And this can't be done until dwc3-core
> actually exist, which it won't until dwc3_probe() has completed
> successfully (or in particular allocated struct dwc).

Maybe you misunderstood the notifier I meant previous, my pointer was
calling glue layer API directly.

Role switch is from dwc3-core, when it occurs, it means structure dwc3 has
allocated successfully, you could call glue layer notifier at function
dwc3_usb_role_switch_set directly.
Some references of my idea [1] [2]

[1] Function ci_hdrc_msm_notify_event at ci_hdrc_msm_notify_event
[2] https://source.codeaurora.org/external/imx/linux-imx/tree/drivers/usb/dwc3/core.c?h=lf-5.10.y#n205

> 
> > Why do you think we need to retry the parent's probe again?
> 
> There's four options here:
> 
> 0) Hope that of_platform_populate() always succeeds in invoking
> dwc3_probe() on the first attempt, so that it is available when
> of_find_device_by_node() is invoked in dwc3_qcom_probe() (and a few of
> the other platform's drivers).
> 
> 1) Ensure that the operations performed by dwc3_probe() happens
> synchronously and return a failure to dwc3-qcom, which depending on how
> dwc3_probe() failed can propagate that failure - i.e. either probe defer
> or clean up its resources if the failure from dwc3-core is permanent.
> 
> 2) Register the dwc3-core and then return from dwc3_qcom_probe() in some
> half-initialized state and through some yet to be invented notification
> mechanism continue the tail of dwc3_qcom_probe() once dwc3_probe() has
> finished. But I know of no such notification mechanism in place today
> and we can just register a callback, because of 1).
> Furthermore we'd leave dwc3-qcom half-initialized until the dwc3-core is
> done probing - which might never happen.
> 
> 3) Make drvdata of all the platform drivers be some known struct that
> dwc3-core can retrieve and dereference - containing the optional
> callback to the role_switch callback.
> 
> 
> We've tried the option 0) for a few years now. Option 2) is a variant of
> what we have today, where we patch one problem up and hope that nothing
> unexpected happens until things has fully probed. We're doing 3) in
> various other places, but in my view it's abusing a void * and has to be
> kept synchronized between all the possible parent drivers.
> 
> Left is 1), which will take some refactoring, but will leave the
> interaction between the two drivers in a state that's very easy to
> reason about.

Function of_find_device_by_node() invoked at glue layer is usually successfully,
The dwc3_probe failure doesn't affect it, unless you enable auto-suspend,
and glue layer's runtime suspend routine depends on dwc3 core's runtime suspend
routine. Would you please describe more about dwc3-core probe failure causes
dwc3-qcom's probe has failed or in half-initialized state you said?

> 
> > I know there are some downstream code which using this way, I would
> > like to know the shortcoming for it.
> > 
> 
> The shortcoming of having dwc3_qcom_probe() invoke dwc3_probe()
> "manually" and then returning -EPROBE_DEFER if the dwc3-core's resources
> aren't yet available is that we're wasting some time tearing down the
> dwc3-qcom state and then re-initialize it next time this is attempted.

Like above, would you explain more about it?

-- 

Thanks,
Peter Chen