[PATCH v2 0/3] drivers/thermal/rcar_gen3_thermal: Fix device initialization

[PATCH v2 0/3] drivers/thermal/rcar_gen3_thermal: Fix device initialization

[Niklas Söderlund]

Hello,

This small series fixes a window where incorrect values can be read from
the driver before it is fully initialized. The root cause is that the
thermal zone is register too early.

Patch 1/3 is new in v2 and removes a unneeded call to set_trips() when 
resuming from suspend, This call was in v1 changed as part of addressing 
the initialization issue, it's nicer to get rid of it before that is 
needed.

Patch 2/3 prepares for the change while also fixing a theoretical issue
where one thermal node described in DT would describe interrupts and
another would not. Resulting in interrupt support being disabled for
both of them. I'm not aware of any case where this configuration would
be used, either the SoC supports interrupts, or it don't.

While patch 3/3 fixes the real issue by fully initializing the device
before registering the zone.

Niklas Söderlund (3):
  drivers/thermal/rcar_gen3_thermal: Do not call set_trips() when
    resuming
  drivers/thermal/rcar_gen3_thermal: Create device local ops struct
  drivers/thermal/rcar_gen3_thermal: Fix device initialization

 drivers/thermal/rcar_gen3_thermal.c | 36 ++++++++++++++---------------
 1 file changed, 18 insertions(+), 18 deletions(-)

-- 
2.39.1

[PATCH v2 1/3] drivers/thermal/rcar_gen3_thermal: Do not call set_trips() when resuming

[Niklas Söderlund]

There is no need to explicitly call set_trips() when resuming from
suspend. The thermal framework calls thermal_zone_device_update() that
restores the trip points.

Suggested-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/thermal/rcar_gen3_thermal.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
index 4c1c6f89aa2f..5a715a58f18b 100644
--- a/drivers/thermal/rcar_gen3_thermal.c
+++ b/drivers/thermal/rcar_gen3_thermal.c
@@ -556,12 +556,8 @@ static int __maybe_unused rcar_gen3_thermal_resume(struct device *dev)
 
 	for (i = 0; i < priv->num_tscs; i++) {
 		struct rcar_gen3_thermal_tsc *tsc = priv->tscs[i];
-		struct thermal_zone_device *zone = tsc->zone;
 
 		priv->thermal_init(tsc);
-		if (zone->ops->set_trips)
-			rcar_gen3_thermal_set_trips(zone, zone->prev_low_trip,
-						    zone->prev_high_trip);
 	}
 
 	return 0;
-- 
2.39.1

[PATCH v2 2/3] drivers/thermal/rcar_gen3_thermal: Create device local ops struct

[Niklas Söderlund]

The callback operations are modified on a driver global level. If one
device tree description do not define interrupts, the set_trips()
operation was disabled globally for all users of the driver.

