diff mbox series

[v2] PM: opp: Fix NULL pointer exception on a v2 table combined with v1 opps

Message ID 20220404123757.798917-1-krzysztof.kozlowski@linaro.org
State New
Headers show
Series [v2] PM: opp: Fix NULL pointer exception on a v2 table combined with v1 opps | expand

Commit Message

Krzysztof Kozlowski April 4, 2022, 12:37 p.m. UTC 
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(-)

Comments

Viresh Kumar April 11, 2022, 2:49 a.m. UTC | #1 
On 04-04-22, 14:37, Krzysztof Kozlowski wrote:
> dev_pm_opp_add() adds a v1 OPP.

That's not correct, it adds a simpler version of OPP and doesn't
support complex types. A opp v2 table with just freq and voltage
should be supported by dev_pm_opp_add() and we shouldn't disallow it.

I think all we need here is a couple of checks to make sure the
earlier OPPs don't have anything which the new OPP can't support. For
example checking required-opps field, etc.
Krzysztof Kozlowski April 19, 2022, 12:40 p.m. UTC | #2 
On 11/04/2022 04:49, Viresh Kumar wrote:
> On 04-04-22, 14:37, Krzysztof Kozlowski wrote:
>> dev_pm_opp_add() adds a v1 OPP.
> 
> That's not correct, it adds a simpler version of OPP and doesn't
> support complex types. A opp v2 table with just freq and voltage
> should be supported by dev_pm_opp_add() and we shouldn't disallow it.
> 
> I think all we need here is a couple of checks to make sure the
> earlier OPPs don't have anything which the new OPP can't support. For
> example checking required-opps field, etc.

Would be useful to have list of such new-OPP properties somewhere. I'll
try to rework the patch based on this.


Best regards,
Krzysztof
diff mbox series

Patch

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);