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 893F0C433E0 for ; Wed, 27 Jan 2021 16:43:46 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 4009A64D9E for ; Wed, 27 Jan 2021 16:43:46 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232324AbhA0Qn3 (ORCPT ); Wed, 27 Jan 2021 11:43:29 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:45098 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231403AbhA0QnO (ORCPT ); Wed, 27 Jan 2021 11:43:14 -0500 Received: from mail-yb1-xb29.google.com (mail-yb1-xb29.google.com [IPv6:2607:f8b0:4864:20::b29]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 2E4B0C061786 for ; Wed, 27 Jan 2021 08:42:28 -0800 (PST) Received: by mail-yb1-xb29.google.com with SMTP id r32so2618666ybd.5 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=hrsKaVZov+Jzk5dQxUxtqM3yHF+NvxWdazApspqQoMLR4YU+jLFy3ilQC4tdzVmFGJ hP2e+VRaGz4+GCgX3ZmDLKe8lvY7DqNrcmxueHrI//GonFKDDT1XnEL7V7HJfyjnYu6u /3XgGTDUlf1cO2JmffQWxNleQq3eLurZnxCDBHuxaz4xUEIKZ+pKoKXXzeNneA1fvPvS uDDDlAbYqYk71DhfDXhSOytU2E3a/CEBfPewEGGUQZscoXqemCcZUhcfv2Ffb/WGNzsE Yg0oHBUU2TxfJGOYmNk8E+Kn6QMckIwnp61URi45KveRP9VR/HoUuHjf3tO4YNMzhEVC XnmQ== X-Gm-Message-State: AOAM530xAC2o1o9P5bMNmgErpcPE0kAhv8Yimnrjpi3IQIUR8NUWN+26 8uksDItVNUQ28n6/vnZCn/dZ0ux06+q8EWbL8oOfFQ== 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-kernel@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 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=-3.5 required=3.0 tests=BAYES_00,DKIM_ADSP_CUSTOM_MED, DKIM_INVALID,DKIM_SIGNED,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED 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 DA9F9C433E0 for ; Wed, 27 Jan 2021 16:44:53 +0000 (UTC) Received: from lists.ozlabs.org (lists.ozlabs.org [203.11.71.2]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 7726964D9D for ; Wed, 27 Jan 2021 16:44:52 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 7726964D9D Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=google.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=linuxppc-dev-bounces+linuxppc-dev=archiver.kernel.org@lists.ozlabs.org Received: from bilbo.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3]) by lists.ozlabs.org (Postfix) with ESMTP id 4DQqFt0ltLzDqP4 for ; Thu, 28 Jan 2021 03:44:50 +1100 (AEDT) Authentication-Results: lists.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=google.com (client-ip=2607:f8b0:4864:20::b2c; helo=mail-yb1-xb2c.google.com; envelope-from=saravanak@google.com; receiver=) Authentication-Results: lists.ozlabs.org; dkim=pass (2048-bit key; unprotected) header.d=google.com header.i=@google.com header.a=rsa-sha256 header.s=20161025 header.b=qhZFI+sM; dkim-atps=neutral Received: from mail-yb1-xb2c.google.com (mail-yb1-xb2c.google.com [IPv6:2607:f8b0:4864:20::b2c]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 4DQqCD5qQpzDqB0 for ; Thu, 28 Jan 2021 03:42:31 +1100 (AEDT) Received: by mail-yb1-xb2c.google.com with SMTP id v200so2638894ybe.1 for ; Wed, 27 Jan 2021 08:42:31 -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=PzHggoBbvssvv5hV7n/mwO+ReF+8+xJyAiaQN7YSM//3PJIMZOyUtKp9KeHNkdV7Hj diElExKJRavVlejT6Yogi5oYEF8IlYQyLcZCmjAlFWL+8j++lCIgNguvgEk4+ePkNF1v 8C7fyUT6417U+SDq/J2mQbfsFPpeYwEQ0LkfeXbDhvyzd+5rvCkpvYliV6MeARafhAlX ar+xB0t3K7so1pYp0WJFdsSgm1lcVN9itdSM6CohH1dXThPJ2hyzE/H86c863KZPZ9tw K+LF+qiKGltTqZRQ3A1v2UCccsxibRYOrqilYJIm6DDE1TSEqmGBNgWlj5yjPY5x9s4x C9pA== X-Gm-Message-State: AOAM533j/Sf9ETWEt1PWgbRFbMQtfCnPFvvzokF1HxOrPisJmHyo0IrE uqZOq46Mil85+IBiedT0dEjUrKeQZK6bLm588caOJw== 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 Content-Type: text/plain; charset="UTF-8" X-BeenThere: linuxppc-dev@lists.ozlabs.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Roy Zang , Lorenzo Pieralisi , PCI , LKML , Minghuan Lian , Michael Walle , linux-arm-kernel , Greg Kroah-Hartman , Bjorn Helgaas , linuxppc-dev , Mingkai Hu Errors-To: linuxppc-dev-bounces+linuxppc-dev=archiver.kernel.org@lists.ozlabs.org Sender: "Linuxppc-dev" 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 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=-4.1 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_ADSP_CUSTOM_MED,DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED 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 1836FC433DB for ; Wed, 27 Jan 2021 16:43:46 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id C0A4260187 for ; Wed, 27 Jan 2021 16:43:45 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org C0A4260187 Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=google.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:To:Subject:Message-ID:Date:From:In-Reply-To: References:MIME-Version:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=4LBF282YA6oOT/Cv9xSLnOr1m/PBG11iEZr0ScrCMYo=; b=gzefu91Rc8sDY5FBEVM9TmxhG PkMhV8x0L/NaziWBV2cuxai0NfuT/6BRi2UtdJJrVE2/dz/ws7vLcVXys+N4JOAYgMZ1FWz2y0mRZ owmgwXd0aDZGPSkgB1Zan0EOvItyDCznL8eeGy4DT0gHMW8cS3Xo9edGSoLMYndRcSyNz2CyLQRvv eoR57+x7Fw00gwD+8m0U0kPbI1Hq6GqPLpdxl9jnDF3Wk60QZ5gElpbhMA3TfxKseX8aEXzsQPiBc dzp5hhn0kJE6y2H5vJypdI8NKDlgzCVHtbbjpdf2mcPObHEWFdocdwB0OQBIJxE5kJN6UMyX8fqJ1 TyZT3W+gA==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1l4ntp-0005Gw-LM; Wed, 27 Jan 2021 16:42:33 +0000 Received: from mail-yb1-xb2d.google.com ([2607:f8b0:4864:20::b2d]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1l4ntm-0005G6-K9 for linux-arm-kernel@lists.infradead.org; Wed, 27 Jan 2021 16:42:31 +0000 Received: by mail-yb1-xb2d.google.com with SMTP id 84so2630774yba.3 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=c7XoXE7YM1ZkxzWn4X36oXylfIitIviQQmYS16DU6Ufkus5QM/4GJCW+ppoV5Bq5SR umW8lRIhuuP1+FDcFu7G66bupL/A5cbpJorDXW0gCAuYW3fgtTQNuML6ZApayniuStaX QHotlAtaO9xNnwH55XKBZ6W4RNv3Yz9xV3Jhr4Aa+fyjkiXA3Q3wZ2Z3E0+Ocq6UIWkk uUpMQz7XV3Tq09vtUDRgtI+b/ir/1uSOU/in6K0rQMJM9h7HapiYL9mdvDURZ6KAwxMX OBpUrVBnCv8RSN0jwjApoUoy3KmYZUAgXmRykRyJnAZJUX3VpOemgGMTwr8NWN75HBj7 Sgvg== X-Gm-Message-State: AOAM53380g9jNLwPvPn8K2BAYkb1UM+Gt9CP8mt6zV0pkBerm1EVloGZ BLcCPNNp4gZcNuBn48L9XtUzXIID28PfpOcUkmS7SA== 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 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210127_114230_705347_763B7D03 X-CRM114-Status: GOOD ( 44.06 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Roy Zang , Lorenzo Pieralisi , PCI , LKML , Minghuan Lian , Michael Walle , linux-arm-kernel , Greg Kroah-Hartman , Bjorn Helgaas , linuxppc-dev , Mingkai Hu Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel