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=-10.7 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS autolearn=unavailable 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 DB65CC433E3 for ; Mon, 24 Aug 2020 11:10:15 +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 99FC1206BE for ; Mon, 24 Aug 2020 11:10:15 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="cHA+4uwz"; dkim=fail reason="signature verification failed" (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b="udinJ1A5" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 99FC1206BE Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linaro.org 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=/mOTZJiygUzvWzAfpc3MNlWGPSyGuRWv45Qt9dQC4FY=; b=cHA+4uwzR4OjpTDABzxBMH6SK EkD97RrtKH83+bVkfFTJotxNWx6wO1xB+PP6y5L7XrRd+X6koCRxyljymylOtXvP4GmOiga79cPs+ ivUYmQ1YTOnwcLlcfMJy0jsYJnEiWccW50kAuM+hyOm1Rm/9/OFFiqukxZsbwFWeSQcWLNC5z8Rvg wcJdDcIPrNy3q5MMQIjJb3f/rr8ZdejoxLvSwivGu8OrsbTO3V1LQZTynRD4jo8unsMj7lMfuBK9V sMzCCEPivCOSXs3RdZjbV86cjuUJXmANttJtV8N0POeXh/fNQ6goH8k6s5650ZYCf0PlEBXXPFOH6 /bEReOazQ==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kAALG-0007fa-1V; Mon, 24 Aug 2020 11:08:46 +0000 Received: from mail-vs1-xe42.google.com ([2607:f8b0:4864:20::e42]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1kAALC-0007ew-KT for linux-arm-kernel@lists.infradead.org; Mon, 24 Aug 2020 11:08:43 +0000 Received: by mail-vs1-xe42.google.com with SMTP id a127so4200650vsd.1 for ; Mon, 24 Aug 2020 04:08:38 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=msqYCrRbBxQjGSgVmfw5ip385GiLaV8PtNTz2kEH5V4=; b=udinJ1A5z+iZF7uVAAznWhloesqrXJlV64xUdiccVnzONh4RYuosigjlW6orJVnKFd kTv9uAlVqLyhCBjVEoZxCMIjizsmgy441Z49u3pVEtPIWpRXkyR/RZ/BxA6HMH+aZS0N X0ghrcHy0SgZjy+8x/77Aq0qO2e+rtdQFwo1GoJl2g5JgqXGtT0SFMwsbuND66noYAo2 lZaj8GOZ5tO333Rp3WtuBroKYCT/QdAIDtedmQokqvgxXywMKMh/dQXRJhf/6TdxevQK kcQlW1LMblOmtkdOySENtKGSfH4X/CyQY4cIbQy52jtKdmU9dXQAQt+4OOeZ0vyxjMpR oRYg== 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=msqYCrRbBxQjGSgVmfw5ip385GiLaV8PtNTz2kEH5V4=; b=E0O1/n8ju9QCHYsJE2a46U/URdxDVgb52swzsRMbm9wuV3sjS4kfFIH+W0tQ5O6uWA L14ENVXSCFZ4gTig6LqXfrzLPjYkXqov7JAX9q6mBqEqR4QUuSGHUv6YEwic8NW2NMcv IWY0QSdTT+KHouH5ONvcO8FEYDf8+pRDrQZ8lOtvs8BsvkiUZbESyWfa+x3XuYu+4LmY S0UdrcBfvYVybzUWhK0GDa3AG/HCr8UjZnp7CXKGosb7Pxi73fzQyclZeM4ooGOF2LOT ngLLfKzlRXcTSj0JNKpF/63hIwJkKnToZcM/NC7r6YPQejO+D9LOPjVFzcQcro8OusJY GevQ== X-Gm-Message-State: AOAM5318WwsdrD8VvBkYwlnr5GsuwHsGplkes68tYReV8gqOKNfrOHHK eij7KpXjYriCvPF+HjhRiJvhIunvyodHyZddL0XrsQ== X-Google-Smtp-Source: ABdhPJyp4PPZcFj/x1pw84othrOgycbCYanPHA6x83mbYGI4IY2iJC/01MKUcqQvQ5RfH1IkezQqbpV7LDEkU63n2nA= X-Received: by 2002:a67:fd0a:: with SMTP id f10mr1831122vsr.35.1598267316963; Mon, 24 Aug 2020 04:08:36 -0700 (PDT) MIME-Version: 1.0 References: <24ff92dd1b0ee1b802b45698520f2937418f8094.1598260050.git.viresh.kumar@linaro.org> In-Reply-To: <24ff92dd1b0ee1b802b45698520f2937418f8094.1598260050.git.viresh.kumar@linaro.org> From: Ulf Hansson Date: Mon, 24 Aug 2020 13:08:00 +0200 Message-ID: Subject: Re: [PATCH V2 1/2] opp: Allow dev_pm_opp_get_opp_table() to return -EPROBE_DEFER To: Viresh Kumar X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200824_070842_722279_A9D485B4 X-CRM114-Status: GOOD ( 45.38 ) 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: Nishanth Menon , Len Brown , linux-samsung-soc , Vincent Guittot , Stephan Gerhold , Kevin Hilman , Stephen Boyd , Viresh Kumar , Linux PM , "Rafael J. Wysocki" , Linux Kernel Mailing List , Krzysztof Kozlowski , Niklas Cassel , Kukjin Kim , Pavel Machek , Greg Kroah-Hartman , Georgi Djakov , Linux ARM 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 Mon, 24 Aug 2020 at 11:09, Viresh Kumar wrote: > > From: Stephan Gerhold > > The OPP core manages various resources, e.g. clocks or interconnect paths. > These resources are looked up when the OPP table is allocated once > dev_pm_opp_get_opp_table() is called the first time (either directly > or indirectly through one of the many helper functions). > > At this point, the resources may not be available yet, i.e. looking them > up will result in -EPROBE_DEFER. Unfortunately, dev_pm_opp_get_opp_table() > is currently unable to propagate this error code since it only returns > the allocated OPP table or NULL. > > This means that all consumers of the OPP core are required to make sure > that all necessary resources are available. Usually this happens by > requesting them, checking the result and releasing them immediately after. > > For example, we have added "dev_pm_opp_of_find_icc_paths(dev, NULL)" to > several drivers now just to make sure the interconnect providers are > ready before the OPP table is allocated. If this call is missing, > the OPP core will only warn about this and then attempt to continue > without interconnect. This will eventually fail horribly, e.g.: > > cpu cpu0: _allocate_opp_table: Error finding interconnect paths: -517 > ... later ... > of: _read_bw: Mismatch between opp-peak-kBps and paths (1 0) > cpu cpu0: _opp_add_static_v2: opp key field not found > cpu cpu0: _of_add_opp_table_v2: Failed to add OPP, -22 > > This example happens when trying to use interconnects for a CPU OPP > table together with qcom-cpufreq-nvmem.c. qcom-cpufreq-nvmem calls > dev_pm_opp_set_supported_hw(), which ends up allocating the OPP table > early. To fix the problem with the current approach we would need to add > yet another call to dev_pm_opp_of_find_icc_paths(dev, NULL). > But actually qcom-cpufreq-nvmem.c has nothing to do with interconnects... > > This commit attempts to make this more robust by allowing > dev_pm_opp_get_opp_table() to return an error pointer. Fixing all > the usages is trivial because the function is usually used indirectly > through another helper (e.g. dev_pm_opp_set_supported_hw() above). > These other helpers already return an error pointer. > > The example above then works correctly because set_supported_hw() will > return -EPROBE_DEFER, and qcom-cpufreq-nvmem.c already propagates that > error. It should also be possible to remove the remaining usages of > "dev_pm_opp_of_find_icc_paths(dev, NULL)" from other drivers as well. > > Note that this commit currently only handles -EPROBE_DEFER for the > clock/interconnects within _allocate_opp_table(). Other errors are just > ignored as before. Eventually those should be propagated as well. > > Signed-off-by: Stephan Gerhold Reviewed-by: Ulf Hansson Kind regards Uffe > [ Viresh: skip checking return value of dev_pm_opp_get_opp_table() for > EPROBE_DEFER in domain.c, fix NULL return value and reorder > code a bit in core.c, and update exynos-asv.c ] > Signed-off-by: Viresh Kumar > --- > Stephan, I have made some changes to the code. Please try it again and > lemme know if it works fine. > > drivers/base/power/domain.c | 14 +++++---- > drivers/opp/core.c | 53 +++++++++++++++++++------------- > drivers/opp/of.c | 8 ++--- > drivers/soc/samsung/exynos-asv.c | 2 +- > 4 files changed, 44 insertions(+), 33 deletions(-) > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > index 2cb5e04cf86c..b92bb61550d3 100644 > --- a/drivers/base/power/domain.c > +++ b/drivers/base/power/domain.c > @@ -2044,8 +2044,9 @@ int of_genpd_add_provider_simple(struct device_node *np, > if (genpd->set_performance_state) { > ret = dev_pm_opp_of_add_table(&genpd->dev); > if (ret) { > - dev_err(&genpd->dev, "Failed to add OPP table: %d\n", > - ret); > + if (ret != -EPROBE_DEFER) > + dev_err(&genpd->dev, "Failed to add OPP table: %d\n", > + ret); > goto unlock; > } > > @@ -2054,7 +2055,7 @@ int of_genpd_add_provider_simple(struct device_node *np, > * state. > */ > genpd->opp_table = dev_pm_opp_get_opp_table(&genpd->dev); > - WARN_ON(!genpd->opp_table); > + WARN_ON(IS_ERR(genpd->opp_table)); > } > > ret = genpd_add_provider(np, genpd_xlate_simple, genpd); > @@ -2111,8 +2112,9 @@ int of_genpd_add_provider_onecell(struct device_node *np, > if (genpd->set_performance_state) { > ret = dev_pm_opp_of_add_table_indexed(&genpd->dev, i); > if (ret) { > - dev_err(&genpd->dev, "Failed to add OPP table for index %d: %d\n", > - i, ret); > + if (ret != -EPROBE_DEFER) > + dev_err(&genpd->dev, "Failed to add OPP table for index %d: %d\n", > + i, ret); > goto error; > } > > @@ -2121,7 +2123,7 @@ int of_genpd_add_provider_onecell(struct device_node *np, > * performance state. > */ > genpd->opp_table = dev_pm_opp_get_opp_table_indexed(&genpd->dev, i); > - WARN_ON(!genpd->opp_table); > + WARN_ON(IS_ERR(genpd->opp_table)); > } > > genpd->provider = &np->fwnode; > diff --git a/drivers/opp/core.c b/drivers/opp/core.c > index 6978b9218c6e..8c69a764d0a4 100644 > --- a/drivers/opp/core.c > +++ b/drivers/opp/core.c > @@ -1068,7 +1068,7 @@ static struct opp_table *_allocate_opp_table(struct device *dev, int index) > */ > opp_table = kzalloc(sizeof(*opp_table), GFP_KERNEL); > if (!opp_table) > - return NULL; > + return ERR_PTR(-ENOMEM); > > mutex_init(&opp_table->lock); > mutex_init(&opp_table->genpd_virt_dev_lock); > @@ -1079,8 +1079,8 @@ static struct opp_table *_allocate_opp_table(struct device *dev, int index) > > opp_dev = _add_opp_dev(dev, opp_table); > if (!opp_dev) { > - kfree(opp_table); > - return NULL; > + ret = -ENOMEM; > + goto err; > } > > _of_init_opp_table(opp_table, dev, index); > @@ -1089,16 +1089,21 @@ static struct opp_table *_allocate_opp_table(struct device *dev, int index) > opp_table->clk = clk_get(dev, NULL); > if (IS_ERR(opp_table->clk)) { > ret = PTR_ERR(opp_table->clk); > - if (ret != -EPROBE_DEFER) > - dev_dbg(dev, "%s: Couldn't find clock: %d\n", __func__, > - ret); > + if (ret == -EPROBE_DEFER) > + goto err; > + > + dev_dbg(dev, "%s: Couldn't find clock: %d\n", __func__, ret); > } > > /* Find interconnect path(s) for the device */ > ret = dev_pm_opp_of_find_icc_paths(dev, opp_table); > - if (ret) > + if (ret) { > + if (ret == -EPROBE_DEFER) > + goto err; > + > dev_warn(dev, "%s: Error finding interconnect paths: %d\n", > __func__, ret); > + } > > BLOCKING_INIT_NOTIFIER_HEAD(&opp_table->head); > INIT_LIST_HEAD(&opp_table->opp_list); > @@ -1107,6 +1112,10 @@ static struct opp_table *_allocate_opp_table(struct device *dev, int index) > /* Secure the device table modification */ > list_add(&opp_table->node, &opp_tables); > return opp_table; > + > +err: > + kfree(opp_table); > + return ERR_PTR(ret); > } > > void _get_opp_table_kref(struct opp_table *opp_table) > @@ -1129,7 +1138,7 @@ static struct opp_table *_opp_get_opp_table(struct device *dev, int index) > if (opp_table) { > if (!_add_opp_dev_unlocked(dev, opp_table)) { > dev_pm_opp_put_opp_table(opp_table); > - opp_table = NULL; > + opp_table = ERR_PTR(-ENOMEM); > } > goto unlock; > } > @@ -1573,8 +1582,8 @@ struct opp_table *dev_pm_opp_set_supported_hw(struct device *dev, > struct opp_table *opp_table; > > opp_table = dev_pm_opp_get_opp_table(dev); > - if (!opp_table) > - return ERR_PTR(-ENOMEM); > + if (IS_ERR(opp_table)) > + return opp_table; > > /* Make sure there are no concurrent readers while updating opp_table */ > WARN_ON(!list_empty(&opp_table->opp_list)); > @@ -1632,8 +1641,8 @@ struct opp_table *dev_pm_opp_set_prop_name(struct device *dev, const char *name) > struct opp_table *opp_table; > > opp_table = dev_pm_opp_get_opp_table(dev); > - if (!opp_table) > - return ERR_PTR(-ENOMEM); > + if (IS_ERR(opp_table)) > + return opp_table; > > /* Make sure there are no concurrent readers while updating opp_table */ > WARN_ON(!list_empty(&opp_table->opp_list)); > @@ -1725,8 +1734,8 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev, > int ret, i; > > opp_table = dev_pm_opp_get_opp_table(dev); > - if (!opp_table) > - return ERR_PTR(-ENOMEM); > + if (IS_ERR(opp_table)) > + return opp_table; > > /* This should be called before OPPs are initialized */ > if (WARN_ON(!list_empty(&opp_table->opp_list))) { > @@ -1833,8 +1842,8 @@ struct opp_table *dev_pm_opp_set_clkname(struct device *dev, const char *name) > int ret; > > opp_table = dev_pm_opp_get_opp_table(dev); > - if (!opp_table) > - return ERR_PTR(-ENOMEM); > + if (IS_ERR(opp_table)) > + return opp_table; > > /* This should be called before OPPs are initialized */ > if (WARN_ON(!list_empty(&opp_table->opp_list))) { > @@ -1901,8 +1910,8 @@ struct opp_table *dev_pm_opp_register_set_opp_helper(struct device *dev, > return ERR_PTR(-EINVAL); > > opp_table = dev_pm_opp_get_opp_table(dev); > - if (!opp_table) > - return ERR_PTR(-ENOMEM); > + if (!IS_ERR(opp_table)) > + return opp_table; > > /* This should be called before OPPs are initialized */ > if (WARN_ON(!list_empty(&opp_table->opp_list))) { > @@ -1982,8 +1991,8 @@ struct opp_table *dev_pm_opp_attach_genpd(struct device *dev, > const char **name = names; > > opp_table = dev_pm_opp_get_opp_table(dev); > - if (!opp_table) > - return ERR_PTR(-ENOMEM); > + if (IS_ERR(opp_table)) > + return opp_table; > > /* > * If the genpd's OPP table isn't already initialized, parsing of the > @@ -2153,8 +2162,8 @@ int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt) > int ret; > > opp_table = dev_pm_opp_get_opp_table(dev); > - if (!opp_table) > - return -ENOMEM; > + if (IS_ERR(opp_table)) > + return PTR_ERR(opp_table); > > /* Fix regulator count for dynamic OPPs */ > opp_table->regulator_count = 1; > diff --git a/drivers/opp/of.c b/drivers/opp/of.c > index 7d9d4455a59e..e39ddcc779af 100644 > --- a/drivers/opp/of.c > +++ b/drivers/opp/of.c > @@ -947,8 +947,8 @@ int dev_pm_opp_of_add_table(struct device *dev) > int ret; > > opp_table = dev_pm_opp_get_opp_table_indexed(dev, 0); > - if (!opp_table) > - return -ENOMEM; > + if (IS_ERR(opp_table)) > + return PTR_ERR(opp_table); > > /* > * OPPs have two version of bindings now. Also try the old (v1) > @@ -1002,8 +1002,8 @@ int dev_pm_opp_of_add_table_indexed(struct device *dev, int index) > } > > opp_table = dev_pm_opp_get_opp_table_indexed(dev, index); > - if (!opp_table) > - return -ENOMEM; > + if (IS_ERR(opp_table)) > + return PTR_ERR(opp_table); > > ret = _of_add_opp_table_v2(dev, opp_table); > if (ret) > diff --git a/drivers/soc/samsung/exynos-asv.c b/drivers/soc/samsung/exynos-asv.c > index 30bb7b7cc769..8abf4dfaa5c5 100644 > --- a/drivers/soc/samsung/exynos-asv.c > +++ b/drivers/soc/samsung/exynos-asv.c > @@ -93,7 +93,7 @@ static int exynos_asv_update_opps(struct exynos_asv *asv) > continue; > > opp_table = dev_pm_opp_get_opp_table(cpu); > - if (IS_ERR_OR_NULL(opp_table)) > + if (IS_ERR(opp_table)) > continue; > > if (!last_opp_table || opp_table != last_opp_table) { > -- > 2.25.0.rc1.19.g042ed3e048af > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel