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

["Niklas Söderlund" <niklas.soderlund+renesas@ragnatech.se>]

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