On Thu, Aug 6, 2020 at 7:49 PM John Stultz <john.stultz@xxxxxxxxxx> wrote: > > On Thu, Aug 6, 2020 at 6:42 PM Bjorn Andersson > <bjorn.andersson@xxxxxxxxxx> wrote: > > On Thu 06 Aug 18:22 PDT 2020, John Stultz wrote: > > > On Thu, Aug 6, 2020 at 5:43 PM Bjorn Andersson > > > <bjorn.andersson@xxxxxxxxxx> wrote: > > > > On Wed 05 Aug 14:57 PDT 2020, John Stultz wrote: > > > > > On Wed, Aug 5, 2020 at 2:47 PM Steev Klimaszewski <steev@xxxxxxxx> wrote: > > > > > > On 8/5/20 4:16 PM, Steev Klimaszewski wrote: > > > > > > > On 8/5/20 3:19 PM, Saravana Kannan wrote: > > > > > > >> On Wed, Aug 5, 2020 at 12:44 AM John Stultz <john.stultz@xxxxxxxxxx> wrote: > > > > > > >>> <sigh> > > > > > > >>> So this is where I bashfully admit I didn't get a chance to try this > > > > > > >>> patch series out, as I had success with a much older version of > > > > > > >>> Saravana's macro magic. > > > > > > >>> > > > > > > >>> But unfortunately, now that this has landed in mainline, I'm seeing > > > > > > >>> boot regressions on db845c. :( This is in the non-modular case, > > > > > > >>> building the driver in. > > > > > > >> Does that mean the modular version is working? Or you haven't tried > > > > > > >> that yet? I'll wait for your reply before I try to fix it. I don't > > > > > > >> have the hardware, but it should be easy to guess this issue looking > > > > > > >> at the code delta. > > > > > > > For what it's worth, I saw this too on the Lenovo C630 (started on -next > > > > > > > around 20200727, but I didn't track it down as, well, there's less way > > > > > > > to get debug output on the C630. > > > > > > > > > > > > > > In my testing, module or built-in doesn't matter, but reverting does > > > > > > > allow me to boot again. > > > > > > > > > > > > > Actually - I spoke too soon - QCOM_PDC built-in with the commit reverted > > > > > > boots, however, module (on the c630 at least) doesn't boot whether it's > > > > > > a module or built-in. > > > > > > > > > > You may need to set deferred_probe_timeout=30 to give things a bit > > > > > more grace time to load. > > > > > > > > With the risk of me reading more into this than what you're saying, > > > > please don't upstream anything that depend this parameter to be > > > > increased. > > > > > > > > Compiling any of these drivers as module should not require the user to > > > > pass additional kernel command line parameters in order to get their > > > > device to boot. > > > > > > So, ideally I agree, and Saravana's fw_devlink work should allow us to > > > avoid it. But the reality is that it is already required (at least in > > > configurations heavily using modules) to give more time for modules > > > loaded to resolve missing dependencies after init begins (due to > > > changes in the driver core to fail loading after init so that optional > > > dt links aren't eternally looked for). This was seen when trying to > > > enable the qualcom clk drivers to modules. > > > > > > > So to clarify what you're saying, any system that boots successfully > > with the default options is a sign of pure luck - regardless of being > > builtin or modules. > > > > > > And there you have my exact argument against the deferred timeout magic > > going on in the driver core. But as you know people insist that it's > > more important to be able to boot some defunct system from NFS than a > > properly configured one reliably. > > I'd agree, but the NFS case was in use before, and when the original > deferred timeout/optional link handling stuff landed no one complained > they were broken by it (at least at the point where it landed). Only > later when we started enabling more lower-level core drivers as > modules did the shortened dependency resolution time start to bite > folks. My attempt to set the default to be 30 seconds helped there, > but caused trouble and delays for the NFS case, and "don't break > existing users" seemed to rule, so I set the default timeout back to > 0. > > > > It doesn't seem necessary in this case, but I suggested it here as > > > I've got it enabled by default in my AOSP builds so that the > > > module-heavy configs for GKI boot properly (even if Saravana's > > > fw_devlink work is disabled). > > > > > > > With all due respect, that's your downstream kernel, the upstream kernel > > should not rely on luck, out-of-tree patches or kernel parameters. > > I agree that would be preferred. But kernel parameters are often there > for these sorts of cases where we can't always do the right thing. As > for out-of-tree patches, broken things don't get fixed until > out-of-tree patches are developed and upstreamed, and I know Saravana > is doing exactly that, and I hope his fw_devlink work helps fix it so > the module loading is not just a matter of luck. Btw, the only downstream fw_devlink change is setting itto =on (vs =permissive in upstream). > Also I think Thierry's comments in the other thread today are also > good ideas for ways to better handle the optional dt link handling > (rather than using a timeout). Could you please give me a lore link to this thread? Just curious. -Saravana
