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

[PATCH 0/2] 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/2 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 2/2 fixes the real issue by fully initializing the device 
before registering the zone.

Niklas Söderlund (2):
  drivers/thermal/rcar_gen3_thermal: Create device local ops struct
  drivers/thermal/rcar_gen3_thermal: Fix device initialization

 drivers/thermal/rcar_gen3_thermal.c | 34 ++++++++++++++++-------------
 1 file changed, 19 insertions(+), 15 deletions(-)

-- 
2.39.1

[PATCH 1/2] 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>
---
 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 4c1c6f89aa2f..bfa2ff20b945 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

Re: [PATCH 1/2] drivers/thermal/rcar_gen3_thermal: Create device local ops struct

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 572 bytes --]

On Tue, Feb 07, 2023 at 06:10:10PM +0100, Niklas Söderlund wrote:
> 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>

I like this!

Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 2/2] 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>
---
 drivers/thermal/rcar_gen3_thermal.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
index bfa2ff20b945..1dedeece1a00 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)
@@ -559,8 +562,8 @@ static int __maybe_unused rcar_gen3_thermal_resume(struct device *dev)
 		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)
+		priv->thermal_init(priv, tsc);
+		if (priv->ops.set_trips)
 			rcar_gen3_thermal_set_trips(zone, zone->prev_low_trip,
 						    zone->prev_high_trip);
 	}
-- 
2.39.1

Re: [PATCH 2/2] drivers/thermal/rcar_gen3_thermal: Fix device initialization

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1151 bytes --]

On Tue, Feb 07, 2023 at 06:10:11PM +0100, Niklas Söderlund wrote:
> The thermal zone is registered before the device is register and the
> thermal coefficients are calculated, providing a window for very
> incorrect readings.

While I could never actually be in this race window, the patch makes a
lot of sense to me.

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

One minor question:

> -		if (status)
> +		if (status && priv->tscs[i]->zone)
>  			thermal_zone_device_update(priv->tscs[i]->zone,
>  						   THERMAL_EVENT_UNSPECIFIED);

Isn't this a seperate change?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/2] drivers/thermal/rcar_gen3_thermal: Fix device initialization

[Niklas Söderlund]

Hi Wolfram,

Thanks for your review.

On 2023-02-08 08:58:33 +0100, Wolfram Sang wrote:
> On Tue, Feb 07, 2023 at 06:10:11PM +0100, Niklas Söderlund wrote:
> > The thermal zone is registered before the device is register and the
> > thermal coefficients are calculated, providing a window for very
> > incorrect readings.
> 
> While I could never actually be in this race window, the patch makes a
> lot of sense to me.
> 
> > 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>
> 
> One minor question:
> 
> > -		if (status)
> > +		if (status && priv->tscs[i]->zone)
> >  			thermal_zone_device_update(priv->tscs[i]->zone,
> >  						   THERMAL_EVENT_UNSPECIFIED);
> 
> Isn't this a seperate change?

Not really.

This patch changes the driver to enable interrupts just before the zone 
is created. So this just guards from the very small window where an 
interrupt could be fired and the zone is created, which if it would 
happen would cause a null pointer dereference.

While not updating a not yet created zone is fine. But this should never 
happen (tm) as the trip points have not been set when this window 
exists, but better safe then sorry.

-- 
Kind Regards,
Niklas Söderlund

Re: [PATCH 2/2] drivers/thermal/rcar_gen3_thermal: Fix device initialization

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 228 bytes --]


> While not updating a not yet created zone is fine. But this should never 
> happen (tm) as the trip points have not been set when this window 
> exists, but better safe then sorry.

Full Ack! Thanks for the heads up.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/2] drivers/thermal/rcar_gen3_thermal: Fix device initialization

[Daniel Lezcano]

On 07/02/2023 18:10, Niklas Söderlund wrote:
> 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>
> ---
>   drivers/thermal/rcar_gen3_thermal.c | 25 ++++++++++++++-----------
>   1 file changed, 14 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
> index bfa2ff20b945..1dedeece1a00 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)
> @@ -559,8 +562,8 @@ static int __maybe_unused rcar_gen3_thermal_resume(struct device *dev)
>   		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)
> +		priv->thermal_init(priv, tsc);
> +		if (priv->ops.set_trips)
>   			rcar_gen3_thermal_set_trips(zone, zone->prev_low_trip,
>   						    zone->prev_high_trip);

This is not needed, at resume time, the thermal framework has a 
pm_notifier and calls thermal_zone_device_update() which in turn calls 
thermal_zone_set_trips(). If the ops is not set, it will continue.

Actually, no call to set_trips should happen in the driver, just pass 
the ops the thermal framework and it will do the actions.

The same happens when you call thermal_zone_device_register(), it calls 
thermal_zone_device_update(), then thermal_zone_set_trips().


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

Re: [PATCH 2/2] drivers/thermal/rcar_gen3_thermal: Fix device initialization

[Niklas Söderlund]

Hi Daniel,

Thanks for your feedback.

On 2023-02-08 12:06:37 +0100, Daniel Lezcano wrote:
> On 07/02/2023 18:10, Niklas Söderlund wrote:
> > 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>
> > ---
> >   drivers/thermal/rcar_gen3_thermal.c | 25 ++++++++++++++-----------
> >   1 file changed, 14 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
> > index bfa2ff20b945..1dedeece1a00 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)
> > @@ -559,8 +562,8 @@ static int __maybe_unused rcar_gen3_thermal_resume(struct device *dev)
> >   		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)
> > +		priv->thermal_init(priv, tsc);
> > +		if (priv->ops.set_trips)
> >   			rcar_gen3_thermal_set_trips(zone, zone->prev_low_trip,
> >   						    zone->prev_high_trip);
> 
> This is not needed, at resume time, the thermal framework has a pm_notifier
> and calls thermal_zone_device_update() which in turn calls
> thermal_zone_set_trips(). If the ops is not set, it will continue.
> 
> Actually, no call to set_trips should happen in the driver, just pass the
> ops the thermal framework and it will do the actions.
> 
> The same happens when you call thermal_zone_device_register(), it calls
> thermal_zone_device_update(), then thermal_zone_set_trips().

I will send a v2 of this series addressing this before fixing the issue 
addressed in this patch.

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

-- 
Kind Regards,
Niklas Söderlund