All of lore.kernel.org
 help / color / mirror / Atom feed
From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
To: Viresh Kumar <vireshk@kernel.org>, Nishanth Menon <nm@ti.com>,
	Stephen Boyd <sboyd@kernel.org>,
	Dmitry Osipenko <digetx@gmail.com>,
	linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org
Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Subject: [PATCH v2] PM: opp: Fix NULL pointer exception on a v2 table combined with v1 opps
Date: Mon,  4 Apr 2022 14:37:57 +0200	[thread overview]
Message-ID: <20220404123757.798917-1-krzysztof.kozlowski@linaro.org> (raw)

dev_pm_opp_add() adds a v1 OPP.  If the Devicetree contains an OPP v2
table with required-opps, but the driver uses dev_pm_opp_add(), the
table might have required_opp_count!=0 but the opp->required_opps will
be NULL.  This leads to NULL pointer exception, e.g. with ufs-qcom.c
driver and v2 OPP table in DTS:

  Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
  ...
  Call trace:
   _required_opps_available+0x20/0x80
   _opp_add_v1+0xa8/0x110
   dev_pm_opp_add+0x54/0xcc
   ufshcd_async_scan+0x190/0x31c
   async_run_entry_fn+0x38/0x144

The case of mixing v1 and v2 OPP tables is not supported.  Add a version
field to the OPP table to handle such cases:
1. Disallow mixing v1 and v2 OPPs,
2. Skip parsing required opps when dealing with a v1 OPP table.

Fixes: cf65948d62c6 ("opp: Filter out OPPs based on availability of a required-OPP")
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

---

Changes since v1:
1. Add version field.
2. Skip allocating required fields in _opp_table_alloc_required_tables()
   instead of iterating over it later.
---
 drivers/opp/core.c | 35 +++++++++++++++++++++++------------
 drivers/opp/of.c   | 22 +++++++++++++++++++++-
 drivers/opp/opp.h  | 12 +++++++++++-
 3 files changed, 55 insertions(+), 14 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 740407252298..0b5357b9d342 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -1227,7 +1227,8 @@ struct opp_device *_add_opp_dev(const struct device *dev,
 	return opp_dev;
 }
 
