Message-ID: <Ys4hJrWjkn+LREZL@google.com>
Date:   Tue, 12 Jul 2022 18:34:30 -0700
From:   Matthias Kaehlcke <mka@...omium.org>
To:     Krishna Kurapati PSSNV <quic_kriskura@...cinc.com>
Cc:     Pavan Kondeti <quic_pkondeti@...cinc.com>, saravanak@...gle.com,
        Stephen Boyd <swboyd@...omium.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Felipe Balbi <balbi@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        Andy Gross <agross@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Doug Anderson <dianders@...omium.org>,
        Mathias Nyman <mathias.nyman@...el.com>,
        devicetree@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-pm@...r.kernel.org, quic_ppratap@...cinc.com,
        quic_vpulyala@...cinc.com
Subject: Re: [PATCH v20 2/5] usb: dwc3: core: Host wake up support from
 system suspend

On Fri, Jul 08, 2022 at 04:37:19PM +0530, Krishna Kurapati PSSNV wrote:
>    On 7/6/2022 12:28 PM, Krishna Kurapati PSSNV wrote:
> 
>    On 7/1/2022 9:22 PM, Matthias Kaehlcke wrote:
> 
> On Fri, Jul 01, 2022 at 03:45:26PM +0530, Pavan Kondeti wrote:
> 
> On Thu, Jun 30, 2022 at 06:10:50PM -0700, Matthias Kaehlcke wrote:
> 
> dwc3-qcom should wait for dwc3 core to call component_add() and then do
> whatever needs to be done once the dwc3 core is registered in the
> dwc3-qcom bind callback. Honestly this may all be a little overkill if
> there's only two drivers here, dwc3-qcom and dwc3 core. It could
> probably just be some callback from dwc3 core at the end of probe that
> calls some function in dwc3-qcom.
> 
> Since the issue we are facing is that the ssphy device links are not ready
> causing the dwc3 probe not being invoked, can we add an API as Pavan
> suggested
> to check if deferred_probe listfor dwc3 device is empty or not andbased on
> that we can choose to defer our qcomprobe ? In this case, we don't need to
> touch the dwc3 core driver and would be making changesonly in qcom glue
> driver.
> 
> As mentioned above, it shouldn't be necessary to add component support to
> all the glue drivers. An API to check for deferred probing is an option,
> however there is a possible race condition: When the dwc3-qcom driver checks
> for a deferred probe the core could still be probing, in that situation the
> glue would proceed before the core driver is ready. That could be avoided
> with the component based approach.
> 
> The race can happen only if asynchronous probe is enabled, otherwise the
> child's probe happens synchronously in of_platform_populate()
> 
> I was thinking about the case where the dwc3-qcom probe is initially deferred,
> then the deferred probe starts shortly after (asynchronously) and now the
> dwc3-qcom driver does its check. Probably it's not very likely to happen ...
> 
> 
> OTOH, would the below condition suffice for our needs here? if our device
> is not bounded to a driver, we check the state of initcalls and return
> either error or -EPROBE_DEFER
> 
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index 7b6eff5..519a503 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
> @@ -722,6 +722,9 @@ static int dwc3_qcom_of_register_core(struct platform_device
>  *pdev)
>                 dev_err(dev, "failed to get dwc3 platform device\n");
>         }
> 
> +       if (!qcom->dwc3->dev.driver)
> +               return driver_deferred_probe_check_state(&qcom->dwc3->dev);
> +
>  node_put:
>         of_node_put(dwc3_np);
> 
> I like the simplicity of it, no need for new APIs.
> 
> The components based approach would be slightly safer, but in practice I
> think this should be good enough.
> 
>    Hi Pavan, Mathias,
>      I have tested the suggested code and verified that it works on
>    sc7180. I see that the API has been removed recently in the following
>    patch :\
>    commit 9cbffc7a59561be950ecc675d19a3d2b45202b2b
>    Author: Saravana Kannan [1]<saravanak@...gle.com>
>    Date:   Wed Jun 1 00:07:05 2022 -0700
>    driver core: Delete driver_deferred_probe_check_state()
>    Can we make a patch and add it back to the kernel for this purpose ?
>    Hi Saravana,
>      Can you help suggest if we can revert your patch or make a new one to
>    add back the function.

The cover letter [1] of the 'deferred_probe_timeout logic clean up'
series [2] has more context:


  A lot of the deferred_probe_timeout logic is redundant with
  fw_devlink=on.  Also, enabling deferred_probe_timeout by default breaks
  a few cases.

  This series tries to delete the redundant logic, simplify the frameworks
  that use driver_deferred_probe_check_state(), enable
  deferred_probe_timeout=10 by default, and fixes the nfsroot failure
  case.

  The overall idea of this series is to replace the global behavior of
  driver_deferred_probe_check_state() where all devices give up waiting on
  supplier at the same time with a more granular behavior:

  1. Devices with all their suppliers successfully probed by late_initcall
     probe as usual and avoid unnecessary deferred probe attempts.

  2. At or after late_initcall, in cases where boot would break because of
     fw_devlink=on being strict about the ordering, we

     a. Temporarily relax the enforcement to probe any unprobed devices
        that can probe successfully in the current state of the system.
        For example, when we boot with a NFS rootfs and no network device
        has probed.
     b. Go back to enforcing the ordering for any devices that haven't
        probed.

  3. After deferred probe timeout expires, we permanently give up waiting
     on supplier devices without drivers. At this point, whatever devices
     can probe without some of their optional suppliers end up probing.

  In the case where module support is disabled, it's fairly
  straightforward and all device probes are completed before the initcalls
  are done.

[1] https://lore.kernel.org/all/20220601070707.3946847-1-saravanak@google.com/
[2] https://patchwork.kernel.org/project/linux-pm/list/?series=646471&archive=both&state=*


Does anything speak against returning -EPROBE_DEFER directly from dwc3-qcom's
probe()? Now with deferred_probe_timeout > 0 there should be at least no endless
probing.

Powered by blists - more mailing lists