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.7 required=3.0 tests=BAYES_00, 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 6BF63C43381 for ; Thu, 28 Jan 2021 09:30:32 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 0775A64DE6 for ; Thu, 28 Jan 2021 09:30:30 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232364AbhA1JaD (ORCPT ); Thu, 28 Jan 2021 04:30:03 -0500 Received: from mail-ot1-f45.google.com ([209.85.210.45]:46789 "EHLO mail-ot1-f45.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232557AbhA1J0Q (ORCPT ); Thu, 28 Jan 2021 04:26:16 -0500 Received: by mail-ot1-f45.google.com with SMTP id d1so4536033otl.13; Thu, 28 Jan 2021 01:25:59 -0800 (PST) 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=NxnDl2GrRpSMyEh3P5ogDr/y/Z8is37UEIaUl8bFtVI=; b=j5g4zOFBuop8ehlvVl6Z5Cb12BGxVUmv5eDp3Un7TZHqSwNzO3HDwxg/w1ydXG1cEj +zaEAu+a+DScuwlZZFj6lrPsAuS/rTdo1R/iUF69LXgamhLkL90pGVImXQd8qHnlNLQg CaYxHuYx1WqyzP60HtaGGK+r1YddAwayPHJvUQC5A/Yl+OmnoDGEnJjkaO+quz9m7mwl Masw4Mm29yOzpk9qV+Y0dmsSR5x2nVJ2Aao2SqLbFKZ8tL8rxaogrs2f7gcC7hDvkL18 /s+1s6PcpALjBKuht1GkbUn5PXjjejPSLa0JLlaE+XgWaOZSbP60nja9x9gc7UB79Mqx /Lig== X-Gm-Message-State: AOAM5307wA3NedqiHD0vTxrYyA86Ih49tNvdhPuwTs0OfVYNWpLmiu59 2r7ODAMt2+qrMDi/OVGMIzQnDXrstKSbUhse5uw= X-Google-Smtp-Source: ABdhPJwIK/ebf1zOwoD5aMbXd7iqWR7pWcC/KS+N8M04xbTvxixhJs51oTBmGIastbl3pUtGaeoY22gVYXFn6StzkmQ= X-Received: by 2002:a05:6830:1489:: with SMTP id s9mr10058455otq.250.1611825934080; Thu, 28 Jan 2021 01:25:34 -0800 (PST) MIME-Version: 1.0 References: <20210120105246.23218-1-michael@walle.cc> In-Reply-To: From: Geert Uytterhoeven Date: Thu, 28 Jan 2021 10:25:22 +0100 Message-ID: Subject: Re: [PATCH] PCI: dwc: layerscape: convert to builtin_platform_driver() To: Saravana Kannan 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 Hi Saravana, On Wed, Jan 27, 2021 at 6:11 PM Saravana Kannan wrote: > On Wed, Jan 27, 2021 at 8:56 AM Geert Uytterhoeven wrote: > > On Wed, Jan 27, 2021 at 5:42 PM Saravana Kannan wrote: > > > On Tue, Jan 26, 2021 at 11:43 PM Geert Uytterhoeven > > > wrote: > > > > 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). > > > > That's intentional: it is used for cases that will (must) never be deferred. > > That's why it's safe to use __init. > > So was the usage of builtin_platform_driver_probe() wrong in the > driver Michael fixed? Because, deferring and probing again clearly > works? It wasn't. The regression is that the driver no longer probes at first try, because its dependencies are now probed later. The question is: why are the dependencies now probed later? How to fix that? > Also, "must never be deferred" seems like a weird condition to > enforce. I think the real "rule" is that if it defers, the platform is > not expected to work. But disallowing a probe reattempt seems weird. > What is it going to hurt if it's attempted again? At worst it fails > one more time? "must never be deferred" is not the real condition, but "must not be probed after __init is freed" is (one of them, there may be other, cfr. the last paragraph below). The simplest way to guarantee that is to probe the driver immediately, and only once. > Also, I'd argue that all/most of the "can't defer, but I'm still a > proper struct device" cases are all just patchwork to deal with the > fact we were playing initcall chicken when there was no fw_devlink. > I'm hoping we can move people away from that mindset. And the first I agree, partially. Still, how come the dependencies are no longer probed before their consumers when fw_devlinks are enabled? I thought fw_devlinks is supposed to avoid exactly that? > step towards that would be to allow *platform_probe() to allow > deferred probes until late_initcall(). At which increase of complexity, to keep track of which drivers can and cannot be reprobed anymore after late_initcall? Still, many of these drivers use platform_driver_probe() early for a reason: because they need to initialize the hardware early. Probing them later may introduce subtle bugs. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds