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

[Bjorn Andersson <bjorn.andersson@linaro.org>]

On Wed 07 Jul 22:06 CDT 2021, Peter Chen wrote:

> 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]
> 

It's probably 5+ years since I ran into something using platform_data,
had totally forgotten about it.

Defining a dwc3_platdata to allow the glue drivers to pass a function
pointer (and Wesley's bool) to the core driver sounds like a possible
way out of this.

> [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,

Went spelunking in drivers/base again, and I think you're right.

of_find_device_by_node() looks for devices on the platform_bus's klist
of devices, so if of_platform_populate() ends up successfully getting
through device_add() the we will find something. It might not have
probed yet, but as long as we don't rely on that we should be good...

> 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.

Right, if we hit qcom_dwc3_resume_irq() before the core driver has
probed it certainly looks like we're going to hit a NULL pointer.

> Would you please describe more about dwc3-core probe failure causes
> dwc3-qcom's probe has failed or in half-initialized state you said?
> 

Bryan had a previous patch where the glue layer was notified about role
switching (iirc) and as soon as we hit a probe deferal in the core
driver we'd dereference some pointer in the glue layer. I don't find the
patch right now, but I suspect it might have been caused by the same
platform_get_drvdata() as we see in qcom_dwc3_resume_irq().

> > 
> > > 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?
> 

I could, but I guess if we use platform_data to pass the callbacks to
the core it doesn't matter if the core driver probes synchronously or in
a week (except if the glue hits qcom_dwc3_resume_irq(), but if that can
happen it can be fixed separately)...

Regards,
Bjorn