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=-0.7 required=3.0 tests=DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED,DKIM_VALID,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, 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 04BD6C433DF for ; Tue, 7 Jul 2020 04:16:09 +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 C5E5720708 for ; Tue, 7 Jul 2020 04:16:08 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="ZQ6wObi0"; dkim=fail reason="signature verification failed" (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="JFx744Kk" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org C5E5720708 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.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:In-Reply-To:MIME-Version:References:Message-ID: Subject:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=3qKqRvWr5Tt5RX7gceheKd0W4PWsmbgVj3263wzCx6o=; b=ZQ6wObi05UN54LE4rfpSRW4bC Dt29WVFsPBt0wihGDMaMzCAWwzTFvRIncBs4kLO/iV6dErxYMTH0W++d75kFyYs9XKKascqahCCFF VKlpwUz1MhrV0Ki38VI6X8qdRGISAW92JifTJD6Tx1adpdq0EVhw+NZ+hkXakXzisbP3yMzxaFAlS d40WZjmhqS20iA8XyN61W+iWHIDB4OYJyLz+c4DPWc7FMU7e93/W4pBU0BhW4uZFkEoQmU1iSMJd9 QRaETyqS77eSxUaKsMFQzdrl99yTDVu8aLBCmfRPlwVvUAjGxstNxnNU016sOsWNhINMkNA1iwmmc G76yx+GHQ==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1jsf0N-0000YE-6I; Tue, 07 Jul 2020 04:14:51 +0000 Received: from mail-pg1-x541.google.com ([2607:f8b0:4864:20::541]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1jsf0L-0000Wu-8D for linux-arm-kernel@lists.infradead.org; Tue, 07 Jul 2020 04:14:50 +0000 Received: by mail-pg1-x541.google.com with SMTP id w2so18588263pgg.10 for ; Mon, 06 Jul 2020 21:14:47 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to; bh=qEprmOlTY6puSyCIWCfUXtLm8xXvjpiYlwOcozAIUio=; b=JFx744KkwGkk5wwXPaVDr5ZU7Bn4Ub8uJ+sZzrgPDW8cUa/LPJUjgyYTdCAobx4Yyi Ne5k7D5yBQ7+33dJ37Bw+Wjbp2u+woknlkfXziwyldKDluEk6w9jtbS4Ns/yQJDgTtKO KfJMbz1oxLHsOxgSe+0vKeD/FZlV1V0UE0Y1zR2qc2HRvXKleTu39hx+ML5IGsFW6oXb BnxAMfn0ZsIxcUtTHGsvlefYLDu6VCfd2begQ0+CzFvQsuimvMamn7jfkqtN2u6g1vKO s3P3FXTdkQP44SkmuDyrLCK03x2W8wq8GLXAHmO04vJLI4I7NrK7TFzDI08iluVGlzJ1 KeTQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to; bh=qEprmOlTY6puSyCIWCfUXtLm8xXvjpiYlwOcozAIUio=; b=rOjSGCVZUI0G9LQF9zJzDb+XVCs1vkucqthMtr5ZFKaHdaJdqaEg+lFTS0/McMNqOv Y+WezCFqe/oITS0TBSo+K3HKA5GcNEshGCc6IWBRMdaI+gjpYS3Tm+XZkNwcS0XIXXol R6t8OTnYBaVkm80Ycg4jETU+iPUf7w1AXObNr67eBru3cWbWsFGPEP4kc+pQY6EaMqnR 37glDe0NeDcJhPmtK1MAGFzYmZaIvqpyHA90PU6qd/wO6BSBY+YK6oIEvtGHtOrG0ycO s5iSIwlZv+LCtNa8WWaZwfovJkqe35/csnq0b1UPUUNexkq/aCldcjQ2saNLe6ukw0Zw TAvA== X-Gm-Message-State: AOAM531fRZVRPSG8TnkfZIlNEd/pmVr80SnLn6SxwufEdeGWQFt5hfhf Lv9mrDhL4hk7IDix0qKHBcU= X-Google-Smtp-Source: ABdhPJwJV1fYmtkHPRdr0loU/ctwB/AbXfQRm/jbU4vXCvCYppXxNN7/AvhjbSvunGHm4gJcJCW29Q== X-Received: by 2002:a05:6a00:5c:: with SMTP id i28mr30727237pfk.274.1594095285301; Mon, 06 Jul 2020 21:14:45 -0700 (PDT) Received: from dtor-ws ([100.99.132.186]) by smtp.gmail.com with ESMTPSA id b21sm11277073pfb.45.2020.07.06.21.14.44 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 06 Jul 2020 21:14:44 -0700 (PDT) Date: Mon, 6 Jul 2020 21:14:42 -0700 From: Dmitry Torokhov To: Andrzej Hajda Subject: Re: [PATCH v6 2/4] driver core: add deferring probe reason to devices_deferred property Message-ID: <20200707041442.GE3273837@dtor-ws> References: <20200626100103.18879-1-a.hajda@samsung.com> <20200626100103.18879-3-a.hajda@samsung.com> <5f159e00-44fd-515b-dd8c-4db9845dc9e6@ti.com> <7e3c924b-c025-a829-6868-78e2935c70eb@samsung.com> <66faa188-5ef6-d449-07fe-28c8be5e559c@ti.com> <21f5ec9c-2d1d-5f28-5aeb-ac0db144a55e@samsung.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200707_001449_317587_7CD85CEF X-CRM114-Status: GOOD ( 36.97 ) 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: Jernej Skrabec , Grygorii Strashko , Bartlomiej Zolnierkiewicz , Greg Kroah-Hartman , Jonas Karlman , Russell King - ARM Linux , "open list:DRM DRIVERS" , lkml , Neil Armstrong , Andy Shevchenko , Mark Brown , Laurent Pinchart , "Rafael J. Wysocki" , linux-arm-kernel , Marek Szyprowski Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Thu, Jul 02, 2020 at 08:57:55AM +0200, Andrzej Hajda wrote: > = > On 30.06.2020 20:00, Dmitry Torokhov wrote: > > On Tue, Jun 30, 2020 at 8:42 AM Andrzej Hajda wro= te: > >> > >> On 30.06.2020 10:59, Grygorii Strashko wrote: > >>> Hi > >>> > >>> On 29/06/2020 14:28, Andrzej Hajda wrote: > >>>> Hi Grygorii, > >>>> > >>>> (...) > >>>> > >>>>>> /* > >>>>>> * deferred_devs_show() - Show the devices in the deferred pro= be > >>>>>> pending list. > >>>>>> */ > >>>>>> @@ -221,7 +241,8 @@ static int deferred_devs_show(struct seq_file = *s, > >>>>>> void *data) > >>>>>> mutex_lock(&deferred_probe_mutex); > >>>>>> list_for_each_entry(curr, &deferred_probe_pending_list, > >>>>>> deferred_probe) > >>>>>> - seq_printf(s, "%s\n", dev_name(curr->device)); > >>>>>> + seq_printf(s, "%s\t%s", dev_name(curr->device), > >>>>>> + curr->device->p->deferred_probe_reason ?: "\n"); > >>>>>> mutex_unlock(&deferred_probe_mutex); > >>>>>> > >>>>> Sry, may be i missing smth, but shouldn't it be optional > >>>>> (CONFIG_DEBUG_FS is probably too generic). > >>>>> > >>>> I am not sure what exactly are you referring to, but this patch does= not > >>>> add new property, it just extends functionality of existing one. > >>> Sry, needed to be more specific. > >>> > >>> You've added device_set_deferred_probe_reson(dev, &vaf); > >>> which expected to be used on every EPROBE_DEFER in dev_err_probe() in > >>> combination with > >>> > >>> + } else { > >>> + device_set_deferred_probe_reson(dev, &vaf); > >>> dev_dbg(dev, "error %d: %pV", err, &vaf); > >>> > >>> ^^ dev_dbg() does not add any runtime overhead during boot unless ena= bled > >>> + } > >>> > >>> But: > >>> > >>> +void device_set_deferred_probe_reson(const struct device *dev, struct > >>> va_format *vaf) > >>> +{ > >>> + const char *drv =3D dev_driver_string(dev); > >>> + > >>> + mutex_lock(&deferred_probe_mutex); > >>> + > >>> + kfree(dev->p->deferred_probe_reason); > >>> + dev->p->deferred_probe_reason =3D kasprintf(GFP_KERNEL, "%s: > >>> %pV", drv, vaf); > >>> + > >>> + mutex_unlock(&deferred_probe_mutex); > >>> +} > >>> > >>> ^^ Adds locking, kfree() and kasprintf() for every deferred probe > >>> during boot and can't be disabled. > >>> > >>> Right? > >> > >> Right, but usually the burden should be insignificant in comparison to > >> probe time, so I do not think it is worth optimizing. > > I do not think this is going to take. You are suggesting that we > > modify pretty much every driver to supply this deferral reason, and I > > doubt it will happen. Can we put this burden on providers that raise > > the deferral? > = > = > I wouldn't say they raise the deferral, they just inform resource is not = > yet available. Only device driver, and only in its probe function can = > "raise the deferral". Well, this is a matter of perspective. If devm_gpiod_get() returns -EBUSY and this is returned to driver core, is it GPIO line signals that line is busy, or is it the driver applies its knowledge. I say that in majority of cases driver does not really get a say in this and simply has to pass whatever error condition that is signalled by providers up the stack. I would consider whenever a driver does not propagate -EPROBE_DEFER to the driver code a bug that needs fixing, because it should not degrade functionality and/or performance just because we have not figured out how to order probing properly and have to rely on deferrals. > = > = > > I.e. majority of code are using devm API now, so we most > > likely know the device for which deferral is being raised. We can have > > a list of deferral reasons and their devices and when in device code > > once probe is done we could try reconciling it with the deferred > > devicelist, and this would mean you only need to implement this in > > gpiolib, regulator core, clocks core, etc. > = > = > This patchset tries to solve simple issue - replace multiple lines of = > code present in multiple probe functions (additionally fixing lot of = > them) with single call and then enhance it little bit, nothing more. > = > What you are proposing is blurry at the moment for me, provider does not = > know if consumer want to defer, This is my point - the consumer does not get to decide. If deferral is raised, it must be honored. > or will continue working without missing resource, Deferral does not mean resource does not exist and the driver has to get by if it can. It means the resource is not ready, and even if the system can work without it, it will not be working optimally. > moreover some consumers can acquire resources after probe - again no > probe deferral. In this case we should not signal deferral either. > Even if it will be done (it can be, for = > example by creating probe version of all resource get functions), it = > will require much more changes but finally it will look like: > = > res =3D devm_get_resource_from_probe(....) > = > if (IS_ERR(res)) > = > =A0=A0=A0 return PTR_ERR(res); > = > vs: > = > res =3D devm_get_resource(...) > = > if (IS_ERR(res)) > = > =A0=A0=A0 return dev_err_probe(dev, PTR_ERR(res), ...); And we will need to adjust how many hundreds of drivers? Consider that most drivers use devm_clk_get(), devm_gpiod_get() and devm_regulator_get() and their friends. All these APIs already have device for which resource is being allocated, and moreover their use outside of probe() path is highly suspicious (because devm outside of probe() typically result in unwinding in really surprising order). So if you could stash device and deferral reason in a list and then scan this list in driver core when handling the raised deferral you would not need to change anything in individual drivers. Hope this clears what I had in mind. Thanks. -- = Dmitry _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel