From patchwork Mon Aug 24 09:09:32 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Viresh Kumar X-Patchwork-Id: 248164 Delivered-To: patch@linaro.org Received: by 2002:a05:6e02:522:0:0:0:0 with SMTP id h2csp2378158ils; Mon, 24 Aug 2020 02:09:52 -0700 (PDT) X-Google-Smtp-Source: ABdhPJzuptQXAeepzfdgdBWD6S5AyRlRnrGljxuLOKHLEdsRGgWvD9quTJ7gnPUhoWa9nwOeIuZ+ X-Received: by 2002:a05:6402:1299:: with SMTP id w25mr4493189edv.349.1598260192274; Mon, 24 Aug 2020 02:09:52 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1598260192; cv=none; d=google.com; s=arc-20160816; b=cpm+Np/I3NiV1/RzG2+pWKQSV3A11WdNNk1fXnlHUaeZfj0BSnDDF2Kyfk8AzW/SuH n0s/Sbmzu7jjLC2gtFVRD9U2f8o3WT4j0pLpGmcTeIFCcee+vHgU1XdKqk3v5nRhO2Sl yRx8okjKTUwupmM6OXMMvlc6zOZd1ijcUuxOa2vhQh0uwrsMqkyZu76g5EjIwyIbZs6/ yU61bLrCaXAm/W+rcV+x1GhzJL21iDFft4h82VIVf5i7HtpNgN3UDbFkxNrxma9RFf4P 765wthIN9Zhg656/Sws9vk6ZR3XzAmnvM7cmW70I3kgZtAS+jCHN6CROLqIR9i6FkD4F yPHQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from:dkim-signature; bh=KkjksCGs4AQuVWCwGfdEcJFYrQa9D8++X80GpI0F1Zo=; b=eTTw8hCDfkmWloo8QwLhBA47QXxvr4ruxuMkoSvcf4v5jzuPMDrMWizFnxzWHupsQo fFO5KL6Af78hO0f50ui1FNhbI09SIsSDmCH6vHTje0F+8MfuJI4+BqefanFhmz3BsO7J oMLYwVqqdL6Cf+zYJ4+DDznA4sAdmMgkn8AQu9udxld0dco0PJwGmgg0RomCrVxQx8Bk arRBwveZBJvX6y7Lma1v+Y/1CueYDAe9184qHrUJsjDu/YWtaSgNB1Yn0Tq+eCJQk1+Q p71BWbkXLRKgYCjWxqGGnjisCOqv6Jd5lxcg55UIlLmCsRp63mvo1hbGy+vSHE+YDYKe CJjA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@linaro.org header.s=google header.b=BPXypc1U; spf=pass (google.com: domain of linux-pm-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-pm-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linaro.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id b19si6623215ejk.429.2020.08.24.02.09.51; Mon, 24 Aug 2020 02:09:52 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-pm-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; dkim=pass header.i=@linaro.org header.s=google header.b=BPXypc1U; spf=pass (google.com: domain of linux-pm-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-pm-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linaro.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727008AbgHXJJt (ORCPT + 10 others); Mon, 24 Aug 2020 05:09:49 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:39756 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728725AbgHXJJl (ORCPT ); Mon, 24 Aug 2020 05:09:41 -0400 Received: from mail-pg1-x542.google.com (mail-pg1-x542.google.com [IPv6:2607:f8b0:4864:20::542]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id B9A7CC061575 for ; Mon, 24 Aug 2020 02:09:40 -0700 (PDT) Received: by mail-pg1-x542.google.com with SMTP id o5so4263856pgb.2 for ; Mon, 24 Aug 2020 02:09:40 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=from:to:cc:subject:date:message-id:mime-version :content-transfer-encoding; bh=KkjksCGs4AQuVWCwGfdEcJFYrQa9D8++X80GpI0F1Zo=; b=BPXypc1U/ODzYLqbC9yrqE4xPIwzl1Ox4y5aPhPJ66J/ahwQz/B0ppevelkSK8WqFo eIIpvjrVmIgJaU5e1AwKWm8c6KhLAb4hq8CQsSlOUddaO9LiCKKfPeM28Ie1Iv/0Qd6p +6Pipv/rmD59wKhGKvZ4J0zLYEkvkSF/FwU5zTxTjvzkYz4fUfXLqBnOKCELssTmv4c4 e8NmNd0QyeFsNdtjg/VtMqUEQcarutVcdG2JCnKMjQrQIqemG6Y2YcB8zzA8P3ksArMW nVVoi2mo0ra88W0QtZECvg/Rn2TXtjM3isK00WZ1zhu92BqbXJX1cfV9DVESbXYO7njY pHBw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:mime-version :content-transfer-encoding; bh=KkjksCGs4AQuVWCwGfdEcJFYrQa9D8++X80GpI0F1Zo=; b=NkJSPeYN+dd0zMrdhZ1sQBDsbLQxkAc64BTUlk0RqNg3DsYPBhdyepMGB8GB0GjO59 rVPbVdaUcFBMCH/uvu0PrPdEXzDzVKJPK8dq8SRKjF6WRG/jYgkoZJ40Z6AoFmgur7xJ tvpb1sx/gzBqn0KS+i3o7XfYspp1e3XLj9FOVi2fanAYUcjmAKjYNJm+cBnzMGo0aRhf YRysowEin+JtGi0O2tkESdV0je3iPEYjqk8G8GeOMpyiWrygAXdTms/1PwhcQOFtsrjN ohJE6YBCglIM2REpE4fYTrzrBLQ/yBOvIXO+Hvt8N+mrvPrwsCZi1gzEYGBnuUDIgaBi 3qTA== X-Gm-Message-State: AOAM533oHPq0mAuWpuDVG3iDb/mc1TocRA2QzlutdJD5Opci6aKgXidP qQ/Pwje2XCXLorHTtqfaV98O9w== X-Received: by 2002:a63:5542:: with SMTP id f2mr2822925pgm.196.1598260180107; Mon, 24 Aug 2020 02:09:40 -0700 (PDT) Received: from localhost ([122.172.43.13]) by smtp.gmail.com with ESMTPSA id r7sm10965833pfl.186.2020.08.24.02.09.39 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Mon, 24 Aug 2020 02:09:39 -0700 (PDT) From: Viresh Kumar To: ulf.hansson@linaro.org, "Rafael J. Wysocki" , Kevin Hilman , Pavel Machek , Len Brown , Greg Kroah-Hartman , Viresh Kumar , Nishanth Menon , Stephen Boyd , Kukjin Kim , Krzysztof Kozlowski Cc: Viresh Kumar , linux-pm@vger.kernel.org, Vincent Guittot , nks@flawful.org, georgi.djakov@linaro.org, Stephan Gerhold , linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-samsung-soc@vger.kernel.org Subject: [PATCH V2 1/2] opp: Allow dev_pm_opp_get_opp_table() to return -EPROBE_DEFER Date: Mon, 24 Aug 2020 14:39:32 +0530 Message-Id: <24ff92dd1b0ee1b802b45698520f2937418f8094.1598260050.git.viresh.kumar@linaro.org> X-Mailer: git-send-email 2.25.0.rc1.19.g042ed3e048af MIME-Version: 1.0 Sender: linux-pm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pm@vger.kernel.org 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 [ 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(-) -- 2.25.0.rc1.19.g042ed3e048af Reviewed-by: Ulf Hansson 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) { From patchwork Mon Aug 24 09:09:33 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Viresh Kumar X-Patchwork-Id: 248165 Delivered-To: patch@linaro.org Received: by 2002:a05:6e02:522:0:0:0:0 with SMTP id h2csp2378466ils; Mon, 24 Aug 2020 02:10:28 -0700 (PDT) X-Google-Smtp-Source: ABdhPJwfkHSVSztbKANVuiCIm1BN0JEftjsMfnkKZAJ/hoyURVe/xTySqfovg/KYcWbqhtYLaKi/ X-Received: by 2002:a17:906:d84:: with SMTP id m4mr4815870eji.205.1598260227898; Mon, 24 Aug 2020 02:10:27 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1598260227; cv=none; d=google.com; s=arc-20160816; b=FO4Lm2lRCuKq8kMso77jLDixY5gHY+OSBelUui9/knk7BeEB3REFHkcUwPZLWkTM7K v1KGY96qZfbvMOWzNeDaWMn1L5O6FOV5Beerwsbduj3/RENsMVDfyowuR0vYwnAK57mI IRPaKEV2hrhllzM8UEBfYYSWMT7+ad78YyVvtnkFWt3Am2+dURpytZ5n/7J7vPjBjfZn MNdHxTS9WmRpQMDZLCSevF8bfwLyu9p8nE7KB1iBSJkznCcbAEJ5/sZG9HwYFNmWuNef N83VNcivtB3zsEBAn5gHn0BHu2Zs7gUAtQX0kwWS9517WwQ7sVNRN6DdvgjDrGEb3yUn J5+w== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=/bBoN/fJ8ZBi4jWE5LR3K6DN1QrhriBADtjA3PVqbrE=; b=PY/nGSnsjMBKkJWYmxEf9mYxm+0dhJQvI7RSGLq8WynX6EzARrRghQ1Xdd3/v3+QLr hJOm0e/YPI5bhWx8JXTqZCi9PEdPzO5xjVH/d+9GZL2wi4RwshilSzs+yg7H71OFKBFU /aa18G7BvtsTzhVOAvgh8hLEEekNTzNWj2mYEEAuza8lds/fUO5AzVAVehX00Hax+sBj 3NFrsAF55VQlvUByiITkR4J18SL18axe5kBxF6MM9qrKATtu27wqJQ0kyd/POWMmRvfw 21GR8NFl4+g+piXT76SSG8OYQW75LZorZQ3mpbBPlCMCp1uaBFSlRGS+WjVMqAVW6F3i e1mg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@linaro.org header.s=google header.b=X2SB9idb; spf=pass (google.com: domain of linux-pm-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-pm-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linaro.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id q7si4203502ejb.505.2020.08.24.02.10.27; Mon, 24 Aug 2020 02:10:27 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-pm-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; dkim=pass header.i=@linaro.org header.s=google header.b=X2SB9idb; spf=pass (google.com: domain of linux-pm-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-pm-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linaro.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728454AbgHXJJx (ORCPT + 10 others); Mon, 24 Aug 2020 05:09:53 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:39766 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728202AbgHXJJt (ORCPT ); Mon, 24 Aug 2020 05:09:49 -0400 Received: from mail-pj1-x1042.google.com (mail-pj1-x1042.google.com [IPv6:2607:f8b0:4864:20::1042]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 5D207C0613ED for ; Mon, 24 Aug 2020 02:09:44 -0700 (PDT) Received: by mail-pj1-x1042.google.com with SMTP id d4so3870791pjx.5 for ; Mon, 24 Aug 2020 02:09:44 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=/bBoN/fJ8ZBi4jWE5LR3K6DN1QrhriBADtjA3PVqbrE=; b=X2SB9idbucp6OUq+YHiAk5O+GWd7c4BKzZZ87q61ExZvqSCX+iNgw9ep9V40U4G6R1 UdC4ZiS37It7yusiAJy3C4jYmZSUVlBqwV00kFEqa+gGd3vEQ1FRCq0Oeo4p8bCpsVd6 Is/bYwmlJ9hJsUxUlKlzWKy6jm48s5ul8oHaph3msMudK9RnvWAxLPP6n8eDPeJ9NIQt Ja8/jDN+oIXSm2L0eBLMYj1FkQgULmgjpGNyjidWflFjohghAcIT5kZz9uAm494azLu4 f0aLOfzFtkmYtHSAxDg7QyIf4ERR4dxWrteyNyFlmvUhXCJ0EXOKc/n0f5RypCM8FWGR YtYw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=/bBoN/fJ8ZBi4jWE5LR3K6DN1QrhriBADtjA3PVqbrE=; b=tZiAcqhG5qE+4ghlR9uRNFlymZy/CabVSzBM+XWY68TIXEkqxHqO4lIUpIh96W8ayi 66O171TF3I+/5JNdpDHwD9BLPY+wQPIhFoxIo4+NZziIDoBX7Gsb8pwCuh06UKB03uFd E4q7gF29CbgmpcKr70YGQXsz3dOb22zAnySBf8oE6LxaZZyCvvTZgI8ZjO+pnqiP13nZ eZXmHnG+3GVRm04qS+6+Mxo5giC4SY4X2bmq9ZX7XFgs17Vmx/r2PB48hsVB8wA5D4Fj WC7Rh5o5AOm3deEZbldY59y8BTmKoAvHDo2gBtw5Ibk5DNOxb0mFG2txakJzTFSGw8fq LCLg== X-Gm-Message-State: AOAM532gvVxu0C6FiK4edzck9UENIym5yoZE26SSeZJtvKjD4yZsUteS 73gW0H9ohWa8OtwXr4AWqdCA+Q== X-Received: by 2002:a17:902:d20e:: with SMTP id t14mr3188949ply.327.1598260183394; Mon, 24 Aug 2020 02:09:43 -0700 (PDT) Received: from localhost ([122.172.43.13]) by smtp.gmail.com with ESMTPSA id h15sm9772040pjf.54.2020.08.24.02.09.42 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Mon, 24 Aug 2020 02:09:42 -0700 (PDT) From: Viresh Kumar To: ulf.hansson@linaro.org, "Rafael J. Wysocki" , Viresh Kumar , Liam Girdwood , Mark Brown Cc: linux-pm@vger.kernel.org, Vincent Guittot , Stephen Boyd , Nishanth Menon , nks@flawful.org, georgi.djakov@linaro.org, Stephan Gerhold , linux-kernel@vger.kernel.org Subject: [PATCH V2 2/2] cpufreq: dt: Refactor initialization to handle probe deferral properly Date: Mon, 24 Aug 2020 14:39:33 +0530 Message-Id: X-Mailer: git-send-email 2.25.0.rc1.19.g042ed3e048af In-Reply-To: <24ff92dd1b0ee1b802b45698520f2937418f8094.1598260050.git.viresh.kumar@linaro.org> References: <24ff92dd1b0ee1b802b45698520f2937418f8094.1598260050.git.viresh.kumar@linaro.org> MIME-Version: 1.0 Sender: linux-pm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pm@vger.kernel.org From: Stephan Gerhold cpufreq-dt is currently unable to handle -EPROBE_DEFER properly because the error code is not propagated for the cpufreq_driver->init() callback. Instead, it attempts to avoid the situation by temporarily requesting all resources within resources_available() and releasing them again immediately after. This has several disadvantages: - Whenever we add something like interconnect handling to the OPP core we need to patch cpufreq-dt to request these resources early. - resources_available() is only run for CPU0, but other clusters may eventually depend on other resources that are not available yet. (See FIXME comment removed by this commit...) - All resources need to be looked up several times. Now that the OPP core can propagate -EPROBE_DEFER during initialization, it would be nice to avoid all that trouble and just propagate its error code when necessary. This commit refactors the cpufreq-dt driver to initialize private_data before registering the cpufreq driver. We do this by iterating over all possible CPUs and ensure that all resources are initialized: 1. dev_pm_opp_get_opp_table() ensures the OPP table is allocated and initialized with clock and interconnects. 2. dev_pm_opp_set_regulators() requests the regulators and assigns them to the OPP table. 3. We call dev_pm_opp_of_get_sharing_cpus() early so that we only initialize the OPP table once for each shared policy. With these changes, we actually end up saving a few lines of code, the resources are no longer looked up multiple times and everything should be much more robust. Signed-off-by: Stephan Gerhold [ Viresh: Use list_head structure for maintaining the list and minor changes ] Signed-off-by: Viresh Kumar --- drivers/cpufreq/cpufreq-dt.c | 286 +++++++++++++++++------------------ 1 file changed, 143 insertions(+), 143 deletions(-) -- 2.25.0.rc1.19.g042ed3e048af Reported-by: Marek Szyprowski Tested-by: Marek Szyprowski diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c index 944d7b45afe9..d29cd93045fa 100644 --- a/drivers/cpufreq/cpufreq-dt.c +++ b/drivers/cpufreq/cpufreq-dt.c @@ -13,6 +13,7 @@ #include #include #include +#include #include #include #include @@ -24,18 +25,35 @@ #include "cpufreq-dt.h" struct private_data { - struct opp_table *opp_table; + struct list_head node; + + cpumask_var_t cpus; struct device *cpu_dev; - const char *reg_name; + struct opp_table *opp_table; + struct opp_table *reg_opp_table; bool have_static_opps; }; +static LIST_HEAD(priv_list); + static struct freq_attr *cpufreq_dt_attr[] = { &cpufreq_freq_attr_scaling_available_freqs, NULL, /* Extra space for boost-attr if required */ NULL, }; +static struct private_data *cpufreq_dt_find_data(int cpu) +{ + struct private_data *priv; + + list_for_each_entry(priv, &priv_list, node) { + if (cpumask_test_cpu(cpu, priv->cpus)) + return priv; + } + + return NULL; +} + static int set_target(struct cpufreq_policy *policy, unsigned int index) { struct private_data *priv = policy->driver_data; @@ -90,83 +108,24 @@ static const char *find_supply_name(struct device *dev) return name; } -static int resources_available(void) -{ - struct device *cpu_dev; - struct regulator *cpu_reg; - struct clk *cpu_clk; - int ret = 0; - const char *name; - - cpu_dev = get_cpu_device(0); - if (!cpu_dev) { - pr_err("failed to get cpu0 device\n"); - return -ENODEV; - } - - cpu_clk = clk_get(cpu_dev, NULL); - ret = PTR_ERR_OR_ZERO(cpu_clk); - if (ret) { - /* - * If cpu's clk node is present, but clock is not yet - * registered, we should try defering probe. - */ - if (ret == -EPROBE_DEFER) - dev_dbg(cpu_dev, "clock not ready, retry\n"); - else - dev_err(cpu_dev, "failed to get clock: %d\n", ret); - - return ret; - } - - clk_put(cpu_clk); - - ret = dev_pm_opp_of_find_icc_paths(cpu_dev, NULL); - if (ret) - return ret; - - name = find_supply_name(cpu_dev); - /* Platform doesn't require regulator */ - if (!name) - return 0; - - cpu_reg = regulator_get_optional(cpu_dev, name); - ret = PTR_ERR_OR_ZERO(cpu_reg); - if (ret) { - /* - * If cpu's regulator supply node is present, but regulator is - * not yet registered, we should try defering probe. - */ - if (ret == -EPROBE_DEFER) - dev_dbg(cpu_dev, "cpu0 regulator not ready, retry\n"); - else - dev_dbg(cpu_dev, "no regulator for cpu0: %d\n", ret); - - return ret; - } - - regulator_put(cpu_reg); - return 0; -} - static int cpufreq_init(struct cpufreq_policy *policy) { struct cpufreq_frequency_table *freq_table; - struct opp_table *opp_table = NULL; struct private_data *priv; struct device *cpu_dev; struct clk *cpu_clk; unsigned int transition_latency; - bool fallback = false; - const char *name; int ret; - cpu_dev = get_cpu_device(policy->cpu); - if (!cpu_dev) { - pr_err("failed to get cpu%d device\n", policy->cpu); + priv = cpufreq_dt_find_data(policy->cpu); + if (!priv) { + pr_err("failed to find data for cpu%d\n", policy->cpu); return -ENODEV; } + cpu_dev = priv->cpu_dev; + cpumask_copy(policy->cpus, priv->cpus); + cpu_clk = clk_get(cpu_dev, NULL); if (IS_ERR(cpu_clk)) { ret = PTR_ERR(cpu_clk); @@ -174,45 +133,6 @@ static int cpufreq_init(struct cpufreq_policy *policy) return ret; } - /* Get OPP-sharing information from "operating-points-v2" bindings */ - ret = dev_pm_opp_of_get_sharing_cpus(cpu_dev, policy->cpus); - if (ret) { - if (ret != -ENOENT) - goto out_put_clk; - - /* - * operating-points-v2 not supported, fallback to old method of - * finding shared-OPPs for backward compatibility if the - * platform hasn't set sharing CPUs. - */ - if (dev_pm_opp_get_sharing_cpus(cpu_dev, policy->cpus)) - fallback = true; - } - - /* - * OPP layer will be taking care of regulators now, but it needs to know - * the name of the regulator first. - */ - name = find_supply_name(cpu_dev); - if (name) { - opp_table = dev_pm_opp_set_regulators(cpu_dev, &name, 1); - if (IS_ERR(opp_table)) { - ret = PTR_ERR(opp_table); - dev_err(cpu_dev, "Failed to set regulator for cpu%d: %d\n", - policy->cpu, ret); - goto out_put_clk; - } - } - - priv = kzalloc(sizeof(*priv), GFP_KERNEL); - if (!priv) { - ret = -ENOMEM; - goto out_put_regulator; - } - - priv->reg_name = name; - priv->opp_table = opp_table; - /* * Initialize OPP tables for all policy->cpus. They will be shared by * all CPUs which have marked their CPUs shared with OPP bindings. @@ -232,31 +152,17 @@ static int cpufreq_init(struct cpufreq_policy *policy) */ ret = dev_pm_opp_get_opp_count(cpu_dev); if (ret <= 0) { - dev_dbg(cpu_dev, "OPP table is not ready, deferring probe\n"); - ret = -EPROBE_DEFER; + dev_err(cpu_dev, "OPP table can't be empty\n"); + ret = -ENODEV; goto out_free_opp; } - if (fallback) { - cpumask_setall(policy->cpus); - - /* - * OPP tables are initialized only for policy->cpu, do it for - * others as well. - */ - ret = dev_pm_opp_set_sharing_cpus(cpu_dev, policy->cpus); - if (ret) - dev_err(cpu_dev, "%s: failed to mark OPPs as shared: %d\n", - __func__, ret); - } - ret = dev_pm_opp_init_cpufreq_table(cpu_dev, &freq_table); if (ret) { dev_err(cpu_dev, "failed to init cpufreq table: %d\n", ret); goto out_free_opp; } - priv->cpu_dev = cpu_dev; policy->driver_data = priv; policy->clk = cpu_clk; policy->freq_table = freq_table; @@ -288,11 +194,6 @@ static int cpufreq_init(struct cpufreq_policy *policy) out_free_opp: if (priv->have_static_opps) dev_pm_opp_of_cpumask_remove_table(policy->cpus); - kfree(priv); -out_put_regulator: - if (name) - dev_pm_opp_put_regulators(opp_table); -out_put_clk: clk_put(cpu_clk); return ret; @@ -320,12 +221,7 @@ static int cpufreq_exit(struct cpufreq_policy *policy) dev_pm_opp_free_cpufreq_table(priv->cpu_dev, &policy->freq_table); if (priv->have_static_opps) dev_pm_opp_of_cpumask_remove_table(policy->related_cpus); - if (priv->reg_name) - dev_pm_opp_put_regulators(priv->opp_table); - clk_put(policy->clk); - kfree(priv); - return 0; } @@ -344,21 +240,119 @@ static struct cpufreq_driver dt_cpufreq_driver = { .suspend = cpufreq_generic_suspend, }; -static int dt_cpufreq_probe(struct platform_device *pdev) +static int dt_cpufreq_early_init(struct device *dev, int cpu) { - struct cpufreq_dt_platform_data *data = dev_get_platdata(&pdev->dev); + struct private_data *priv; + struct device *cpu_dev; + const char *reg_name; int ret; + /* Check if this CPU is already covered by some other policy */ + if (cpufreq_dt_find_data(cpu)) + return 0; + + cpu_dev = get_cpu_device(cpu); + if (!cpu_dev) + return -EPROBE_DEFER; + + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); + if (!priv) + return -ENOMEM; + + if (!alloc_cpumask_var(&priv->cpus, GFP_KERNEL)) + return -ENOMEM; + + priv->cpu_dev = cpu_dev; + + /* Try to get OPP table early to ensure resources are available */ + priv->opp_table = dev_pm_opp_get_opp_table(cpu_dev); + if (IS_ERR(priv->opp_table)) { + ret = PTR_ERR(priv->opp_table); + if (ret != -EPROBE_DEFER) + dev_err(cpu_dev, "failed to get OPP table: %d\n", ret); + goto free_cpumask; + } + /* - * All per-cluster (CPUs sharing clock/voltages) initialization is done - * from ->init(). In probe(), we just need to make sure that clk and - * regulators are available. Else defer probe and retry. - * - * FIXME: Is checking this only for CPU0 sufficient ? + * OPP layer will be taking care of regulators now, but it needs to know + * the name of the regulator first. */ - ret = resources_available(); - if (ret) - return ret; + reg_name = find_supply_name(cpu_dev); + if (reg_name) { + priv->reg_opp_table = dev_pm_opp_set_regulators(cpu_dev, + ®_name, 1); + if (IS_ERR(priv->reg_opp_table)) { + ret = PTR_ERR(priv->reg_opp_table); + if (ret != -EPROBE_DEFER) + dev_err(cpu_dev, "failed to set regulators: %d\n", + ret); + goto put_table; + } + } + + /* Find OPP sharing information so we can fill pri->cpus here */ + /* Get OPP-sharing information from "operating-points-v2" bindings */ + ret = dev_pm_opp_of_get_sharing_cpus(cpu_dev, priv->cpus); + if (ret) { + if (ret != -ENOENT) + goto put_reg; + + /* + * operating-points-v2 not supported, fallback to all CPUs share + * OPP for backward compatibility if the platform hasn't set + * sharing CPUs. + */ + if (dev_pm_opp_get_sharing_cpus(cpu_dev, priv->cpus)) { + cpumask_setall(priv->cpus); + + /* + * OPP tables are initialized only for cpu, do it for + * others as well. + */ + ret = dev_pm_opp_set_sharing_cpus(cpu_dev, priv->cpus); + if (ret) + dev_err(cpu_dev, "%s: failed to mark OPPs as shared: %d\n", + __func__, ret); + } + } + + list_add(&priv->node, &priv_list); + return 0; + +put_reg: + if (priv->reg_opp_table) + dev_pm_opp_put_regulators(priv->reg_opp_table); +put_table: + dev_pm_opp_put_opp_table(priv->opp_table); +free_cpumask: + free_cpumask_var(priv->cpus); + return ret; +} + +static void dt_cpufreq_release(void) +{ + struct private_data *priv, *tmp; + + list_for_each_entry_safe(priv, tmp, &priv_list, node) { + if (priv->reg_opp_table) + dev_pm_opp_put_regulators(priv->reg_opp_table); + dev_pm_opp_put_opp_table(priv->opp_table); + free_cpumask_var(priv->cpus); + list_del(&priv->node); + } +} + +static int dt_cpufreq_probe(struct platform_device *pdev) +{ + struct cpufreq_dt_platform_data *data = dev_get_platdata(&pdev->dev); + int ret, cpu; + + /* Request resources early so we can return in case of -EPROBE_DEFER */ + for_each_possible_cpu(cpu) { + ret = dt_cpufreq_early_init(&pdev->dev, cpu); + if (ret) + goto err; + }; if (data) { if (data->have_governor_per_policy) @@ -374,15 +368,21 @@ static int dt_cpufreq_probe(struct platform_device *pdev) } ret = cpufreq_register_driver(&dt_cpufreq_driver); - if (ret) + if (ret) { dev_err(&pdev->dev, "failed register driver: %d\n", ret); + goto err; + } + return 0; +err: + dt_cpufreq_release(); return ret; } static int dt_cpufreq_remove(struct platform_device *pdev) { cpufreq_unregister_driver(&dt_cpufreq_driver); + dt_cpufreq_release(); return 0; }