Fix this by creating a device local copy of the operations structure and
modify the copy depending on what the device can do.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/thermal/rcar_gen3_thermal.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
index 5a715a58f18b..2dfce4f09631 100644
--- a/drivers/thermal/rcar_gen3_thermal.c
+++ b/drivers/thermal/rcar_gen3_thermal.c
@@ -87,6 +87,7 @@ struct rcar_gen3_thermal_tsc {
 
 struct rcar_gen3_thermal_priv {
 	struct rcar_gen3_thermal_tsc *tscs[TSC_MAX_NUM];
+	struct thermal_zone_device_ops ops;
 	unsigned int num_tscs;
 	void (*thermal_init)(struct rcar_gen3_thermal_tsc *tsc);
 	int ptat[3];
@@ -225,7 +226,7 @@ static int rcar_gen3_thermal_set_trips(struct thermal_zone_device *tz, int low,
 	return 0;
 }
 
-static struct thermal_zone_device_ops rcar_gen3_tz_of_ops = {
+static const struct thermal_zone_device_ops rcar_gen3_tz_of_ops = {
 	.get_temp	= rcar_gen3_thermal_get_temp,
 	.set_trips	= rcar_gen3_thermal_set_trips,
 };
@@ -466,6 +467,7 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
 	if (!priv)
 		return -ENOMEM;
 
+	priv->ops = rcar_gen3_tz_of_ops;
 	priv->thermal_init = rcar_gen3_thermal_init;
 	if (soc_device_match(r8a7795es1))
 		priv->thermal_init = rcar_gen3_thermal_init_r8a7795es1;
@@ -473,7 +475,7 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, priv);
 
 	if (rcar_gen3_thermal_request_irqs(priv, pdev))
-		rcar_gen3_tz_of_ops.set_trips = NULL;
+		priv->ops.set_trips = NULL;
 
 	pm_runtime_enable(dev);
 	pm_runtime_get_sync(dev);
@@ -508,8 +510,7 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
 	for (i = 0; i < priv->num_tscs; i++) {
 		struct rcar_gen3_thermal_tsc *tsc = priv->tscs[i];
 
-		zone = devm_thermal_of_zone_register(dev, i, tsc,
-						     &rcar_gen3_tz_of_ops);
+		zone = devm_thermal_of_zone_register(dev, i, tsc, &priv->ops);
 		if (IS_ERR(zone)) {
 			dev_err(dev, "Sensor %u: Can't register thermal zone\n", i);
 			ret = PTR_ERR(zone);
-- 
2.39.1

[PATCH v2 3/3] drivers/thermal/rcar_gen3_thermal: Fix device initialization

[Niklas Söderlund]

The thermal zone is registered before the device is register and the
thermal coefficients are calculated, providing a window for very
incorrect readings.

The reason why the zone was register before the device was fully
initialized was that the presence of the set_trips() callback is used to
determine if the driver supports interrupt or not, as it is not defined
if the device is incapable of interrupts.

Fix this by using the operations structure in the private data instead
of the zone to determine if interrupts are available or not, and
initialize the device before registering the zone.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/thermal/rcar_gen3_thermal.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
index 2dfce4f09631..8835846884d1 100644
--- a/drivers/thermal/rcar_gen3_thermal.c
+++ b/drivers/thermal/rcar_gen3_thermal.c
@@ -89,7 +89,8 @@ struct rcar_gen3_thermal_priv {
 	struct rcar_gen3_thermal_tsc *tscs[TSC_MAX_NUM];
 	struct thermal_zone_device_ops ops;
 	unsigned int num_tscs;
-	void (*thermal_init)(struct rcar_gen3_thermal_tsc *tsc);
+	void (*thermal_init)(struct rcar_gen3_thermal_priv *priv,
+			     struct rcar_gen3_thermal_tsc *tsc);
 	int ptat[3];
 };
 
@@ -240,7 +241,7 @@ static irqreturn_t rcar_gen3_thermal_irq(int irq, void *data)
 	for (i = 0; i < priv->num_tscs; i++) {
 		status = rcar_gen3_thermal_read(priv->tscs[i], REG_GEN3_IRQSTR);
 		rcar_gen3_thermal_write(priv->tscs[i], REG_GEN3_IRQSTR, 0);
-		if (status)
+		if (status && priv->tscs[i]->zone)
 			thermal_zone_device_update(priv->tscs[i]->zone,
 						   THERMAL_EVENT_UNSPECIFIED);
 	}
@@ -311,7 +312,8 @@ static bool rcar_gen3_thermal_read_fuses(struct rcar_gen3_thermal_priv *priv)
 	return true;
 }
 
-static void rcar_gen3_thermal_init_r8a7795es1(struct rcar_gen3_thermal_tsc *tsc)
+static void rcar_gen3_thermal_init_r8a7795es1(struct rcar_gen3_thermal_priv *priv,
+					      struct rcar_gen3_thermal_tsc *tsc)
 {
 	rcar_gen3_thermal_write(tsc, REG_GEN3_CTSR,  CTSR_THBGR);
 	rcar_gen3_thermal_write(tsc, REG_GEN3_CTSR,  0x0);
@@ -322,7 +324,7 @@ static void rcar_gen3_thermal_init_r8a7795es1(struct rcar_gen3_thermal_tsc *tsc)
 
 	rcar_gen3_thermal_write(tsc, REG_GEN3_IRQCTL, 0x3F);
 	rcar_gen3_thermal_write(tsc, REG_GEN3_IRQMSK, 0);
-	if (tsc->zone->ops->set_trips)
+	if (priv->ops.set_trips)
 		rcar_gen3_thermal_write(tsc, REG_GEN3_IRQEN,
 					IRQ_TEMPD1 | IRQ_TEMP2);
 
@@ -338,7 +340,8 @@ static void rcar_gen3_thermal_init_r8a7795es1(struct rcar_gen3_thermal_tsc *tsc)
 	usleep_range(1000, 2000);
 }
 
-static void rcar_gen3_thermal_init(struct rcar_gen3_thermal_tsc *tsc)
+static void rcar_gen3_thermal_init(struct rcar_gen3_thermal_priv *priv,
+				   struct rcar_gen3_thermal_tsc *tsc)
 {
 	u32 reg_val;
 
@@ -350,7 +353,7 @@ static void rcar_gen3_thermal_init(struct rcar_gen3_thermal_tsc *tsc)
 
 	rcar_gen3_thermal_write(tsc, REG_GEN3_IRQCTL, 0);
 	rcar_gen3_thermal_write(tsc, REG_GEN3_IRQMSK, 0);
-	if (tsc->zone->ops->set_trips)
+	if (priv->ops.set_trips)
 		rcar_gen3_thermal_write(tsc, REG_GEN3_IRQEN,
 					IRQ_TEMPD1 | IRQ_TEMP2);
 
@@ -510,6 +513,9 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
 	for (i = 0; i < priv->num_tscs; i++) {
 		struct rcar_gen3_thermal_tsc *tsc = priv->tscs[i];
 
+		priv->thermal_init(priv, tsc);
+		rcar_gen3_thermal_calc_coefs(priv, tsc, *ths_tj_1);
+
 		zone = devm_thermal_of_zone_register(dev, i, tsc, &priv->ops);
 		if (IS_ERR(zone)) {
 			dev_err(dev, "Sensor %u: Can't register thermal zone\n", i);
@@ -518,9 +524,6 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
 		}
 		tsc->zone = zone;
 
-		priv->thermal_init(tsc);
-		rcar_gen3_thermal_calc_coefs(priv, tsc, *ths_tj_1);
-
 		tsc->zone->tzp->no_hwmon = false;
 		ret = thermal_add_hwmon_sysfs(tsc->zone);
 		if (ret)
@@ -558,7 +561,7 @@ static int __maybe_unused rcar_gen3_thermal_resume(struct device *dev)
 	for (i = 0; i < priv->num_tscs; i++) {
 		struct rcar_gen3_thermal_tsc *tsc = priv->tscs[i];
 
-		priv->thermal_init(tsc);
+		priv->thermal_init(priv, tsc);
 	}
 
 	return 0;
-- 
2.39.1

Re: [PATCH v2 0/3] drivers/thermal/rcar_gen3_thermal: Fix device initialization

[Daniel Lezcano]

On Wed, Feb 08, 2023 at 08:03:30PM +0100, Niklas Söderlund wrote:
> Hello,
> 
> This small series fixes a window where incorrect values can be read from
> the driver before it is fully initialized. The root cause is that the
> thermal zone is register too early.
> 
> Patch 1/3 is new in v2 and removes a unneeded call to set_trips() when 
> resuming from suspend, This call was in v1 changed as part of addressing 
> the initialization issue, it's nicer to get rid of it before that is 
> needed.
> 
> Patch 2/3 prepares for the change while also fixing a theoretical issue
> where one thermal node described in DT would describe interrupts and
> another would not. Resulting in interrupt support being disabled for
> both of them. I'm not aware of any case where this configuration would
> be used, either the SoC supports interrupts, or it don't.
> 
> While patch 3/3 fixes the real issue by fully initializing the device
> before registering the zone.
> 
> Niklas Söderlund (3):
>   drivers/thermal/rcar_gen3_thermal: Do not call set_trips() when
>     resuming
>   drivers/thermal/rcar_gen3_thermal: Create device local ops struct
>   drivers/thermal/rcar_gen3_thermal: Fix device initialization
> 
>  drivers/thermal/rcar_gen3_thermal.c | 36 ++++++++++++++---------------
>  1 file changed, 18 insertions(+), 18 deletions(-)

Applied, thhanks

-- 

 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog