From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-13.2 required=3.0 tests=BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_IN_DEF_DKIM_WL autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id B0A7BC433E0 for ; Wed, 27 Jan 2021 16:43:28 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 61E0360187 for ; Wed, 27 Jan 2021 16:43:28 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232005AbhA0QnY (ORCPT ); Wed, 27 Jan 2021 11:43:24 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:45100 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229682AbhA0QnI (ORCPT ); Wed, 27 Jan 2021 11:43:08 -0500 Received: from mail-yb1-xb30.google.com (mail-yb1-xb30.google.com [IPv6:2607:f8b0:4864:20::b30]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 329BAC061788 for ; Wed, 27 Jan 2021 08:42:28 -0800 (PST) Received: by mail-yb1-xb30.google.com with SMTP id i141so2646013yba.0 for ; Wed, 27 Jan 2021 08:42:28 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=DhlJDOHouhyzoZng9fJ2frIQEnLbOzAgsBucZ1xKE+Q=; b=qhZFI+sMK2QvOM0tX+sbe7tSSfEUacHXU7Wc/fiZiGImf44dQ3z3Uund/QxKZ/1h9Q s/GDEhs3T8IvEXbPJ6jxrzs+IEeoSm8SbL88rB75v9+7PODSL5SGoUGJT29bIRR3qL+O UFojdWN6LdFC74lUmRR7GHaahe7McQNBwzFUKs3KTsBg8QM3+Yryqa3n6oOmkYNqq0Wn SEwaPMwAE4YCyne6qX0eMoZ4ng3Xf66DEfIDE4UarGF1ToJkA5Vtdw69wEtbLSzyVZNr JF3QgHYa5DcdVJipQQ9JGZxtkoQU9Y9Y+fja30YayyMKL7/ZOUUmZM9jCZa+nktN0coR y9yQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=DhlJDOHouhyzoZng9fJ2frIQEnLbOzAgsBucZ1xKE+Q=; b=ZAuM3+ac9dW7ITXppllMfAM1u4IXbtzsHX2EYI3krTPgA7lPCr5LobJLIMl6mIW0Gw EiI/6FJtfVWphg3Ov4OYKyCMx8wD1/ADHrpE9sHNJ8EP6LDcHqRRySfpYtsZr6z771Bs u2bLvWyE+KkM8mGBEAOH0tgSRsJXczFP4bhctribMv/8ZmiYh7n3h9px91UqkbXaGf35 aufuuycfWy60/t3m9utDqFiqK5cbpR4WOWtv31fZoe7SjJK2uSxsisXLAUivnIMNHmkJ 48+hI7x/mlTFcWUUSpZXAp69dpGKIqg7BAZ2BFx9L75d3n00WLEU81V8PxwSzNldycAj ePXw== X-Gm-Message-State: AOAM533iqM58OzMKqJ4VIfAF4578c+UOvCU8sAvFO4vUpaMnGD3DFuoE varzDwoFHVCmm71zoHVXUeRtRVVEkat9iAWTgPki3v0pcgg= X-Google-Smtp-Source: ABdhPJyTvhJWCJ0GgEFEPNNK+Aj1wHbHsyL+lMhynRRzjgtQPwprNz5H2mOCSJ9e3/s4ARAtNSkp6kxlHCFtD6MzH3I= X-Received: by 2002:a25:77d4:: with SMTP id s203mr19594855ybc.32.1611765747235; Wed, 27 Jan 2021 08:42:27 -0800 (PST) MIME-Version: 1.0 References: <20210120105246.23218-1-michael@walle.cc> In-Reply-To: From: Saravana Kannan Date: Wed, 27 Jan 2021 08:41:50 -0800 Message-ID: Subject: Re: [PATCH] PCI: dwc: layerscape: convert to builtin_platform_driver() To: Geert Uytterhoeven Cc: Michael Walle , Lorenzo Pieralisi , Roy Zang , PCI , LKML , Minghuan Lian , Mingkai Hu , Greg Kroah-Hartman , Bjorn Helgaas , linuxppc-dev , linux-arm-kernel Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-pci@vger.kernel.org On Tue, Jan 26, 2021 at 11:43 PM Geert Uytterhoeven wrote: > > Hi Saravana, > > On Wed, Jan 27, 2021 at 1:44 AM Saravana Kannan wrote: > > On Tue, Jan 26, 2021 at 12:50 AM Geert Uytterhoeven > > wrote: > > > On Mon, Jan 25, 2021 at 11:42 PM Saravana Kannan wrote: > > > > On Mon, Jan 25, 2021 at 11:49 AM Michael Walle wrote: > > > > > Am 2021-01-21 12:01, schrieb Geert Uytterhoeven: > > > > > > On Thu, Jan 21, 2021 at 1:05 AM Saravana Kannan > > > > > > wrote: > > > > > >> On Wed, Jan 20, 2021 at 3:53 PM Michael Walle > > > > > >> wrote: > > > > > >> > Am 2021-01-20 20:47, schrieb Saravana Kannan: > > > > > >> > > On Wed, Jan 20, 2021 at 11:28 AM Michael Walle > > > > > >> > > wrote: > > > > > >> > >> > > > > > >> > >> [RESEND, fat-fingered the buttons of my mail client and converted > > > > > >> > >> all CCs to BCCs :(] > > > > > >> > >> > > > > > >> > >> Am 2021-01-20 20:02, schrieb Saravana Kannan: > > > > > >> > >> > On Wed, Jan 20, 2021 at 6:24 AM Rob Herring wrote: > > > > > >> > >> >> > > > > > >> > >> >> On Wed, Jan 20, 2021 at 4:53 AM Michael Walle > > > > > >> > >> >> wrote: > > > > > >> > >> >> > > > > > > >> > >> >> > fw_devlink will defer the probe until all suppliers are ready. We can't > > > > > >> > >> >> > use builtin_platform_driver_probe() because it doesn't retry after probe > > > > > >> > >> >> > deferral. Convert it to builtin_platform_driver(). > > > > > >> > >> >> > > > > > >> > >> >> If builtin_platform_driver_probe() doesn't work with fw_devlink, then > > > > > >> > >> >> shouldn't it be fixed or removed? > > > > > >> > >> > > > > > > >> > >> > I was actually thinking about this too. The problem with fixing > > > > > >> > >> > builtin_platform_driver_probe() to behave like > > > > > >> > >> > builtin_platform_driver() is that these probe functions could be > > > > > >> > >> > marked with __init. But there are also only 20 instances of > > > > > >> > >> > builtin_platform_driver_probe() in the kernel: > > > > > >> > >> > $ git grep ^builtin_platform_driver_probe | wc -l > > > > > >> > >> > 20 > > > > > >> > >> > > > > > > >> > >> > So it might be easier to just fix them to not use > > > > > >> > >> > builtin_platform_driver_probe(). > > > > > >> > >> > > > > > > >> > >> > Michael, > > > > > >> > >> > > > > > > >> > >> > Any chance you'd be willing to help me by converting all these to > > > > > >> > >> > builtin_platform_driver() and delete builtin_platform_driver_probe()? > > > > > >> > >> > > > > > >> > >> If it just moving the probe function to the _driver struct and > > > > > >> > >> remove the __init annotations. I could look into that. > > > > > >> > > > > > > > >> > > Yup. That's pretty much it AFAICT. > > > > > >> > > > > > > > >> > > builtin_platform_driver_probe() also makes sure the driver doesn't ask > > > > > >> > > for async probe, etc. But I doubt anyone is actually setting async > > > > > >> > > flags and still using builtin_platform_driver_probe(). > > > > > >> > > > > > > >> > Hasn't module_platform_driver_probe() the same problem? And there > > > > > >> > are ~80 drivers which uses that. > > > > > >> > > > > > >> Yeah. The biggest problem with all of these is the __init markers. > > > > > >> Maybe some familiar with coccinelle can help? > > > > > > > > > > > > And dropping them will increase memory usage. > > > > > > > > > > Although I do have the changes for the builtin_platform_driver_probe() > > > > > ready, I don't think it makes much sense to send these unless we agree > > > > > on the increased memory footprint. While there are just a few > > > > > builtin_platform_driver_probe() and memory increase _might_ be > > > > > negligible, there are many more module_platform_driver_probe(). > > > > > > > > While it's good to drop code that'll not be used past kernel init, the > > > > module_platform_driver_probe() is going even more extreme. It doesn't > > > > even allow deferred probe (well before kernel init is done). I don't > > > > think that behavior is right and that's why we should delete it. Also, > > > > > > This construct is typically used for builtin hardware for which the > > > dependencies are registered very early, and thus known to probe at > > > first try (if present). > > > > > > > I doubt if any of these probe functions even take up 4KB of memory. > > > > > > How many 4 KiB pages do you have in a system with 10 MiB of SRAM? > > > How many can you afford to waste? > > > > There are only a few instances of this macro in the kernel. How many > > $ git grep -lw builtin_platform_driver_probe | wc -l > 21 > $ git grep -lw module_platform_driver_probe | wc -l > 86 > > + the ones that haven't been converted to the above yet: > > $ git grep -lw platform_driver_probe | wc -l > 58 > Yeah, this adds up in terms of the number of places we'd need to fix. But thinking more about it, a couple of points: 1. Not all builtin_platform_driver_probe() are problems for fw_devlink. So we can just fix them as we go if we need to. 2. The problem with builtin_platform_driver_probe() isn't really with the use of __init. It's the fact that it doesn't allow deferred probes. builtin_platform_driver_probe()/platform_driver_probe() could still be fixed up to allow deferred probe until we get to the point where we free the __init section (so at least till late_initcall). > > of those actually fit the description above? We can probably just > > check the DT? > > What do you mean by checking the DT? I was talking about checking the DT to see if the board has very little memory, but that's not always obvious from DT nor does it scale with the number of instances we have. So, ignore this comment. Anyway, time to get back to actually writing the code to deal with this and other corner cases. -Saravana