lkml.org 
[lkml]   [2020]   [Aug]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v3 2/4] irqchip/qcom-pdc: Switch to using IRQCHIP_PLATFORM_DRIVER helper macros
    On Thu, Aug 6, 2020 at 7:49 PM John Stultz <john.stultz@linaro.org> wrote:
    >
    > On Thu, Aug 6, 2020 at 6:42 PM Bjorn Andersson
    > <bjorn.andersson@linaro.org> 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@linaro.org> wrote:
    > > > > On Wed 05 Aug 14:57 PDT 2020, John Stultz wrote:
    > > > > > On Wed, Aug 5, 2020 at 2:47 PM Steev Klimaszewski <steev@kali.org> 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@linaro.org> 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

    \
     
     \ /
      Last update: 2020-08-07 05:03    [W:2.509 / U:0.184 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site