| Message ID | 20230301201446.3713334-17-daniel.lezcano@linaro.org |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | Self-encapsulate the thermal zone device structure | expand |
On Wed, Mar 01, 2023 at 09:14:44PM +0100, Daniel Lezcano wrote: > The function tegra_tsensor_enable_hw_channel() takes the thermal zone > lock to prevent "a potential" race with a call to set_trips() > callback. > > The driver must not play with the thermal framework core code > internals. > > The tegra_tsensor_enable_hw_channel() is called by: > > - the suspend / resume callbacks > - the probe function after the thermal zones are registered > > The thermal zone lock taken in this function is supposed to protect > from a call to the set_trips() callback which writes in the same > register. > > The potential race is when suspend / resume are called at the same > time as set_trips. This one is called only in > thermal_zone_device_update(). > > - At suspend time, the 'in_suspend' is set, thus the > thermal_zone_device_update() bails out immediately and set_trips is > not called during this moment. > > - At resume time, the thermal zone is updated at PM_POST_SUSPEND, > thus the driver has already set the TH2 temperature. > > - At probe time, we register the thermal zone and then we set the > TH2. The only scenario I can see so far is the interrupt fires, the > thermal_zone_update() is called exactly at the moment > tegra_tsensor_enable_hw_channel() a few lines after registering it. > > Enable the channels before setting up the interrupt. We close the > potential race window without using the thermal zone's lock. > > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> > Suggested-by: Thierry Reding <thierry.reding@gmail.com> > --- > drivers/thermal/tegra/tegra30-tsensor.c | 25 ++++++++++++++----------- > 1 file changed, 14 insertions(+), 11 deletions(-) Acked-by: Thierry Reding <treding@nvidia.com>
On 02/03/2023 10:47, Thierry Reding wrote: > On Wed, Mar 01, 2023 at 09:14:44PM +0100, Daniel Lezcano wrote: >> The function tegra_tsensor_enable_hw_channel() takes the thermal zone >> lock to prevent "a potential" race with a call to set_trips() >> callback. >> >> The driver must not play with the thermal framework core code >> internals. >> >> The tegra_tsensor_enable_hw_channel() is called by: >> >> - the suspend / resume callbacks >> - the probe function after the thermal zones are registered >> >> The thermal zone lock taken in this function is supposed to protect >> from a call to the set_trips() callback which writes in the same >> register. >> >> The potential race is when suspend / resume are called at the same >> time as set_trips. This one is called only in >> thermal_zone_device_update(). >> >> - At suspend time, the 'in_suspend' is set, thus the >> thermal_zone_device_update() bails out immediately and set_trips is >> not called during this moment. >> >> - At resume time, the thermal zone is updated at PM_POST_SUSPEND, >> thus the driver has already set the TH2 temperature. >> >> - At probe time, we register the thermal zone and then we set the >> TH2. The only scenario I can see so far is the interrupt fires, the >> thermal_zone_update() is called exactly at the moment >> tegra_tsensor_enable_hw_channel() a few lines after registering it. >> >> Enable the channels before setting up the interrupt. We close the >> potential race window without using the thermal zone's lock. >> >> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> >> Suggested-by: Thierry Reding <thierry.reding@gmail.com> >> --- >> drivers/thermal/tegra/tegra30-tsensor.c | 25 ++++++++++++++----------- >> 1 file changed, 14 insertions(+), 11 deletions(-) > > Acked-by: Thierry Reding <treding@nvidia.com> Thanks for your review May I ask you to have a look at patch 2 and 8 ? Thanks -- Daniel
diff --git a/drivers/thermal/tegra/tegra30-tsensor.c b/drivers/thermal/tegra/tegra30-tsensor.c index 4b2ea17910cd..cb584a5735ed 100644 --- a/drivers/thermal/tegra/tegra30-tsensor.c +++ b/drivers/thermal/tegra/tegra30-tsensor.c @@ -359,9 +359,6 @@ static int tegra_tsensor_enable_hw_channel(const struct tegra_tsensor *ts, tegra_tsensor_get_hw_channel_trips(tzd, &hot_trip, &crit_trip); - /* prevent potential racing with tegra_tsensor_set_trips() */ - mutex_lock(&tzd->lock); - dev_info_once(ts->dev, "ch%u: PMC emergency shutdown trip set to %dC\n", id, DIV_ROUND_CLOSEST(crit_trip, 1000)); @@ -404,8 +401,6 @@ static int tegra_tsensor_enable_hw_channel(const struct tegra_tsensor *ts, val |= FIELD_PREP(TSENSOR_SENSOR0_CONFIG0_INTR_THERMAL_RST_EN, 1); writel_relaxed(val, tsc->regs + TSENSOR_SENSOR0_CONFIG0); - mutex_unlock(&tzd->lock); - err = thermal_zone_device_enable(tzd); if (err) { dev_err(ts->dev, "ch%u: failed to enable zone: %d\n", id, err); @@ -585,6 +580,20 @@ static int tegra_tsensor_probe(struct platform_device *pdev) return err; } + /* + * Enable the channels before setting the interrupt so + * set_trips() can not be called while we are setting up the + * register TSENSOR_SENSOR0_CONFIG1. With this we close a + * potential race window where we are setting up the TH2 and + * the temperature hits TH1 resulting to an update of the + * TSENSOR_SENSOR0_CONFIG1 register in the ISR. + */ + for (i = 0; i < ARRAY_SIZE(ts->ch); i++) { + err = tegra_tsensor_enable_hw_channel(ts, i); + if (err) + return err; + } + err = devm_request_threaded_irq(&pdev->dev, irq, NULL, tegra_tsensor_isr, IRQF_ONESHOT, "tegra_tsensor", ts); @@ -592,12 +601,6 @@ static int tegra_tsensor_probe(struct platform_device *pdev) return dev_err_probe(&pdev->dev, err, "failed to request interrupt\n"); - for (i = 0; i < ARRAY_SIZE(ts->ch); i++) { - err = tegra_tsensor_enable_hw_channel(ts, i); - if (err) - return err; - } - return 0; }
The function tegra_tsensor_enable_hw_channel() takes the thermal zone lock to prevent "a potential" race with a call to set_trips() callback. The driver must not play with the thermal framework core code internals. The tegra_tsensor_enable_hw_channel() is called by: - the suspend / resume callbacks - the probe function after the thermal zones are registered The thermal zone lock taken in this function is supposed to protect from a call to the set_trips() callback which writes in the same register. The potential race is when suspend / resume are called at the same time as set_trips. This one is called only in thermal_zone_device_update(). - At suspend time, the 'in_suspend' is set, thus the thermal_zone_device_update() bails out immediately and set_trips is not called during this moment. - At resume time, the thermal zone is updated at PM_POST_SUSPEND, thus the driver has already set the TH2 temperature. - At probe time, we register the thermal zone and then we set the TH2. The only scenario I can see so far is the interrupt fires, the thermal_zone_update() is called exactly at the moment tegra_tsensor_enable_hw_channel() a few lines after registering it. Enable the channels before setting up the interrupt. We close the potential race window without using the thermal zone's lock. Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> Suggested-by: Thierry Reding <thierry.reding@gmail.com> --- drivers/thermal/tegra/tegra30-tsensor.c | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-)