-static struct opp_table *_allocate_opp_table(struct device *dev, int index)
+static struct opp_table *_allocate_opp_table(struct device *dev, int index,
+					     enum opp_table_version version)
 {
 	struct opp_table *opp_table;
 	struct opp_device *opp_dev;
@@ -1248,6 +1249,7 @@ static struct opp_table *_allocate_opp_table(struct device *dev, int index)
 
 	/* Mark regulator count uninitialized */
 	opp_table->regulator_count = -1;
+	opp_table->version = version;
 
 	opp_dev = _add_opp_dev(dev, opp_table);
 	if (!opp_dev) {
@@ -1332,7 +1334,8 @@ static struct opp_table *_update_opp_table_clk(struct device *dev,
  * of adding an OPP table and others should wait for it to finish.
  */
 struct opp_table *_add_opp_table_indexed(struct device *dev, int index,
-					 bool getclk)
+					 bool getclk,
+					 enum opp_table_version version)
 {
 	struct opp_table *opp_table;
 
@@ -1367,7 +1370,7 @@ struct opp_table *_add_opp_table_indexed(struct device *dev, int index,
 
 		mutex_lock(&opp_table_lock);
 	} else {
-		opp_table = _allocate_opp_table(dev, index);
+		opp_table = _allocate_opp_table(dev, index, version);
 
 		mutex_lock(&opp_table_lock);
 		if (!IS_ERR(opp_table))
@@ -1382,9 +1385,10 @@ struct opp_table *_add_opp_table_indexed(struct device *dev, int index,
 	return _update_opp_table_clk(dev, opp_table, getclk);
 }
 
-static struct opp_table *_add_opp_table(struct device *dev, bool getclk)
+static struct opp_table *_add_opp_table(struct device *dev, bool getclk,
+					enum opp_table_version version)
 {
-	return _add_opp_table_indexed(dev, 0, getclk);
+	return _add_opp_table_indexed(dev, 0, getclk, version);
 }
 
 struct opp_table *dev_pm_opp_get_opp_table(struct device *dev)
@@ -1794,6 +1798,13 @@ int _opp_add_v1(struct opp_table *opp_table, struct device *dev,
 	unsigned long tol;
 	int ret;
 
+	if (opp_table->version != OPP_TABLE_VERSION_UNKNOWN &&
+	    opp_table->version != OPP_TABLE_VERSION_1) {
+		dev_warn(dev, "%s: Mixing v1 and v2 OPP bindings not supported (%lu)\n",
+			 __func__, freq);
+		return -EINVAL;
+	}
+
 	new_opp = _opp_allocate(opp_table);
 	if (!new_opp)
 		return -ENOMEM;
@@ -1844,7 +1855,7 @@ struct opp_table *dev_pm_opp_set_supported_hw(struct device *dev,
 {
 	struct opp_table *opp_table;
 
-	opp_table = _add_opp_table(dev, false);
+	opp_table = _add_opp_table(dev, false, OPP_TABLE_VERSION_2);
 	if (IS_ERR(opp_table))
 		return opp_table;
 
@@ -1932,7 +1943,7 @@ struct opp_table *dev_pm_opp_set_prop_name(struct device *dev, const char *name)
 {
 	struct opp_table *opp_table;
 
-	opp_table = _add_opp_table(dev, false);
+	opp_table = _add_opp_table(dev, false, OPP_TABLE_VERSION_2);
 	if (IS_ERR(opp_table))
 		return opp_table;
 
@@ -1994,7 +2005,7 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev,
 	struct regulator *reg;
 	int ret, i;
 
-	opp_table = _add_opp_table(dev, false);
+	opp_table = _add_opp_table(dev, false, OPP_TABLE_VERSION_UNKNOWN);
 	if (IS_ERR(opp_table))
 		return opp_table;
 
@@ -2149,7 +2160,7 @@ struct opp_table *dev_pm_opp_set_clkname(struct device *dev, const char *name)
 	struct opp_table *opp_table;
 	int ret;
 
-	opp_table = _add_opp_table(dev, false);
+	opp_table = _add_opp_table(dev, false, OPP_TABLE_VERSION_UNKNOWN);
 	if (IS_ERR(opp_table))
 		return opp_table;
 
@@ -2247,7 +2258,7 @@ struct opp_table *dev_pm_opp_register_set_opp_helper(struct device *dev,
 	if (!set_opp)
 		return ERR_PTR(-EINVAL);
 
-	opp_table = _add_opp_table(dev, false);
+	opp_table = _add_opp_table(dev, false, OPP_TABLE_VERSION_UNKNOWN);
 	if (IS_ERR(opp_table))
 		return opp_table;
 
@@ -2380,7 +2391,7 @@ struct opp_table *dev_pm_opp_attach_genpd(struct device *dev,
 	int index = 0, ret = -EINVAL;
 	const char * const *name = names;
 
-	opp_table = _add_opp_table(dev, false);
+	opp_table = _add_opp_table(dev, false, OPP_TABLE_VERSION_UNKNOWN);
 	if (IS_ERR(opp_table))
 		return opp_table;
 
@@ -2637,7 +2648,7 @@ int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt)
 	struct opp_table *opp_table;
 	int ret;
 
-	opp_table = _add_opp_table(dev, true);
+	opp_table = _add_opp_table(dev, true, OPP_TABLE_VERSION_1);
 	if (IS_ERR(opp_table))
 		return PTR_ERR(opp_table);
 
diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index 440ab5a03df9..aa757f73277a 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -169,6 +169,9 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table,
 		return;
 	}
 
+	if (opp_table->version == OPP_TABLE_VERSION_1)
+		goto skip_required_opp;
+
 	count = of_count_phandle_with_args(np, "required-opps", NULL);
 	if (count <= 0)
 		goto put_np;
@@ -193,6 +196,7 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table,
 			lazy = true;
 	}
 
+skip_required_opp:
 	/* Let's do the linking later on */
 	if (lazy)
 		list_add(&opp_table->lazy, &lazy_opp_tables);
@@ -883,6 +887,13 @@ static struct dev_pm_opp *_opp_add_static_v2(struct opp_table *opp_table,
 	int ret;
 	bool rate_not_available = false;
 
+	if (opp_table->version != OPP_TABLE_VERSION_UNKNOWN &&
+	    opp_table->version != OPP_TABLE_VERSION_2) {
+		dev_warn(dev, "%s: Mixing v1 and v2 OPP bindings not supported\n",
+			 __func__);
+		return ERR_PTR(-EINVAL);
+	}
+
 	new_opp = _opp_allocate(opp_table);
 	if (!new_opp)
 		return ERR_PTR(-ENOMEM);
@@ -985,6 +996,9 @@ static int _of_add_opp_table_v2(struct device *dev, struct opp_table *opp_table)
 	opp_table->parsed_static_opps = 1;
 	mutex_unlock(&opp_table->lock);
 
+	WARN_ON(opp_table->version != OPP_TABLE_VERSION_UNKNOWN);
+	opp_table->version = OPP_TABLE_VERSION_2;
+
 	/* We have opp-table node now, iterate over it and add OPPs */
 	for_each_available_child_of_node(opp_table->np, np) {
 		opp = _opp_add_static_v2(opp_table, dev, np);
@@ -1020,6 +1034,7 @@ static int _of_add_opp_table_v2(struct device *dev, struct opp_table *opp_table)
 
 remove_static_opp:
 	_opp_remove_all_static(opp_table);
+	opp_table->version = OPP_TABLE_VERSION_UNKNOWN;
 
 	return ret;
 }
@@ -1041,6 +1056,9 @@ static int _of_add_opp_table_v1(struct device *dev, struct opp_table *opp_table)
 	opp_table->parsed_static_opps = 1;
 	mutex_unlock(&opp_table->lock);
 
+	WARN_ON(opp_table->version != OPP_TABLE_VERSION_UNKNOWN);
+	opp_table->version = OPP_TABLE_VERSION_1;
+
 	prop = of_find_property(dev->of_node, "operating-points", NULL);
 	if (!prop) {
 		ret = -ENODEV;
@@ -1080,6 +1098,7 @@ static int _of_add_opp_table_v1(struct device *dev, struct opp_table *opp_table)
 
 remove_static_opp:
 	_opp_remove_all_static(opp_table);
+	opp_table->version = OPP_TABLE_VERSION_UNKNOWN;
 
 	return ret;
 }
@@ -1100,7 +1119,8 @@ static int _of_add_table_indexed(struct device *dev, int index, bool getclk)
 			index = 0;
 	}
 
-	opp_table = _add_opp_table_indexed(dev, index, getclk);
+	opp_table = _add_opp_table_indexed(dev, index, getclk,
+					   OPP_TABLE_VERSION_UNKNOWN);
 	if (IS_ERR(opp_table))
 		return PTR_ERR(opp_table);
 
diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
index 45e3a55239a1..bd8349f8c7b0 100644
--- a/drivers/opp/opp.h
+++ b/drivers/opp/opp.h
@@ -124,6 +124,12 @@ enum opp_table_access {
 	OPP_TABLE_ACCESS_SHARED = 2,
 };
 
+enum opp_table_version {
+	OPP_TABLE_VERSION_UNKNOWN = 0,
+	OPP_TABLE_VERSION_1 = 1,
+	OPP_TABLE_VERSION_2 = 2,
+};
+
 /**
  * struct opp_table - Device opp structure
  * @node:	table node - contains the devices with OPPs that
@@ -138,6 +144,7 @@ enum opp_table_access {
  * @clock_latency_ns_max: Max clock latency in nanoseconds.
  * @parsed_static_opps: Count of devices for which OPPs are initialized from DT.
  * @shared_opp: OPP is shared between multiple devices.
+ * @version: OPP bindings version to disallow mixing (e.g. v1 and v2).
  * @current_rate: Currently configured frequency.
  * @current_opp: Currently configured OPP for the table.
  * @suspend_opp: Pointer to OPP to be used during device suspend.
@@ -188,6 +195,7 @@ struct opp_table {
 
 	unsigned int parsed_static_opps;
 	enum opp_table_access shared_opp;
+	enum opp_table_version version;
 	unsigned long current_rate;
 	struct dev_pm_opp *current_opp;
 	struct dev_pm_opp *suspend_opp;
@@ -232,7 +240,9 @@ int _opp_compare_key(struct dev_pm_opp *opp1, struct dev_pm_opp *opp2);
 int _opp_add(struct device *dev, struct dev_pm_opp *new_opp, struct opp_table *opp_table, bool rate_not_available);
 int _opp_add_v1(struct opp_table *opp_table, struct device *dev, unsigned long freq, long u_volt, bool dynamic);
 void _dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask, int last_cpu);
-struct opp_table *_add_opp_table_indexed(struct device *dev, int index, bool getclk);
+struct opp_table *_add_opp_table_indexed(struct device *dev, int index,
+					 bool getclk,
+					 enum opp_table_version version);
 void _put_opp_list_kref(struct opp_table *opp_table);
 void _required_opps_available(struct dev_pm_opp *opp, int count);
 
-- 
2.32.0


             reply	other threads:[~2022-04-04 12:38 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-04-04 12:37 Krzysztof Kozlowski [this message]
2022-04-11  2:49 ` [PATCH v2] PM: opp: Fix NULL pointer exception on a v2 table combined with v1 opps Viresh Kumar
2022-04-19 12:40   ` Krzysztof Kozlowski

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20220404123757.798917-1-krzysztof.kozlowski@linaro.org \
    --to=krzysztof.kozlowski@linaro.org \
    --cc=digetx@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=nm@ti.com \
    --cc=sboyd@kernel.org \
    --cc=vireshk@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.