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.0 required=3.0 tests=BAYES_00,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 57B3BC433EF for ; Thu, 23 Sep 2021 12:49:20 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 30516610C8 for ; Thu, 23 Sep 2021 12:49:20 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S240995AbhIWMuu (ORCPT ); Thu, 23 Sep 2021 08:50:50 -0400 Received: from mail-ot1-f52.google.com ([209.85.210.52]:37566 "EHLO mail-ot1-f52.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S240787AbhIWMub (ORCPT ); Thu, 23 Sep 2021 08:50:31 -0400 Received: by mail-ot1-f52.google.com with SMTP id r43-20020a05683044ab00b0054716b40005so1434657otv.4; Thu, 23 Sep 2021 05:49:00 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=HPho5pdPE80Ol42nlkZkWLtxK1B46FluJvPe15qBAgc=; b=7rVbIfl1b/C++IYVIWXpNB3EA8xFGzByqqszAQZAdjoOq30ZWWwVGi9e/MxCoBamhF btsben525ir2iPjbuYOHsA2EWnM2LLVPOJg0OLZrzUE6TiSN0Vp4b1HW08eKs7W/lqyq efWjm9C5slyMqsYr0vsd/2+oIcLvrRGWqZ634k8MlfjcTGdKCsDXPdnW1tG1kZk3kSf+ RNHufJzgQK2EXGvl1JfmHFmHZje+hsJmD2mmw4jNWH/2FA0PiB8Rx4feT4SRLFdXtLxb 0gpGSGi09ICzLYDbjAJNoXdeXlwCE+jG557qCAFiG4k5OPSViPwSx1Ug7jdHOjNXqSPD rE1A== X-Gm-Message-State: AOAM530Ch7mGj31FqJh5giljUlwAiJIqtndlgeGp/lGgetpM9DQwgU3Q mc/YaL8aqp70vSBbI5i8hTDx9SjY4LFSIb3bRZ4= X-Google-Smtp-Source: ABdhPJzqIyB8PZUiSI/+lxuDijrE0Lm85sXH1TPxYCbh18wwxFZKnOcawv+pJ9UsVPnnVvNwjuumEoIDYRDPdAZKE10= X-Received: by 2002:a9d:4d93:: with SMTP id u19mr4185271otk.86.1632401339990; Thu, 23 Sep 2021 05:48:59 -0700 (PDT) MIME-Version: 1.0 References: <20210915170940.617415-1-saravanak@google.com> <20210915170940.617415-3-saravanak@google.com> In-Reply-To: From: "Rafael J. Wysocki" Date: Thu, 23 Sep 2021 14:48:48 +0200 Message-ID: Subject: Re: [PATCH v3 2/3] driver core: fw_devlink: Add support for FWNODE_FLAG_NEEDS_CHILD_BOUND_ON_ADD To: Saravana Kannan Cc: Andrew Lunn , "Rafael J. Wysocki" , Greg Kroah-Hartman , Heiner Kallweit , Russell King , "David S. Miller" , Jakub Kicinski , Len Brown , Geert Uytterhoeven , Vladimir Oltean , "Cc: Android Kernel" , Linux Kernel Mailing List , netdev , ACPI Devel Maling List Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-acpi@vger.kernel.org On Tue, Sep 21, 2021 at 10:07 PM Saravana Kannan wrote: > > Sorry I've been busy with LPC and some other stuff and could respond earlier. > > On Tue, Sep 21, 2021 at 12:50 PM Andrew Lunn wrote: > > > > > It works at a device level, so it doesn't know about resources. The > > > only information it has is of the "this device may depend on that > > > other device" type and it uses that information to figure out a usable > > > probe ordering for drivers. > > > > And that simplification is the problem. A phandle does not point to a > > device, it points to a resource of a device. It should really be doing > > what the driver would do, follow the phandle to the resource and see > > if it exists yet. If it does not exist then yes it can defer the > > probe. If the resource does exist, allow the driver to probe. > > > > > Also if the probe has already started, it may still return > > > -EPROBE_DEFER at any time in theory > > > > Sure it can, and does. And any driver which is not broken will > > unregister its resources on the error path. And that causes users of > > the resources to release them. It all nicely unravels, and then tries > > again later. This all works, it is what these drivers do. > > One of the points of fw_devlink=on is to avoid the pointless deferred > probes that'd happen in this situation. So saying "let this happen" > when fw_devlink=on kinda beats the point of it. See further below. Well, you need to define "pointless deferred probes" in the first place. fw_devlink adds deferred probes by itself, so why are those not pointless whereas the others are? > > > > > However, making children wait for their parents to complete probing is > > > generally artificial, especially in the cases when the children are > > > registered by the parent's driver. So waiting should be an exception > > > in these cases, not a rule. > > Rafael, > > There are cases where the children try to probe too quickly (before > the parent has had time to set up all the resources it's setting up) > and the child defers the probe. Even Andrew had an example of that > with some ethernet driver where the deferred probe is attempted > multiple times wasting time and then it eventually succeeds. You seem to be arguing that it may be possible to replace multiple probe attempts that each are deferred with one probe deferral which then is beneficial from the performance perspective. Yes, there are cases like that, but when this is used as a general rule, it introduces a problem if it does a deferred probe when there is no need for a probe deferral at all (like in the specific problem case at hand). Also if the probing of the child is deferred just once, adding an extra dependency on the parent to it doesn't really help. > Considering there's no guarantee that a device_add() will result in > the device being bound immediately, why shouldn't we make the child > device wait until the parent has completely probed and we know all the > resources from the parent are guaranteed to be available? Why can't we > treat drivers that assume a device will get bound as soon as it's > added as the exception (because we don't guarantee that anyway)? Because this adds artificial constraints that otherwise aren't there in some cases to the picture and asking drivers to mark themselves as "please don't add these artificial constraints for me" is not particularly straightforward. Moreover, it does that retroactively causing things that are entirely correct and previously worked just fine to now have to paint themselves red to continue working as before. The fact that immediate probe completion cannot be guaranteed in general doesn't mean that it cannot be assumed in certain situations. For example, a parent driver registering a child may know what the child driver is and so it may know that the child will either probe successfully right away or the probing of it will fail and your extra constraint breaks that assumption. You can't really know how many of such cases there are and trying to cover them with a new flag is a retroactive whack-a-mole game. > Also, this assumption that the child will be bound successfully upon > addition forces the parent/child drivers to play initcall chicken -- > the child's driver has to be registered before the parent's driver. That's true, but why is this a general problem? For example, they both may be registered by the same function in the right order. What's wrong with that? > We should be getting away from those by fixing the parent driver that's > making these assumptions (I'll be glad to help with that). We need to > be moving towards reducing pointless deferred probes and initcall > ordering requirements instead of saying "this bad assumption used to > work, so allow me to continue doing that". It is not always a bad assumption. It may be code designed this way.